diff --git a/.changeset/sweep-store-per-key-try-5763.md b/.changeset/sweep-store-per-key-try-5763.md new file mode 100644 index 0000000000..615b71b68f --- /dev/null +++ b/.changeset/sweep-store-per-key-try-5763.md @@ -0,0 +1,39 @@ +--- +'@object-ui/auth': patch +--- + +`sweepStore()` — the walk that drops the previous user's `localStorage`/`sessionStorage` +state on a change of session user (objectui#5664 part 3) — now costs one key when a +single `removeItem` throws, instead of aborting the rest of the sweep (objectui#5763). + +The `try` wrapped the WHOLE loop, not each removal. A `removeItem` that threw on key +`n` aborted the walk, so keys `n+1..end` were never swept, and the failure was +swallowed — `purgePreviousUserClientState()` returned normally and `SessionUserScope.adopt` +believed the sign-in purge had completed. This is an ALLOWLIST sweep precisely so the +next un-namespaced key — one nobody has written yet — cannot re-open the cross-user +pollution class #5664 fixed; a partial sweep is a partial allowlist, and which keys +survived depended on `Object.keys` iteration order rather than on anything bounded. The +previous user's org id, recents, favourites, or a `sessionStorage` metadata seed (their +permission-filtered app list, a cross-principal disclosure per objectui#5198) could all +land on the wrong side of the abort. + +`Object.keys(store)` — the reason a guard exists here at all — stays guarded on its +own; only the per-key guard is new, so one uncooperative key now costs exactly that key. + +Unlike `ActiveOrganizationStorage.clear()` (objectui#5731), a failed removal here is +NOT verified by read-back and NOT quarantined: `clear()` owns every future read of its +one key through `ActiveOrganizationStorage.get()`, so a "still readable" verdict and a +quarantine are what keep a failed `clear()` from handing the value straight back. +`sweepStore` walks keys it does not own reads for — another package's recents cache, a +metadata seed — so there is no `get()` here to guard and nothing to quarantine; adding +read-back verification for keys this function does not otherwise touch would be a +general storage-error-handling refactor of the module, which this card is scoped away +from. What IS mirrored is the reporting channel: a key whose `removeItem` throws is +named in a `console.warn`, the same channel `clear()` uses, so a partial sweep is +discoverable instead of silent. The caller (`SessionUserScope.adopt`, on the sign-in +path, inside an `AuthProvider` effect) still cannot act on the failure and must not +throw either. + +A working `localStorage`/`sessionStorage` behaves exactly as before: every +non-device-scoped key is removed, nothing is reported, and the device-scoped allowlist +(`auth-session-token`, `auth-session-user-id`, `vite-ui-theme`) is unaffected. diff --git a/packages/auth/src/ActiveOrganizationStorage.ts b/packages/auth/src/ActiveOrganizationStorage.ts index 393a0cc49f..463bf73ec6 100644 --- a/packages/auth/src/ActiveOrganizationStorage.ts +++ b/packages/auth/src/ActiveOrganizationStorage.ts @@ -183,16 +183,57 @@ function removePersisted(key: string): boolean { * Snapshot the keys first (`Object.keys`) — removing entries during a live * index walk shifts the ones behind it and skips half of them. Same idiom as * `AuthProvider`'s sign-out purge loop. + * + * ## The `try` guards the SNAPSHOT, not the walk (objectui#5763) + * + * `Object.keys(store)` can itself throw — partitioned iframes, some privacy + * modes — and that is the reason a guard exists here at all, so it stays. + * What it must NOT do is wrap the loop: a `removeItem` that throws on key `n` + * would abort the walk, leaving keys `n+1..end` unswept, with the failure + * swallowed so the caller believes the purge completed. This is an ALLOWLIST + * sweep (see the module doc, part 3) precisely so the next un-namespaced key + * cannot re-open the cross-user pollution class — a partial sweep is a + * partial allowlist. So each `removeItem` gets its own `try`: one + * uncooperative key costs exactly that key. + * + * ## Reported, not quarantined — unlike {@link ActiveOrganizationStorage.clear} + * + * `clear()` (objectui#5731) judges a removal by READ-BACK and quarantines a + * key that fails, because it owns every future read of that one key through + * {@link ActiveOrganizationStorage.get} — the quarantine is what keeps a + * failed `clear()` from handing the value straight back. `sweepStore` walks + * keys it does not own reads for (another package's recents cache, a + * metadata seed) — there is no `get()` here to guard, so there is nothing to + * quarantine, and adding a read-back verdict for keys this function does not + * otherwise touch would be exactly the "general storage-error-handling + * refactor" the card scopes this fix away from. What IS mirrored is the + * reporting channel: a key whose `removeItem` throws is named in a + * `console.warn`, same as `clear()`, so a partial sweep is discoverable + * instead of silent — the caller (`SessionUserScope.adopt`, on the sign-in + * path) still cannot act on it and must not throw either. */ function sweepStore(store: Storage | undefined): void { if (!store) return; + let keys: string[]; try { - for (const key of Object.keys(store)) { - if (DEVICE_SCOPED_KEYS.has(key)) continue; + keys = Object.keys(store); + } catch { + return; /* storage unavailable */ + } + const unswept: string[] = []; + for (const key of keys) { + if (DEVICE_SCOPED_KEYS.has(key)) continue; + try { store.removeItem(key); + } catch { + unswept.push(key); } - } catch { - /* storage unavailable */ + } + if (unswept.length > 0) { + console.warn( + `[purgePreviousUserClientState] could not remove ${unswept.length} key(s) from storage: ` + + `${unswept.join(', ')}. The previous user's state may still be readable under these keys.`, + ); } } diff --git a/packages/auth/src/__tests__/sessionUserChangePurge-5664.test.tsx b/packages/auth/src/__tests__/sessionUserChangePurge-5664.test.tsx index ed95027417..69ae10f93c 100644 --- a/packages/auth/src/__tests__/sessionUserChangePurge-5664.test.tsx +++ b/packages/auth/src/__tests__/sessionUserChangePurge-5664.test.tsx @@ -304,6 +304,78 @@ describe('a change of session user drops the previous user’s state wholesale ( vi.unstubAllGlobals(); }); + + // ------------------------------------------------------------------------- + // objectui#5763 — a throwing `removeItem` must cost exactly that key + // ------------------------------------------------------------------------- + + it('sweeps every other non-device-scoped key when one removeItem throws (#5763)', () => { + SessionUserScope.adopt(USER_A); + ActiveOrganizationStorage.set('org_a'); + // Keys on BOTH SIDES of the poisoned one in insertion order, so a green + // run proves the walk continues past the throw rather than stopping at + // the first non-poisoned key it happens to reach. + localStorage.setItem(`objectui-recent-items:u:${USER_A}`, '[{"id":"acct_1"}]'); + const POISON_KEY = 'objectui-nav-order-crm'; + localStorage.setItem(POISON_KEY, '["accounts"]'); + localStorage.setItem('objectui-favorites', '["acct_2"]'); + sessionStorage.setItem('objectui:metadata:app:org_a:@anon', '[{"name":"crm"}]'); + // Device-scoped, written for the INCOMING user the same way the passing + // sibling case above does it. + TokenStorage.set('tok-bob'); + localStorage.setItem('vite-ui-theme', 'dark'); + + const realRemoveItem = localStorage.removeItem.bind(localStorage); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const removeItemSpy = vi.spyOn(localStorage, 'removeItem').mockImplementation((key: string) => { + if (key === POISON_KEY) throw new Error('simulated removeItem failure'); + realRemoveItem(key); + }); + + try { + expect(() => SessionUserScope.adopt(USER_B)).not.toThrow(); + + // The poisoned key is the residue the card is about — it survives. + expect(localStorage.getItem(POISON_KEY)).toBe('["accounts"]'); + + // Everything else non-device-scoped is still swept, in BOTH stores — + // this is the pin: one uncooperative key costs one key, not the rest. + expect(localStorage.getItem(scopedOrgKey(USER_A))).toBeNull(); + expect(localStorage.getItem(`objectui-recent-items:u:${USER_A}`)).toBeNull(); + expect(localStorage.getItem('objectui-favorites')).toBeNull(); + expect(sessionStorage.getItem('objectui:metadata:app:org_a:@anon')).toBeNull(); + + // The device-scoped allowlist is still respected around the failure. + expect(TokenStorage.get()).toBe('tok-bob'); + expect(localStorage.getItem('vite-ui-theme')).toBe('dark'); + expect(localStorage.getItem('auth-session-user-id')).toBe(USER_B); + + // Discoverable, not silent — the failure is named, not swallowed whole. + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0]?.[0]).toContain(POISON_KEY); + } finally { + // Explicit, not left to `afterEach`'s `restoreAllMocks()`: a spy + // installed on a jsdom `Storage` instance (rather than a plain object) + // has been observed to survive `restoreAllMocks()` across tests — the + // NEXT test in this file measured the mock still active on entry. Left + // to the shared teardown, this poisoned `removeItem` for every later + // case in the file that happens to touch the same key name. + removeItemSpy.mockRestore(); + } + }); + + it('reports nothing when every removal sticks', () => { + // Control on the case above: the warning is a measurement of an actual + // failure, not a constant emitted on every purge. + SessionUserScope.adopt(USER_A); + localStorage.setItem('objectui-nav-order-crm', '["accounts"]'); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + SessionUserScope.adopt(USER_B); + + expect(localStorage.getItem('objectui-nav-order-crm')).toBeNull(); + expect(warnSpy).not.toHaveBeenCalled(); + }); }); // ---------------------------------------------------------------------------