fix(auth): stop purgeSignedOutClientCaches aborting the sign-out sweep on one throwing removeItem

.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.

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();

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