test(frontend/auth): cover cross-tab logout sync (closes #493) - #535
Conversation
PR #487 added installStorageListener() in api/client.ts as the compensating control for moving tokens off sessionStorage. The behaviour is implemented but had no automated regression test; if the listener's key filter, newValue check, or idempotency guard ever regresses, the SECURITY claim in client.ts becomes silently false. Add crosstab-session.test.ts covering: - cleared auth keys (authToken, apiKey, csrfToken) zero in-memory state and call window.location.reload - non-auth keys are ignored - partial updates (newValue !== null, e.g. token refresh) are ignored - initAuth() is idempotent: storage listener installs exactly once Each test loads api/client in jest.isolateModules() so the storageListenerInstalled guard is fresh per test.
Capture the original window.location reference before tests and restore it in an afterEach, so each test starts from a clean Location object. This implements CR's teardown recommendation while keeping the Object.defineProperty(window, 'location', ...) approach required by jsdom (window.location.reload is non-configurable and cannot be patched directly).
|
Warning Review limit reached
More reviews will be available in 7 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Jest test file that captures the storage event listener installed by api.initAuth(), simulates StorageEvent notifications for auth key changes, and asserts in-memory auth invalidation, single reload, ignore conditions, and listener idempotency. ChangesCross-tab logout sync test coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
Rate Limit Exceeded
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/__tests__/crosstab-session.test.ts (1)
13-27: ⚡ Quick winConsider adding localStorage cleanup for test isolation.
Each test uses
jest.isolateModulesto get fresh module state, which is excellent. However,localStorageis global and persists across tests. Sinceapi.initAuth()reads fromlocalStorage(per the relevant snippet), stale values from previous tests could potentially be loaded.While the tests currently pass because they explicitly set auth state before assertions, adding explicit cleanup in
afterEachwould strengthen test isolation:afterEach(() => { Object.defineProperty(window, 'location', { configurable: true, value: originalLocation }); localStorage.clear(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/crosstab-session.test.ts` around lines 13 - 27, Add explicit test cleanup to ensure isolation by clearing localStorage and restoring any mocked window.location after each test; specifically, in the test file containing loadAuth and calls to api.initAuth(), add an afterEach that calls localStorage.clear() and restores the original window.location (the value saved before overriding in loadAuth) so api.initAuth() cannot read stale values between tests and the mocked reload is removed; ensure the restoration references the same originalLocation used when mocking window.location and also restores/clears the jest spy addSpy if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/__tests__/crosstab-session.test.ts`:
- Around line 13-27: Add explicit test cleanup to ensure isolation by clearing
localStorage and restoring any mocked window.location after each test;
specifically, in the test file containing loadAuth and calls to api.initAuth(),
add an afterEach that calls localStorage.clear() and restores the original
window.location (the value saved before overriding in loadAuth) so
api.initAuth() cannot read stale values between tests and the mocked reload is
removed; ensure the restoration references the same originalLocation used when
mocking window.location and also restores/clears the jest spy addSpy if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18c2ff47-b1b4-4864-b62a-fedb0a99a0ee
📒 Files selected for processing (1)
frontend/src/__tests__/crosstab-session.test.ts
…s (CR nit) initAuth() seeds in-memory auth from localStorage, so without clearing it between tests a stale token from one case could leak into the next. Extend the existing afterEach to call localStorage.clear() for proper isolation.
|
Addressed the test-isolation nitpick: the existing afterEach now also calls localStorage.clear(), since initAuth() seeds in-memory auth from localStorage and a stale token from one case could otherwise leak into the next. crosstab-session tests (6) pass, tsc and build are clean. @coderabbitai review |
|
Triggering a full review now. ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/__tests__/crosstab-session.test.ts (2)
16-30: 💤 Low valueHelper correctly captures the storage listener.
The
loadAuth()function properly usesjest.isolateModulesto obtain a fresh module instance and spies onwindow.addEventListenerto capture the storage handler installed byinitAuth().♻️ Optional: Replace non-null assertion with explicit check
Line 29 uses a non-null assertion (
out!). While safe, an explicit check would be clearer:function loadAuth(): { api: typeof import('../api'); handler: (e: StorageEvent) => void; reload: jest.Mock; addSpy: jest.SpyInstance } { let out: ReturnType<typeof loadAuth> | undefined; jest.isolateModules(() => { const addSpy = jest.spyOn(window, 'addEventListener'); const reload = jest.fn(); Object.defineProperty(window, 'location', { configurable: true, value: { ...window.location, reload } }); // eslint-disable-next-line `@typescript-eslint/no-require-imports` const api = require('../api') as typeof import('../api'); api.initAuth(); const call = addSpy.mock.calls.find(c => c[0] === 'storage'); if (!call) throw new Error('initAuth() did not install a storage listener'); out = { api, handler: call[1] as (e: StorageEvent) => void, reload, addSpy }; }); - return out!; + if (!out) throw new Error('isolateModules did not execute'); + return out;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/crosstab-session.test.ts` around lines 16 - 30, The helper loadAuth currently returns out! using a non-null assertion; replace this with an explicit check to avoid the assertion. After jest.isolateModules finishes, verify that out is defined (e.g. if (!out) throw new Error('loadAuth failed to capture storage listener')) and then return out (without using !). Update the loadAuth signature or runtime check as needed so callers get a concrete value or a clear error; reference the loadAuth function, the out local, and the addSpy/call check to guide where the validation should be added.
46-60: 💤 Low valueTests correctly verify guard conditions.
Both tests validate that the storage listener properly filters events: non-auth keys and partial updates (newValue !== null) are ignored, matching the implementation in
client.ts.♻️ Optional: Add test case for null key
The storage listener in
client.tsalso guards against!e.key. Consider adding a test to confirm thatevt(null, null)is ignored:test('ignores events with null key (e.g., localStorage.clear())', () => { const { api, handler, reload } = loadAuth(); api.setAuthToken('t'); handler(evt(null, null)); expect(api.isAuthenticated()).toBe(true); expect(reload).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/crosstab-session.test.ts` around lines 46 - 60, Add a test that verifies the storage listener ignores events with a null key by reusing the existing test pattern: call loadAuth() to get { api, handler, reload }, set the token via api.setAuthToken('t'), invoke the listener with handler(evt(null, null)), and assert api.isAuthenticated() remains true and reload was not called; place the test alongside the existing 'ignores non-auth keys' and 'ignores partial updates' tests and name it e.g. 'ignores events with null key (e.g., localStorage.clear())' so it covers the !e.key guard in client.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/__tests__/crosstab-session.test.ts`:
- Around line 16-30: The helper loadAuth currently returns out! using a non-null
assertion; replace this with an explicit check to avoid the assertion. After
jest.isolateModules finishes, verify that out is defined (e.g. if (!out) throw
new Error('loadAuth failed to capture storage listener')) and then return out
(without using !). Update the loadAuth signature or runtime check as needed so
callers get a concrete value or a clear error; reference the loadAuth function,
the out local, and the addSpy/call check to guide where the validation should be
added.
- Around line 46-60: Add a test that verifies the storage listener ignores
events with a null key by reusing the existing test pattern: call loadAuth() to
get { api, handler, reload }, set the token via api.setAuthToken('t'), invoke
the listener with handler(evt(null, null)), and assert api.isAuthenticated()
remains true and reload was not called; place the test alongside the existing
'ignores non-auth keys' and 'ignores partial updates' tests and name it e.g.
'ignores events with null key (e.g., localStorage.clear())' so it covers the
!e.key guard in client.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 153855e7-97ba-4f2d-bcdc-230bbc20496a
📒 Files selected for processing (1)
frontend/src/__tests__/crosstab-session.test.ts
- replace the out! non-null assertion in loadAuth() with an explicit guard that throws a clear error if isolateModules never ran - add a regression case for the !e.key guard in installStorageListener: a null-key storage event (localStorage.clear()) must not clear in-memory auth or reload
|
Addressed both nitpicks from the latest review:
@coderabbitai review |
|
7 passing tests + clean build looks good. Triggering a full review now. ✅ Action performedFull review finished. |
Summary
frontend/src/__tests__/crosstab-session.test.tswhich fires aStorageEventagainst the token key and asserts the second-tab listener clears in-memory auth and triggerswindow.location.reload().window.location.reloadviaObject.definePropertyrather than direct assignment, matching jsdom's constraint thatlocationis non-configurable (CodeRabbit nit addressed in second commit).This test is the regression guard for
installStorageListener()added in PR #487, which is the compensating control for widening the XSS exposure window from tab-close to explicit-logout when tokens moved offsessionStorage.Closes #493.
Test plan
npx jest --testPathPattern="crosstab"passes (1 suite, 4 assertions).settings-purchasing-gracefailure is inorigin/feat/multicloud-web-frontendand is unrelated to this PR.Summary by CodeRabbit