diff --git a/.changeset/signout-purge-per-key-try-5777.md b/.changeset/signout-purge-per-key-try-5777.md new file mode 100644 index 0000000000..8b5252c493 --- /dev/null +++ b/.changeset/signout-purge-per-key-try-5777.md @@ -0,0 +1,31 @@ +--- +'@object-ui/auth': patch +--- + +`purgeSignedOutClientCaches()` — the sweep that drops the signed-out user's +`objectui:metadata:*` seed cache on sign-out (objectui#5198) — now costs one key when a +single `removeItem` throws, instead of aborting the rest of the sweep (objectui#5777). + +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 — +`AuthProvider`'s `signOut` believed the purge had completed. The entries this sweeps are +the previous principal's org-scoped, PERMISSION-FILTERED app list — objectui#5198 +classifies a surviving entry as a cross-principal disclosure on a shared browser, not +mere staleness — so a partial sweep here is the sharper half of the same defect class +objectui#5763 fixed on the sign-in path (`sweepStore` in `ActiveOrganizationStorage.ts`). + +`Object.keys(sessionStorage)` — the reason a guard exists here at all — stays guarded on +its own; only the per-key guard is new. Same as `sweepStore`, a failed removal here is +not verified by read-back and not quarantined the way `ActiveOrganizationStorage.clear()` +(objectui#5731) quarantines a key: this function does not own reads for the metadata +seed cache (`MetadataProvider` in `@object-ui/app-shell` does), so there is no `get()` to +guard and nothing to quarantine — adding read-back verification would be a general +storage-error-handling refactor of the module, out of this card's scope. What is +mirrored is the reporting channel: a key whose `removeItem` throws is named in a +`console.warn`, the same channel `sweepStore` and `clear()` use, so a partial sweep is +discoverable instead of silent. + +Adds a partial-failure test: a `sessionStorage` whose `removeItem` throws on one metadata +key, asserting every other metadata key on both sides of it is still swept and unrelated +non-matching keys are untouched, plus a control that the warning fires only on an actual +failure. diff --git a/packages/auth/src/AuthProvider.tsx b/packages/auth/src/AuthProvider.tsx index 62af48f648..ab853cab61 100644 --- a/packages/auth/src/AuthProvider.tsx +++ b/packages/auth/src/AuthProvider.tsx @@ -56,20 +56,61 @@ const METADATA_SEED_CACHE_PREFIX = 'objectui:metadata:'; * blob that escapes this purge (a tab open across the upgrade, a session ended * by expiry rather than by this call) is unreadable rather than merely * undeleted. The two halves are deliberately independent. + * + * ## The `try` guards the SNAPSHOT, not the walk (objectui#5777) + * + * `Object.keys(sessionStorage)` 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 every still-unvisited MATCHING key + * unswept, with the failure swallowed so the caller believes the sweep + * completed. The keys this purges are a cross-principal disclosure risk on a + * shared browser (objectui#5198), not ordinary staleness — a partial sweep + * leaves an arbitrary subset of the previous principal's permission-filtered + * app list readable to whoever signs in next in this tab. So each + * `removeItem` gets its own `try`: one uncooperative key costs exactly that + * key. Same defect class and same remedy as `sweepStore` in + * `ActiveOrganizationStorage.ts` (objectui#5763) — different file and + * different caller (sign-out, not sign-in) is why it is fixed here rather + * than there. + * + * ## Reported, not quarantined — same reason as `sweepStore` + * + * This function walks keys it does not own reads for: `MetadataProvider` + * (`@object-ui/app-shell`) is the reader of the seed cache, not this + * provider — there is no local `get()` here to guard, so there is nothing to + * quarantine the way `ActiveOrganizationStorage.clear()` quarantines a key + * (objectui#5731). What IS mirrored is the reporting channel: a key whose + * `removeItem` throws is named in a `console.warn`, so a partial sweep is + * discoverable instead of silent. The caller (`signOut`, above) cannot act on + * it either way — the session is already ending — so, like `sweepStore` on + * the sign-in path, this must not throw. */ function purgeSignedOutClientCaches(): void { if (typeof sessionStorage !== 'undefined') { + let keys: string[]; try { // 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 the `MarketplacePackagePage` purge loop. - for (const key of Object.keys(sessionStorage)) { - if (key.startsWith(METADATA_SEED_CACHE_PREFIX)) { - sessionStorage.removeItem(key); - } - } + keys = Object.keys(sessionStorage); } catch { - /* storage unavailable */ + keys = []; /* storage unavailable */ + } + const unswept: string[] = []; + for (const key of keys) { + if (!key.startsWith(METADATA_SEED_CACHE_PREFIX)) continue; + try { + sessionStorage.removeItem(key); + } catch { + unswept.push(key); + } + } + if (unswept.length > 0) { + console.warn( + `[purgeSignedOutClientCaches] could not remove ${unswept.length} key(s) from sessionStorage: ` + + `${unswept.join(', ')}. The signed-out user's metadata seed cache may still be readable under these keys.`, + ); } } ActiveOrganizationStorage.clear(); diff --git a/packages/auth/src/__tests__/signOut-client-cache-purge-5198.test.tsx b/packages/auth/src/__tests__/signOut-client-cache-purge-5198.test.tsx index 3ef61afb11..d1ec6f1d9e 100644 --- a/packages/auth/src/__tests__/signOut-client-cache-purge-5198.test.tsx +++ b/packages/auth/src/__tests__/signOut-client-cache-purge-5198.test.tsx @@ -197,6 +197,80 @@ describe('signOut purges the signed-out principal’s client caches (#5198)', () expect(observed[observed.length - 1]).toEqual([]); }); + // --------------------------------------------------------------------- + // objectui#5777 — a throwing `removeItem` must cost exactly that key + // --------------------------------------------------------------------- + + it('sweeps every other metadata key when one removeItem throws (#5777)', async () => { + const client = createMockClient(); + await renderSignedIn(client); + + // 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. + sessionStorage.setItem('objectui:metadata:app:org_a:abc', JSON.stringify([{ name: 'crm' }])); + const POISON_KEY = 'objectui:metadata:nav:org_a:abc'; + sessionStorage.setItem(POISON_KEY, JSON.stringify(['accounts'])); + sessionStorage.setItem('objectui:metadata:object:org_a:abc', JSON.stringify([{ name: 'account' }])); + // Non-matching key — never a removal candidate either way. + sessionStorage.setItem('objectui:sidebar:collapsed', 'true'); + ActiveOrganizationStorage.set('org_a'); + + const realRemoveItem = sessionStorage.removeItem.bind(sessionStorage); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const removeItemSpy = vi + .spyOn(sessionStorage, 'removeItem') + .mockImplementation((key: string) => { + if (key === POISON_KEY) throw new Error('simulated removeItem failure'); + realRemoveItem(key); + }); + + try { + await act(async () => { + await authRef.current!.signOut(); + }); + + // The poisoned key is the residue the card is about — it survives. + expect(sessionStorage.getItem(POISON_KEY)).toBe(JSON.stringify(['accounts'])); + + // Everything else matching the prefix is still swept — this is the + // pin: one uncooperative key costs one key, not the rest of the sweep. + expect(sessionStorage.getItem('objectui:metadata:app:org_a:abc')).toBeNull(); + expect(sessionStorage.getItem('objectui:metadata:object:org_a:abc')).toBeNull(); + + // Non-matching keys were never removal candidates either way. + expect(sessionStorage.getItem('objectui:sidebar:collapsed')).toBe('true'); + + // The active-org clear is unconditional and runs after the sweep either way. + expect(ActiveOrganizationStorage.get()).toBeNull(); + + // 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 has been observed to survive + // `restoreAllMocks()` across tests (objectui#5763's sibling case). + removeItemSpy.mockRestore(); + } + }); + + it('reports nothing when every removal sticks (#5777)', async () => { + // Control on the case above: the warning is a measurement of an actual + // failure, not a constant emitted on every purge. + const client = createMockClient(); + await renderSignedIn(client); + seedPreviousSessionCaches(); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await act(async () => { + await authRef.current!.signOut(); + }); + + expect(metadataKeys()).toEqual([]); + expect(warnSpy).not.toHaveBeenCalled(); + }); + it('drops the previous principal’s organization block from context', async () => { // The list is the workspaces THAT user belongs to (the switcher renders // it), and a surviving `activeOrganization` would also suppress the