-
Notifications
You must be signed in to change notification settings - Fork 557
feat(auth): add optional jwt parameter to getAuthenticatorAssuranceLevel #1940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
mandarini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@7ttp thanks for this PR. Can you please:
- Add some tests for the new JWT code path. Should test:
- JWT with aal1, no factors returns aal1/aal1
- JWT with aal1, verified factors returns aal1/aal2
- Invalid JWT returns error
- JWT where getUser fails returns error - Fix the documentation mismatch. tsdoc says "rarely uses the network" but the new path calls getUser(jwt) which always makes a network request
After these are done, I will share with auth team to get final ok!
| if (jwt) { | ||
| try { | ||
| const { payload } = decodeJWT(jwt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Severity: HIGH
Critical: JWT signature not verified before using claims. The code calls decodeJWT() which only decodes the JWT without verifying its signature, then immediately uses payload.aal and payload.amr from the unverified token. An attacker can craft a malicious JWT with elevated aal levels or fake authentication methods. Although getUser(jwt) is called later, the unverified claims are already extracted and returned to the caller. Only use claims after successful getUser() verification.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Move JWT decoding to after getUser() verification succeeds. Currently, the code decodes the JWT and extracts aal/amr claims before verifying the token's signature via getUser(). This allows unverified claims to be extracted and returned. The fix reorders the code to: 1) Call getUser(jwt) first to verify the token server-side, 2) Only if verification succeeds, decode the JWT and extract claims, 3) Return the claims. This ensures only verified tokens have their claims used.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if (jwt) { | |
| try { | |
| const { payload } = decodeJWT(jwt) | |
| if (jwt) { | |
| try { | |
| // Verify the JWT first by calling getUser | |
| const { | |
| data: { user }, | |
| error: userError, | |
| } = await this.getUser(jwt) | |
| if (userError) { | |
| return this._returnResult({ data: null, error: userError }) | |
| } | |
| // Only decode and use JWT claims after verification succeeds | |
| const { payload } = decodeJWT(jwt) | |
| let currentLevel: AuthenticatorAssuranceLevels | null = null | |
| if (payload.aal) { | |
| currentLevel = payload.aal | |
| } | |
| let nextLevel: AuthenticatorAssuranceLevels | null = currentLevel | |
| const verifiedFactors = | |
| user?.factors?.filter((factor: Factor) => factor.status === 'verified') ?? [] | |
| if (verifiedFactors.length > 0) { | |
| nextLevel = 'aal2' | |
| } | |
| const currentAuthenticationMethods = payload.amr || [] | |
| return { data: { currentLevel, nextLevel, currentAuthenticationMethods }, error: null } |
Summary
Adds an optional
jwtparameter tomfa.getAuthenticatorAssuranceLevel(), enabling it to work in server side environments like Edge Functions where no session is stored.Problem
In Edge Functions,
getAuthenticatorAssuranceLevel()always returns{ currentLevel: null, nextLevel: null, currentAuthenticationMethods: [] }because it relies ongetSession()which reads from storage. Edge Functions don't have a stored session, only the JWT from the Authorization header.Solution
Added an optional
jwtparameter following the same pattern asgetUser(jwt?):closes #1677