Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Adds PKCE regression coverage across OAuth validation, token redemption, Firefox parameter forwarding, and WebChannel isolation.
Changes:
- Tests
/oauth/tokencredential XOR and PKCE rejection paths. - Adds end-to-end authorization-code redemption coverage.
- Verifies Firefox does not expose or forward the PKCE verifier.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
firefox.test.ts |
Tests OAuth parameter allowlisting. |
oauth_tests.in.spec.ts |
Tests redemption without a verifier. |
token.spec.ts |
Tests XOR validation and PKCE guards. |
codeVerifierIsolation.spec.ts |
Tests verifier isolation in Firefox/WebChannel. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+222
to
+224
| for (const message of replies) { | ||
| expect(JSON.stringify(message)).not.toContain(storedVerifier); | ||
| } |
Comment on lines
+1915
to
+1920
| it('rejects a verifier that does not hash to the stored challenge', async () => { | ||
| await expect( | ||
| redeem(CHALLENGED_CODE, { | ||
| code_verifier: 'w'.repeat(PKCE_CODE_VERIFIER.length), | ||
| }) | ||
| ).rejects.toMatchObject({ errno: OAUTH_ERRNO.INCORRECT_CODE_CHALLENGE }); |
Comment on lines
+128
to
+130
| test('Firefox keeps the code_verifier out of the params it hands to content', async ({ | ||
| marionetteAuthority, | ||
| }) => { |
Because: - The pairing security review turns on two claims: FxA never sees the code_verifier Firefox generates, and a code cannot be redeemed without it. - Nothing proved the first claim, and the handler PKCE gate had no tests at all. The /oauth/token xor that Firefox's redemption depends on had none either. This commit: - Adds six tests for the /oauth/token guards: the authorization_code xor a public client cannot sidestep with a client_secret, and the handler PKCE gate. - Adds an integration test that refuses to redeem a real code without the verifier, sending grant_type explicitly as Firefox does. - Adds a functional test driving real Firefox over Marionette, proving the verifier reaches neither the params Firefox hands to content nor page script. - Adds a test pinning buildSyncOAuthSearch to its allowlist, so no extra web-channel field can reach /authorization. - Hoists the /oauth/token payload validation helper so both describes share it.
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
code_verifierFirefox generates, and a code cannot be redeemed without it.mismatchCodeChallengeandmissingPkceParametersappeared nowhere in the auth-server suite. The/oauth/tokenxor that Firefox's redemption depends on had none either.This pull request
token.spec.tsfor the/oauth/tokenauthorization_code xor, the guard a public client cannot sidestep with aclient_secret.oauth_tests.in.spec.tsthat refuses to redeem a real code without the verifier.codeVerifierIsolation.spec.tsdriving a real Firefox over Marionette, asserting the verifier reaches neither the params handed to content nor any WebChannel reply.firefox.test.tspinningbuildSyncOAuthSearchto its allowlist, so no extra web-channel field reaches/authorization./oauth/tokenpayload validation helper so both describes share it.Tests only. No production code changed.
Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-14316
Checklist
Put an
xin the boxes that applyOther information
Both ACs are covered. AC 2 is covered at the validation, handler, and integration layers. AC 1 is covered by
codeVerifierIsolation.spec.ts, which drives a real Firefox over Marionette rather than a mock: it reads the verifier in chrome context, then asserts the value appears in neither the paramsbeginOAuthFlow()hands back nor any WebChannel envelope that page script can observe.firefox.test.tscovers the FxA half, that FxA cannot forward a verifier it is handed.Mutation-verified against this diff:
.xor('client_secret', 'code_verifier')fails both xor negatives.if (codeObj.codeChallenge)gate fails both gate negatives, while the happy path keeps passing.buildSyncOAuthSearchallowlist with a spread fails the settings test, which then seescode_verifier,sessionTokenandunexpectedon the wire.Deliberate scope limits.
/authorizationpins the method to S256 and writes a challenge if and only if the client is public.token.js:355callscrypto.timingSafeEqualdirectly, so an unequal-length storedcodeChallengethrowsRangeErrorrather than returningINCORRECT_CODE_CHALLENGE. It fails closed and issues no token. Swapping in the length-safebufferEqualsConstantTimeis a production change, so it stays out of this tests-only PR and is tracked separately.How to test: