fix(auth): stop sweepStore aborting the sign-in purge on one throwing removeItem (#5763)

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

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.`,
+ );
  }
 }
 

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();
+ });
 });
 
 // ---------------------------------------------------------------------------