From 7a1ea842e4b18dfe8a0a7387e4a3e36966db8db4 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Wed, 2 Sep 2026 09:42:40 +0200 Subject: [PATCH] fix(guests): read identity from whichever store holds it, write it as a pair Two defects in the storage fallback, both reproduced: The quota fallback repointed reads at sessionStorage through module state, which a reload discards. The next page load probed localStorage, passed the one-byte probe, tried to promote the pair and was refused on the same quota, swallowed that, and read an empty localStorage: the identity sat one store over, unreadable, and the guest re-registered. Reads are now read-through: primary store first, sessionStorage second, promoting into the primary only when it will take the pair and leaving it where it fits when it will not. No module state has to remember which store won. The migration wrote the token before the profile, so a store that accepted the first write and refused the second left a token with no profile: x-guest-token was sent while the provider prompted to register, producing a second row with two live tokens. Every write is now profile-first and rolls back on failure, so a store holds the whole pair or none of it. --- .../guestIdentityStorage.persistence.test.ts | 106 +++++++++ frontend/src/utils/guestIdentityStorage.ts | 208 ++++++++++++------ 2 files changed, 251 insertions(+), 63 deletions(-) diff --git a/frontend/src/utils/__tests__/guestIdentityStorage.persistence.test.ts b/frontend/src/utils/__tests__/guestIdentityStorage.persistence.test.ts index 4017d945..c3f1bc0a 100644 --- a/frontend/src/utils/__tests__/guestIdentityStorage.persistence.test.ts +++ b/frontend/src/utils/__tests__/guestIdentityStorage.persistence.test.ts @@ -171,6 +171,112 @@ describe('guest identity persistence (#1265)', () => { __resetStorageResolutionForTests(); }); + describe('the sessionStorage fallback has to be readable after a reload', () => { + // Making a store that accepts the one-byte probe and nothing else, exactly + // like a nearly-full localStorage. + const installProbeOnlyLocalStorage = () => { + const real = window.localStorage; + const probeOnly = { + getItem: real.getItem.bind(real), + removeItem: real.removeItem.bind(real), + key: real.key.bind(real), + get length() { return real.length; }, + clear: real.clear.bind(real), + setItem: (k: string, _v: string) => { + if (k === '__picpeak_probe__') return; + throw new DOMException('QuotaExceededError'); + }, + } as unknown as Storage; + Object.defineProperty(window, 'localStorage', { value: probeOnly, configurable: true }); + __resetStorageResolutionForTests(); + return () => { + Object.defineProperty(window, 'localStorage', { value: real, configurable: true }); + __resetStorageResolutionForTests(); + }; + }; + + it('still finds the identity on the next page load', () => { + const restore = installProbeOnlyLocalStorage(); + storeGuestIdentity(SLUG, IDENTITY as never, TOKEN); + + // A reload discards module state: storage is re-resolved, localStorage + // passes the probe again, and the identity is sitting in sessionStorage. + __resetStorageResolutionForTests(); + + expect(getGuestToken(SLUG)).toBe(TOKEN); + expect(getGuestIdentity(SLUG)).toMatchObject({ id: 42 }); + restore(); + }); + + it('does not keep retrying the refused promotion on every read', () => { + const restore = installProbeOnlyLocalStorage(); + storeGuestIdentity(SLUG, IDENTITY as never, TOKEN); + __resetStorageResolutionForTests(); + + // First read: tries localStorage, is refused, remembers that. + getGuestToken(SLUG); + let writes = 0; + const store = window.localStorage; + const original = store.setItem; + store.setItem = ((k: string, v: string) => { writes++; return original.call(store, k, v); }) as never; + getGuestToken(SLUG); + getGuestIdentity(SLUG); + expect(writes).toBe(0); + restore(); + }); + }); + + describe('a store never holds a token without its profile', () => { + // localStorage that takes the token key and refuses the profile key — + // the partial acceptance a nearly-full store can produce. A token without + // a profile means x-guest-token is sent while the provider prompts to + // register: the second gallery_guests row, with two live tokens. + const installTokenOnlyLocalStorage = () => { + const real = window.localStorage; + const tokenOnly = { + getItem: real.getItem.bind(real), + removeItem: real.removeItem.bind(real), + key: real.key.bind(real), + get length() { return real.length; }, + clear: real.clear.bind(real), + setItem: (k: string, v: string) => { + if (k.startsWith('guest_identity_')) throw new DOMException('QuotaExceededError'); + real.setItem(k, v); + }, + } as unknown as Storage; + Object.defineProperty(window, 'localStorage', { value: tokenOnly, configurable: true }); + __resetStorageResolutionForTests(); + return () => { + Object.defineProperty(window, 'localStorage', { value: real, configurable: true }); + __resetStorageResolutionForTests(); + }; + }; + + it('on registration, falls back as a pair and leaves no token behind', () => { + const restore = installTokenOnlyLocalStorage(); + storeGuestIdentity(SLUG, IDENTITY as never, TOKEN); + + expect(window.localStorage.getItem(`guest_token_${SLUG}`)).toBeNull(); + expect(window.sessionStorage.getItem(`guest_token_${SLUG}`)).toBe(TOKEN); + expect(getGuestToken(SLUG)).toBe(TOKEN); + expect(getGuestIdentity(SLUG)).toMatchObject({ id: 42 }); + restore(); + }); + + it('on migration, leaves the legacy pair intact rather than moving half of it', () => { + const restore = installTokenOnlyLocalStorage(); + window.sessionStorage.setItem(`guest_token_${SLUG}`, TOKEN); + window.sessionStorage.setItem(`guest_identity_${SLUG}`, JSON.stringify(IDENTITY)); + + expect(getGuestToken(SLUG)).toBe(TOKEN); + expect(getGuestIdentity(SLUG)).toMatchObject({ id: 42 }); + // Nothing half-moved: the token did not land in localStorage on its own. + expect(window.localStorage.getItem(`guest_token_${SLUG}`)).toBeNull(); + expect(window.sessionStorage.getItem(`guest_token_${SLUG}`)).toBe(TOKEN); + restore(); + }); + }); + it('clears both stores, so a cleared identity cannot be migrated back', () => { // A guest who registered before the upgrade and again after it has a copy // in each store; "forget me" has to remove both. diff --git a/frontend/src/utils/guestIdentityStorage.ts b/frontend/src/utils/guestIdentityStorage.ts index bc8e3b59..876e6286 100644 --- a/frontend/src/utils/guestIdentityStorage.ts +++ b/frontend/src/utils/guestIdentityStorage.ts @@ -84,6 +84,7 @@ const getStorage = (): Storage | null => { /** Test seam — storage availability is resolved once per page load. */ export function __resetStorageResolutionForTests(): void { resolvedStorage = undefined; + promotionFailed.clear(); } /** @@ -115,87 +116,167 @@ function isExpired(token: string): boolean { } /** - * Move a pre-#1265 identity out of sessionStorage on first read. - * - * Without this, everyone with a gallery open at upgrade time is treated as a - * new guest the moment they reload — which is the exact duplicate-row bug this - * change exists to stop, fired once per in-flight guest. + * The secondary store: sessionStorage, whenever it is distinct from the store + * reads are resolved against. Two things can live there — a pre-#1265 + * identity the previous build wrote, and the quota fallback below. */ -function migrateFromSessionStorage(slug: string): void { - if (!isBrowser) return; - const target = getStorage(); - if (!target || target === window.sessionStorage) return; - let legacy: Storage; +function secondaryStore(primary: Storage): Storage | null { + if (!isBrowser) return null; try { - legacy = window.sessionStorage; + const candidate = window.sessionStorage; + return candidate && candidate !== primary ? candidate : null; } catch { - return; + return null; } +} + +/** + * Write the pair to one store, PROFILE FIRST. A store that is nearly full can + * accept the first write and reject the second, and "token stored, profile + * missing" is the one partial state that produces the duplicate-row bug this + * file exists to stop: the interceptor sends x-guest-token while the provider + * sees no identity, prompts, and registers a second guest. A profile without + * a token is inert. Anything half-written is removed again before reporting + * failure, so a store either holds the whole pair or none of it. + */ +function writePair(target: Storage, slug: string, identityRaw: string, token: string): boolean { const tokenKey = `${TOKEN_KEY_PREFIX}${slug}`; const identityKey = `${IDENTITY_KEY_PREFIX}${slug}`; try { - const legacyToken = legacy.getItem(tokenKey); - // Only migrate when the new store has nothing — a fresh registration in - // this tab must always win over a stale copy left in sessionStorage. - if (legacyToken && !target.getItem(tokenKey)) { - target.setItem(tokenKey, legacyToken); - const legacyIdentity = legacy.getItem(identityKey); - if (legacyIdentity) target.setItem(identityKey, legacyIdentity); - } - legacy.removeItem(tokenKey); - legacy.removeItem(identityKey); + target.setItem(identityKey, identityRaw); + target.setItem(tokenKey, token); + return true; } catch { - // Best-effort: a failed migration just means the guest re-registers. + try { + target.removeItem(tokenKey); + target.removeItem(identityKey); + } catch { + // Nothing more to do. + } + return false; } } +// Slugs whose pair could not be promoted into the primary store on this page +// load. getGuestToken() runs on every API request; once the primary store has +// refused the write there is no point retrying it per request. +const promotionFailed = new Set(); + +/** + * Read the pair, looking in the primary store first and the secondary one + * second. Finding it in the secondary store covers two cases that used to be + * one-way migrations and are now read-through: + * + * - a pre-#1265 identity the previous build left in sessionStorage. Without + * this everyone with a gallery open at upgrade time is treated as a new + * guest on reload — the exact duplicate-row bug, fired once per guest; + * - the quota fallback in storeGuestIdentity(). That used to repoint reads + * at sessionStorage through module state, which a reload discards: the + * next page load probed localStorage, passed, and read nothing while the + * identity sat unreadable one store over. Reads now find it wherever it + * demonstrably fits. + * + * A pair found in the secondary store is promoted into the primary one when + * that store will take it (MOVED, not copied — a stale copy would otherwise + * be migrated straight back after "forget me"), and left where it is when it + * will not. A pair already in the primary store always wins over the + * secondary copy: a fresh registration in this tab beats a stale leftover. + */ +function readPair(slug: string): { token: string | null; identityRaw: string | null } { + const primary = getStorage(); + if (!primary) return { token: null, identityRaw: null }; + const tokenKey = `${TOKEN_KEY_PREFIX}${slug}`; + const identityKey = `${IDENTITY_KEY_PREFIX}${slug}`; + + let token: string | null = null; + let identityRaw: string | null = null; + try { + token = primary.getItem(tokenKey); + identityRaw = primary.getItem(identityKey); + } catch { + return { token: null, identityRaw: null }; + } + + const secondary = secondaryStore(primary); + if (!secondary) return { token, identityRaw }; + + let secondaryToken: string | null = null; + let secondaryIdentity: string | null = null; + try { + secondaryToken = secondary.getItem(tokenKey); + secondaryIdentity = secondary.getItem(identityKey); + } catch { + return { token, identityRaw }; + } + + if (token) { + // Primary wins. Drop the leftover so it can never be promoted later. + if (secondaryToken || secondaryIdentity) { + try { + secondary.removeItem(tokenKey); + secondary.removeItem(identityKey); + } catch { + // Best-effort. + } + } + return { token, identityRaw }; + } + + if (!secondaryToken) return { token: null, identityRaw }; + + if (!promotionFailed.has(slug) && secondaryIdentity && writePair(primary, slug, secondaryIdentity, secondaryToken)) { + try { + secondary.removeItem(tokenKey); + secondary.removeItem(identityKey); + } catch { + // The copy stays behind; the primary now wins on every later read. + } + } else if (secondaryIdentity) { + promotionFailed.add(slug); + } + return { token: secondaryToken, identityRaw: secondaryIdentity }; +} + export function storeGuestIdentity(slug: string, identity: GuestIdentity, token: string): void { const storage = getStorage(); if (!storage || !slug) return; - const write = (target: Storage): boolean => { - try { - target.setItem(`${TOKEN_KEY_PREFIX}${slug}`, token); - target.setItem(`${IDENTITY_KEY_PREFIX}${slug}`, JSON.stringify(identity)); - return true; - } catch { - return false; - } - }; + const identityRaw = JSON.stringify(identity); - if (write(storage)) return; + if (writePair(storage, slug, identityRaw, token)) { + // A fresh registration supersedes anything the secondary store holds. + const secondary = secondaryStore(storage); + if (secondary) { + try { + secondary.removeItem(`${TOKEN_KEY_PREFIX}${slug}`); + secondary.removeItem(`${IDENTITY_KEY_PREFIX}${slug}`); + } catch { + // Best-effort. + } + } + return; + } // The probe in getStorage() only proves a one-byte write fits; a JWT plus a // profile is far larger, so a nearly-full store can pass the probe and still // reject the real write. Retry in sessionStorage rather than leaving the // context believing it is signed in with nothing persisted — that state // 401s immediately and re-registers (another duplicate row) on reload. - if (isBrowser) { - try { - if (window.sessionStorage && window.sessionStorage !== storage) { - if (write(window.sessionStorage)) { - // Repoint reads at the store that actually accepted the write. - // Without this the identity is written to sessionStorage while - // getGuestToken() keeps reading localStorage, so x-guest-token is - // never sent and the identity is lost on reload — the fallback - // would look like it worked while achieving nothing. - resolvedStorage = window.sessionStorage; - } - } - } catch { - // No store will take it. The identity lasts as long as this page does, - // which is strictly better than rejecting a registration the server has - // already completed. - } + // readPair() looks there on every read, including after a reload, so no + // module state has to remember which store took the write. + const secondary = secondaryStore(storage); + if (secondary) { + promotionFailed.add(slug); + writePair(secondary, slug, identityRaw, token); } + // Otherwise no store will take it. The identity lasts as long as this page + // does, which is strictly better than rejecting a registration the server + // has already completed. } export function getGuestToken(slug?: string | null): string | null { - const storage = getStorage(); - if (!storage) return null; const resolvedSlug = slug || extractSlugFromLocation(); if (!resolvedSlug) return null; - migrateFromSessionStorage(resolvedSlug); - const token = storage.getItem(`${TOKEN_KEY_PREFIX}${resolvedSlug}`); + const { token } = readPair(resolvedSlug); if (token && isExpired(token)) { clearGuestIdentity(resolvedSlug); return null; @@ -204,18 +285,18 @@ export function getGuestToken(slug?: string | null): string | null { } export function getGuestIdentity(slug?: string | null): GuestIdentity | null { - const storage = getStorage(); - if (!storage) return null; const resolvedSlug = slug || extractSlugFromLocation(); if (!resolvedSlug) return null; - migrateFromSessionStorage(resolvedSlug); - // An identity whose token has expired must not be presented as signed in — - // clearGuestIdentity() has already run inside getGuestToken() in that case. - if (!getGuestToken(resolvedSlug)) return null; - const raw = storage.getItem(`${IDENTITY_KEY_PREFIX}${resolvedSlug}`); - if (!raw) return null; + const { token, identityRaw } = readPair(resolvedSlug); + // An identity whose token has expired must not be presented as signed in. + if (!token) return null; + if (isExpired(token)) { + clearGuestIdentity(resolvedSlug); + return null; + } + if (!identityRaw) return null; try { - return JSON.parse(raw) as GuestIdentity; + return JSON.parse(identityRaw) as GuestIdentity; } catch { return null; } @@ -223,6 +304,7 @@ export function getGuestIdentity(slug?: string | null): GuestIdentity | null { export function clearGuestIdentity(slug: string): void { if (!slug) return; + promotionFailed.delete(slug); // Clear BOTH stores: a copy left behind in sessionStorage would be migrated // straight back on the next read, silently undoing "forget me". const stores: Storage[] = [];