From 0610ca74b6db14d9d6fa5aaa1e0da34ee5a774e3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 05:26:37 +0000 Subject: [PATCH] fix(auth): stop `ActiveOrganizationStorage.clear()` swallowing a failed removal (#5731) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `clear()` nulled `_memoryValue` and then removed the persisted key inside a `try`/`catch` that discarded any failure. Since #5703 `get()` prefers a non-null `localStorage` read, so a removal that did not stick left the key readable, the read order preferred it, and sign-out silently did not stick — the cleared org went back on the wire as `X-Tenant-ID`. The removal is now judged by a read-back rather than by catching the throw, which also covers a wrapped `localStorage` whose `removeItem` is a silent no-op, and does not misreport SSR or a fully-throwing store as a failure. A key whose removal cannot be verified is quarantined in memory for the rest of the page-load: `get()` skips its persisted branch and answers from `_memoryValue`. The quarantine is released as soon as a removal on that key sticks. Not thrown and not returned: all five callers arrive after the transition they follow up on has already happened and none can act on a storage failure, so the invariant is restored inside `clear()` and the failure is reported to the console. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m --- .../active-org-clear-removal-failure-5731.md | 44 +++ .../auth/src/ActiveOrganizationStorage.ts | 118 +++++++- ...activeOrgClearRemovalFailure-5731.test.tsx | 286 ++++++++++++++++++ 3 files changed, 444 insertions(+), 4 deletions(-) create mode 100644 .changeset/active-org-clear-removal-failure-5731.md create mode 100644 packages/auth/src/__tests__/activeOrgClearRemovalFailure-5731.test.tsx diff --git a/.changeset/active-org-clear-removal-failure-5731.md b/.changeset/active-org-clear-removal-failure-5731.md new file mode 100644 index 0000000000..aacb03bc76 --- /dev/null +++ b/.changeset/active-org-clear-removal-failure-5731.md @@ -0,0 +1,44 @@ +--- +'@object-ui/auth': patch +--- + +`ActiveOrganizationStorage.clear()` now verifies that the persisted key is actually +gone and, when it is not, both reports the failure and stops `get()` answering from +the surviving value — instead of swallowing the failed removal (objectui#5731). + +`clear()` nulled `_memoryValue` and then removed the persisted key inside a +`try`/`catch` that discarded any failure. Since objectui#5703 `get()` prefers a +NON-NULL `localStorage` read and only falls through to `_memoryValue`, so the two +halves of `clear()` were not equally strong: nulling memory always sticks, while a +removal that did not stick left the key readable and the read order preferred it. +Sign-out is one of `clear()`'s five callers, so the failure mode was "sign-out does +not stick", and it was silent — the cleared organization went back on the wire as +`X-Tenant-ID` on every subsequent request. + +The removal is now judged by a READ-BACK rather than by catching the throw, which is +both narrower and wider in the right directions. Wider: a wrapped or proxied +`localStorage` whose `removeItem` is a silent no-op never throws and leaves identical +residue, and is now covered. Narrower: SSR and the partitioned-iframe browser where +every operation throws have nothing readable to resurrect, were already safe, and are +not reported as failures. + +A key whose removal could not be verified is quarantined in memory for the rest of the +page-load: `get()` skips the persisted branch for it and answers from `_memoryValue`, +which `clear()` has just nulled and which a later `set()` refills with the value that +write was meant to persist. The quarantine is released as soon as a removal on that key +sticks. An unstamped `X-Tenant-ID` is a documented state of the edge contract +(objectui#5279); a re-stamped signed-out organization is not. + +The failure is not thrown and not returned. All five call sites — sign-out's +`purgeSignedOutClientCaches`, `switchOrganization`, `deleteOrganization`, +`leaveOrganization`, and the session-user purge that runs on the SIGN-IN path — arrive +after the transition they follow up on has already happened, and none can act on a +storage failure; a `boolean` every caller ignores would read as handled when it is not. +So the invariant is restored inside `clear()` and the failure is reported to the +console. + +A working `localStorage` behaves exactly as before: the removal sticks, nothing is +quarantined, nothing is reported, and a non-null persisted read is still authoritative. +`set()`'s swallowed write failure is deliberately untouched — that swallow is +objectui#5703's memory fallback, and it is the correct kind, because the memory copy +upholds `set()`'s postcondition where nulling memory could not uphold `clear()`'s. diff --git a/packages/auth/src/ActiveOrganizationStorage.ts b/packages/auth/src/ActiveOrganizationStorage.ts index 3dfb90c6f5..393a0cc49f 100644 --- a/packages/auth/src/ActiveOrganizationStorage.ts +++ b/packages/auth/src/ActiveOrganizationStorage.ts @@ -145,12 +145,36 @@ function writePersisted(key: string, value: string): void { } } -function removePersisted(key: string): void { +/** + * Remove `key`, and report whether it is gone — `true` when a read-back can no + * longer produce a value for it. + * + * ## Why the verdict is a READ-BACK and not "did `removeItem` throw" + * (objectui#5731) + * + * `get()` can only answer with what `getItem` hands back, so "did this removal + * stick" is exactly the question "is the key still readable". Deciding it from + * the throw instead would be both too narrow and too wide: + * + * - TOO NARROW. A wrapped or proxied `localStorage` — an extension, a + * polyfill — whose `removeItem` is a silent no-op never throws, and leaves + * exactly the residue this guards against. objectui#5731 named the throwing + * variant; the no-op variant is the same defect, and a read-back catches + * both without having to enumerate them. + * - TOO WIDE. In SSR, and in a partitioned iframe where every operation + * throws, there is nothing readable to resurrect and the paths are already + * safe. A throw-based verdict would report those two states as failures, + * which is how a report earns a reputation for crying wolf. + * + * The throw is therefore still swallowed here: it is not the verdict. + */ +function removePersisted(key: string): boolean { try { safeStore('local')?.removeItem(key); } catch { - /* storage unavailable */ + /* not the verdict — the read-back below is */ } + return readPersisted(key) === null; } /** @@ -304,6 +328,17 @@ function scopedActiveOrgKey(): string | null { export const ActiveOrganizationStorage = { _memoryValue: null as string | null, + /** + * Keys whose removal by {@link clear} did not stick — the value was still + * readable afterwards (objectui#5731). + * + * {@link get} refuses to answer from the persisted layer for a key in here, + * which is the mechanism that keeps a cleared organization cleared. Held in + * memory, so it lasts exactly one page-load: a browser that starts fresh + * re-measures rather than inheriting a verdict. + */ + _unremovedKeys: new Set(), + /** * The active org id for the CURRENT user, or `null`. * @@ -336,7 +371,18 @@ export const ActiveOrganizationStorage = { */ get(): string | null { const key = scopedActiveOrgKey(); - if (key) { + // A key {@link clear} could not remove is QUARANTINED for the rest of this + // page-load, and skipping the persisted branch is what stops the cleared + // org from being handed straight back (objectui#5731). `_memoryValue` then + // answers, and for that key it is the only copy this page-load can trust: + // `clear()` nulled it, and a later `set()` refills it with the value that + // write was meant to persist — so this stays correct in both directions + // with no release step of its own. What it gives up is cross-tab freshness + // for one key, in a browser that has just demonstrated it cannot delete + // from storage. An unstamped `X-Tenant-ID` is a documented state of the + // edge contract (see `createAuthenticatedFetch`); a re-stamped signed-out + // org is not. + if (key && !this._unremovedKeys.has(key)) { try { const persisted = safeStore('local')?.getItem(key) ?? null; if (persisted !== null) return persisted; @@ -357,6 +403,43 @@ export const ActiveOrganizationStorage = { writePersisted(key, orgId); }, + /** + * Drop the active organization, from memory and from the persisted key. + * + * POSTCONDITION: {@link get} answers `null` afterwards. Sign-out is one of + * the callers, so that is a security-relevant guarantee rather than a + * convenience — and before objectui#5731 it was not one, because a + * `removeItem` that failed was swallowed and the surviving key won `get()`'s + * read order. + * + * ## Why a failed removal is neither thrown nor returned (objectui#5731) + * + * Every caller arrives here AFTER the transition it is following up on has + * already happened, and not one of them can act on a storage failure: + * + * - `AuthProvider`'s `purgeSignedOutClientCaches` — the session is already + * ended. Sign-out cannot be refused because a key would not delete. + * - `switchOrganization` — the server already switched or cleared the + * active org; a throw would report a successful switch as a failure and + * route the caller into its error branch. + * - `deleteOrganization` / `leaveOrganization` — the org is already deleted + * or already left. + * - {@link purgePreviousUserClientState}, via {@link SessionUserScope.adopt} + * — runs on the SIGN-IN path inside an `AuthProvider` effect, where a + * throw breaks the boot of the ARRIVING user. + * + * A `boolean` return only moves the problem one step: all five call sites + * would have nothing to write in the failure branch, and a return value that + * every caller ignores reads as handled when it is not. So the invariant is + * restored HERE, where it can be, and the failure is reported to the console, + * where a human can find it. + * + * The third shape considered and rejected was re-writing the key with an + * empty value. That relocates the fix into every consumer's truthiness test + * (`createAuthenticatedFetch` does `if (activeOrgId)`), which is the lenient + * consumer AGENTS.md #0.1 forbids, and it leaves a signed-out browser holding + * a live key. + */ clear(): void { // Nulling the fallback is SECURITY-RELEVANT, not bookkeeping: `get()` // falls through to `_memoryValue` whenever the `localStorage` read comes @@ -366,9 +449,36 @@ export const ActiveOrganizationStorage = { // `X-Tenant-ID`. Pinned by `__tests__/activeOrgStorageFallback-5703.test.tsx`. this._memoryValue = null; const key = scopedActiveOrgKey(); - if (key) removePersisted(key); + if (key) { + if (removePersisted(key)) { + // Confirmed gone. Recomputed on every call rather than left alone, so + // a key quarantined by an earlier failure is RELEASED the moment a + // removal sticks: the quarantine describes the last attempt, not a + // permanent verdict on the browser. + this._unremovedKeys.delete(key); + } else { + // The removal did not stick and the value is still readable, so + // `get()` would hand the signed-out org straight back. Two things + // happen here and they are deliberately separable: the stale read is + // SUPPRESSED, which restores the postcondition without any caller's + // help, and the failure is REPORTED, because "sign-out did not fully + // stick" is something whoever is reading a console has to be able to + // find. Neither half substitutes for the other — a report alone leaves + // the org on the wire, and suppression alone is the same silence this + // card was filed about, just with a better outcome. + this._unremovedKeys.add(key); + console.warn( + `[ActiveOrganizationStorage] clear() could not remove "${key}" from localStorage — ` + + 'it is still readable. Reads for it are answered from memory for the rest of this ' + + 'page-load, so the cleared organization is not re-stamped as X-Tenant-ID.', + ); + } + } // Also drop the pre-#5664 bare key, so a `clear()` on a browser upgrading // from an older build leaves nothing behind under the retired spelling. + // Its verdict is deliberately ignored: `scopedActiveOrgKey()` never returns + // the bare spelling, so nothing reads it and a survival here cannot + // resurrect an org through `get()`. removePersisted(LEGACY_ACTIVE_ORG_KEY); }, }; diff --git a/packages/auth/src/__tests__/activeOrgClearRemovalFailure-5731.test.tsx b/packages/auth/src/__tests__/activeOrgClearRemovalFailure-5731.test.tsx new file mode 100644 index 0000000000..f48f7229c5 --- /dev/null +++ b/packages/auth/src/__tests__/activeOrgClearRemovalFailure-5731.test.tsx @@ -0,0 +1,286 @@ +/** + * objectui#5731 — `ActiveOrganizationStorage.clear()` must not swallow a failed + * removal, because `get()` would then hand the cleared organization back. + * + * ## The asymmetry, re-derived on the post-#5664 code + * + * `clear()` nulls `_memoryValue` and then removes the persisted key. `get()` + * prefers a NON-NULL persisted read and only falls through to `_memoryValue` + * (the objectui#5703 read order). So the two halves of `clear()` are not + * equally strong: nulling memory is unconditional and always sticks, while the + * removal was best-effort and its failure was swallowed. A removal that does + * not stick leaves the key readable, the read order prefers it, and sign-out — + * one of `clear()`'s five callers — silently does not stick. The org goes back + * on the wire as `X-Tenant-ID`. + * + * ## Why there is no browser repro here, on purpose + * + * The filer refused to claim a defect they had not reproduced, and triage + * graded the card anyway with the reason recorded: *"Acceptance must NOT + * require reproducing a browser state ... the deliverable is that the failure + * stops being silent, plus a unit-level pin that a throwing `localStorage` no + * longer leaves `get()` returning the cleared org."* A `localStorage` double is + * the instrument, and it is the RIGHT instrument: the invariant under test is a + * property of this module, not of any particular browser's storage quirk. + * + * Two shapes of failing removal are driven, because the fix verifies by + * READ-BACK rather than by catching the throw and therefore covers both: + * + * - `removeItem` THROWS — the shape the card describes. + * - `removeItem` is a SILENT NO-OP — a wrapped or proxied `localStorage`, the + * only candidate the filer could name. It never throws, so a throw-based + * guard would miss it entirely while leaving identical residue. + * + * ## What must NOT be weakened + * + * `activeOrgStorageFallback-5703.test.tsx` pins that `clear()` nulls + * `_memoryValue` BEFORE touching storage and that a non-null persisted read + * wins; `sessionUserChangePurge-5664.test.tsx` pins the session-user purge. + * Suppression here is scoped to a key whose removal was MEASURED to have + * failed, so neither of those properties moves: on every store that can + * actually delete, the persisted read is still preferred and still authoritative. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { createAuthenticatedFetch } from '../createAuthenticatedFetch'; +import { ActiveOrganizationStorage, SessionUserScope } from '../ActiveOrganizationStorage'; +import { TokenStorage } from '../createAuthClient'; + +const SESSION_USER_ID = 'u_5731'; +const SCOPED_ORG_KEY = `auth-active-organization-id:u:${SESSION_USER_ID}`; +const API_URL = 'http://localhost/api/v1/meta/object/account'; + +/** + * A `localStorage` double whose `removeItem` fails in a chosen way, keeping the + * backing Map visible so a case can assert what actually survived. + * + * `removal: 'throws'` is the card's shape; `'noop'` is the wrapped-storage + * shape. `'works'` is the control — the same double with a removal that sticks, + * so a green case cannot be explained by "the double is broken". + */ +function installLocalStorage(removal: 'works' | 'throws' | 'noop' = 'works') { + const store = new Map(); + vi.stubGlobal('localStorage', { + getItem: (k: string) => (store.has(k) ? store.get(k)! : null), + setItem: (k: string, v: string) => { store.set(k, v); }, + removeItem: (k: string) => { + if (removal === 'throws') throw new Error('SecurityError: removeItem is not allowed'); + if (removal === 'noop') return; + store.delete(k); + }, + }); + return store; +} + +/** Stub the global fetch and capture the Headers it was called with. */ +function stubFetch() { + const calls: Array<{ url: string; headers: Headers }> = []; + vi.stubGlobal('fetch', vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : (input as Request).url; + calls.push({ url, headers: new Headers(init?.headers) }); + return new Response('{}', { status: 200 }); + })); + return calls; +} + +let warn: ReturnType; + +beforeEach(() => { + // Resolve the scope from the in-memory pointer, so it survives every double + // the cases install below. Without a session user `set()` writes to memory + // only and every "reached the persisted layer" assertion would be vacuous. + SessionUserScope._resetForTests(); + SessionUserScope.adopt(SESSION_USER_ID); + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(TokenStorage, 'get').mockReturnValue(null); +}); + +afterEach(() => { + // Order matters: unstub FIRST so this `clear()` runs against a working store + // and releases any quarantine the case left on the scoped key, and reset the + // scope LAST so that `clear()` still resolves the same key. + vi.unstubAllGlobals(); + ActiveOrganizationStorage.clear(); + vi.restoreAllMocks(); + SessionUserScope._resetForTests(); +}); + +// --------------------------------------------------------------------------- +// (a) the pin triage asked for: an outcome, not a "did it warn" +// --------------------------------------------------------------------------- + +describe('a clear() whose removal fails no longer leaves the org readable (#5731)', () => { + it('answers null after clear(), when removeItem THROWS', () => { + const persisted = installLocalStorage('throws'); + ActiveOrganizationStorage.set('org-42'); + // Preconditions, asserted rather than assumed: the value really reached + // the persisted layer, and `get()` really was answering from it. + expect(persisted.get(SCOPED_ORG_KEY)).toBe('org-42'); + expect(ActiveOrganizationStorage.get()).toBe('org-42'); + + ActiveOrganizationStorage.clear(); + + // The premise of the case: the removal did NOT stick. Without this the + // case would pass against a store that quietly deleted the key, and would + // be testing nothing. + expect(persisted.get(SCOPED_ORG_KEY)).toBe('org-42'); + expect(localStorage.getItem(SCOPED_ORG_KEY)).toBe('org-42'); + + // THE PIN. Measured as 'org-42' before the fix. + expect(ActiveOrganizationStorage.get()).toBeNull(); + }); + + it('answers null after clear(), when removeItem is a SILENT NO-OP', () => { + // The wrapped/proxied `localStorage` the filer named as the only candidate + // they could point at. It never throws, so a fix that only caught the + // throw would leave this one exactly as broken as before. + const persisted = installLocalStorage('noop'); + ActiveOrganizationStorage.set('org-42'); + expect(ActiveOrganizationStorage.get()).toBe('org-42'); + + ActiveOrganizationStorage.clear(); + + expect(persisted.get(SCOPED_ORG_KEY)).toBe('org-42'); + expect(ActiveOrganizationStorage.get()).toBeNull(); + }); + + it('sends NO tenant header after a clear() whose removal failed', async () => { + // The same statement one level out, where the consequence actually lands: + // the header must be ABSENT, per the `X-Tenant-ID` edge contract this + // package's README documents (objectui#5279). + installLocalStorage('throws'); + ActiveOrganizationStorage.set('org-42'); + ActiveOrganizationStorage.clear(); + + const calls = stubFetch(); + await createAuthenticatedFetch()(API_URL); + + expect(calls[0].headers.has('X-Tenant-ID')).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// (b) the mechanism, pinned directly — the effect is one line from accidental +// --------------------------------------------------------------------------- + +describe('the suppression mechanism, not just its effect (#5731)', () => { + it('quarantines the key whose removal could not be verified', () => { + installLocalStorage('throws'); + ActiveOrganizationStorage.set('org-42'); + expect(ActiveOrganizationStorage._unremovedKeys.has(SCOPED_ORG_KEY)).toBe(false); + + ActiveOrganizationStorage.clear(); + + // `get()` returning null is achievable by accident — by never reading + // storage, by returning null unconditionally. This asserts WHICH mechanism + // produced it, so a later refactor that keeps the outcome by some other + // means has to say so here. + expect(ActiveOrganizationStorage._unremovedKeys.has(SCOPED_ORG_KEY)).toBe(true); + }); + + it('RELEASES the quarantine as soon as a removal sticks', () => { + // The quarantine describes the last attempt, not a permanent verdict on + // the browser: a store that recovers gets its persisted read back. + installLocalStorage('throws'); + ActiveOrganizationStorage.set('org-42'); + ActiveOrganizationStorage.clear(); + expect(ActiveOrganizationStorage._unremovedKeys.has(SCOPED_ORG_KEY)).toBe(true); + + vi.unstubAllGlobals(); + const persisted = installLocalStorage('works'); + ActiveOrganizationStorage.clear(); + + expect(ActiveOrganizationStorage._unremovedKeys.has(SCOPED_ORG_KEY)).toBe(false); + // ... and the persisted layer is authoritative again for that key. + persisted.set(SCOPED_ORG_KEY, 'org-fresh'); + expect(ActiveOrganizationStorage.get()).toBe('org-fresh'); + }); + + it('does not go blind: a set() after a failed clear() is readable again', () => { + // The case that dies if "suppress the stale read" is ever implemented as + // "answer null forever". A quarantined key is answered from `_memoryValue`, + // which is the copy this page-load can trust — so the NEW value comes back + // whether or not the write reached a store that cannot delete. + installLocalStorage('throws'); + ActiveOrganizationStorage.set('org-42'); + ActiveOrganizationStorage.clear(); + expect(ActiveOrganizationStorage.get()).toBeNull(); + + ActiveOrganizationStorage.set('org-99'); + + expect(ActiveOrganizationStorage.get()).toBe('org-99'); + }); +}); + +// --------------------------------------------------------------------------- +// (c) the failure stops being SILENT — the other half of the deliverable +// --------------------------------------------------------------------------- + +describe('a failed clear() is reported (#5731)', () => { + it('warns once, naming the key it could not remove', () => { + installLocalStorage('throws'); + ActiveOrganizationStorage.set('org-42'); + + ActiveOrganizationStorage.clear(); + + // Exactly once: `clear()` also removes the retired bare key, whose removal + // fails on this same double. That one is deliberately NOT reported — + // `scopedActiveOrgKey()` never returns the bare spelling, so nothing reads + // it and its survival cannot resurrect an org through `get()`. A report per + // failed removeItem would be two lines of noise for one real failure. + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain(SCOPED_ORG_KEY); + }); + + it('stays quiet when the removal sticks', () => { + const persisted = installLocalStorage('works'); + ActiveOrganizationStorage.set('org-42'); + + ActiveOrganizationStorage.clear(); + + expect(persisted.has(SCOPED_ORG_KEY)).toBe(false); + expect(ActiveOrganizationStorage.get()).toBeNull(); + expect(warn).not.toHaveBeenCalled(); + }); + + it('stays quiet when there is no storage at all', () => { + // SSR, and the partitioned-iframe browser where every operation throws. + // Nothing is readable in either, so nothing can be resurrected and both + // were already safe. A report that fired here would be crying wolf on the + // two states the card explicitly ruled OUT as the defect. + vi.stubGlobal('localStorage', undefined); + + expect(() => ActiveOrganizationStorage.clear()).not.toThrow(); + + expect(warn).not.toHaveBeenCalled(); + expect(ActiveOrganizationStorage._unremovedKeys.has(SCOPED_ORG_KEY)).toBe(false); + expect(ActiveOrganizationStorage.get()).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// (d) the neighbouring swallow that is DELIBERATE and must survive +// --------------------------------------------------------------------------- + +describe('set()’s swallowed write failure is left alone (#5703 design)', () => { + it('still keeps the value in memory when localStorage rejects writes', () => { + // `set()` swallows a failing `setItem` too, and that one is correct: the + // memory copy upholds the postcondition ("what was set reads back"), and + // `get()`'s fallback is built to consult it. `clear()`'s swallow was the + // accidental one because nulling memory does NOT uphold ITS postcondition + // — the surviving persisted value SHADOWS it. That is the test for telling + // the two apart, and this case pins that the deliberate one is untouched. + const store = new Map(); + vi.stubGlobal('localStorage', { + getItem: (k: string) => (store.has(k) ? store.get(k)! : null), + setItem: () => { throw new Error('QuotaExceededError'); }, + removeItem: (k: string) => { store.delete(k); }, + }); + + ActiveOrganizationStorage.set('org-42'); + + expect(store.has(SCOPED_ORG_KEY)).toBe(false); + expect(ActiveOrganizationStorage.get()).toBe('org-42'); + expect(warn).not.toHaveBeenCalled(); + }); +});