Conversation
Because: * `POST /account/device/destroy` accepted a bare `sessionToken`, so a password-only attacker holding an unverified AAL1 session could enumerate deviceIds via `GET /account/devices` and disconnect the victim's AAL2 devices, cascading to their sessionTokens and device-bound OAuth refresh tokens, without proving the second factor. * No first-party client presents a sessionToken to this route. Firefox Desktop never calls it, signing out via `/session/destroy` and `/oauth/destroy` instead; mobile reaches it with a refreshToken from application-services; `fxa-settings` uses `/account/attached_client/destroy`. This commit: * Gates the route on `verifiedSessionTokenBearer` and `verifiedSessionToken`, matching the sibling `/account/attached_client/destroy`, and keeps `refreshToken` so the AAL checks never apply to mobile. * Pins the strategy chain's fall-through contract. This is the first route to put `verifiedSessionToken` ahead of `refreshToken`, and Hapi only advances to the next strategy when the error carries `isMissing`. Mobile sends a plain `Bearer <hex>`, which the Hawk-backed strategy rejects via `Boom.unauthorized(null, 'Hawk')`; losing that flag would 401 every mobile disconnect. * Asserts the strategy list on the route, following the existing `/account/attached_client/destroy` test. * Drops the incidental `destroyDevice` teardown from two remote device registration tests and adds one asserting that an unconfirmed account is now rejected with errno 104. Inheriting the strategy's email-verified check is wider than the finding requires, but it matches the already-shipped sibling route and no first-party client reaches this path with a sessionToken. Leaves `GET /account/devices` unchanged: Sync clients need the device list on AAL1 sessions, and it is same-account read-only disclosure to someone who already has the password.
LZoog
approved these changes
Aug 10, 2026
LZoog
left a comment
LZoog
left a comment
Contributor
There was a problem hiding this comment.
LGTM, but I'd double check that clients uses the refresh token for this and not session token. Since Desktop drops it, is there a scenario where a user signs into Firefox on Desktop device A, then on another device B sets up TOTP, and then on device A could there be an issue when trying to sign out / destroy that session/device?
| expect(devices[0].pushEndpointExpired).toBe(false); | ||
| }); | ||
|
|
||
| it('device disconnect fails on an unconfirmed account', async () => { |
Contributor
There was a problem hiding this comment.
Should we have a test for an account with TOTP and only entering the password?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because
POST /account/device/destroyrequire a verifiedSessionTokenThis pull request
devices-and-sessions.jsonverifiedSessionTokenBearerandverifiedSessionToken, matching the sibling/account/attached_client/destroy, and keepsrefreshTokenso the AAL checks never apply to mobile.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-14033
Checklist
Other information