Repository navigation
Refuse an OAuth parameter that arrives as a list where one value is meant - #3236
Conversation
…eant A name[]= form field or a JSON array or object reaches params as an Array or nested Parameters. Handed to find_by(client_id: ...), an Array becomes an SQL IN list: client_id[]=<id> found the client it named, past the exact-string comparison every other check assumes. Codex found this on #3228 (CIMD), which fixed its own path; this closes the class on the train's endpoints. Each OAuth endpoint refuses any single-valued parameter (RFC 6749 §3.1) that isn't a string or null, after its availability gate and before any lookup: invalid_request at the token and revocation endpoints, invalid_client_metadata at registration, where scope stays a permitted list. The authorization endpoint refuses client_id and redirect_uri before it finds the client, on its own page, and the rest once the redirect target is validated, so they reach a vetted client as an error redirect like any other. client_secret stays with Oauth::ClientAuthentication, where a non-string secret already fails as invalid_client. JSON parameter wrapping is off for OAuth controllers: it copied every JSON body under the controller's name, which for the token endpoint is token, itself an OAuth parameter.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Status at |
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive stacked change remains held for Jeremy’s review and has an unresolved JSON-format error-rendering issue.
1 open issue
What changed in this PR
This PR hardens Fizzy’s OAuth endpoints against arrays and objects supplied where a single string is expected, preventing malformed values from reaching client lookups.
Changes:
- Adds shared parameter validation while preserving availability gates and registration’s list-valued scope.
- Separates authorization errors by whether the redirect target can be trusted and disables JSON parameter wrapping.
- Adds integration coverage for malformed parameters and existing null-scope behavior.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
test/integration/oauth_single_valued_parameters_test.rb |
Covers malformed input rejection across OAuth endpoints. |
app/controllers/oauth/tokens_controller.rb |
Validates parameter shapes before grant processing. |
app/controllers/oauth/revocations_controller.rb |
Rejects malformed parameters before credential checks. |
app/controllers/oauth/clients_controller.rb |
Uses registration-specific errors and permits scope lists. |
app/controllers/oauth/base_controller.rb |
Defines shared validation and disables JSON wrapping. |
app/controllers/oauth/authorizations_controller.rb |
Adds staged validation and shared local error rendering. |
Review effort: Balanced
| def render_error(error, description) | ||
| @error = error | ||
| @error_description = description | ||
| render :error, status: :bad_request |
* origin/oauth-token-digests: Refuse an OAuth parameter that arrives as a list where one value is meant
With #3236 below, resource[]= and account_id[]= are refused by the single-valued check before any audience or account is read. Rails' list syntax isn't RFC 8707's repeated parameter: a bare key repeated stays invalid_target, from single_resource?. The test that sent a list of resources now expects invalid_request, and new tests cover a list account_id at consent and a list resource at the token endpoint.
* restack-oauth-audience: Refuse resource and account_id sent as lists as invalid_request Refuse an OAuth parameter that arrives as a list where one value is meant
With #3236 below, the exchange runs require_single_valued_parameters right after its availability gates, so subject_token, subject_token_type, requested_token_type, audience, resource, scope and client_id sent as lists are invalid_request before any read. Its own scope-only scalar check goes. A bare repeated resource stays invalid_target, from single_resource?.
* origin/oauth-token-digests: Refuse an OAuth parameter that arrives as a list where one value is meant
#3236's require_single_valued_parameters now refuses client_id[]= at authorize before any lookup, so set_client drops its own string check. A metadata document client is still reached only through its own URL. The array test now asserts the shared refusal with the switch off and on, and that no client query runs.
* origin/oauth-token-digests: Refuse an OAuth parameter that arrives as a list where one value is meant
* origin/oauth-token-digests: Refuse an OAuth parameter that arrives as a list where one value is meant
Held for Jeremy. Draft, stacked on #3192 (
oauth-token-digests). It moves no converged train head.The bug
Codex found this on #3228 (CIMD), and the CIMD lane fixed its own path there. The same class sits on the train itself. A
name[]=form field, or a JSON array or object, reachesparamsas an Array or a nestedParameters. Handed toOauth::Client.find_by(client_id: params[:client_id])(authorizations_controller.rb#L74 at 617edb87b), an Array becomes an SQLINlist. Soclient_id[]=<id>found the client it named, getting past the exact-string comparison that every later check assumes.The fix
Oauth::BaseControllernames the parameters that carry one value (RFC 6749 §3.1). Each endpoint refuses any of them that arrives as anything but a string or a JSON null:invalid_request, orinvalid_client_metadataat registration.scopestill means "no scope" andinvalid_grantfollows.wrap_parameters falsefor OAuth controllers. Rails copied every JSON body under the controller's name, and for the token endpoint that'stoken, which is itself an OAuth parameter.client_id,redirect_uriresponse_type,code_challenge,code_challenge_method,scope,state(andresource,account_idonce #3213 is below)grant_type,code,code_verifier,redirect_uri,refresh_token,client_id,scope(and the exchange'ssubject_token,subject_token_type,requested_token_type,audience,resourceonce #3214 is below)invalid_requesttoken,token_type_hint,client_idinvalid_requestscopestays a permitted list (validated_scopes)invalid_client_metadataNot changed:
client_secretstays withOauth::ClientAuthentication. A non-string secret already fails there asinvalid_client, whichoauth_flow_test.rbpins.redirect_urisandgrant_typeslists,token_endpoint_auth_method,client_name) is validated byClientsController's own checks, as before.What it doesn't cover
app/controllers/oauth:My::AccessTokensController(PATs), Connected Apps' revoke (a route id), API bearer authentication (the Authorization header), sessions.ClientAuthentication, and only ever a string.error: only compared to"access_denied".resource=a&resource=b: Rack keeps the last value as a String, so this check can't see it. Audience grants for the hosted MCP server and bind each to one account (phase B) #3213'ssingle_resource?raw count stays the guard for that.Tests
test/integration/oauth_single_valued_parameters_test.rbhas 11 tests. 7 fail at 617edb8; the other 4 pin behaviour that was already right (an arrayredirect_uri, dark-server 404, the token endpoint's stringclient_id, a refresh's nullscope).IN-list case:client_id[]at authorize GET, at the consent POST, at the token endpoint and at revocation, each asserted withassert_no_queries_match(/oauth_clients/). Nothing touches the clients table before the refusal.scopeas a list.The existing OAuth suites pass unchanged: 326 tests, 0 failures, OSS SQLite on the Mac. Full suites on thelio are below.
Expected conflicts
Oauth::BaseController. Adjacent, no overlap.resourceandaccount_idare covered here once B is underneath; B's rawresourcecount stays.TokensController'sbefore_actionlist. Its exchange parameters are covered with no change once stacked.AuthorizationsController#set_client. CIMD's own "only a single string names a client" check can go once this lands; this runs first.ClientsController'sbefore_actionlist. Adjacent.