fix(auth): validate JWT exp claim in getSession before returning cached session - #2542
fix(auth): validate JWT exp claim in getSession before returning cached session#2542clanzhang wants to merge 3 commits into
Conversation
…ed session Previously, getSession() only checked the session-level expires_at field set by the server response. If the JWT's own exp claim had already passed (e.g. clock skew, token tampering, or server-side revocation), the client would still return the session as valid, causing downstream requests to fail with 401 errors. This adds a JWT exp decode step in __loadSession() so the access token's actual expiry is verified before the session is returned to callers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
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. 🔧 Biome (2.5.3)packages/core/auth-js/test/GoTrueClient.test.tsFile contains syntax errors that prevent linting: Line 4982: expected 🔧 ESLint
packages/core/auth-js/test/GoTrueClient.test.tsParsing error: '}' expected. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/core/auth-js/src/GoTrueClient.ts`:
- Around line 2984-3000: Update GoTrueClient.ts lines 2984-3000 to remove the
JWT expiration branch and unconditionally assign currentSession = maybeSession
after decoding, while retaining catch-based cleanup for malformed tokens. Update
GoTrueClient.test.ts lines 348-394 to verify that an expired JWT reaches the
refresh flow, asserting either the expected unmocked refresh error or a mocked
successful refresh response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d081a133-0bcf-4c88-bc71-531830eadc3d
📒 Files selected for processing (2)
packages/core/auth-js/src/GoTrueClient.tspackages/core/auth-js/test/GoTrueClient.test.ts
…ing session Previously, getSession() would destroy the session when the JWT exp had expired, even if the refresh token was still valid. Now the session is preserved and flows through to the existing refresh mechanism via drift correction, allowing recovery when the refresh token is still good.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| // will align expires_at with the JWT exp, and the existing | ||
| // hasExpired check will trigger a refresh if needed. | ||
| try { | ||
| decodeJWT(maybeSession.access_token) |
There was a problem hiding this comment.
⚪ Severity: LOW
This parses the JWT but does not validate required payload claims such as a numeric exp. A stored token with a null, missing, or non-object payload therefore reaches currentSession; the later payload.exp check skips or throws, and getSession() returns it using stale expires_at instead of clearing it.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: After decoding the JWT, validate that the decoded payload is a non-null object and that exp is a finite number before assigning currentSession. If the payload is invalid, clear the session instead of using it. Replace the two lines that decode the JWT and assign currentSession with validation logic:
const { payload } = decodeJWT(maybeSession.access_token)
if (payload && typeof payload === 'object' && typeof payload.exp === 'number' && isFinite(payload.exp)) {
currentSession = maybeSession
} else {
this._debug('#getSession()', 'JWT payload missing valid exp claim')
await this._removeSession()
}
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| decodeJWT(maybeSession.access_token) | |
| const { payload } = decodeJWT(maybeSession.access_token) | |
| if (payload && typeof payload === 'object' && typeof payload.exp === 'number' && isFinite(payload.exp)) { | |
| currentSession = maybeSession | |
| } else { | |
| this._debug('#getSession()', 'JWT payload missing valid exp claim') | |
| await this._removeSession() | |
| } |
1 participant
🔍 Description
What changed?
getSession()now independently decodes the JWT access token'sexpclaim before returning a cached session. Previously it only checked the session-levelexpires_atfield set by the server response.Two additions in
__loadSession():exphas already passed, the session is invalidated and cleared, even ifexpires_atis still in the future.expand sessionexpires_atdiverge by more than 60 seconds,expires_atis corrected to match the JWTexp(the actual token-level source of truth).Malformed JWTs are handled gracefully (session cleared, no crash).
Why was this change needed?
getSession()trusts the session-levelexpires_atwithout verifying the JWT's ownexpclaim. This causes problems when:expires_atsuggests.In all these cases, developers receive a non-null session from
getSession()but downstream API calls fail with 401 — a confusing debugging experience, especially in SSR/Edge environments.Closes #1191
Approach considered and rejected
_isValidSession()— rejected because it's a synchronous structural check; adding async JWT decode would break its type guard contract.expires_atat storage-write time — rejected because it doesn't catch post-write corruption or clock drift that happens after the session is stored.__loadSession()(this PR) — chosen because it's the single read path for allgetSession()callers, and keeps the existing expiry-margin refresh logic intact.📸 Screenshots/Examples
Before:
getSession() → { session: { access_token: "expired-jwt", ... }, error: null }→ API call → 401 Unauthorized
After:
getSession() → { session: null, error: null }→ caller knows immediately, can redirect to login
None. Normal session behavior is unchanged. The new logic only triggers when the JWT
exphas genuinely expired or diverges significantly fromexpires_at.🧪 Tests added
3 new test cases (no Docker/backend required):
getSession() invalidates session when JWT exp has expired but expires_at is in the futuregetSession() corrects expires_at when JWT exp diverges by more than 60sgetSession() clears session when JWT is malformed