diff --git a/.specs/README.md b/.specs/README.md index 708923b..f6882b8 100644 --- a/.specs/README.md +++ b/.specs/README.md @@ -78,6 +78,7 @@ board (`backlog/` · `in-progress/` · `blocked/` · `done/`). | [plans/2026-07-02-webhook_user_sync_conformance/plan.md](plans/2026-07-02-webhook_user_sync_conformance/plan.md) | Done | [changes/merged/2026-07-01-webhook_user_sync_conformance.md](changes/merged/2026-07-01-webhook_user_sync_conformance.md) | | [plans/2026-07-02-server_error_handling_and_shutdown/plan.md](plans/2026-07-02-server_error_handling_and_shutdown/plan.md) | Done | [changes/merged/2026-07-01-server_error_handling_and_shutdown.md](changes/merged/2026-07-01-server_error_handling_and_shutdown.md) | | [plans/2026-07-02-implement_lambda_runtime/plan.md](plans/2026-07-02-implement_lambda_runtime/plan.md) | Done | [changes/merged/2026-07-01-implement_lambda_runtime.md](changes/merged/2026-07-01-implement_lambda_runtime.md) | +| [plans/2026-08-05-bind_grant_type_at_token_endpoint/plan.md](plans/2026-08-05-bind_grant_type_at_token_endpoint/plan.md) | Ready | [changes/2026-08-05-bind_grant_type_at_token_endpoint.md](changes/2026-08-05-bind_grant_type_at_token_endpoint.md) | ## Conventions diff --git a/.specs/changes/2026-08-05-bind_grant_type_at_token_endpoint.md b/.specs/changes/2026-08-05-bind_grant_type_at_token_endpoint.md new file mode 100644 index 0000000..056b907 --- /dev/null +++ b/.specs/changes/2026-08-05-bind_grant_type_at_token_endpoint.md @@ -0,0 +1,444 @@ +# Change: Make `grant_type` binding at `POST /token` + +**Status:** Proposed · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Target:** crates/core (service) + +Make the declared `grant_type` the sole selector of the flow `POST /token` executes. Each grant +gains a closed parameter set — its members are mandatory, and a parameter belonging to another +grant is rejected rather than ignored — and an absent or unrecognised `grant_type` is an error +in the RFC 6749 §5.2 envelope. The enforcement is structural: `ExchangeRequest` stops being a +bag of `Option` and becomes a struct carrying an `ExchangeCredential` enum whose +variants own their own fields, so a request that mixes two grants is a parse failure at the HTTP +boundary rather than a branch the service can take. + +--- + +## Motivation + +`grant_type` is read in exactly one place — the `match` at `crates/server/src/routes/token.rs:29` +— and never travels further. `ExchangeRequest` has no `grant_type` field, and +`crates/core/src/service/exchange.rs:73-94` picks the credential path by asking a different +question: line 74 is `if let Some(ref id_token) = request.id_token`. The +`"authorization_code" | "id_token"` arm forwards `code`, `redirect_uri` and `id_token` +unconditionally, so a request declaring `grant_type=authorization_code` that also carries an +`id_token` field takes the direct-assertion path. The code is never redeemed at the provider, +the client secret is never presented, and the `redirect_uri` requirement — whose own error +string at `exchange.rs:90` names the `authorization_code` grant — is never reached. The declared +grant is documentation; the payload shape is the decision. This is the scan finding +[`g1-grant-type-confusion-token-endpoint`](../../.security/oidc-exchange/53cbdec9_20260804T102454Z/findings/g1-grant-type-confusion-token-endpoint/g1-grant-type-confusion-token-endpoint.md) +(CWE-841/843/287), and it also runs in the other direction: a deployment that configured, +documented and advertised only the authorization-code grant still accepts a bare provider ID +token as a complete credential. + +The defect is representational, not a missing `if`. Because the request is a struct of +independent optional fields, "which grant is executing" is a property of control flow that any +refactor can silently move. `.specs/development-guidelines.md` already states the rule this +change makes real — *"parse/validate before the service sees it; reject unknown `grant_type`"* — +and the hardening proposal +[`credential-lifecycle-contract.md`](../../.security/oidc-exchange/53cbdec9_20260804T102454Z/hardening/proposals/credential-lifecycle-contract.md) +reaches the same conclusion in its Option 2: *"represent the grant as an enum whose variants own +their fields"*, so that invariant CL1 — *"the grant a request executes is determined by its +declared `grant_type` alone"* — "stops being a check and becomes a parse". That proposal names +`grant_type` binding as one of three fixes to land immediately and independently of the wider +port-contract and refresh-rotation work; this change spec is that piece, plus the smallest of +the other two: the `/revoke` validation has its own spec +([2026-08-05-validate_revoke_token_claims.md](2026-08-05-validate_revoke_token_claims.md)), +while the RFC 6749 §5.1 `Cache-Control: no-store` directives +([`g1-token-response-missing-no-store`](../../.security/oidc-exchange/53cbdec9_20260804T102454Z/findings/g1-token-response-missing-no-store/g1-token-response-missing-no-store.md)) +had no spec at all and land here, because this spec owns the `/token` request and response +shape. + +--- + +## Affected spec pages + +| Canonical page | Nature of change | +|---|---| +| [`.specs/service/specs/04-http-api.md`](../service/specs/04-http-api.md) | Rewrite `POST /token request` with the per-grant parameter table, the rejection rule, and the token-endpoint error table; add the RFC 6749 §5.1 response cache directives | +| [`.specs/service/specs/03-service-flows.md`](../service/specs/03-service-flows.md) | `Token exchange (exchange.rs)` step 2 selects on `ExchangeCredential`, not field presence; add a Decision | +| [`.specs/service/specs/01-domain-model.md`](../service/specs/01-domain-model.md) | Add an `Exchange request types` entity block for `ExchangeRequest` / `ExchangeCredential` | +| [`.specs/service/specs/00-overview.md`](../service/specs/00-overview.md) | Sharpen the *Two grant inputs* Decision to say the grant is declared, not inferred | +| [`canonical-types.schema.json`](../service/specs/canonical-types.schema.json) | Add `ExchangeCredential` and `ExchangeRequest` `$defs` | + +[02-ports-and-adapters.md](../service/specs/02-ports-and-adapters.md) is unaffected — no port +signature changes. The bindings specs are unaffected: `crates/ffi/src/lib.rs:69,112` builds and +drives the same router, so the Node, Python and Lambda bindings inherit the new behaviour +without a delta of their own. + +--- + +## Proposed changes + +### `.specs/service/specs/04-http-api.md` → POST /token request (Modify) + +> ### POST /token request +> +> `application/x-www-form-urlencoded`. `grant_type` is required and **binding**: it alone +> selects the flow, and a request may carry only the parameters its declared grant defines. +> +> ``` +> # code exchange: grant_type=authorization_code & code=… & redirect_uri=… & provider=google +> # direct token: grant_type=id_token & id_token=… & provider=google +> # refresh: grant_type=refresh_token & refresh_token=… +> ``` +> +> | `grant_type` | Required parameters | Rejected if present | +> |---|---|---| +> | `authorization_code` | `provider`, `code`, `redirect_uri` | `id_token`, `refresh_token` | +> | `id_token` | `provider`, `id_token` | `code`, `redirect_uri`, `refresh_token` | +> | `refresh_token` | `refresh_token` | `provider`, `code`, `redirect_uri`, `id_token` | +> +> A parameter this server knows but that belongs to another grant is **rejected**, not ignored +> — RFC 6749 §3.2's "MUST ignore unrecognized request parameters" covers parameters the server +> does not recognise, which these are not. Parameters outside this set entirely are ignored. +> +> The client names the provider (`provider=google`), not a raw issuer URL. The handler parses +> the form into a `TokenGrant` before calling the service, so a request whose fields do not +> match its declared grant never reaches `AppService`. Response body is `TokenResponse` +> ([01-domain-model.md](01-domain-model.md)). +> +> Token-endpoint errors, in the RFC 6749 §5.2 envelope: +> +> | Condition | HTTP | `error` | `error_description` | +> |---|---|---|---| +> | `grant_type` absent | 400 | `invalid_request` | `missing required parameter: grant_type` | +> | `grant_type` present but not one of the three (including empty) | 400 | `unsupported_grant_type` | `The grant_type parameter is not supported` | +> | a required parameter of the declared grant absent | 400 | `invalid_request` | `missing required parameter: ` | +> | a parameter of another grant present | 400 | `invalid_request` | ` is not a parameter of the grant` | + +### `.specs/service/specs/04-http-api.md` → POST /token response headers (Add) + +Appended to the `POST /token request` section: + +> Every `/token` response — success and error alike — carries `Cache-Control: no-store` and +> `Pragma: no-cache` (RFC 6749 §5.1 and §5.2; OpenID Connect Core §3.1.3.3). The body of a +> successful response *is* the credential — the signed access token and, on exchange, the +> plaintext refresh token, whose only copy in flight is that response — and the header is +> the origin's sole mechanism for marking it non-storable: a `200` to a `POST` is +> heuristically cacheable under RFC 9111 §3, so without the directive a conforming shared +> cache is *permitted to store* the credential even though it may never reuse it. The +> directives are applied by a route-scoped layer (`middleware/cache_control.rs`) on the +> credential-bearing route group — `/token` and `/revoke` — not per handler, so the next +> credential-returning route inherits them by being mounted in the group. `/revoke`'s +> responses carry no token and RFC 7009 imposes no cache requirement; it is in the group +> because its *requests* carry credentials and because a group-level property survives the +> refactors that a per-handler memory does not. `/keys` and +> `/.well-known/openid-configuration` sit outside the group and keep their own (cacheable) +> policy. + +### `.specs/service/specs/03-service-flows.md` → Token exchange (`exchange.rs`) (Modify) + +The opening line and step 2 become: + +> `POST /token` with `grant_type=authorization_code` or `grant_type=id_token`. The handler has +> already parsed the form into a `TokenGrant`, so `AppService::exchange` receives an +> `ExchangeRequest` whose `credential` names the grant that was declared +> ([04-http-api.md](04-http-api.md)). +> +> 1. **Resolve provider** — look up `request.provider` in the `providers` map; missing → +> `UnknownProvider`. +> 2. **Obtain verified claims** — match on `request.credential`: +> - `ExchangeCredential::AuthorizationCode { code, redirect_uri }` → `provider.exchange_code` +> to get `ProviderTokens`, then `validate_id_token` on the returned `id_token`. +> - `ExchangeCredential::IdTokenAssertion { id_token }` → `provider.validate_id_token`. +> +> Both fields of the authorization-code variant are non-optional, so the `redirect_uri` +> binding is a property of the type rather than a runtime check: there is no field +> combination that reaches this step carrying a credential for one grant while executing +> another. + +### `.specs/service/specs/03-service-flows.md` → Decisions (Add) + +> - *The declared grant is the flow selector.* **`ExchangeRequest` carries an +> `ExchangeCredential` enum parsed at the HTTP boundary; the service matches on it and never +> inspects field presence.** An incoherent grant/field combination fails to parse at the edge +> instead of choosing a branch, so a later refactor cannot re-flatten the decision without +> deleting the type. + +### `.specs/service/specs/01-domain-model.md` → Entities, after `Token types` (Add) + +> ### Exchange request types (`service/exchange.rs`) +> +> ```rust +> enum ExchangeCredential { +> AuthorizationCode { code: String, redirect_uri: String }, +> IdTokenAssertion { id_token: String }, +> } +> +> struct ExchangeRequest { +> credential: ExchangeCredential, +> provider: String, +> ip_address: Option, +> user_agent: Option, +> device_id: Option, +> } +> ``` +> +> `ExchangeCredential` is the typed form of the declared `grant_type`: one variant per exchange +> grant, each owning that grant's required parameters as non-optional fields. The refresh grant +> has its own input type, `RefreshRequest`. `ExchangeRequest` derives no `Default` — a request +> with no credential is not constructible. The three trailing fields are client context captured +> by the audit-context middleware, not grant parameters. + +### `.specs/service/specs/00-overview.md` → Decisions (Modify) + +> - *Two grant inputs, each explicitly declared.* **`/token` accepts both a provider `code` and +> a raw `id_token`, and the declared `grant_type` selects which.** Browser SDKs (Google +> Identity Services) can post the credential they already hold without a second server-side +> code exchange, while which grant runs stays something the caller declares rather than +> something inferred from the fields they happened to send. + +--- + +## Type changes + +Two new `$defs` on the service schema. `ExchangeCredential` models the closed per-grant +parameter sets directly — `additionalProperties: false` on each variant is the schema-level +statement of the rejection rule. + +```json +{ + "$comment": "Fragment for 2026-08-05-bind_grant_type_at_token_endpoint. Adds two $defs to .specs/service/specs/canonical-types.schema.json on merge.", + "$defs": { + "ExchangeCredential": { + "description": "The typed form of the declared grant_type on POST /token. Exactly one variant; each owns its grant's required parameters.", + "oneOf": [ + { + "type": "object", + "title": "AuthorizationCode", + "required": ["grant_type", "code", "redirect_uri"], + "additionalProperties": false, + "properties": { + "grant_type": { "const": "authorization_code" }, + "code": { "$ref": "../../canonical-types.schema.json#/$defs/NonEmptyString" }, + "redirect_uri": { "$ref": "../../canonical-types.schema.json#/$defs/Url" } + } + }, + { + "type": "object", + "title": "IdTokenAssertion", + "required": ["grant_type", "id_token"], + "additionalProperties": false, + "properties": { + "grant_type": { "const": "id_token" }, + "id_token": { "$ref": "../../canonical-types.schema.json#/$defs/NonEmptyString" } + } + } + ] + }, + "ExchangeRequest": { + "type": "object", + "description": "Input to AppService::exchange. No default construction: credential and provider are always present.", + "required": ["credential", "provider"], + "additionalProperties": false, + "properties": { + "credential": { "$ref": "#/$defs/ExchangeCredential" }, + "provider": { "$ref": "../../canonical-types.schema.json#/$defs/NonEmptyString" }, + "ip_address": { "type": ["string", "null"] }, + "user_agent": { "type": ["string", "null"] }, + "device_id": { "type": ["string", "null"] } + } + } + } +} +``` + +The error codes this change emits — `invalid_request`, `unsupported_grant_type` — are already in +`OAuthErrorEnvelope` in the repo-wide [`canonical-types.schema.json`](../canonical-types.schema.json); +no change there. + +--- + +## Implementation notes + +1. `crates/core/src/service/exchange.rs:12-27` — replace the `code` / `redirect_uri` / + `id_token` fields with `credential: ExchangeCredential`, add the `ExchangeCredential` enum, + and **delete `#[derive(Default)]`**. The derive is what currently permits an `ExchangeRequest` + with no credential at all; keeping it would leave half the hole open. +2. `crates/core/src/service/exchange.rs:73-94` — replace the `if let Some(ref id_token)` selector + with a `match request.credential`. Both arms shrink: the code arm no longer unwraps + `Option`s, so its two `InvalidRequest` constructions move to the HTTP boundary and the + `"either 'code' or 'id_token' is required"` message disappears (nothing can reach the service + in that state). +3. `crates/server/src/routes/token.rs:14-22` — keep `TokenForm` as the untrusted wire shape and + add `TokenGrant` (the three grants) plus a `TryFrom for TokenGrant` that applies + the per-grant table: unwrap the required members, reject the non-members. `serde_urlencoded` + (which axum 0.8's `Form` uses, `axum-0.8.9/src/form.rs:87`) supports neither `#[serde(flatten)]` + nor tagged enums, so this is a hand-written parse, not a serde attribute. +4. `crates/server/src/routes/token.rs:29-64` — give `authorization_code` and `id_token` separate + arms and dispatch on the `TokenGrant`. Keep the existing `_ => ApiError::UnsupportedGrantType` + arm for unrecognised values. +5. Absent `grant_type` currently escapes the error envelope entirely: `TokenForm.grant_type` is a + bare `String`, so a body without it fails deserialization and axum returns + `FailedToDeserializeFormBody` — **422 with a plain-text body** + (`axum-0.8.9/src/extract/rejection.rs:76-82`), not `{"error": …}`. Two ways to reach the + specified `400 invalid_request`: derive `FromRequest` on a wrapper with + `rejection(ApiError)` and map `FormRejection`, keeping `grant_type: String`; or make + `grant_type: Option` on the wire type and reject `None` in `TryFrom`. Prefer the + first — the finding flags "moved a required parameter from `String` to `Option`" as + the exact shape of the original regression, and the wire type is the wrong place to relax it. +6. `crates/core/tests/exchange.rs` — every `ExchangeRequest { … ..Default::default() }` literal + (269, 327, 340, 381, and the rest) must be rewritten to name a credential variant and the + three context fields explicitly. If the churn is unwelcome, group `ip_address` / `user_agent` + / `device_id` into a `ClientContext` struct that keeps `Default`; that is a mechanical + refactor and does not weaken the credential invariant. +7. Regression tests, in `crates/server/tests/routes.rs` (all five would have caught the defect or + a near variant; the scan's `poc/grant_confusion.rs` `ProbeProvider` is the observation point + for the first two): + 1. `grant_type=authorization_code` carrying both `code` and `id_token` is rejected + `invalid_request` — asserted on a provider double that neither `exchange_code` nor + `validate_id_token` was called: the request dies at the parse, and in particular the + direct-assertion path never runs. + 2. `grant_type=authorization_code` without `redirect_uri` is rejected *even when* an + `id_token` is supplied. + 3. `grant_type=id_token` without an `id_token` parameter is rejected rather than falling + through to a code redemption. + 4. `grant_type=refresh_token` carrying `provider` or `code` is rejected. + 5. A body with no `grant_type` at all returns `400 {"error":"invalid_request"}`, not 422. +8. The existing tests at `crates/server/tests/routes.rs:99` (unknown grant → 400 + `unsupported_grant_type`) and `:130` (`authorization_code` with no `code` → 400 + `invalid_request`) keep passing unchanged; they already assert the post-change behaviour. +9. `crates/server/src/middleware/cache_control.rs` — a `from_fn` `no_store_layer` inserting + `Cache-Control: no-store` and `Pragma: no-cache`, applied in `routes::public_routes()` + (`crates/server/src/routes/mod.rs:13-23`) to a merged route group containing `/token` and + `/revoke` only. No new dependency (`tower-http`'s `set-header` feature is not enabled, and + this avoids the feature bump). `Router::layer` wraps the route's endpoint, so the layer + runs after `ApiError::into_response` and the §5.2 error envelope is covered; responses + manufactured by the router-wide layers (the timeout `408`, the catch-panic `500`) carry no + credential and stay unmarked. Tests in `crates/server/tests/routes.rs`: a successful + exchange response carries both headers; an `unsupported_grant_type` error response + carries both; `/keys` and the discovery document carry neither — the fix must not + blanket-mark the cacheable routes. The finding's probe + (`.security/oidc-exchange/53cbdec9_20260804T102454Z/findings/g1-token-response-missing-no-store/poc/`) + exits non-zero once every token response carries both headers, so `make run` failing there + is the signal the fix landed. + +--- + +## Compatibility and migration + +This change rejects requests that succeed today. That is the point, and it is worth stating +plainly rather than burying. + +**What breaks.** Any client that relies on field presence rather than its declared grant: + +- `grant_type=authorization_code` with an `id_token` field — today runs the direct path, after + this change is `400 invalid_request`. This is the bypass itself; there is no safe way to keep + it working. +- `grant_type=id_token` with a stray `code` or `redirect_uri` — today ignored, after this change + rejected. +- `grant_type=refresh_token` with a `provider` field — today ignored, after this change rejected. + This is the one rejection with no security value behind it (see Decisions). + +**What does not break.** The three shapes documented in +[04-http-api.md](../service/specs/04-http-api.md) work unchanged, and every caller in this +repository that reaches the endpoint already uses one of them: `crates/server/tests/{routes,e2e}.rs` +and `examples/aws-web/demo-app/src/routes/api/login/+server.ts` (which posts exactly +`grant_type=id_token & id_token & provider`). `bindings/lambda/__tests__/adapters.test.ts` +builds abbreviated bodies (`grant_type=authorization_code&code=abc`) that are not among the +three, but they are event-translation fixtures asserted byte-for-byte and never routed, so +those tests are unaffected. No configuration key changes and no stored data changes, so the +change is a straight revert if it goes wrong. + +**Migration.** There is no compatibility shim and none is proposed: a mode that keeps accepting +mismatched requests keeps the bypass. Instead — + +1. Ship behind a release note that names the three rejected shapes above and their replacements, + since a caller reading only a 400 cannot tell which parameter offended. The + `error_description` strings specified in the error table exist for exactly this reason: they + name the offending parameter and the grant it belongs to. +2. Before release, grep deployment logs or the audit trail for exchanges whose declared grant and + supplied fields disagree. There is no audit field recording this today, so the practical + pre-flight is a staging deployment with the change on and the rejection logged at `warn`. +3. Fix the two binding READMEs (`bindings/nodejs/README.md:41`, `bindings/python/README.md:52`) + while here — they show `authorization_code` with `code` and `provider` but no `redirect_uri`, + which this endpoint already rejects today, so the snippets are wrong before as well as after. + +--- + +## Merge plan + +1. Apply the two [04-http-api.md](../service/specs/04-http-api.md) blocks — the + `POST /token request` rewrite and the appended response cache directives; bump its + `**Date:**`. The `Error mapping` + table on that page already covers `InvalidRequest` and `UnsupportedGrantType` and needs no + edit. +2. Apply the two blocks to [03-service-flows.md](../service/specs/03-service-flows.md) — the + exchange-flow rewrite and the new Decision; bump its `**Date:**`. +3. Apply the `Exchange request types` block to + [01-domain-model.md](../service/specs/01-domain-model.md) after the `Token types` block; + bump its `**Date:**`. +4. Apply the Decision rewrite to [00-overview.md](../service/specs/00-overview.md); bump its + `**Date:**`. +5. Fold the `Type changes` `$defs` into + [`canonical-types.schema.json`](../service/specs/canonical-types.schema.json). +6. Flip this file's `**Status:**` to `Merged`, add `**Merged:** YYYY-MM-DD`, and move it to + `.specs/changes/merged/`. +7. Update `.specs/README.md`'s Change specs table — this file is not currently in the pending + list, so add its row directly under the `changes/merged/` entries. + +--- + +## Assumptions and open questions + +### Assumptions + +- No shipped client depends on the lax behaviour. Verified for every caller inside this + repository (see Compatibility and migration); external deployments cannot be verified from + here, which is why the release note is part of the change rather than an afterthought. +- The `id_token` grant stays a supported grant. This change makes it explicitly declared, not + optional or disabled; whether it should be gated by configuration is a separate change. +- `crates/ffi` continues to dispatch through `build_router`, so no binding-side parsing needs to + learn the grant rules. + +### Decisions + +- *Reject, do not ignore.* **A parameter belonging to another grant is a `400 invalid_request`.** + A caller sending `grant_type=authorization_code&id_token=…` has a wrong mental model of what + they are authenticating with, and telling them so is more useful than silently dropping the + field. RFC 6749 §3.2's obligation to ignore unrecognised parameters covers parameters the + server does not know; these are ones it knows and has assigned to a different grant. +- *One rule, applied uniformly, including `provider` on refresh.* **`provider` is a member of + the two exchange grants only, so a refresh request carrying it is rejected.** This single + rejection carries no security value — `provider` is not a credential and the session already + records it — but a rule with a carve-out is harder to state, to test, and to keep true through + a refactor than a rule without one. It is the cheapest thing in this spec to relax if it + causes real friction. +- *Structural, not a boundary check.* **`ExchangeCredential` is an enum whose variants own their + fields, and `ExchangeRequest` loses its `Default`.** The previous version of this code also had + boundary checks and a refactor removed them without anything complaining; a check can be + deleted, a type has to be replaced. This follows Option 2 of + [`credential-lifecycle-contract.md`](../../.security/oidc-exchange/53cbdec9_20260804T102454Z/hardening/proposals/credential-lifecycle-contract.md), + which lands `grant_type` binding immediately and independently of that proposal's port + contract and refresh-rotation phases. +- *No compatibility mode.* **The strict behaviour ships on, with no switch to restore the lax + one.** A switch that keeps accepting mismatched requests keeps the bypass, and an operator + cannot tell from the outside whether anyone is relying on it. +- *Present-but-unrecognised is `unsupported_grant_type`; absent is `invalid_request`.* **An empty + `grant_type` value counts as present.** RFC 6749 §5.2 assigns a missing required parameter to + `invalid_request` and an unsupported grant to `unsupported_grant_type`; treating empty as a + value keeps the rule "read the parameter, then classify it" with no third case. +- *`no-store` rides with the `grant_type` binding.* **The RFC 6749 §5.1 response directives + land in this change, as a route-group layer over `/token` and `/revoke`.** The hardening + proposal names three fixes to land immediately — the `/revoke` validation, the + `grant_type` binding, and `Cache-Control: no-store` — and the first two have change specs + while the third had none; it belongs here because this spec owns the `/token` request and + response shape. A layer on the route group rather than in the handlers keeps the property + one of mounting, which is the same structural instinct as the credential enum: a control a + handler must remember is a control a refactor can forget. + +### Open questions + +- The discovery document advertises `["authorization_code", "refresh_token"]` as a hand-written + literal (`crates/server/src/routes/well_known.rs:16`) while the endpoint accepts a third grant + unconditionally. Gating the `id_token` grant on a config key (the finding proposes + `token.allow_id_token_grant`, defaulting to `false`) and generating the advertised list from + the same value is a real change with its own compatibility story — it turns off a grant a + deployment may be using. It belongs in its own change spec, and now has one: + [`2026-08-05-bind_id_token_grant_replay_protection.md`](2026-08-05-bind_id_token_grant_replay_protection.md) + adds a `[grants] id_token` switch (default `false` — the finding's proposed key under a + different name) and derives the advertised list from it. This change does not depend on that + one landing; whichever merges second reconciles the `POST /token request` section. +- Should a rejected grant/field mismatch emit an audit event? `ValidationFailed` exists as an + `AuditEventType` and this is precisely a validation failure at the trust boundary, but the + rejection happens in the HTTP handler, which has no `AppService` audit path today. Left out of + this change to keep it to the fix the hardening proposal says to land immediately. diff --git a/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/01-typed_exchange_credential.md b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/01-typed_exchange_credential.md new file mode 100644 index 0000000..d032d9c --- /dev/null +++ b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/01-typed_exchange_credential.md @@ -0,0 +1,29 @@ +# 01 · Typed exchange credential + +**Plan:** [plan.md](../plan.md) · **Source:** [.specs/changes/2026-08-05-bind_grant_type_at_token_endpoint.md](../../../changes/2026-08-05-bind_grant_type_at_token_endpoint.md) + +**Implements:** source-spec implementation notes 1–2 and 6; the structural portion of [03-service-flows.md](../../../service/specs/03-service-flows.md) and [01-domain-model.md](../../../service/specs/01-domain-model.md). + +**Depends on:** — + +**Produces:** a non-default `ExchangeRequest` with exactly one typed `ExchangeCredential`, so `AppService::exchange` chooses code exchange vs direct ID-token validation by the credential variant rather than optional-field presence. + +**Pointers:** `crates/core/src/service/exchange.rs:12-27,64-94`; all `ExchangeRequest` constructors in `crates/core/tests/{exchange,refresh,revoke,user_admin}.rs`; server construction in `crates/server/src/routes/token.rs`. + +## Steps + +- [ ] In `crates/core/src/service/exchange.rs`, define public `ExchangeCredential` variants `AuthorizationCode { code: String, redirect_uri: String }` and `IdTokenAssertion { id_token: String }`; replace the optional credential fields in `ExchangeRequest` with `credential: ExchangeCredential` and remove `#[derive(Default)]`. +- [ ] Refactor `AppService::exchange` to resolve the provider once and then exhaustively `match request.credential`: the code variant calls `exchange_code(&code, &redirect_uri)` then validates its returned ID token; the assertion variant calls `validate_id_token(&id_token)` directly. Remove unreachable core-level missing-field / “either code or id_token” errors. +- [ ] Preserve the existing request context (`ip_address`, `user_agent`, `device_id`) explicitly on each `ExchangeRequest`; do not make credentials defaultable. If reducing test-literal churn, introduce a separate defaultable context value only if it does not permit credential omission. +- [ ] Migrate every repository-owned constructor across `crates/core/tests/exchange.rs`, `refresh.rs`, `revoke.rs`, and `user_admin.rs`, plus the server route after task 02's contract lands. Use the authorization-code variant for existing code-flow tests and add/retain an ID-token variant test that proves direct assertion still succeeds. +- [ ] Add meaningful precondition/invariant assertions in touched core functions consistent with the guidelines without testing untrusted HTTP input in core; the route remains the validation boundary. +- [ ] Run targeted core tests covering both variants, existing-user/suspended/registration/audit behavior, and helpers that indirectly construct exchange requests. + +## Definition of done + +- [ ] `ExchangeRequest` cannot be default-constructed and cannot express code plus ID token or a missing credential. +- [ ] `AppService::exchange` has no branch based on `Option` field presence; it is exhaustive over `ExchangeCredential`. +- [ ] Authorization-code exchange still sends both required values to the provider; direct ID-token assertion still validates the supplied assertion directly. +- [ ] Every existing core test constructor compiles with a coherent credential variant; no stale `code`, `redirect_uri`, or `id_token` request fields remain. +- [ ] Negative space is structural: no service-level call site can construct a request mixing the two exchange credentials. +- [ ] No certificate file is created; the user explicitly prohibited done certificates. diff --git a/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/02-strict_token_form_parser.md b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/02-strict_token_form_parser.md new file mode 100644 index 0000000..8404ec7 --- /dev/null +++ b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/02-strict_token_form_parser.md @@ -0,0 +1,32 @@ +# 02 · Strict token form parser + +**Plan:** [plan.md](../plan.md) · **Source:** [.specs/changes/2026-08-05-bind_grant_type_at_token_endpoint.md](../../../changes/2026-08-05-bind_grant_type_at_token_endpoint.md) + +**Implements:** source-spec `POST /token request` table and errors, implementation notes 3–8, and grant-boundary behavior for [04-http-api.md](../../../service/specs/04-http-api.md). + +**Depends on:** 01 (contract — parser constructs the new `ExchangeCredential` / `ExchangeRequest` types) + +**Produces:** an HTTP-boundary parser that makes declared `grant_type` binding and returns every specified malformed-grant case as a 400 OAuth error envelope before core or a provider is called. + +**Pointers:** `crates/server/src/routes/token.rs:14-65`; `crates/server/src/error.rs:16-38`; `crates/server/tests/routes.rs:68-146`; `crates/test-utils/src/lib.rs:493-554`. + +## Steps + +- [ ] Keep `TokenForm` as the untrusted flattened wire shape and add a private `TokenGrant` discriminated representation for authorization-code, ID-token assertion, and refresh grants. Parse explicitly; do not use serde tagged enums/flattening, which is incompatible with the axum 0.8 form path described by the source spec. +- [ ] Ensure a missing `grant_type` reaches `ApiError` as `400 invalid_request` with `missing required parameter: grant_type`, rather than axum's default form-rejection response. Prefer a `FromRequest` wrapper with an `ApiError` rejection mapping that preserves `TokenForm.grant_type: String`; validate the exact pinned-axum rejection API while implementing. +- [ ] Implement `TryFrom for TokenGrant` (or equivalent isolated parser) with the source table exactly: `authorization_code` requires `provider`, `code`, `redirect_uri` and rejects `id_token`, `refresh_token`; `id_token` requires `provider`, `id_token` and rejects `code`, `redirect_uri`, `refresh_token`; `refresh_token` requires `refresh_token` and rejects `provider`, `code`, `redirect_uri`, `id_token`. +- [ ] Return `InvalidRequest` for each missing required member with `missing required parameter: `, reject a known field assigned to another grant with ` is not a parameter of the grant`, and retain `ApiError::UnsupportedGrantType` for present unsupported or empty values. Ignore parameters entirely outside the known form set. +- [ ] Dispatch `token_handler` on `TokenGrant`, constructing `ExchangeRequest { credential, provider, audit context }` only for exchange grants and `RefreshRequest` only for refresh. Keep handlers as parse/validate/call-core/map-response only. +- [ ] Add parser/route assertions and tests in `crates/server/tests/routes.rs`. Extend `MockIdentityProvider` with safe, deterministic call counters or use a local observing double to prove rejected payloads do not call either provider method. +- [ ] Cover: valid authorization-code, ID-token, and refresh requests; existing unknown grant and missing-code behavior; missing `grant_type` → JSON 400 invalid_request; empty/unknown `grant_type` → 400 unsupported_grant_type; each missing required member; every cross-grant field rejection; unknown unrelated form parameter ignored; and the regression where declared authorization-code includes code plus ID token, which must fail before either provider call. +- [ ] Preserve existing server E2E code-and-refresh flow and audit-context tests, updating only their constructor/import expectations as task 01 requires. + +## Definition of done + +- [ ] The declared, non-empty-supported `grant_type` is the sole selector of the executed token flow. +- [ ] A field belonging to another known grant is rejected at the HTTP boundary; it is never silently ignored or used to choose a service branch. +- [ ] Missing `grant_type` is a `400` JSON OAuth `invalid_request`, not axum's `422` plain-text/form rejection. +- [ ] Empty and unrecognized grant values are `400 unsupported_grant_type` with the existing stable description. +- [ ] Required-field error descriptions and cross-grant error descriptions exactly match the source specification. +- [ ] Regression tests prove malformed mixed requests invoke neither provider method; valid id-token and refresh flows retain their documented behavior. +- [ ] No certificate file is created; the user explicitly prohibited done certificates. diff --git a/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/03-credential_route_cache_control.md b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/03-credential_route_cache_control.md new file mode 100644 index 0000000..dadb35c --- /dev/null +++ b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/03-credential_route_cache_control.md @@ -0,0 +1,28 @@ +# 03 · Credential route cache control + +**Plan:** [plan.md](../plan.md) · **Source:** [.specs/changes/2026-08-05-bind_grant_type_at_token_endpoint.md](../../../changes/2026-08-05-bind_grant_type_at_token_endpoint.md) + +**Implements:** source-spec `POST /token response headers` section and implementation note 9; the cache-directive portion of [04-http-api.md](../../../service/specs/04-http-api.md). + +**Depends on:** 02 (build — validates route error responses after token parsing has the required OAuth envelope) + +**Produces:** route-scoped `Cache-Control: no-store` and `Pragma: no-cache` headers on all `/token` and `/revoke` responses, without changing cache behavior for public metadata endpoints. + +**Pointers:** `crates/server/src/routes/mod.rs:13-23`; `crates/server/src/middleware/mod.rs`; existing `from_fn` middleware patterns in `crates/server/src/middleware/{audit_context,request_id}.rs`; `crates/server/tests/routes.rs` public-router helper and token/discovery tests. + +## Steps + +- [ ] Add `crates/server/src/middleware/cache_control.rs` with a small `from_fn` middleware that runs the next service, inserts `Cache-Control: no-store` and `Pragma: no-cache` into the resulting response, and returns it. Export it from `middleware/mod.rs`. +- [ ] In `public_routes()`, build a merged credential route group containing only `POST /token` and `POST /revoke`, apply the cache layer to that group, then merge it with `/health`, `/keys`, and discovery. Do not apply the layer router-wide and do not add a `tower-http` feature/dependency just for header insertion. +- [ ] Preserve behavior for handler-produced successes and `ApiError::into_response` errors: since the route-scoped middleware observes `next.run()`'s response, both must carry the headers. Do not claim coverage for router-wide timeout/catch-panic responses, which are outside this route group and do not return credentials. +- [ ] Add server route tests proving: a successful `/token` response has both exact headers; an unsupported-grant `/token` OAuth error has both; `/revoke` has both; `/keys` and `/.well-known/openid-configuration` have neither. Keep the current success/error body and status assertions. +- [ ] Run focused server route and E2E tests, then workspace format/lint/tests as appropriate. If full `cargo test --workspace` still reports the known three missing-`providers.*.adapter` config failures, record them as pre-existing and do not modify config code. + +## Definition of done + +- [ ] Every successful and handler-error `/token` response contains `Cache-Control: no-store` and `Pragma: no-cache`. +- [ ] `/revoke` receives the same headers through the shared credential route group. +- [ ] `/keys`, discovery, and health are not blanket-marked no-store. +- [ ] The layer is route-scoped and mechanically inherited by any route intentionally added to the credential group. +- [ ] No new dependency or feature bump is introduced solely for cache headers. +- [ ] No certificate file is created; the user explicitly prohibited done certificates. diff --git a/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/04-canonical_specs_and_examples.md b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/04-canonical_specs_and_examples.md new file mode 100644 index 0000000..8109ace --- /dev/null +++ b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/backlog/04-canonical_specs_and_examples.md @@ -0,0 +1,30 @@ +# 04 · Canonical specs and binding examples + +**Plan:** [plan.md](../plan.md) · **Source:** [.specs/changes/2026-08-05-bind_grant_type_at_token_endpoint.md](../../../changes/2026-08-05-bind_grant_type_at_token_endpoint.md) + +**Implements:** all affected canonical pages and the source-spec compatibility/documentation requirements. + +**Depends on:** 01 (contract), 02 (contract), 03 (review) + +**Produces:** canonical service prose/schema and binding README examples that exactly describe the implemented strict grant parsing and credential-response cache policy. + +**Pointers:** `.specs/service/specs/{00-overview,01-domain-model,03-service-flows,04-http-api}.md`; `.specs/service/specs/canonical-types.schema.json`; `bindings/nodejs/README.md:37-42`; `bindings/python/README.md:48-53`; source-spec merge plan and proposed blocks. + +## Steps + +- [ ] Update `04-http-api.md`: replace the `/token` request text with the binding `grant_type` per-grant required/rejected parameter table; document ignored unknown parameters, exact OAuth error table, and all-response `Cache-Control: no-store` / `Pragma: no-cache` behavior for the `/token` + `/revoke` route group. Bump its date. +- [ ] Update `03-service-flows.md`: state that the handler passes a typed `ExchangeRequest` selected by declared grant; describe exhaustive `ExchangeCredential` matching; add the declared-grant decision. Bump its date. +- [ ] Update `01-domain-model.md`: add the `ExchangeCredential` and non-default `ExchangeRequest` entity block after token types, including context fields and the separate `RefreshRequest` boundary. Bump its date. +- [ ] Update `00-overview.md`: revise the two-grant-input decision so `grant_type`, not field presence, selects code vs ID-token exchange. Bump its date. +- [ ] Add `ExchangeCredential` and `ExchangeRequest` definitions to `.specs/service/specs/canonical-types.schema.json`, using closed `oneOf` variants with the implementation's naming and all required fields. Validate JSON and `$ref` paths; retain the repo-wide OAuth error-envelope schema unchanged. +- [ ] Correct the Node and Python binding README request snippets to use `application/x-www-form-urlencoded`, encode an authorization-code request with `redirect_uri`, and retain `provider`; ensure examples match the endpoint’s required shape rather than the current JSON/missing-redirect form. +- [ ] Review all internal Markdown links and source spec references. Do not mark the source spec Merged, move it, update the change-spec table, or add done certificates; those merge lifecycle actions are explicitly outside this unstacked PR's plan implementation. + +## Definition of done + +- [ ] Each of the five source-identified canonical targets is updated and says no more or less than the implementation/tests prove. +- [ ] Schema definitions represent the closed exchange credential variants and a required credential/provider request without adding invented API fields. +- [ ] Node/Python examples are valid form-encoded authorization-code requests including `redirect_uri`. +- [ ] Discovery/id-token grant gating remains explicitly out of scope; no unrelated proposed spec is absorbed. +- [ ] All local Markdown links resolve, JSON parses, source-spec requirements map to one of tasks 01–04, and the README plan index is updated by the planning change (not deferred). +- [ ] No certificate file is created; the user explicitly prohibited done certificates. diff --git a/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/plan.md b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/plan.md new file mode 100644 index 0000000..add1f8b --- /dev/null +++ b/.specs/plans/2026-08-05-bind_grant_type_at_token_endpoint/plan.md @@ -0,0 +1,97 @@ +# Bind `grant_type` at the token endpoint — implementation plan + +**Status:** Ready · **Layout:** kanban · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Source spec:** [.specs/changes/2026-08-05-bind_grant_type_at_token_endpoint.md](../../changes/2026-08-05-bind_grant_type_at_token_endpoint.md) + +This unstacked-on-`main` plan fixes the token-endpoint grant-confusion vulnerability by making the declared `grant_type` the only selector of the executed flow. It first makes exchange credentials unrepresentable as a mixture of optional fields, then parses the wire form into that type at the HTTP boundary, protects credential-bearing responses with route-scoped cache directives, and finally synchronizes the canonical service specification and binding examples. The plan does not implement the separately proposed id-token grant gate/replay protection or revoke-claim validation; those are external proposed changes and are not prerequisites for this scoped security fix. + +No done certificates will be created. The user explicitly forbade certificate files; task packages remain in `backlog/` until implementation moves them through the kanban, and completion evidence belongs in the implementation PR/review rather than `done/*-certificate.md` files. + +--- + +## Source and definition-of-done baseline + +- **Source change spec:** [2026-08-05-bind_grant_type_at_token_endpoint.md](../../changes/2026-08-05-bind_grant_type_at_token_endpoint.md), specifically its affected canonical pages, implementation notes, compatibility section, and decisions. +- **Canonical targets:** [00-overview.md](../../service/specs/00-overview.md), [01-domain-model.md](../../service/specs/01-domain-model.md), [03-service-flows.md](../../service/specs/03-service-flows.md), [04-http-api.md](../../service/specs/04-http-api.md), and [service canonical-types.schema.json](../../service/specs/canonical-types.schema.json). +- **Guidelines:** [.specs/development-guidelines.md](../../development-guidelines.md), especially HTTP-boundary validation, invalid-state prevention, handler/core separation, negative-space testing, canonical-schema synchronization, and the Rust format/clippy/test gates. +- **Current implementation facts:** `ExchangeRequest` is `Default` and exposes optional `code`, `redirect_uri`, and `id_token` fields; `AppService::exchange` selects the direct-token path by `id_token` field presence; `TokenForm` has a required `String` `grant_type`; `public_routes` mounts `/token` and `/revoke` with the cacheable public endpoints; `MockIdentityProvider` does not currently record calls. +- **Current tests to preserve:** `crates/server/tests/routes.rs` already covers unknown grant → `400 unsupported_grant_type`, missing code → `400 invalid_request`, successful code exchange, discovery, and audit context. `crates/server/tests/e2e.rs` covers code + refresh flow. Core `ExchangeRequest` literals occur in `crates/core/tests/exchange.rs`, `refresh.rs`, `revoke.rs`, and `user_admin.rs`. +- **Baseline limitation:** `cargo test --workspace` on `main` is already red with three configuration tests failing because `providers.*.adapter` is missing. Do not alter unrelated configuration code or tests; report those failures separately if the full suite is run. Use targeted tests to establish this plan's behavior, then run the prescribed workspace checks and distinguish the known baseline failures. +- **Definition of done:** every task inherits the development-guidelines definition of done: behavior and negative space tested, meaningful assertions in touched functions, canonical types/prose updated with domain changes, `cargo fmt --all --check`, `cargo clippy --workspace -- -D warnings`, and workspace tests assessed against the stated baseline. + +### Mandatory remediation constraints + +- Do not create any `done/*-certificate.md` files or otherwise add done certificates. +- Do not modify code, non-plan specs, or any files outside this plan directory and `.specs/README.md`. +- Verify links, source coverage, task DoDs, DAG/order, and the no-certificate constraint before merging this plan/index update. + +--- + +## Task graph + +```mermaid +graph TD + 01["01 · typed_exchange_credential"] --> 02["02 · strict_token_form_parser"] + 01 --> 04["04 · canonical_specs_and_examples"] + 02 --> 03["03 · credential_route_cache_control"] + 02 --> 04 + 03 --> 04 +``` + +The dependency table is the source of truth; Mermaid is a visualization only. + +| Task | Depends on | Edge kind | Produces | +|---|---|---|---| +| 01 · typed_exchange_credential | — | — | `ExchangeRequest` has a non-default `ExchangeCredential` enum and core callers construct one coherent exchange credential | +| 02 · strict_token_form_parser | 01 | contract | `POST /token` parses declared grants into typed credentials, rejects cross-grant fields and missing/unknown grants in the OAuth error envelope | +| 03 · credential_route_cache_control | 02 | build | only `/token` and `/revoke` responses carry `Cache-Control: no-store` and `Pragma: no-cache`, including token errors | +| 04 · canonical_specs_and_examples | 01, 02, 03 | review | canonical pages/schema and Node/Python snippets accurately describe and demonstrate the shipped strict behavior | + +Every dependency targets a lower-numbered task. Task files are keyed by number/title and may move between kanban folders without invalidating the graph. + +--- + +## Implementation order and milestones + +**Order:** `01 → 02 → 03 → 04`. + +1. **M1 — structural grant binding (01–02):** Core cannot receive an incoherent exchange credential, and the route converts the untrusted form into the correct typed request before invoking the service. A probe with `grant_type=authorization_code` plus `id_token` fails before either provider operation runs. +2. **M2 — response confidentiality (03):** Successful and OAuth-error `/token` responses (and `/revoke` under the shared credential route group) carry both required cache-control headers, while `/keys` and discovery remain unmarked. +3. **M3 — canonical contract (04):** Service prose, schema, and binding examples agree with the implementation; no affected canonical target is left stale. + +--- + +## Scope boundaries, assumptions, and open questions + +### Scope boundaries + +- This plan implements only the source change spec. Do not add a configuration switch, replay protection, or discovery-list derivation for the `id_token` grant; the source spec identifies `2026-08-05-bind_id_token_grant_replay_protection.md` as a separate proposed change, not a dependency. +- Do not implement `/revoke` token-claim validation; the source spec names `2026-08-05-validate_revoke_token_claims.md` as separate work. Task 03 includes `/revoke` only because the source spec explicitly places it in the no-store route group. +- Do not move the change spec, flip it to Merged, or perform other merge housekeeping. The plan covers canonical spec synchronization, while merge status/move is an orchestrator action. +- No bindings code change is needed: FFI already dispatches through `build_router`. Binding README corrections are in scope because their examples are presently invalid endpoint requests. + +### Assumptions + +- The handler can map a missing-form-field extraction rejection to `ApiError::Domain(Error::InvalidRequest { reason: "missing required parameter: grant_type" })` without weakening the `TokenForm` wire field to `Option`; task 02 verifies this against the pinned axum 0.8 API. +- The strict parser ignores truly unknown form parameters while rejecting only known parameters belonging to a different grant, matching the source spec's table. +- The mock provider must gain call observation, or a local test double with equivalent counters must be added, so the grant-confusion regression proves neither `exchange_code` nor `validate_id_token` runs on parse failure. +- The existing public router test helper will include the new cache layer through `public_routes()`, so route tests exercise production route grouping rather than a copied test-only stack. + +### Open questions for review + +- The source spec calls for two assertions per touched function. If a minimal `TryFrom` or `no_store_layer` cannot support two non-artificial assertions without redundant runtime behavior, confirm the project reviewer accepts targeted validation assertions in the parser and test coverage for the middleware rather than `assert!(true)`-style padding. +- The source spec directs the no-store group to include `/revoke`, although RFC 7009 does not require it. Preserve that explicit design decision; if a reviewer wants `/revoke` excluded, that is a source-spec change, not an implementation shortcut. +- The two referenced external proposed specs are not present in this unstacked workspace. Treat their absence as confirmation that this plan cannot and should not implement them here. + +--- + +## Coverage map + +| Source requirement | Task(s) | +|---|---| +| `ExchangeCredential` owns non-optional code/id-token fields; `ExchangeRequest` loses `Default` | 01 | +| Core selects on typed credential, never input field presence; all core callers migrate | 01 | +| Required/binding `grant_type`; per-grant closed parameter sets; absent/unknown/error-envelope semantics | 02 | +| Cross-grant rejection occurs before service/provider calls; existing valid code/id-token/refresh paths remain supported | 02 | +| `/token` and `/revoke` receive no-store/no-cache headers; `/keys` and discovery do not | 03 | +| Canonical pages, service schema, and binding README examples align with shipped behavior | 04 | +| User-directed omission of done certificates | This plan and all four task packages |