Conversation
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
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
metricsContextpayload and never check it.METRICS_CONTEXT_SCHEMAonly validates the shape, so nothing verifies the HMAC signature onflowId.auth: false. Any caller can make up aflowIdand we record it in flow events and in the OTP email metadata. To be clear about severity: this is a metrics integrity bug, not an auth bypass. No account, session, or token decision reads the value.This pull request
request.validateMetricsContext()tosendCode,confirmCode, andresendCodeinpasswordless.ts. Same one-liner as the existing call sites insession.js,unblock-codes.js,account.ts, andpassword.ts.passwordless.spec.tsfor a valid context, a forged signature, and an expiredflowBeginTimeon each route, plus a check that a forgedflowIdnever reaches the OTP email. These run the realvalidate()fromlib/metrics/context.js, not a stub, so they cover the HMAC check itself.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-14340
Checklist
Put an
xin the boxes that applyOther information
Worth a second opinion before this lands:
validate()does not throw. On failure it deletesflowIdandflowBeginTimeand returns false. So if passwordless clients are not sending a signedflowIdtoday, this strips the field and the flow metrics for these three routes will change. That call is the reviewer's, not mine.