From 51db1e09e901d8e7494e0182cad6ac6b9b6209d8 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Wed, 2 Sep 2026 07:44:09 +0200 Subject: [PATCH] fix(guests): expire stale tokens, sync tabs, survive unwritable storage Codex review round 1 on #1268. All three findings are consequences of the storage move itself. Expired tokens now read as absent. GUEST_TOKEN_TTL is 30 days and sessionStorage almost never survived that long, so 'stored but expired' was unreachable before; persisting the token makes it routine. Nothing else clears it -- the 401 handler in config/api.ts only drops gallery_event_ -- so the visitor was shown as signed in while every like 401'd, and ensureIdentity() short-circuited so recovery was never offered. The signature is still the server's business; an unparseable token is left alone. Tabs now stay in step. localStorage is shared where sessionStorage gave each tab its own copy, so 'Not you?' or a registration in one tab silently changed the token every other tab sends while they still displayed the old name -- their likes would land on the new guest, the exact misattribution this branch set out to stop. A storage listener rehydrates the others. Storage is probed for writability, not just readability. A store that reads but throws on setItem (quota, private mode) sailed past the read-only guard, and storeGuestIdentity threw after the server had created the guest: failed registration, retry, duplicate row. Writes are also wrapped so a storage failure degrades to a per-session identity instead of rejecting registration. --- .../src/contexts/GuestIdentityContext.tsx | 21 ++++ .../guestIdentityStorage.persistence.test.ts | 65 ++++++++++- frontend/src/utils/guestIdentityStorage.ts | 102 ++++++++++++++++-- 3 files changed, 176 insertions(+), 12 deletions(-) diff --git a/frontend/src/contexts/GuestIdentityContext.tsx b/frontend/src/contexts/GuestIdentityContext.tsx index 1ba5fb71..cb4d9552 100644 --- a/frontend/src/contexts/GuestIdentityContext.tsx +++ b/frontend/src/contexts/GuestIdentityContext.tsx @@ -67,6 +67,27 @@ export const GuestIdentityProvider: React.FC = ({ setIdentity(getGuestIdentity(slug)); }, [slug]); + // Keep tabs in step. The identity now lives in localStorage, which is shared + // across tabs — where sessionStorage gave each tab its own copy. So "Not + // you?" or a fresh registration in one tab silently changes the token the + // axios interceptor sends from every other tab, while those tabs still show + // the old name. Their likes would then be recorded against the new guest: + // the same misattribution this change set out to stop. + // + // `storage` fires only in the OTHER tabs, which is exactly the audience that + // needs to catch up. A null key means the whole store was cleared. + useEffect(() => { + if (typeof window === 'undefined') return; + const onStorage = (event: StorageEvent) => { + if (event.key && event.key !== `guest_token_${slug}` && event.key !== `guest_identity_${slug}`) { + return; + } + setIdentity(getGuestIdentity(slug)); + }; + window.addEventListener('storage', onStorage); + return () => window.removeEventListener('storage', onStorage); + }, [slug]); + // When an invite token is present on the URL (?invite=xxx), redeem it once // on mount. The server returns a guest token we can persist. useEffect(() => { diff --git a/frontend/src/utils/__tests__/guestIdentityStorage.persistence.test.ts b/frontend/src/utils/__tests__/guestIdentityStorage.persistence.test.ts index edee73b1..88c25719 100644 --- a/frontend/src/utils/__tests__/guestIdentityStorage.persistence.test.ts +++ b/frontend/src/utils/__tests__/guestIdentityStorage.persistence.test.ts @@ -13,6 +13,7 @@ import { beforeEach, describe, expect, it } from 'vitest'; import { + __resetStorageResolutionForTests, clearGuestIdentity, getGuestIdentity, getGuestToken, @@ -21,7 +22,15 @@ import { const SLUG = 'wedding-summer-2026'; const IDENTITY = { id: 42, name: 'Tina', email: 'tina@example.com', identifier: 'abc-123' }; -const TOKEN = 'header.payload.signature'; + +/** A JWT-shaped token whose `exp` is `secondsFromNow` away. Signature is irrelevant. */ +function tokenExpiringIn(secondsFromNow: number): string { + const claims = { exp: Math.floor(Date.now() / 1000) + secondsFromNow }; + const payload = btoa(JSON.stringify(claims)).replace(/\+/g, '-').replace(/\//g, '_'); + return `header.${payload}.signature`; +} + +const TOKEN = tokenExpiringIn(30 * 24 * 60 * 60); /** A tab close clears sessionStorage and leaves localStorage alone. */ function closeTab(): void { @@ -32,6 +41,7 @@ describe('guest identity persistence (#1265)', () => { beforeEach(() => { window.localStorage.clear(); window.sessionStorage.clear(); + __resetStorageResolutionForTests(); }); it('survives a tab close, so a returning guest is still recognised', () => { @@ -81,6 +91,59 @@ describe('guest identity persistence (#1265)', () => { }); }); + describe('an expired token must not look like a signed-in guest', () => { + // Persisting the token makes "stored but expired" reachable for the first + // time — sessionStorage almost never survived the 30-day TTL. Nothing else + // clears it: the 401 handler in config/api.ts only drops + // `gallery_event_`, so without this the visitor is shown as signed + // in while every like silently 401s. + it('drops an identity whose token has expired', () => { + storeGuestIdentity(SLUG, IDENTITY as never, tokenExpiringIn(-60)); + + expect(getGuestToken(SLUG)).toBeNull(); + expect(getGuestIdentity(SLUG)).toBeNull(); + // Purged, not just hidden, so the next read does no work. + expect(window.localStorage.getItem(`guest_token_${SLUG}`)).toBeNull(); + }); + + it('keeps a token that is still valid', () => { + storeGuestIdentity(SLUG, IDENTITY as never, tokenExpiringIn(3600)); + + expect(getGuestToken(SLUG)).not.toBeNull(); + expect(getGuestIdentity(SLUG)).toMatchObject({ id: 42 }); + }); + + it('leaves an unparseable token alone, so the server stays the authority', () => { + storeGuestIdentity(SLUG, IDENTITY as never, 'not-a-jwt'); + + expect(getGuestToken(SLUG)).toBe('not-a-jwt'); + }); + }); + + it('does not reject registration when the store refuses writes', () => { + // localStorage that reads fine but throws on write (quota exhausted). + // Throwing here would surface as a failed registration *after* the server + // created the guest — the visitor retries and gets a duplicate row. + const real = window.localStorage; + const throwing = { + 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: () => { throw new DOMException('QuotaExceededError'); }, + } as unknown as Storage; + Object.defineProperty(window, 'localStorage', { value: throwing, configurable: true }); + __resetStorageResolutionForTests(); + + expect(() => storeGuestIdentity(SLUG, IDENTITY as never, TOKEN)).not.toThrow(); + // Fell back to sessionStorage rather than losing the identity entirely. + expect(window.sessionStorage.getItem(`guest_token_${SLUG}`)).toBe(TOKEN); + + Object.defineProperty(window, 'localStorage', { value: real, configurable: true }); + __resetStorageResolutionForTests(); + }); + 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 4ed98a28..78008d2c 100644 --- a/frontend/src/utils/guestIdentityStorage.ts +++ b/frontend/src/utils/guestIdentityStorage.ts @@ -30,22 +30,87 @@ const IDENTITY_KEY_PREFIX = 'guest_identity_'; const isBrowser = typeof window !== 'undefined'; +/** + * A store is only usable if it can be WRITTEN, not merely read. + * + * Guarding the property access alone is not enough: there are browsers and + * states (quota exhausted, Safari private mode historically) where + * `window.localStorage` resolves fine but `setItem` throws. That would sail + * past a read-only guard, and then storeGuestIdentity() would throw *after* + * the server had already created the guest — the registration would report + * failure, the visitor would try again, and the retry would insert the second + * gallery_guests row this whole change exists to prevent. + */ +function isUsable(storage: Storage): boolean { + const probe = '__picpeak_probe__'; + try { + storage.setItem(probe, '1'); + storage.removeItem(probe); + return true; + } catch { + return false; + } +} + +// Resolved once. getGuestToken() runs on every API request through the axios +// interceptor, and probing a write each time would be wasteful. +let resolvedStorage: Storage | null | undefined; + const getStorage = (): Storage | null => { if (!isBrowser) return null; - try { - return window.localStorage; - } catch { - // Safari in private mode, or storage blocked by policy. Fall back to - // sessionStorage rather than dropping identity entirely — that is the old - // behaviour, which is degraded but still works within a single tab. + if (resolvedStorage !== undefined) return resolvedStorage; + + for (const pick of [() => window.localStorage, () => window.sessionStorage]) { + let candidate: Storage; try { - return window.sessionStorage; + candidate = pick(); } catch { - return null; + continue; + } + if (candidate && isUsable(candidate)) { + resolvedStorage = candidate; + return resolvedStorage; } } + // sessionStorage is the degraded fallback: identity lasts one tab, which is + // the pre-#1265 behaviour rather than no identity at all. + resolvedStorage = null; + return resolvedStorage; }; +/** Test seam — storage availability is resolved once per page load. */ +export function __resetStorageResolutionForTests(): void { + resolvedStorage = undefined; +} + +/** + * Treat a token past its `exp` as absent. + * + * GUEST_TOKEN_TTL is 30 days, and until now sessionStorage almost never + * survived long enough to reach it. Persisting the token makes "stored but + * expired" a reachable state, and nothing clears it: the 401 handler in + * config/api.ts only drops `gallery_event_`. Without this the visitor is + * shown as signed in while every like silently 401s, and ensureIdentity() + * short-circuits so they are never offered recovery. + * + * The signature is not verified here — that is the server's job. This only + * reads the expiry so the client stops presenting an identity the backend has + * already stopped honouring. A token we cannot parse is left alone rather than + * discarded, so the server stays the authority on anything ambiguous. + */ +function isExpired(token: string): boolean { + const payload = token.split('.')[1]; + if (!payload) return false; + try { + const base64 = payload.replace(/-/g, '+').replace(/_/g, '/'); + const claims = JSON.parse(atob(base64)) as { exp?: number }; + if (typeof claims.exp !== 'number') return false; + return claims.exp * 1000 <= Date.now(); + } catch { + return false; + } +} + /** * Move a pre-#1265 identity out of sessionStorage on first read. * @@ -84,8 +149,15 @@ function migrateFromSessionStorage(slug: string): void { export function storeGuestIdentity(slug: string, identity: GuestIdentity, token: string): void { const storage = getStorage(); if (!storage || !slug) return; - storage.setItem(`${TOKEN_KEY_PREFIX}${slug}`, token); - storage.setItem(`${IDENTITY_KEY_PREFIX}${slug}`, JSON.stringify(identity)); + try { + storage.setItem(`${TOKEN_KEY_PREFIX}${slug}`, token); + storage.setItem(`${IDENTITY_KEY_PREFIX}${slug}`, JSON.stringify(identity)); + } catch { + // Never let a storage failure reject registration: the guest row already + // exists server-side by this point, so throwing here would show the + // visitor an error and make them register again, creating a duplicate. + // Losing persistence degrades them to a per-session identity instead. + } } export function getGuestToken(slug?: string | null): string | null { @@ -94,7 +166,12 @@ export function getGuestToken(slug?: string | null): string | null { const resolvedSlug = slug || extractSlugFromLocation(); if (!resolvedSlug) return null; migrateFromSessionStorage(resolvedSlug); - return storage.getItem(`${TOKEN_KEY_PREFIX}${resolvedSlug}`); + const token = storage.getItem(`${TOKEN_KEY_PREFIX}${resolvedSlug}`); + if (token && isExpired(token)) { + clearGuestIdentity(resolvedSlug); + return null; + } + return token; } export function getGuestIdentity(slug?: string | null): GuestIdentity | null { @@ -103,6 +180,9 @@ export function getGuestIdentity(slug?: string | null): GuestIdentity | 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; try {