Conversation
0bf7a3c to
9880e1f
Compare
|
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 |
2dc3537 to
97e645f
Compare
26916d7 to
c2d14bd
Compare
| }); | ||
|
|
||
| // Deliberately unlike the write path, which binds to the asserted credential. | ||
| it('serves a wrap to a token earned on a different credential', async () => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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
Because
kB, and themfa:passkeytoken minted from a passkey assertion is what authorizes the read.kBunseals a key Sync no longer uses.This pull request
GET /passkey/wraps/{credentialId}, gated on anmfa:passkeytoken bound to the credential being fetched, and on the passwordless sync flag. Rate-limited aspasskeyWrapsGet.keysChangedAt(new errno 236).encodePasskeyWrapEnvelope, keyed off the v1 widths so a new field cannot be dropped by a stale mapping.Issue that this pull request solves
Closes: FXA-13143
Checklist
Put an
xin the boxes that applyHow to review (Optional)
mainand the diff is only the GET endpoint.isBoundTois shared with the write path, so both routes enforce the credential binding identically.PasskeyService.getPasskeyWrapalready landed in FXA-13140.isWrapStaleis written inverted (!(createdAt >= keysChangedAt)) so aNaNkeysChangedAtwithholds rather than serves — every comparison againstNaNis false, and this is the only control enforcing the invariant.Other information (Optional)
passkeyWrapsGethas no rate-limit rule configured yet, matchingpasskeyWrapsCreateandpasskeysList. The call site is in place for when one is added.