Skip to content

feat(passkey): Add GET /passkey/wraps/{credentialId} - #21130

Merged
nshirley merged 1 commit into
mainfrom
FXA-13143
Sep 4, 2026
Merged

feat(passkey): Add GET /passkey/wraps/{credentialId}#21130
nshirley merged 1 commit into
mainfrom
FXA-13143

Conversation

@nshirley

@nshirley nshirley commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Because

  • Passwordless sign-in needs the wrap envelope to unseal kB, and the mfa:passkey token minted from a passkey assertion is what authorizes the read.
  • A wrap written before the account's current kB unseals a key Sync no longer uses.

This pull request

  • Adds GET /passkey/wraps/{credentialId}, gated on an mfa:passkey token bound to the credential being fetched, and on the passwordless sync flag. Rate-limited as passkeyWrapsGet.
  • 401s when the token is not bound to the requested credential (errno 223); 404s for a credential with no wrap (234) and a wrap older than keysChangedAt (new errno 236).
  • Adds encodePasskeyWrapEnvelope, keyed off the v1 widths so a new field cannot be dropped by a stale mapping.
  • Records no security event: this runs on every passwordless sign-in.

Issue that this pull request solves

Closes: FXA-13143

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

  • feat(passkey): Mint an MFA token from passkey sign-in #21137 has landed, so this is now based on main and the diff is only the GET endpoint.
  • isBoundTo is shared with the write path, so both routes enforce the credential binding identically. PasskeyService.getPasskeyWrap already landed in FXA-13140.
  • isWrapStale is written inverted (!(createdAt >= keysChangedAt)) so a NaN keysChangedAt withholds rather than serves — every comparison against NaN is false, and this is the only control enforcing the invariant.

Other information (Optional)

  • passkeyWrapsGet has no rate-limit rule configured yet, matching passkeyWrapsCreate and passkeysList. The call site is in place for when one is added.

@nshirley
nshirley force-pushed the FXA-13143 branch 3 times, most recently from 0bf7a3c to 9880e1f Compare September 3, 2026 18:45
@nshirley nshirley changed the title task(passkey): Return the wrap envelope from passkey sign-in feat(passkey): Add GET /passkey/wraps/{credentialId} Sep 3, 2026
@nshirley
nshirley changed the base branch from main to passkey-wrap-mfa-token September 3, 2026 18:47
@nshirley

nshirley commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

PR is currently targeting a non-main branch because it's built off of work from that branch. I'll re-target main when the other branch lands

@nshirley
nshirley force-pushed the FXA-13143 branch 2 times, most recently from 2dc3537 to 97e645f Compare September 3, 2026 19:24
@nshirley
nshirley force-pushed the passkey-wrap-mfa-token branch 3 times, most recently from 26916d7 to c2d14bd Compare September 4, 2026 00:22
Base automatically changed from passkey-wrap-mfa-token to main September 4, 2026 02:57
});

// Deliberately unlike the write path, which binds to the asserted credential.
it('serves a wrap to a token earned on a different credential', async () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not 'business' logic to omit the cid check similar to how the POST endpoint does, so it's a bit odd to have a test for it... but, I could see a case where some frontend logic goes to check for an existing wrap on a different credential, then we later decide to lock down the endpoint more and break that inadvertently. This just gives us an early warning if that ever happens and costs us nothing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this more last night, and I honestly cannot think of any case where we would want to allow fetching a warp with a different credentialId than what granted the token. And, if we did hit a case where we're minting a JWT with a different credential than the wrap we're requesting, that's a separate bug that should be fixed - I'm going to add back the cid check similar to the POST endpoint and then drop this test.

@nshirley
nshirley marked this pull request as ready for review September 4, 2026 04:56
@nshirley
nshirley requested a review from a team as a code owner September 4, 2026 04:56
Copilot AI balanced review requested due to automatic review settings September 4, 2026 04:56

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Because:
- Passwordless sign-in needs the wrap envelope to unseal kB, and the
  mfa:passkey token minted from an assertion authorizes the read.
- A wrap written before the account's current kB unseals a key Sync no
  longer uses, so serving it would break the sign-in it is meant to fix.

This commit:
- Adds GET /passkey/wraps/{credentialId} behind an mfa:passkey token,
  rate-limited as passkeyWrapsGet.
- Requires the token be bound to the credential being fetched, as the
  write does.
- Withholds a wrap older than keysChangedAt as a 404, errno 236.
- Adds encodePasskeyWrapEnvelope, keyed off the v1 widths.
- Records no security event; this runs on every passwordless sign-in.

Closes #FXA-13143
@nshirley
nshirley merged commit c7ae05b into main Sep 4, 2026
21 checks passed
@nshirley
nshirley deleted the FXA-13143 branch September 4, 2026 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants