feat(passkey): Mint an MFA token from passkey sign-in - #21137
Conversation
d0f3bf2 to
77cd5fa
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The production client flow cannot request or consume the new token, and the API documentation remains inconsistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds MFA token minting to passkey authentication and uses credential-bound tokens to protect passkey wrap creation.
Changes:
- Persists requested MFA scopes through passkey challenges.
- Centralizes MFA JWT signing and adds credential binding.
- Guards wrap creation with
mfa:passkeyand expands tests.
File summaries
| File | Description |
|---|---|
packages/fxa-auth-server/test/remote/passkey_wraps.in.spec.ts |
Tests credential-bound wrap authorization. |
packages/fxa-auth-server/lib/routes/utils/mfa-token.ts |
Adds shared MFA JWT signer. |
packages/fxa-auth-server/lib/routes/utils/mfa-token.spec.ts |
Tests MFA token claims. |
packages/fxa-auth-server/lib/routes/passkeys.ts |
Mints MFA tokens after passkey authentication. |
packages/fxa-auth-server/lib/routes/passkeys.spec.ts |
Extends route configuration fixture. |
packages/fxa-auth-server/lib/routes/passkey-wraps.ts |
Requires credential-bound MFA authorization. |
packages/fxa-auth-server/lib/routes/passkey-wraps.spec.ts |
Tests binding enforcement. |
packages/fxa-auth-server/lib/routes/mfa.ts |
Uses the shared token signer. |
packages/fxa-auth-server/lib/routes/auth-schemes/mfa.ts |
Propagates the credential claim. |
packages/fxa-auth-server/lib/routes/auth-schemes/mfa.spec.ts |
Tests credential propagation. |
libs/accounts/passkey/src/lib/passkey.service.ts |
Returns scope and canonical credential ID. |
libs/accounts/passkey/src/lib/passkey.service.spec.ts |
Tests authentication result changes. |
libs/accounts/passkey/src/lib/passkey.challenge.manager.ts |
Stores MFA scope with authentication challenges. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
77cd5fa to
7a06d5a
Compare
|
Fix for unrelated unit test failures is here: #21142 |
Including the cid in the jwt token seems more robust to me. It makes little difference for now, but I can see this approach being superior if/when we add the option to enable passkey wraps from settings -> conceivably, if multiple passkeys are accessible on the same device, we'll want to match the token to the passkey being "upgraded" not to the passkey used for the initial session creation (otherwise we could have a conflict). |
vpomerleau
left a comment
There was a problem hiding this comment.
LGTM, I like where we landed for the auth strategy!
Do we have an existing ticket for JWT token minting on retry (after initial sign-in)?
| */ | ||
| function sameCredential(a: string, b: string): boolean { | ||
| try { | ||
| return Buffer.from(a, 'base64url').equals(Buffer.from(b, 'base64url')); |
There was a problem hiding this comment.
Would this ever throw, or can the try/catch be omitted?
There was a problem hiding this comment.
If, somehow, either value were not an actual string then it could throw. I suppose we don't have such wrappers on other Buffer.from calls so yeah, probably safe to drop it. At that point, probably doesn't need to be a dedicated function, so I'll put it back into the handler as is
| ).rejects.toMatchObject({ code: 401 }); | ||
| }); | ||
|
|
||
| it('refuses a token earned on a different credential', async () => { |
There was a problem hiding this comment.
This looks like a dupe of the test on L276?
7a06d5a to
26916d7
Compare
Because: - POST /passkey/wraps takes only a verified session, and the inline sign-in flow has no MFA JWT to guard it with — the user has just signed in with a passkey. - A passkey assertion can earn the same mfa:<scope> token an emailed code does, so the wrap route can use the guard every other MFA-protected route already uses. This commit: - Accepts an optional scope on /passkey/authentication/start, validated against config.mfa.actions and stored on the challenge before the user is prompted. - Reads that scope back off the consumed challenge at /finish and mints an mfa:<scope> token, once the session its claims bind to exists. - Returns the stored credentialId from verifyAuthenticationResponse and mints it as `cid`, carried onto the session token beside the scope the mfa strategy already copies. - Guards POST /passkey/wraps on mfa:passkey, matching `cid` against the request by bytes since that is how the id is stored and looked up, and updates the passkey Swagger notes to match. - Moves the JWT claim set into one signer, so /mfa/otp/verify and the assertion path cannot drift on the shape the strategy verifies.
26916d7 to
c2d14bd
Compare

Because
POST /passkey/wrapstakes only a verified session, and the inline sign-in flow has no MFA JWT to guard it with — the user has just signed in with a passkey.mfa:<scope>token an emailed code does, so the wrap route can use the guard every other MFA-protected route already uses.This pull request
scopeonPOST /passkey/authentication/start, validated againstconfig.mfa.actionsand stored on the challenge before the user is prompted./finishand mints anmfa:<scope>token, once the session its claims bind to exists.cid, carried onto the session token beside the scope the mfa strategy already copies, and matched at the wrap route.POST /passkey/wrapsonmfa:passkey.scoperequest field and conditionalmfaTokenresponse on the authentication routes./mfa/otp/verifyand the assertion path cannot drift on the shape the strategy verifies.Issue that this pull request solves
Closes: N/A — follows on from FXA-13142.
Checklist
Put an
xin the boxes that applyHow to review (Optional)
lib/routes/utils/mfa-token.ts, and the mint point inauthenticationFinish.mfa-token.ts→passkeys.ts→auth-schemes/mfa.ts→passkey-wraps.tsstrategy: 'mfa'and its existing consumers are unchanged —credentialsis still the session token, withcidadded beside thescopethe strategy already copies onto it./mfa/otp/verifynow signs through the shared helper; the claim set is the same, and its existing tests verify real tokens end to end.Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
mfa:passkeyis the scope passkey registration and deletion already use. The same scope will cover upgrading an existing passkey from settings.cidcould instead live on the session row, which would keep the mfa machinery generic and suit the session-AMR gate discussed separately. Left alone here since that would need asessionTokensmigration.