Repository navigation
fix(auth): accept stored scopes over 256 characters in OAuth responses - #21569
Open
fxa-agent[bot] wants to merge 1 commit into
Open
fxa-agent[bot] wants to merge 1 commit into
fxa-agent[bot] wants to merge 1 commit into
Conversation
Because: - The /token and /oauth/token response schemas used the 256-char request scope validator. - A stored access-token scope can be longer, so the response check fails. This commit: - Adds a response-only storedScopeString validator to lib/oauth/validators.js. - Uses it for every response scope in token.js and in introspect.js. - Adds response-schema and validator specs for a scope over 256 chars. Closes FXA-14812
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Token exchange may fail at the database persistence boundary before response validation runs.
1 open issue
What changed in this PR
This PR updates OAuth response validation to accept stored scopes longer than 256 characters without changing request validation.
Changes:
- Adds the
storedScopeStringresponse validator. - Applies it to token and introspection responses.
- Adds validator and response-schema tests.
| File | Summary |
|---|---|
packages/fxa-auth-server/lib/routes/oauth/token.spec.ts |
Adds long-scope response tests. |
packages/fxa-auth-server/lib/routes/oauth/token.js |
Uses the stored-scope validator; token-exchange persistence remains unresolved. |
packages/fxa-auth-server/lib/routes/oauth/introspect.js |
Reuses the stored-scope validator. |
packages/fxa-auth-server/lib/oauth/validators.spec.ts |
Tests long and empty stored scopes. |
packages/fxa-auth-server/lib/oauth/validators.js |
Defines the response-only stored-scope validator. |
Review effort: Lite
| access_token: validators.accessToken.required(), | ||
| refresh_token: validators.refreshToken.required(), | ||
| scope: validators.scope.required(), | ||
| scope: validators.storedScopeString.required(), |
Contributor
Author
There was a problem hiding this comment.
Not changed: Out of scope: the refresh token column cap needs a DB migration or a product decision, so it goes to a follow-up ticket
This branch has not been deployed
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
/tokenand/oauth/tokenresponse schemas used the requestscopevalidator. That validator caps the scope at 256 characters./introspectwith an inline validator.This pull request
storedScopeString(Joi.string().allow('')) tolib/oauth/validators.js. It is for responses only.scopefields inlib/routes/oauth/token.js.lib/routes/oauth/introspect.jsin place of the inline validator.token.spec.tsandstoredScopeStringtests invalidators.spec.ts.scopevalidators.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-14812
Checklist
Put an
xin the boxes that applyHow to review (Optional)
lib/oauth/validators.jsand the response schemas inlib/routes/oauth/token.js.token.js,introspect.js, specs.Screenshots (Optional)
Other information (Optional)
Where the validator lives. The ticket suggested
lib/routes/validators.js.token.jsandintrospect.jsimportlib/oauth/validators.js, so the validator is next to the OAuthscopevalidator.Not changed on purpose. The
/authorizationresponsescopefields (authorization.jslines 605 and 694) use an inlineJoi.string()with no length cap. They do not fail on a long scope. I left them for a follow-up.Tests I ran (
/fxa-verify --plan), all PASS:test/remote/oauth_token_route.in.spec.ts, eslint, prettier, andtscwithtsconfig.build.json.POST /v1/oauth/token(fxa-credentialsgrant, Settings public client). It returnedscope=profile token_type=bearer refresh=true./fxa-verify --revertontoken.spec.ts: the four new response-schema cases fail without the fix and pass with it.Not run. The 123done sign-in spec
tests/oauth/signin.spec.tscould not run here. The token exchange failed with errno 109 because the sandbox has nopackages/123done/secrets.json. CI covers it.Not reachable over HTTP locally. A scope over 256 characters did not occur on the local stack. The long-scope case is covered only by the unit specs.
Question for the reviewer: does this fix the reported 500 on its own? I traced how a scope can pass 256 characters:
scopeis capped at 256.refresh_tokengrant replaces the scope withparams.scope, so it stays at or under 256.codes.scope VARCHAR(256).combinedScope = subjectToken.scope.union(requestedScope)(token.jsline 618). That scope can reach about 512 characters.generateTokensinlib/oauth/grant.jswrites the access token to Redis first. It then inserts the refresh token intorefreshTokens.scope VARCHAR(256). The MySQL connection usesSTRICT_ALL_TABLES, so that insert should fail before the response schema runs. This would leave an orphan access token in Redis.The token exchange path may be a third site that needs a fix. I changed no code for this.