domain: consent scope-clamp is vacuous — tampered consent form can widen OAuth grant #48

New issue

Closed

opened 2026-07-03 01:40:25 +00:00 by rosa · 1 comment

rosa commented

2026-07-03 01:40:25 +00:00

Owner

Found during a code review of the domain crate.

Location: crates/domain/src/services.rs:788 (complete_authorization)
Severity: Security — OAuth grant integrity / PKCE bypass

Problem

complete_authorization clamps granted scopes against consent.requested, but the web handler rebuilds requested (and the PKCE code_challenge, via CodeChallenge::from_storage, which skips S256 validation) from client-controlled hidden form fields. Because requested is itself attacker-controlled, the clamp is vacuous: a tampered POST /auth/consent can mint a code carrying scopes the client never requested, plus an arbitrary or empty code challenge.

Failure scenario

A client requests scope=create at GET /auth. The signed-in user edits the hidden requested_scope field to create update delete (and an arbitrary code_challenge) before submitting POST /auth/consent. Scopes::parse_lenient accepts it, the clamp passes all three scopes, and issue_authorization_code mints a code granting update+delete that no validated AuthorizationRequest ever authorized.

Suggested fix

Re-derive requested and code_challenge from a server-side-stored, validated authorization request keyed by an opaque id, rather than trusting the consent form. Route the challenge through CodeChallenge::parse so S256 validation is enforced.

Found during a code review of the `domain` crate. **Location:** `crates/domain/src/services.rs:788` (`complete_authorization`) **Severity:** Security — OAuth grant integrity / PKCE bypass ## Problem `complete_authorization` clamps granted scopes against `consent.requested`, but the web handler rebuilds `requested` (and the PKCE `code_challenge`, via `CodeChallenge::from_storage`, which skips S256 validation) from client-controlled hidden form fields. Because `requested` is itself attacker-controlled, the clamp is vacuous: a tampered `POST /auth/consent` can mint a code carrying scopes the client never requested, plus an arbitrary or empty code challenge. ## Failure scenario A client requests `scope=create` at `GET /auth`. The signed-in user edits the hidden `requested_scope` field to `create update delete` (and an arbitrary `code_challenge`) before submitting `POST /auth/consent`. `Scopes::parse_lenient` accepts it, the clamp passes all three scopes, and `issue_authorization_code` mints a code granting `update`+`delete` that no validated `AuthorizationRequest` ever authorized. ## Suggested fix Re-derive `requested` and `code_challenge` from a server-side-stored, validated authorization request keyed by an opaque id, rather than trusting the consent form. Route the challenge through `CodeChallenge::parse` so S256 validation is enforced.

rosa added the

workflow

needs-triage

label

2026-07-03 01:40:32 +00:00

rosa self-assigned this

2026-07-03 01:47:12 +00:00

rosa commented

2026-07-03 01:49:59 +00:00

Author

Owner

This was generated by AI during triage.

Agent Brief

Category: bug
Summary: Bind consent to the server-side validated authorization request so a tampered consent form cannot widen the OAuth grant or smuggle an unvalidated PKCE challenge

Verification: Confirmed by code inspection during triage. The consent POST handler rebuilds AuthorizationConsent entirely from hidden form fields: requested via Scopes::parse_lenient and the challenge via CodeChallenge::from_storage (a storage-rehydration constructor that performs no validation). The scope clamp in AppService::complete_authorization filters selected against that same form-derived requested, so it cannot narrow anything an attacker widened. Severity qualifier: the CSRF origin-checking layer on the router blocks cross-site form submission, so the practical attacker is the signed-in user tampering with their own consent form — a grant-integrity / least-privilege violation (and a bypass of ADR-0008's "PKCE validated before any code is issued" contract), not a remote account takeover. Scopes::parse_lenient only admits known scopes, so widening is bounded to the defined scope set.

Current behavior:
The GET /auth authorization endpoint validates the request with AuthorizationRequest::parse (S256-only PKCE, unknown scopes dropped, redirect-URI/client trust) but uses the result only to render the consent page; nothing validated is retained server-side. The requested scopes, code challenge, client id, redirect URI, and state round-trip through hidden fields on the consent form. POST /auth/consent trusts those fields, so a user who edits requested_scope before submitting gets an authorization code minted for scopes no validated AuthorizationRequest ever carried, and an edited code_challenge reaches code issuance without ever passing CodeChallenge::parse.

Desired behavior:
When GET /auth validates an AuthorizationRequest, the validated request is stored server-side in the user's session, keyed by an opaque, unguessable id (this design choice was pinned during triage: session storage, matching the existing pattern that parks a pending authorization request across the login redirect — no database schema change). The consent form carries only that opaque id, the scope checkboxes the user left checked, and the approve/deny action. POST /auth/consent looks up the stored request by id and re-derives client id, redirect URI, state, code_challenge, and requested scopes from it; the form contributes nothing but the id, the selected subset, and the decision. A consent POST whose id doesn't resolve (missing, expired session, already consumed) is rejected as a bad request without redirecting to any client-supplied URI. The stored request should be consumed by the consent decision so it cannot be replayed. The existing clamp in complete_authorization then does its job against a trustworthy requested.

Key interfaces:

AuthorizationConsent — should be constructible only from a server-side validated request plus the user's selection/decision, not from raw form strings. Consider whether the type itself can enforce this (e.g. built from an AuthorizationRequest rather than parallel parsed fields).
AuthorizationRequest — needs to be storable in and retrievable from the session (serializable), or accompanied by a small session-storable representation that preserves its validated invariants.
CodeChallenge::from_storage — after this change it must have no callers on user input; its only legitimate use is rehydrating already-validated values from the repository layer. If nothing outside the repository uses it, restrict its visibility accordingly.
The consent form DTO — shrinks to the opaque request id, selected scopes, and action; the hidden requested_scope, code_challenge, client_id, redirect_uri, and state fields disappear from the template.
The existing session-parking of a pending authorization request (used to survive the login redirect) — reconcile it with the new store rather than keeping two parallel mechanisms for "an authorization request in flight".

Acceptance criteria:

A consent POST whose selected scopes exceed what the validated GET /auth request asked for results in a code granting at most the validated request's scopes (test at the web-handler level, not just the domain clamp).
A consent POST cannot influence the code_challenge bound to the issued code; the challenge always comes from the validated request and only ever enters the system through CodeChallenge::parse.
A consent POST with an unknown, missing, or already-consumed request id is rejected without redirecting to a client-controlled URI.
A consent decision consumes the stored request; replaying the same consent POST fails.
The happy path still works end-to-end: authorize → consent → code → redemption, including the flow that parks the request across a login redirect.
The ci mise task passes.

Out of scope:

Changing anything about code redemption (redeem_authorization_code) or the token endpoint — the bindings checked at redemption are already correct.
Adding CSRF tokens to forms — the origin-checking CSRF layer stays as is.
Persisting pending authorization requests in the database or adding expiry sweepers for them.
Revisiting ADR-0008's decisions (single-use codes, mandatory S256, hash storage).

> *This was generated by AI during triage.* ## Agent Brief **Category:** bug **Summary:** Bind consent to the server-side validated authorization request so a tampered consent form cannot widen the OAuth grant or smuggle an unvalidated PKCE challenge **Verification:** Confirmed by code inspection during triage. The consent POST handler rebuilds `AuthorizationConsent` entirely from hidden form fields: `requested` via `Scopes::parse_lenient` and the challenge via `CodeChallenge::from_storage` (a storage-rehydration constructor that performs no validation). The scope clamp in `AppService::complete_authorization` filters `selected` against that same form-derived `requested`, so it cannot narrow anything an attacker widened. Severity qualifier: the CSRF origin-checking layer on the router blocks cross-site form submission, so the practical attacker is the signed-in user tampering with their own consent form — a grant-integrity / least-privilege violation (and a bypass of ADR-0008's "PKCE validated before any code is issued" contract), not a remote account takeover. `Scopes::parse_lenient` only admits known scopes, so widening is bounded to the defined scope set. **Current behavior:** The `GET /auth` authorization endpoint validates the request with `AuthorizationRequest::parse` (S256-only PKCE, unknown scopes dropped, redirect-URI/client trust) but uses the result only to render the consent page; nothing validated is retained server-side. The requested scopes, code challenge, client id, redirect URI, and state round-trip through hidden fields on the consent form. `POST /auth/consent` trusts those fields, so a user who edits `requested_scope` before submitting gets an authorization code minted for scopes no validated `AuthorizationRequest` ever carried, and an edited `code_challenge` reaches code issuance without ever passing `CodeChallenge::parse`. **Desired behavior:** When `GET /auth` validates an `AuthorizationRequest`, the validated request is stored server-side in the user's session, keyed by an opaque, unguessable id (this design choice was pinned during triage: session storage, matching the existing pattern that parks a pending authorization request across the login redirect — no database schema change). The consent form carries only that opaque id, the scope checkboxes the user left checked, and the approve/deny action. `POST /auth/consent` looks up the stored request by id and re-derives client id, redirect URI, state, `code_challenge`, and `requested` scopes from it; the form contributes nothing but the id, the selected subset, and the decision. A consent POST whose id doesn't resolve (missing, expired session, already consumed) is rejected as a bad request without redirecting to any client-supplied URI. The stored request should be consumed by the consent decision so it cannot be replayed. The existing clamp in `complete_authorization` then does its job against a trustworthy `requested`. **Key interfaces:** - `AuthorizationConsent` — should be constructible only from a server-side validated request plus the user's selection/decision, not from raw form strings. Consider whether the type itself can enforce this (e.g. built from an `AuthorizationRequest` rather than parallel parsed fields). - `AuthorizationRequest` — needs to be storable in and retrievable from the session (serializable), or accompanied by a small session-storable representation that preserves its validated invariants. - `CodeChallenge::from_storage` — after this change it must have no callers on user input; its only legitimate use is rehydrating already-validated values from the repository layer. If nothing outside the repository uses it, restrict its visibility accordingly. - The consent form DTO — shrinks to the opaque request id, selected scopes, and action; the hidden `requested_scope`, `code_challenge`, `client_id`, `redirect_uri`, and `state` fields disappear from the template. - The existing session-parking of a pending authorization request (used to survive the login redirect) — reconcile it with the new store rather than keeping two parallel mechanisms for "an authorization request in flight". **Acceptance criteria:** - [ ] A consent POST whose selected scopes exceed what the validated `GET /auth` request asked for results in a code granting at most the validated request's scopes (test at the web-handler level, not just the domain clamp). - [ ] A consent POST cannot influence the `code_challenge` bound to the issued code; the challenge always comes from the validated request and only ever enters the system through `CodeChallenge::parse`. - [ ] A consent POST with an unknown, missing, or already-consumed request id is rejected without redirecting to a client-controlled URI. - [ ] A consent decision consumes the stored request; replaying the same consent POST fails. - [ ] The happy path still works end-to-end: authorize → consent → code → redemption, including the flow that parks the request across a login redirect. - [ ] The `ci` mise task passes. **Out of scope:** - Changing anything about code redemption (`redeem_authorization_code`) or the token endpoint — the bindings checked at redemption are already correct. - Adding CSRF tokens to forms — the origin-checking CSRF layer stays as is. - Persisting pending authorization requests in the database or adding expiry sweepers for them. - Revisiting ADR-0008's decisions (single-use codes, mandatory S256, hash storage).

rosa added

and removed

labels

2026-07-03 01:50:09 +00:00

rosa referenced this issue from a commit

2026-07-03 04:42:06 +00:00

fix: Bind consent to the server-side validated authorization request

rosa referenced this issue from a pull request that will close it,