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_<slug> -- 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.
This commit is contained in:
@@ -67,6 +67,27 @@ export const GuestIdentityProvider: React.FC<GuestIdentityProviderProps> = ({
|
|||||||
setIdentity(getGuestIdentity(slug));
|
setIdentity(getGuestIdentity(slug));
|
||||||
}, [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
|
// 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.
|
// on mount. The server returns a guest token we can persist.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
|||||||
@@ -13,6 +13,7 @@
|
|||||||
|
|
||||||
import { beforeEach, describe, expect, it } from 'vitest';
|
import { beforeEach, describe, expect, it } from 'vitest';
|
||||||
import {
|
import {
|
||||||
|
__resetStorageResolutionForTests,
|
||||||
clearGuestIdentity,
|
clearGuestIdentity,
|
||||||
getGuestIdentity,
|
getGuestIdentity,
|
||||||
getGuestToken,
|
getGuestToken,
|
||||||
@@ -21,7 +22,15 @@ import {
|
|||||||
|
|
||||||
const SLUG = 'wedding-summer-2026';
|
const SLUG = 'wedding-summer-2026';
|
||||||
const IDENTITY = { id: 42, name: 'Tina', email: '[email protected]', identifier: 'abc-123' };
|
const IDENTITY = { id: 42, name: 'Tina', email: '[email protected]', 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. */
|
/** A tab close clears sessionStorage and leaves localStorage alone. */
|
||||||
function closeTab(): void {
|
function closeTab(): void {
|
||||||
@@ -32,6 +41,7 @@ describe('guest identity persistence (#1265)', () => {
|
|||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
window.localStorage.clear();
|
window.localStorage.clear();
|
||||||
window.sessionStorage.clear();
|
window.sessionStorage.clear();
|
||||||
|
__resetStorageResolutionForTests();
|
||||||
});
|
});
|
||||||
|
|
||||||
it('survives a tab close, so a returning guest is still recognised', () => {
|
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_<slug>`, 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', () => {
|
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
|
// A guest who registered before the upgrade and again after it has a copy
|
||||||
// in each store; "forget me" has to remove both.
|
// in each store; "forget me" has to remove both.
|
||||||
|
|||||||
@@ -30,22 +30,87 @@ const IDENTITY_KEY_PREFIX = 'guest_identity_';
|
|||||||
|
|
||||||
const isBrowser = typeof window !== 'undefined';
|
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 => {
|
const getStorage = (): Storage | null => {
|
||||||
if (!isBrowser) return null;
|
if (!isBrowser) return null;
|
||||||
|
if (resolvedStorage !== undefined) return resolvedStorage;
|
||||||
|
|
||||||
|
for (const pick of [() => window.localStorage, () => window.sessionStorage]) {
|
||||||
|
let candidate: Storage;
|
||||||
try {
|
try {
|
||||||
return window.localStorage;
|
candidate = pick();
|
||||||
} catch {
|
} catch {
|
||||||
// Safari in private mode, or storage blocked by policy. Fall back to
|
continue;
|
||||||
// sessionStorage rather than dropping identity entirely — that is the old
|
}
|
||||||
// behaviour, which is degraded but still works within a single tab.
|
if (candidate && isUsable(candidate)) {
|
||||||
try {
|
resolvedStorage = candidate;
|
||||||
return window.sessionStorage;
|
return resolvedStorage;
|
||||||
} catch {
|
|
||||||
return null;
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// 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_<slug>`. 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.
|
* 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 {
|
export function storeGuestIdentity(slug: string, identity: GuestIdentity, token: string): void {
|
||||||
const storage = getStorage();
|
const storage = getStorage();
|
||||||
if (!storage || !slug) return;
|
if (!storage || !slug) return;
|
||||||
|
try {
|
||||||
storage.setItem(`${TOKEN_KEY_PREFIX}${slug}`, token);
|
storage.setItem(`${TOKEN_KEY_PREFIX}${slug}`, token);
|
||||||
storage.setItem(`${IDENTITY_KEY_PREFIX}${slug}`, JSON.stringify(identity));
|
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 {
|
export function getGuestToken(slug?: string | null): string | null {
|
||||||
@@ -94,7 +166,12 @@ export function getGuestToken(slug?: string | null): string | null {
|
|||||||
const resolvedSlug = slug || extractSlugFromLocation();
|
const resolvedSlug = slug || extractSlugFromLocation();
|
||||||
if (!resolvedSlug) return null;
|
if (!resolvedSlug) return null;
|
||||||
migrateFromSessionStorage(resolvedSlug);
|
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 {
|
export function getGuestIdentity(slug?: string | null): GuestIdentity | null {
|
||||||
@@ -103,6 +180,9 @@ export function getGuestIdentity(slug?: string | null): GuestIdentity | null {
|
|||||||
const resolvedSlug = slug || extractSlugFromLocation();
|
const resolvedSlug = slug || extractSlugFromLocation();
|
||||||
if (!resolvedSlug) return null;
|
if (!resolvedSlug) return null;
|
||||||
migrateFromSessionStorage(resolvedSlug);
|
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}`);
|
const raw = storage.getItem(`${IDENTITY_KEY_PREFIX}${resolvedSlug}`);
|
||||||
if (!raw) return null;
|
if (!raw) return null;
|
||||||
try {
|
try {
|
||||||
|
|||||||
Reference in New Issue
Block a user