feat(passkey): Add DELETE /passkey/wraps/{credentialId} - #21165
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It changes passkey authorization and credential-management behavior, warranting final human security review.
Pull request overview
Adds authenticated deletion of individual passkey wrap envelopes without removing the underlying passkey.
Changes:
- Adds the rate-limited DELETE endpoint and security event.
- Implements account-scoped deletion across repository, manager, and service layers.
- Adds auth-client support and comprehensive tests.
File summaries
| File | Description |
|---|---|
packages/fxa-auth-server/test/remote/passkey_wraps.in.spec.ts |
Tests endpoint behavior and authorization. |
packages/fxa-auth-server/lib/routes/passkey-wraps.ts |
Defines the DELETE route and handler. |
packages/fxa-auth-server/lib/routes/passkey-wraps.spec.ts |
Tests routing, rate limiting, and auditing. |
packages/fxa-auth-server/docs/swagger/passkeys-api.ts |
Documents the endpoint. |
packages/fxa-auth-client/test/client.ts |
Tests client request and response handling. |
packages/fxa-auth-client/lib/client.ts |
Exposes deletePasskeyWrap. |
libs/accounts/passkey/src/lib/passkey.wrap.repository.ts |
Adds account-scoped database deletion. |
libs/accounts/passkey/src/lib/passkey.wrap.repository.in.spec.ts |
Tests repository isolation and deletion. |
libs/accounts/passkey/src/lib/passkey.service.ts |
Adds deletion orchestration and metrics. |
libs/accounts/passkey/src/lib/passkey.service.spec.ts |
Tests service outcomes and failures. |
libs/accounts/passkey/src/lib/passkey.manager.ts |
Connects service and repository layers. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7bc20fb to
3aff787
Compare
3aff787 to
f8ff0e1
Compare
f8ff0e1 to
16aed8a
Compare
| // wrap_retrieved restates passkey.authentication_success) or is a side effect | ||
| // they did not take (wrap_deleted rides the passkey-deletion cascade). | ||
| // wrap_retrieved restates passkey.authentication_success) or lands on every | ||
| // passwordless sign-in, at a volume that would crowd out the rest. |
There was a problem hiding this comment.
This would be a separate issue - but this filtering might make more sense on the backend.
There was a problem hiding this comment.
Though I had replied already. but it was funny you mentioned this, I had logged this up right before you commented https://mozilla-hub.atlassian.net/browse/FXA-14492
| 'passkeyWrapsGet' | ||
| ); | ||
|
|
||
| if (!isBoundTo(cid, credentialId)) { |
There was a problem hiding this comment.
Moved below to ensure token check is rate limited?
There was a problem hiding this comment.
Oh I see the explanation above 👌
There was a problem hiding this comment.
oh yep, sorry, meant to add a comment. Something I found while reviewing all of the work so far and figured I could sneak it in here 😬
16aed8a to
0e215cd
Compare
|
Force push was to handle merge conflict and update with main |
bcolsson
left a comment
There was a problem hiding this comment.
Approving but I'd consider adjusting the wording on one of the strings.
| # A passkey is a sign-in method that replaces a password. This string is shown when a passkey that could unlock the user's synced browser data had that access turned off, leaving the passkey itself usable for signing in. | ||
| recent-activity-account-passkey-wrap-deleted = Passkey sync access removed | ||
| # A passkey is a sign-in method that replaces a password. This string is shown when an attempt to turn off a passkey's access to the user's synced browser data did not complete. | ||
| recent-activity-account-passkey-wrap-deletion-failure = Removing passkey sync access failed |
There was a problem hiding this comment.
Might just be me but Passkey sync access removal failed sounds more natural.
0e215cd to
5589a38
Compare
|
Force push was to address the string update |
Because:
- A wrap the client can no longer unseal is permanent, leaving the passkey
stuck without passwordless Sync.
- Settings needs a per-passkey switch for passwordless sign-in.
- POST and GET record their failures; a failed delete would have left no
audit trail.
This commit:
- Adds DELETE /passkey/wraps/{credentialId} behind an mfa:passkey token,
rate-limited as passkeyWrapsDelete, with no credential binding.
- Adds deletePasskeyWrap through the repository, manager, service, and
fxa-auth-client.
- Reports a credential with no wrap as deleted: false, not 404.
- Records wrap_deleted and wrap_deletion_failure, both hidden from the
settings activity list.
- Adds patch 198-199 for the wrap_deletion_failure event name.
5589a38 to
9d4f267
Compare
|
force push was just updating with main |
Because:
stuck without passwordless Sync.
This commit:
rate-limited as passkeyWrapsDelete.