fix: validate session token update events#2018
Merged
senamakel merged 3 commits intoMay 18, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds client-side JWT plausibility checks to CoreStateProvider: payloads are base64url-decoded and must have three segments and a numeric, unexpired ChangesSession token validation on update
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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/providers/__tests__/CoreStateProvider.test.tsx (1)
281-330: ⚡ Quick winAdd an explicit expired-token regression test.
This PR contract includes rejecting expired
expvalues, but current additions only lock malformed and unexpired paths.🧪 Suggested test addition
+ it('ignores expired JWT-shaped session-token-updated events (`#1937`)', async () => { + const expiredToken = makeJwt({ exp: Math.floor(Date.now() / 1000) - 1 }); + fetchSnapshot.mockResolvedValue(makeSnapshot({ userId: null, sessionToken: null })); + listTeams.mockResolvedValue([]); + + render( + <CoreStateProvider> + <Consumer /> + </CoreStateProvider> + ); + + await waitFor(() => expect(screen.getByTestId('ready').textContent).toBe('ready')); + + await act(async () => { + window.dispatchEvent( + new CustomEvent('core-state:session-token-updated', { + detail: { sessionToken: expiredToken }, + }) + ); + }); + + expect(screen.getByTestId('token').textContent).toBe('none'); + expect(fetchSnapshot).toHaveBeenCalledTimes(1); + });🤖 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 `@app/src/providers/__tests__/CoreStateProvider.test.tsx` around lines 281 - 330, Add a new test in CoreStateProvider.test.tsx that verifies expired JWT-shaped session-token-updated events are rejected: use makeJwt to create a token with exp set to Math.floor(Date.now()/1000) - 60 (or another past timestamp), stub fetchSnapshot to return initial snapshot (sessionToken null) and to remain unchanged after the event, and ensure dispatching the CustomEvent('core-state:session-token-updated', { detail: { sessionToken: expiredToken } }) does not set the token in the rendered Consumer and fetchSnapshot is not called again; place the test alongside the existing malformed and unexpired tests that use CoreStateProvider, Consumer, fetchSnapshot, listTeams and makeSnapshot for consistency.
🤖 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 `@app/src/providers/__tests__/CoreStateProvider.test.tsx`:
- Around line 281-330: Add a new test in CoreStateProvider.test.tsx that
verifies expired JWT-shaped session-token-updated events are rejected: use
makeJwt to create a token with exp set to Math.floor(Date.now()/1000) - 60 (or
another past timestamp), stub fetchSnapshot to return initial snapshot
(sessionToken null) and to remain unchanged after the event, and ensure
dispatching the CustomEvent('core-state:session-token-updated', { detail: {
sessionToken: expiredToken } }) does not set the token in the rendered Consumer
and fetchSnapshot is not called again; place the test alongside the existing
malformed and unexpired tests that use CoreStateProvider, Consumer,
fetchSnapshot, listTeams and makeSnapshot for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22fb4803-3724-4da7-8f0a-4bede38926a9
📒 Files selected for processing (2)
app/src/providers/CoreStateProvider.tsxapp/src/providers/__tests__/CoreStateProvider.test.tsx
# Conflicts: # app/src/providers/CoreStateProvider.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/providers/__tests__/CoreStateProvider.test.tsx (2)
331-357: ⚡ Quick winStabilize expiry tests by controlling time.
Using
Date.now()directly makes these tests wall-clock dependent. Prefer fixed system time (vi.useFakeTimers()+vi.setSystemTime(...)) or much larger offsets to avoid time-sensitive flakes.As per coding guidelines: “Keep tests deterministic: avoid real network calls, time-sensitive flakes, or hidden global state.”
🤖 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 `@app/src/providers/__tests__/CoreStateProvider.test.tsx` around lines 331 - 357, The tests "ignores expired JWT-shaped session-token-updated events (`#1937`)" and "accepts unexpired JWT-shaped session-token-updated events (`#1937`)" are fragile because they call makeJwt with Date.now(); stabilize them by using vi.useFakeTimers() and vi.setSystemTime(...) to fix the current time (or alternatively construct exp values using a very large offset) before calling makeJwt, and then restore timers with vi.useRealTimers() after each test; update the setup for these tests (referencing makeJwt and the two it(...) blocks) so the expiry calculations are deterministic.
356-380: ⚡ Quick winAssert the refresh side effect on accepted token events.
This test validates token acceptance, but it doesn’t explicitly verify the refresh flow was triggered. Add a call-count assertion so regressions don’t silently pass if only optimistic state update remains.
Suggested assertion
await act(async () => { window.dispatchEvent( new CustomEvent('core-state:session-token-updated', { detail: { sessionToken: token } }) ); }); expect(screen.getByTestId('token').textContent).toBe(token); + expect(fetchSnapshot).toHaveBeenCalledTimes(2);🤖 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 `@app/src/providers/__tests__/CoreStateProvider.test.tsx` around lines 356 - 380, The test "accepts unexpired JWT-shaped session-token-updated events (`#1937`)" accepts the token but doesn't assert the refresh side-effect; after dispatching the 'core-state:session-token-updated' CustomEvent (detail: { sessionToken: token }) add an assertion that the snapshot refresh mock (fetchSnapshot) was invoked (e.g., expect(fetchSnapshot).toHaveBeenCalled() or toHaveBeenCalledTimes with the expected count) to ensure the refresh flow in CoreStateProvider was triggered by the event.
🤖 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 `@app/src/providers/__tests__/CoreStateProvider.test.tsx`:
- Around line 331-357: The tests "ignores expired JWT-shaped
session-token-updated events (`#1937`)" and "accepts unexpired JWT-shaped
session-token-updated events (`#1937`)" are fragile because they call makeJwt with
Date.now(); stabilize them by using vi.useFakeTimers() and vi.setSystemTime(...)
to fix the current time (or alternatively construct exp values using a very
large offset) before calling makeJwt, and then restore timers with
vi.useRealTimers() after each test; update the setup for these tests
(referencing makeJwt and the two it(...) blocks) so the expiry calculations are
deterministic.
- Around line 356-380: The test "accepts unexpired JWT-shaped
session-token-updated events (`#1937`)" accepts the token but doesn't assert the
refresh side-effect; after dispatching the 'core-state:session-token-updated'
CustomEvent (detail: { sessionToken: token }) add an assertion that the snapshot
refresh mock (fetchSnapshot) was invoked (e.g.,
expect(fetchSnapshot).toHaveBeenCalled() or toHaveBeenCalledTimes with the
expected count) to ensure the refresh flow in CoreStateProvider was triggered by
the event.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8327c325-1d7d-457b-875f-692a5f166ae2
📒 Files selected for processing (2)
app/src/providers/CoreStateProvider.tsxapp/src/providers/__tests__/CoreStateProvider.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/providers/CoreStateProvider.tsx
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 free
to 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.
Summary
core-state:session-token-updatedpayloads before accepting a session token.expclaim.Problem
Issue #1937 reports that the
core-state:session-token-updatedwindow event accepted arbitrary strings fromevent.detail.sessionToken. Any code able to dispatch that event could replace the frontend session token until the next authoritative refresh.Solution
CoreStateProviderfor this event-only path.exp, expired tokens, empty tokens, whitespace-wrapped tokens, and non-three-part values.storeSessionTokenpath unchanged so existing Tauri/session storage flows continue to use their current contract.Submission Checklist
## Related— N/A: no matrix feature ID applies.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release manual smoke surface changed.Closes #NNNin the## RelatedsectionImpact
This affects the React frontend
CoreStateProviderevent listener only. Malformed or expired session-token update events are ignored instead of being committed optimistically. Valid unexpired JWT-shaped events continue to trigger the existing optimistic session update and refresh flow.No migration, performance, or dependency impact.
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/validate-session-token-event1145563Validation Run
pnpm --filter openhuman-app format:check— passed via the full pre-push hook after installing the local Rust toolchain.pnpm typecheck—corepack pnpm --filter openhuman-app compilecorepack pnpm --filter openhuman-app exec vitest run --config test/vitest.config.ts src/providers/__tests__/CoreStateProvider.test.tsxpnpm rust:checkpassed; rustfmt check also passed through the full pre-push hook.cargo check --manifest-path app/src-tauri/Cargo.tomlpassed viapnpm rust:check; no Tauri files changed.Validation Blocked
command:N/Aerror:N/Aimpact:N/A — the earliercargo: not foundblocker was resolved by installing the local Rust toolchain; the full pre-push hook now passes.Behavior Changes
core-state:session-token-updatedevent tokens.Parity Contract
storeSessionTokenpath is unchanged; validation is scoped to the window event listener.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests