Uh oh!
There was an error while loading. Please reload this page.
fix(auth): make the ActiveOrganizationStorage memory fallback reachable when localStorage rejects writes - #5730
Merged
Conversation
…le when localStorage rejects writes (#5703) `ActiveOrganizationStorage.get()` returned the `localStorage` read unconditionally, so `_memoryValue` was reachable only when the read itself threw. In a browser where `localStorage` is present and readable but rejects writes -- Safari private browsing, any quota-exhausted origin, where `setItem` throws `QuotaExceededError` -- `set()` correctly swallowed the write failure into `_memoryValue` and `get()` then never consulted it: the active org was stored and could not be read back, so `X-Tenant-ID` went unstamped for the whole session rather than only the documented first-boot window. `get()` now prefers a non-null `localStorage` read and falls back to `_memoryValue` otherwise. A working `localStorage` is unchanged, and a non-null persisted read still wins over the memory value. The fallback fires exactly when the `localStorage` read is null, which is also the state `clear()` leaves behind, so sign-out is the case that had to be pinned rather than assumed: it answers null because `clear()` nulls `_memoryValue` too. `activeOrgStorageFallback-5703.test.tsx` asserts that property directly, so a future `clear()` that only removed the persisted key fails a test instead of silently re-stamping a cleared org. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-zhuang
marked this pull request as ready for review
August 23, 2026 02:13
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#5703
ActiveOrganizationStorage.get()returned thelocalStorageread unconditionally, so the module-level_memoryValuefallback was reachable only when the read itself threw. There is a real browser state where the read does not throw and the fallback is nonetheless the only copy of the value:localStoragepresent and readable but rejecting writes — Safari private browsing, and any quota-exhausted origin, wheresetItemthrowsQuotaExceededError.In that state
set()correctly swallowed the write failure into_memoryValue, andget()then never consulted it. The active org was stored and could not be read back.The repair
get()now prefers a non-nulllocalStorageread and falls back to_memoryValueotherwise:A working
localStorageis untouched, and a non-null persisted read still wins over the memory value — so another tab (or a page that outlived a memory value) remains the authority.The half that had to be pinned, not assumed
The new fallback fires exactly when the
localStorageread is null — which is also the stateclear()leaves behind. Sign-out callsclear(), so "fall back whenever the read is null" is, on its own, the precise shape that would re-stamp a cleared org.It answers
nullhere becauseclear()nulls_memoryValuebefore it toucheslocalStorage. That is a property ofclear()'s body, not a guarantee ofget(), so this PR pins it directly rather than reading it off the current source and trusting it to stay:_memoryValueitself is asserted null afterclear(), not merelyget(). A futureclear()that only removed the persisted key fails a test instead of silently resurrecting the org.clear(),createAuthenticatedFetchsends noX-Tenant-IDat all — absent, not present-and-empty.clear()carries a comment naming the line as security-relevant, since that is where a future editor would break it.Tests
packages/auth/src/__tests__/activeOrgStorageFallback-5703.test.tsx— 9 cases covering triage's matrix: (a) read-succeeds/write-fails, (b) clear-then-get stays null, (c) plain workinglocalStorageunchanged, plus (d) the read-throws path the fallback was originally written for, so the repair does not cost the case it already handled.The card's probe is reproduced as a committed test against source (the repo's vitest aliases
@object-ui/authtopackages/auth/src), not the builtdist/the one-off probe used.Measured red before the fix, on the same tree:
Worth noting which three went red: the clear-case failed at its own precondition (
get()must return the org beforeclear()can be tested), because the fallback was not live. That case was vacuous before this fix — which is exactly why it needed pinning rather than relying on the current behaviour.Green after, on
4cae3d6a8:Blast radius — noted, deliberately not fixed here
Two consequences disappear on their own once
get()is repaired; neither is touched by this PR:X-Tenant-IDwas never stamped for the whole session, not just the documented first-boot window. Per the edge 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 a reader falls through on — the framework scopes from the session — so this is not a data-scoping bug; what was missing is the tenant-routing input, on every request.activeOrgScope()inpackages/app-shell/src/providers/MetadataProvider.tsxreads the same storage, so its org-scoped session cache stayed permanently keyed@noneand the first-boot relabel never fired.packages/app-shell/**is out of scope for this PR; its threeMetadataProviderconsumer tests are run above and stay green.Cross-references #5279, which documented this window's observable consequence without repairing it.
Verification
pnpm exec vitest run packages/authTest Files 21 passed (21)/Tests 218 passed (218)pnpm exec vitest run(3 MetadataProvider consumers)Test Files 3 passed (3)/Tests 13 passed (13)pnpm --filter @object-ui/auth type-checktsc --noEmit && tsc -p tsconfig.test.json)eslint .inpackages/authcheck-control-bytes.mjsOK (scanned 4782 tracked text file(s); skipped 85 binary)check-changeset-presence.mjs2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-no-major.mjsNo changeset declares a major bump.check-changeset-fixed.mjsAll workspace packages are in the changeset fixed group.check-phantom-dependencies.mjsEvery in-scope import is declared by the package that publishes it.check-package-self-import.mjsNo package names itself inside its own src/.The repo-wide
pnpm testandpnpm lintruns are left to CI, which runs the farm exactly once regardless. The local lint above is a declared narrowing: the population is ESLint's own selection (42 files), the count is read from--format json, and no type-aware linting is configured (eslint.config.jssets noprojectService,project:, orparserOptions), so this diff cannot move the verdict on any file it does not itself contain.Generated by Claude Code