ActiveOrganizationStorage's in-memory fallback is unreachable when localStorage exists but rejects writes — the tenant header is then never stamped for the whole session

Found while documenting the X-Tenant-ID edge contract for #5279 (that card documents the observable consequence; it does not fix this). Measured, not reasoned — the probe is below.

The shape

packages/auth/src/createAuthenticatedFetch.ts — ActiveOrganizationStorage keeps a module-level _memoryValue as the fallback for environments without localStorage. set() always writes it. get() reads it only when the localStorage read itself throws:

get(): string | null {
  try {
    if (typeof localStorage !== 'undefined') {
      return localStorage.getItem(ACTIVE_ORG_STORAGE_KEY);   // returns null, no throw
    }
  } catch { /* SSR / test */ }
  return this._memoryValue;                                   // unreachable in that case
},

There is a real browser state where localStorage exists, reads fine, and rejects writes: Safari private browsing and any quota-exhausted origin, where setItem throws QuotaExceededError. In that state set()'s try swallows the write failure (correctly — the fallback is right there), but get() never consults the fallback, because the read path succeeded and returned null.

Probe

Run against the built dist/, with a localStorage that reads but refuses writes:

const store = new Map();
globalThis.localStorage = {
  getItem: (k) => (store.has(k) ? store.get(k) : null),
  setItem: () => { throw new Error('QuotaExceededError'); },
  removeItem: (k) => store.delete(k),
};
ActiveOrganizationStorage.set('org-42');

Result:

after set(), _memoryValue = org-42
after set(), get()        = null

The value was stored and cannot be read back.

What it costs

For the whole session, in such a browser:

X-Tenant-ID is never stamped. createAuthenticatedFetch reads get(), so the request looks like a permanent first boot. Per the contract documented on Confirm whether X-Tenant-ID has a reader: the framework derives the tenant from the session, not the header #5279 the header is a routing hint that the edge falls through on, so this is not a data-scoping bug — the framework scopes from the session — but it does mean the tenant-routing input is missing on every request, not just early ones.
MetadataProvider's org-scoped session cache is permanently keyed @none. activeOrgScope() (packages/app-shell/src/providers/MetadataProvider.tsx) reads the same storage, and the whole point of that key is that it equals the tenant the entries were fetched under. It stays @none forever, and the first-boot relabel path never fires because the org id it waits for never becomes readable.
switchOrganization appears to succeed — the server-side active org does change — while the client-side stamp and cache scope never follow.

None of this is loud. There is no error, no warning, and no test covering the read-succeeds/write-fails combination; the existing tests exercise localStorage present and working, or absent entirely.

Repair sketch, for triage rather than as a decision

Make get() prefer a non-null localStorage read and fall back to _memoryValue otherwise, rather than returning the localStorage result unconditionally. That keeps the persisted value authoritative when it exists, and makes the memory fallback reachable in the state it was written for. The one thing to be careful about: after clear() the memory value is nulled too, so a fallback on null does not resurrect a cleared org — worth a test either way, since sign-out's clear is a security-relevant path.

Triage: lands in packages/auth/src/createAuthenticatedFetch.ts (ActiveOrganizationStorage.get()) ⇒ domain:ui, graded Bug (label bug), queued (pm:queue).

Grading rationale: measured with a real probe (value stored, unreadable), reachable by ordinary users (Safari private browsing; quota-exhausted origins), and the cost is a silent whole-session degradation — tenant routing header never stamped, MetadataProvider cache permanently keyed @none, switchOrganization half-succeeding. The card's repair sketch (prefer non-null localStorage read, else fall back to _memoryValue) is the obvious mechanical shape; the one constraint the fix must pin with a test is the card's own caveat: sign-out's clear() must not be resurrected by the fallback — that path is security-relevant, and a naive "fallback on null" would re-stamp a cleared org. Test matrix: (a) read-succeeds/write-fails (the probe), (b) clear-then-get stays null, (c) plain working localStorage unchanged.

Sizing note: S. Related in-flight context: #5279 documents this window's observable consequence — if that PR's README section describes the unstamped window, the fix here narrows it; no ordering constraint, just cross-reference in the PR body.

Generated by Claude Code

Claim: PM loop round 19
Session: session_01EuPCi56cnGyykygi3z9w4m
Branch: claude/issue-5703-active-org-storage-fallback
Worktree: objectui-issue-5703
Domain: domain:ui
File surface: packages/auth/src/createAuthenticatedFetch.ts and its tests (stop on breach; explain in the report)
Container & model: S, mode:subagent, model: opus
Clause-②: no
Serial constraints cleared: packages/auth is free — #5724 (#5424) merged 01:07Z and released it. Round-19 siblings fenced disjoint: #5717 (packages/components + one apps/console specimen test) and #5660 (apps/console/index.html + src/main.tsx). Predecessor on this exact file, #5279/PR #5706, merged 2026-08-22 13:12Z — it documented this window; it did not fix it.

Tier basis. No path-derived mandate. Content judgement: clause ② does not trip — this is internal storage-read logic behind a module-local object; no contract accept/reject behaviour moves and no public surface widens. Size follows triage's S; tier at the default judgement档 because the clear() interaction is security-relevant.

Premise re-verified on origin/main d8afbe519 — the shape reproduces exactly:

:47 — return localStorage.getItem(ACTIVE_ORG_STORAGE_KEY); returned unconditionally when localStorage exists.
:50 — return this._memoryValue; reachable only when the read threw.
:54 / :57 — set() writes _memoryValue first, then attempts localStorage.setItem.
:63 / :66 — clear() nulls _memoryValue and removes the key.

So in a browser where localStorage exists, reads fine, and rejects writes (Safari private browsing; any quota-exhausted origin), set() correctly swallows the write failure into the memory fallback — and get() never consults it, because the read path succeeded and returned null. The card's probe measured exactly that: "after set(), _memoryValue = org-42 / after set(), get() = null."

Triage's ruling, binding. The repair sketch is the ruled shape: prefer a non-null localStorage read, else fall back to _memoryValue. ⚠️ The one constraint the fix must pin is the card's own caveat, and it is the security-relevant half: sign-out's clear() must not be resurrected by the fallback. A naive "fall back whenever the read is null" would re-stamp a cleared org after sign-out. clear() nulls _memoryValue too, so the correct behaviour falls out — but it must be pinned, not relied on.

Required test matrix, from triage: (a) read-succeeds/write-fails — the probe; (b) clear-then-get stays null; (c) plain working localStorage unchanged.

Blast radius worth knowing while you work (do not fix these here — they are consequences that disappear when get() is repaired): createAuthenticatedFetch reads get(), so X-Tenant-ID is never stamped for the whole session; and activeOrgScope() in packages/app-shell/src/providers/MetadataProvider.tsx reads the same storage, so its org-scoped session cache stays permanently keyed @none and the first-boot relabel never fires. ⛔ packages/app-shell/** is out of fence. Cross-reference #5279 in the PR body per triage.

Generated by Claude Code

fix(auth): make the ActiveOrganizationStorage memory fallback reachab…

os-dev-report

{
  "issue": 5703,
  "status": "done",
  "branch": "claude/issue-5703-active-org-storage-fallback",
  "pr": "https://github.com/objectstack-ai/objectui/pull/5730",
  "premise_still_valid": true,
  "card_body_complete": "Verified not truncated — the body ends with the 'Repair sketch, for triage rather than as a decision' paragraph, whose last sentence is the clear()/sign-out caveat. Both comments read: triage's grading (binding) and the round-19 claim, which carries the dispatching session id.",
  "summary": "ActiveOrganizationStorage.get() now prefers a non-null localStorage read and falls back to _memoryValue otherwise, instead of returning the localStorage read unconditionally. This is the ruled shape, unchanged. All four of PM's line measurements reproduced exactly on d8afbe519 (:47 unconditional return, :50 reachable only when the read threw, :54/:57 set() writes memory first, :63/:66 clear() nulls memory AND removes the key), so the premise holds. Three files changed, all in fence.",
  "clear_case_how_pinned": "Pinned three ways rather than relied on. (1) The test asserts _memoryValue itself is null after clear(), not merely that get() returns null — asserting only get() would pass even if clear() stopped nulling the memory value, because the assertion could not tell 'the fallback correctly answered null' apart from 'the fallback was never consulted'. (2) The same statement is pinned one level out on the wire: after clear(), createAuthenticatedFetch sends no X-Tenant-ID at all (headers.has is false — absent, not present-and-empty). (3) clear() carries a comment naming the line as security-relevant, at the site where a future editor would delete it. Both the write-rejecting and the normally-working localStorage variants of clear-then-get are covered.",
  "naive_fallback_consequence": "A fallback keyed on 'the localStorage read came back null' is exactly the shape that re-stamps a cleared org: after clear() the read is null BY CONSTRUCTION, so the fallback always fires on that path. Had clear() not nulled _memoryValue, sign-out would have removed the persisted key, get() would have fallen through to the still-set memory value, and the cleared org would have gone straight back on the wire as X-Tenant-ID for the rest of the session — with no error and no warning. The ruled shape is safe only because of a property of clear()'s body, not of get(), which is why it is now asserted rather than read off the current source.",
  "tests": "All runs against SOURCE, not dist/ — the repo's vitest config aliases @object-ui/auth to packages/auth/src, and the new test imports the module relatively. RED first, on the unmodified tree: 'Tests 3 failed | 6 passed (9)', all three 'AssertionError: expected null to be org-42' — the card's probe reproduced as a committed test. Notable: the clear-case went red at its own PRECONDITION (get() must return the org before clear() can be tested), i.e. that case was vacuous before the fix — which is the reason it needed pinning. GREEN after, re-run on the final commit 4cae3d6a8 with a clean working tree: 'Test Files 21 passed (21) / Tests 218 passed (218)' for all of packages/auth, and 'Test Files 3 passed (3) / Tests 13 passed (13)' for the three MetadataProvider consumer tests. Verdicts read from each gate's own printed line, with exit codes captured before any pipe.",
  "consumer_risk_checked": "The one way this change could break a consumer is a test that clears localStorage directly WITHOUT going through ActiveOrganizationStorage.clear() — a stale _memoryValue would then newly leak through the fallback. Grepped for it: all three MetadataProvider tests and signOut-client-cache-purge-5198 call BOTH localStorage.clear() and ActiveOrganizationStorage.clear() in their beforeEach, so no precondition moves. Cross-file leakage is impossible in the dom project (isolate: true re-executes the module graph per file, resetting _memoryValue).",
  "verify_scope": "Reachability computed mechanically, not by eye: a script over actual source imports (not package.json declarations), transitively closed, gives 8 of 46 workspace packages that can reach the symbol, and 346 test files at the module-graph level (291 of them app-shell, via barrel re-exports — a conservative over-approximation). Two attempts to run a full superset locally were killed by the ~10-minute foreground cap (exit 143 on repo-wide pnpm test; exit 124 on the 8-package closure). DECLARED NARROWING, per the standing rule that the repo-wide farm is CI's run: I ran the whole package containing the change (21 files, covering all 14 reaching auth test files) plus the 3 test files that name the storage symbol or activeOrgScope directly — the only semantically relevant consumers. Both lock timeouts released the lock cleanly (verified via --status; a sibling held it afterwards) and left no stray processes.",
  "lint_narrowing_evidence": "Not 'skipped' — measured, with all three required pieces. (1) Population read from ESLint's own config: 'eslint .' in packages/auth selected 42 files. (2) Count and per-file verdicts read from --format json: 42 files, 0 errors, 29 warnings, and both changed files present with 0 errors / 0 warnings each. (3) Invariance: eslint.config.js configures no type-aware linting (no projectService, no project:, no parserOptions), so this diff cannot move the verdict on any file it does not itself contain. The repo-wide pnpm lint remains CI's.",
  "gates": [
    "pnpm exec vitest run packages/auth — PASS: 'Test Files 21 passed (21)' / 'Tests 218 passed (218)' (on 4cae3d6a8)",
    "pnpm exec vitest run (3 MetadataProvider consumers) — PASS: 'Test Files 3 passed (3)' / 'Tests 13 passed (13)'",
    "pnpm --filter @object-ui/auth type-check — PASS, exit 0; script name echoed in output ('tsc --noEmit && tsc -p tsconfig.test.json'), confirming it was not a zero-match no-op",
    "eslint . in packages/auth — PASS: 0 errors, 29 warnings, all in files this PR does not touch",
    "node scripts/check-control-bytes.mjs — PASS: 'OK (scanned 4782 tracked text file(s); skipped 85 binary)'; plus a manual control-byte scan of my three files, no matches",
    "node scripts/check-changeset-presence.mjs — PASS: '2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)'",
    "node scripts/check-changeset-no-major.mjs — PASS: 'No changeset declares a major bump.'",
    "node scripts/check-changeset-fixed.mjs — PASS: 'All workspace packages are in the changeset fixed group.'",
    "node scripts/check-phantom-dependencies.mjs — PASS: 'Every in-scope import is declared by the package that publishes it.'",
    "node scripts/check-package-self-import.mjs — PASS: 'No package names itself inside its own src/.'",
    "repo-wide pnpm test / pnpm lint — NOT RUN LOCALLY, left to CI (see verify_scope and lint_narrowing_evidence)"
  ],
  "files_changed": [
    "packages/auth/src/createAuthenticatedFetch.ts — get() prefers a non-null localStorage read; read-order and clear()-invariant comments",
    "packages/auth/src/__tests__/activeOrgStorageFallback-5703.test.tsx — NEW, 9 cases",
    ".changeset/active-org-storage-memory-fallback-5703.md — NEW, patch on @object-ui/auth"
  ],
  "toolchain_notes": "Two objectui-specific traps avoided. (1) 'pnpm --filter @object-ui/auth test' is REFUSED by scripts/vitest-invocation-guard.mjs — a package-cwd vitest run collects apps/console's files and reports a false green. The canonical form is 'pnpm exec vitest run PATH' from the repo root, which is what every test verdict above used. (2) The typecheck script in objectui is spelled 'type-check' with a hyphen; a misspelling would exit 0 having matched zero scripts, so the echoed script name is quoted above as evidence it actually ran. dispatch-gates.mjs was correctly not used — it is objectstack's tree and objectui has no scripts/pm/.",
  "fence": "Respected, no breaches. packages/app-shell was read and its tests RUN but never edited. Nothing touched in packages/components, apps/console, content/docs, or apps/site. No forced changes elsewhere in packages/auth. The skip-changeset label is not applicable — a changeset is present — and no label was created or applied in this repo.",
  "open_questions": [],
  "out_of_scope_findings": [
    "filed as #5731: clear() swallows a failed localStorage.removeItem, so a cleared org can stay readable and sign-out would not stick. Filed UNASSIGNED and UNLABELED with reachability explicitly NOT demonstrated — I ruled out the obvious candidates (Safari private browsing and quota exhaustion throw on setItem only; revoked iframe storage access throws on getItem too, which is already safe via the existing catch), leaving only proxied localStorage, which is speculation. Not fixed here: the bounded in-place exemption fails on its second condition, since the correct shape is not pinned by existing evidence. Searched first (no duplicate); checked #5664 and filed standalone rather than as its sub-issue, because completing #5664's namespacing would not close it."
  ]
}

Generated by Claude Code

ACCEPT (on merits) — PR #5730 · reviewer of record, `domain:ui` seat, round 19

⚠️ On merits; landing held. Gates by name at 02:06Z, four minutes after the PR opened: 15 success + 3 skipped + 6 in_progress (four shards, Type Check, Lint). ⛔ Not flipped ready until 19 + 3 is verified. Fence clean: 3 files, all packages/auth + changeset; packages/app-shell was read and its tests run but never edited.

The `clear()` case was vacuous before the fix, and that is why it needed pinning

The single best thing in this report: the clear-then-get case went red at its own precondition — get() must return the org before clear() can be tested at all, and pre-fix it never did. So that case was vacuous on the old code: it would have passed against a renderer that had no fallback whatsoever. This lane's standing rule is "test whether the pin can go VACUOUS, not just whether it catches a defect", and here the vacuity was discovered rather than reasoned about.

Three pins, and the argument for why one is not enough

Asserting only that get() returns null after clear() would pass even if clear() stopped nulling _memoryValue — because that assertion cannot distinguish "the fallback correctly answered null" from "the fallback was never consulted." So the test asserts _memoryValue itself is null; the same statement is pinned one level out on the wire (after clear(), createAuthenticatedFetch sends no X-Tenant-ID at all — headers.has false, absent rather than present-and-empty); and clear() carries a comment naming the line as security-relevant at the site where a future editor would delete it.

The safety property belongs to `clear()`, not to `get()` — which is exactly why it is now asserted

I asked for this distinction and the report makes it precisely: a fallback keyed on "the localStorage read came back null" is the shape that re-stamps a cleared org, because after clear() the read is null by construction, so the fallback always fires on that path. Had clear() not nulled _memoryValue, sign-out would have removed the persisted key, get() would have fallen through to the still-set memory value, and the cleared org would have gone back on the wire as X-Tenant-ID for the rest of the session, with no error and no warning. The ruled shape is safe only because of a property of clear()'s body. That is now pinned rather than read off the current source — which is the whole difference between a fix that stays correct and one that is correct today.

Consumer risk was checked mechanically, not eyeballed. The one way this change could newly break a consumer is a test that clears localStorage directly without going through ActiveOrganizationStorage.clear() — a stale _memoryValue would then leak through the new fallback where it previously could not. Grepped: the three MetadataProvider tests and signOut-client-cache-purge-5198 all call both in their beforeEach, so no precondition moves; and cross-file leakage is impossible under isolate: true, which re-executes the module graph per file.

⚠️ Two objectui toolchain traps this report surfaced — both are "a gate that cannot fail"

pnpm --filter @object-ui/auth test is REFUSED by scripts/vitest-invocation-guard.mjs — because a package-cwd vitest run collects apps/console's files and reports a false green. The canonical form is pnpm exec vitest run PATH from the repo root. A guard that refuses the wrong invocation is doing real work here; without it, "the auth tests passed" would have meant nothing.
The type-check script is spelled type-check, hyphenated — and a misspelled --filter/script name exits 0 having matched zero scripts. That is a gate that returns success for the wrong reason, in the exact family this lane keeps recording. The report quotes the echoed script name (tsc --noEmit && tsc -p tsconfig.test.json) as evidence it actually ran, which is the correct instrument.

Both carried to the seat post.

Two runs were killed by the ~10-minute foreground cap (exit 143 repo-wide, exit 124 on the 8-package closure), declared rather than quietly dropped, with the narrowing justified: reachability computed mechanically over actual source imports (8 of 46 workspace packages; 346 test files at module-graph level, 291 of them app-shell via barrel re-exports — named as "a conservative over-approximation"), then the whole containing package (21 files, covering all 14 reaching auth test files) plus the 3 tests that name the storage symbol or activeOrgScope directly. Both lock timeouts released the lock cleanly, verified via --status, with a sibling holding it afterwards.

Out-of-scope finding #5731 — and the restraint is the point. clear() swallows a failed localStorage.removeItem, so a cleared org could stay readable and sign-out would not stick. It was filed with reachability explicitly NOT demonstrated: Safari private browsing and quota exhaustion throw on setItem only; revoked iframe storage access throws on getItem too, which the existing catch already handles — leaving only proxied localStorage, which the report calls speculation. And it explains why it was not fixed in place: the in-place exemption fails its second condition, because the correct shape is not pinned by existing evidence. Filing a security-shaped finding while labelling its own reachability undemonstrated is better than either fixing it on a guess or not filing it.

Landing blocked only on the by-name gate read.

Generated by Claude Code

fix(auth): stop `ActiveOrganizationStorage.clear()` swallowing a fail…

fix(auth): make the ActiveOrganizationStorage memory fallback reachab…

Navigation Menu

Repository navigation

`ActiveOrganizationStorage`'s in-memory fallback is unreachable when `localStorage` exists but rejects writes — the tenant header is then never stamped for the whole session #5703

The shape

Probe

What it costs

Repair sketch, for triage rather than as a decision

ACCEPT (on merits) — PR #5730 · reviewer of record, `domain:ui` seat, round 19

The `clear()` case was vacuous before the fix, and that is why it needed pinning

Three pins, and the argument for why one is not enough

The safety property belongs to `clear()`, not to `get()` — which is exactly why it is now asserted

⚠️ Two objectui toolchain traps this report surfaced — both are "a gate that cannot fail"

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

ActiveOrganizationStorage's in-memory fallback is unreachable when localStorage exists but rejects writes — the tenant header is then never stamped for the whole session #5703

Description

The shape

Probe

What it costs

Repair sketch, for triage rather than as a decision

Activity

huangyiirene commented on Aug 22, 2026

os-zhuang commented on Aug 23, 2026

os-zhuang commented on Aug 23, 2026

os-zhuang commented on Aug 23, 2026

ACCEPT (on merits) — PR #5730 · reviewer of record, domain:ui seat, round 19

The clear() case was vacuous before the fix, and that is why it needed pinning

Three pins, and the argument for why one is not enough

The safety property belongs to clear(), not to get() — which is exactly why it is now asserted

⚠️ Two objectui toolchain traps this report surfaced — both are "a gate that cannot fail"

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`ActiveOrganizationStorage`'s in-memory fallback is unreachable when `localStorage` exists but rejects writes — the tenant header is then never stamped for the whole session #5703

ACCEPT (on merits) — PR #5730 · reviewer of record, `domain:ui` seat, round 19

The `clear()` case was vacuous before the fix, and that is why it needed pinning

The safety property belongs to `clear()`, not to `get()` — which is exactly why it is now asserted