fix: allow bare custom-scheme redirects for public clients - #28041
Conversation
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 3 findings (1 P2, 2 Note), COMMENT. Review Finding inventoryFinding inventory (PR #28041)Findings
Round logRound 1Netero-only first pass. 1 P2, 2 Notes. Reviewed against 912ce41..d95d4f9. Panel deferred: P2 gates panel selection per Netero decision gate. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
First-pass review from Netero only; the full review panel has not yet looked at this PR and will run after the mechanical findings are addressed.
What this PR does well: the change is small, focused, and the removed helper had no other callers. The two new subtests (vscode://coder.authenticate, jetbrains://coder-callback) fail against the base and pass at head, so they exercise the behavior change rather than restating it.
Severity count: 1 P2, 2 Notes.
One blocker before the panel spends time on this: the title CI job is red because the PR title's scope codersdk/oauth2_validation does not cover coderd/oauth2_security_test.go. AGENTS.md requires the scope to be a real filesystem path that contains every changed file, or omitted for cross-cutting changes. See CRF-1 for the mechanical fix.
One behavior question worth answering before the panel picks this up: the removed isValidCustomScheme check also blocked mailto, tel, and sms for public clients; validateScheme does not. The new tests only cover vscode:// and jetbrains://. Intended scope of "allow bare custom schemes": strictly bare app-style schemes like vscode, jetbrains, cursor, or any non-dangerous scheme? See CRF-2.
No Netero verbatim quote worth carrying up: the first-pass output was mechanical.
codersdk/oauth2_validation.go:1
P2 [CRF-1] PR title scope does not contain every changed file, so the title CI job fails. (Netero)
The title is
fix(codersdk/oauth2_validation): allow bare custom-scheme redirects for public clients. The scopecodersdk/oauth2_validationresolves viaisStemagainstcodersdk/oauth2_validation.go, butcoderd/oauth2_security_test.gois undercoderd/, notcodersdk/oauth2_validation/, so.github/workflows/contrib.yaml:212-220reports it as an "outside file" and callscore.setFailed.
AGENTS.md states the scope must be a real filesystem path containing every changed file, and to use a broader path or omit the scope for cross-cutting changes. There is no shared prefix between codersdk/ and coderd/, so the mechanical fix is to drop the scope: fix: allow bare custom-scheme redirects for public clients. The commit title on the branch is currently the same as the PR title and should be rewritten in lockstep.
🤖
codersdk/oauth2_validation.go:85
Note [CRF-3] ValidateRedirectURIScheme's doc comment is duplicated verbatim on lines 78-84 and 85-91. (Netero)
Introduced in commit ed908ed019, not by this PR; godoc renders both copies as one long comment. Out of scope for the diff but sitting immediately next to the touched code, so worth cleaning up in a follow-up (or a one-line ride-along here). Drop lines 85-91.
🤖
🤖 This review was automatically generated with Coder Agents.
isValidCustomScheme required a literal "." in the scheme for a public client's redirect URI, so vscode://, jetbrains://, and cursor:// all 400'd while the identical schemes passed for a confidential client through the separate, more permissive validateScheme. Native and CLI apps, the population public clients exist for, register those exact schemes with their OS. Removed the extra restriction: validateScheme already blocks the schemes that are actually dangerous in a redirect context, and RFC 8252 section 7.1 only recommends reverse-domain notation rather than requiring it. PKCE, not the scheme's spelling, is what secures a public client's redirect. That removal also stopped rejecting mailto, tel, and sms for public clients specifically, since validateScheme's dangerous-scheme blocklist never covered them either. Those three hand off to a mail client, dialer, or SMS app rather than returning control to the client, so unlike vscode:// or jetbrains://, none of them can deliver an authorization code. A public client's redirect URI scheme is its only mechanism for regaining control, so they are rejected again here, scoped specifically to public clients rather than folded into validateScheme's blocklist, since they are harmless for a confidential client's redirect.
d95d4f9 to
9440708
Compare
…/tel/sms scope, not an invented one The previous comment claimed mailto, tel, and sms are harmless for a confidential client's redirect specifically. That is not true: the client_secret only matters at token exchange, not at redirect delivery, so nothing about being confidential changes what happens when the browser is sent to one of these schemes. The actual reason they are checked only in the isPublicClient branch is that custom-scheme validation was already scoped there before this PR; confidential clients were never subject to any scheme-shape check here, independent of any judgment about these three schemes.
Documentation CheckUpdates Needed
All documentation needs for this PR are addressed. Automated review via Coder Agents |
…lients The mailto, tel, and sms scheme rejection for public clients had no documentation, flagged by the doc-check bot on this PR. Note the restriction in the Callback URL schemes section and the Security Considerations list, and add a troubleshooting entry for the error.
jdomeracki-coder
left a comment
There was a problem hiding this comment.
Security review of 7e616a3 — no regression found.
- PKCE is enforced end-to-end (
code_challengerequired at authorize; token endpoint rejects codes without a stored challenge, constant-time S256 verify), which is the actual mitigation for custom-scheme hijacking — reverse-domain notation never prevented it. - Exact redirect URI matching (OAuth 2.1) is unchanged, so no prefix/subdomain abuse opens up.
- Old blocklist coverage is fully preserved for public clients: http/https via the loopback branch, ftp via
validateScheme, mailto/tel/sms via the new switch. - No case-bypass:
url.Parselowercases schemes, so the exact-match switch is effectively case-insensitive;Validate()runs on both DCR create and update.
LGTM.
The token endpoint accepted any non-empty `code_verifier`, so a one-character verifier was enough to authenticate. RFC 7636 §4.1 requires 43 to 128 characters from the unreserved set. That fix plus the related gaps review surfaced in the same path: - Enforce the length and charset floor on the verifier before the S256 comparison runs. - Validate the challenge at the authorize endpoint too. It was only checked for non-emptiness, so a malformed challenge was stored and then failed late at token exchange, blaming the wrong parameter. - A malformed verifier now returns `invalid_request` (RFC 6749 §5.2); a well-formed but wrong one still returns `invalid_grant` (RFC 7636 §4.6). Both looked identical before, so a client had no way to tell a syntax error from a hash mismatch and would retry the same bad verifier forever. - Revoke the authorization code when a PKCE check fails. Without that, a leaked code could be replayed with unlimited verifier guesses for its remaining lifetime, and RFC 6749 §10.5 requires codes to be single use. - Fix verifier generation in `scripts/oauth2/*.sh` and the docs example. They deleted reserved base64 characters instead of translating them to the URL-safe alphabet, so most runs produced verifiers under the new floor. Also carries #28041, which merged into this branch: public clients may register bare custom schemes such as `vscode://` again, with `mailto`, `tel`, and `sms` rejected. Split out of #27873 (public OAuth2 client support). PKCE is already mandatory for every client, so this stands on its own. <details> <summary>Manual verification</summary> Ran against a local dev server on this branch, using a session token and a throwaway app from `scripts/oauth2/setup-test-app.sh`. 1. Happy path unchanged: HTTP 200, verifier length 43. 2. `code_verifier=short`, and a 43-character verifier ending in `!`: both HTTP 400 `invalid_request`, so charset is enforced and not just length. 3. `code_challenge=tooshort` at authorize: HTTP 400 `invalid_request`, no code issued. An empty challenge still hits the older "required and cannot be empty" message. 4. Well-formed but wrong verifier: HTTP 400 `invalid_grant`, distinct from the cases above. 5. Retrying that same code with the correct verifier: HTTP 400, code already revoked by the failed check. 6. `generate-pkce.sh` produces a 43-character verifier (20 out of 20 runs); the docs example produces 128. 7. `scripts/oauth2/test-mcp-oauth2.sh` passes end to end. The two bearer-token failures in its output are a pre-existing script bug (`09c50559f3`, July 2025) that reuses a resource-scoped token against the real API, not a regression here. </details>
Adds an OAuth2 client type (public vs confidential, RFC 7591 §2) derived from the requested auth method instead of hardcoded confidential. The type is stored and guarded here, but no endpoint enforces on it yet; public behavior at the token endpoint follows in the next PR in the stack. - Client type is derived once and reused by both registration and redirect URI validation, so they can't disagree - IsPublic() fails closed: an unrecognized or missing value reads as confidential - RFC 7592 update (PUT) now rejects moving a client between public and confidential (400) instead of silently flipping it when the auth method is omitted - Discovery still doesn't advertise "none"; follows once the token endpoint honors it ### Behavior by client shape `client_type` is derived from `token_endpoint_auth_method` at POST and pinned at PUT. RFC 7592 GET/PUT authenticate with the registration access token, not the client secret, so neither endpoint reads a secret. | Registered with | Stored `client_type` / method | GET reports | PUT that flips the method | |------------------------------------|---------------------------------------|-----------------------|-----------------------------------------| | omitted, or `client_secret_basic` | `confidential` / `client_secret_basic` | `client_secret_basic` | `none` → 400 `invalid_client_metadata` | | `none` (new) | `public` / `none` | `none` | `client_secret_*` → 400 `invalid_client_metadata` | | `none` (before this PR) | `confidential` / `none` | `none` | either → 200, type stays `confidential` | - PUT still replaces every other RFC 7591 field. `client_type` is the only pinned one; the method may move within a type (`client_secret_basic` ↔ `client_secret_post`). - Row 3 is the only shape where the two columns disagree. The guard fires only on a method change that crosses the type line, so those clients keep managing themselves instead of being locked out of their own configuration endpoint. - The token endpoint does not consult `client_type` yet, so every client still authenticates with a secret and registration still issues one. Split out of #27873, second in the stack (on top of #28041). Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client
**TL;DR** Last of the stack, split out of #27873, that makes Coder usable by public OAuth2 clients (CLIs, IDE plugins, MCP clients), which cannot hold a secret and authenticate with PKCE alone. | PR | What it does | |---|---| | #27712 | Schema: `app_secret_id` becomes nullable and tokens gain an always-populated `app_id`, so ownership checks work without a secret row. | | #28041 | Accepts bare custom-scheme redirect URIs (`vscode://`, `cursor://`) that native apps actually register. | | #28043 | Derives and stores `client_type` (public vs confidential) from the requested `token_endpoint_auth_method`, and pins it across updates. | | #28046 | Registration issues no secret for a public client and returns no `client_secret`. | | **#28047 (this)** | The token endpoint accepts the exchange without a `client_secret`, so a public client can finally get a token. Discovery now advertises `none`. | Each of the earlier PRs is inert on its own: until this one, a registered public client still could not complete a flow. Dynamic client registration is off by default, so nothing here is user-visible until it is enabled. **Where in the flow** - Token endpoint only — the `authorization_code` exchange. Authorize, consent, and code issuance are untouched. - Refresh and revocation come along for free: both already bind by app_id, so a secretless token row works as-is. - Discovery starts advertising `none`. **What it satisfies** - OAuth 2.1 §3.2.1 — only *confidential* clients must authenticate at the token endpoint. A public client is no longer rejected for a missing secret. - OAuth 2.1 §4.1.3 — for a public client the server must instead ensure the code was issued to the request's client_id. That check already existed; here it becomes the sole binding. - OAuth 2.1 §4.1.3 — client_id is required when the client does not authenticate, and a code yields a token at most once: a failed PKCE comparison consumes the code, so a leaked one cannot be brute-forced. - OAuth 2.1 §2.1 + RFC 7591 §2 — `none` means public client with no secret. The type is derived at registration (#28043) and read here. - RFC 7636 §4.1 / §4.6 — a malformed verifier is `invalid_request`, a wrong one `invalid_grant`. PKCE was already mandatory for every code flow, so public clients inherit it. - RFC 8414 §2 — advertised auth methods now match what the token endpoint actually accepts. - RFC 7009 — a cross-app revoke of a public client's token still returns 200 without revoking. --- - The token endpoint stops requiring client_secret for a public client, in both the presence check and secret validation. PKCE was already mandatory for every authorization_code flow, so public clients inherit it with no new validation code. - The code ownership check (dbCode.AppID != app.ID) becomes the only binding between the exchange and the app named by client_id for a public client, where it was defense in depth for confidential ones. Retained, now covered with a public client on both sides. - Public client tokens carry a NULL app_secret_id rather than referencing a secret row that does not exist. Refresh and revocation already verify ownership via app_id, so they change only comments and coverage. - Discovery advertises `none` now that the token endpoint honors it. - Valid() derives from the single canonical auth method list, so what registration accepts and what discovery advertises cannot disagree. - Swagger marks client_secret confidential-only. Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client
…8097) **TL;DR** On top of #28047, closing the last two gaps in the stack split out of #27873. A public client can now register (#28046) and exchange a code (#28047), but the admin secrets API will still issue it a secret that authenticates nothing, and the registration responses still report an auth method the token endpoint will not accept. | PR | What it does | |---|---| | #27712 | Schema: `app_secret_id` becomes nullable and tokens gain an always-populated `app_id`, so ownership checks work without a secret row. | | #28041 | Accepts bare custom-scheme redirect URIs (`vscode://`, `cursor://`) that native apps actually register. | | #28043 | Derives and stores `client_type` (public vs confidential) from the requested `token_endpoint_auth_method`, and pins it across updates. | | #28046 | Registration issues no secret for a public client and returns no `client_secret`. | | #28047 | The token endpoint accepts the exchange without a `client_secret`. Discovery advertises `none`. | | **#28097 (this)** | The admin secrets API refuses to create a secret for a public client, and registration, read, and update report the auth method the token endpoint actually enforces. | A public client can only come into existence through dynamic client registration, which is off by default, so neither guard is reachable until DCR is enabled. **Where in the flow** - `POST /oauth2-provider/apps/{app}/secrets`, the admin secrets API. The rest of the secret lifecycle is untouched: `GET` and `DELETE` are unchanged, and a confidential app behaves exactly as before. - The three RFC 7591 and 7592 responses that carry `token_endpoint_auth_method`: registration (`POST`), read (`GET`), and update (`PUT`). Only what is reported changes; the stored column is left as the client sent it. - Authorize, token exchange, refresh, and revocation are untouched. **What it satisfies** - RFC 7591 §2: `none` means the client is public and "does not have a client secret", so the secrets API should not create one for it. - RFC 7591 §3.2.1: the server MUST return all registered metadata and MAY replace a requested value with a suitable one. Reporting the enforced method in place of a stored one that contradicts `client_type` is that substitution. - RFC 7592 §2.2: an update MUST include all metadata as returned by a previous registration, read, or update. Because clients echo back what Coder reports, reporting the enforced method is what lets a legacy row heal on the client's next `PUT`. - RFC 7591 §2 default: an app with no stored method reports `client_secret_basic`, which is both the RFC default and what `ApplyDefaults` substitutes on update. - OAuth 2.1 §3.2.1: the token endpoint enforces on `client_type`, so a reported method that disagrees with it tells a client to authenticate in a way the server will reject. --- - `CreateAppSecret` returns 400 for a public client before generating anything. The secret would authenticate nothing, since the token endpoint no longer checks one for a public client. Deleting it would be worse: `oauth2_provider_app_tokens.app_secret_id` is `ON DELETE CASCADE`, so removing a confidential app's secret takes its tokens with it, but a public client's tokens carry a NULL `app_secret_id` (#27712) and survive. An admin who deleted the secret would think they had cut off the client when they had not. - `reportedAuthMethod()` replaces the raw stored value in all three registration responses. It returns the stored method when it agrees with `client_type`, `none` for a public app, and `client_secret_basic` otherwise. - The rows that can disagree are clients registered before #28043 derived the type from the method: the method was stored as sent while the type was always `confidential`. Such a client holds a secret its exchange still requires, while `GET` reported `none` and told it to drop that secret. - The stored column is deliberately not rewritten, so the response can still differ from the row. That divergence is what a read-modify-write client resolves on its next update, and the test asserts the row keeps what the client sent. - Coverage: the secrets guard asserts the 400 and that no partial secret row was left behind. A table test walks `GET` then `PUT` over six stored-method and client-type pairs: the legacy mismatch in both resend shapes, the reverse mismatch that registration cannot produce but the function still handles, an empty stored method, and the single pair that already agrees and must be reported as stored. - Docs: the client type is fixed at registration and a type-changing update is rejected with `invalid_client_metadata` (behavior from #28043 that was never written down), plus what a legacy `none` client now sees. Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client ## Manual Tests Verified by hand against a local dev deployment (`v2.36.3-devel+c85e4621ef`, dev Postgres), in addition to the automated suite. Both guards are reachable only through dynamic client registration, which was confirmed off by default on this deployment and enabled for the run. Two of the scenarios need a row where `client_type` and `token_endpoint_auth_method` disagree, which registration cannot produce since #28043 derives one from the other, so those rows were seeded by updating the method column directly on a purpose-built client. | # | Scenario | Result | |---|----------|--------| | 1 | Secrets API refuses a public client, with the guard's own message | Pass | | 2 | No partial secret row is left behind by the rejection | Pass | | 3 | A confidential app's secret create, list, and delete are unchanged | Pass | | 4 | Admin-created apps are always confidential, so the guard is unreachable there | Pass | | 5 | When stored method and client type agree, the stored value is reported unchanged | Pass | | 6 | A legacy confidential row storing `none` reports `client_secret_basic` | Pass | | 7 | The reported method is the one the token endpoint enforces, in both directions | Pass | | 8 | An update resending the stored value is accepted and leaves the row alone | Pass | | 9 | An update resending the reported value heals the row | Pass | | 10 | The reverse mismatch, a public row storing a secret method, reports `none` | Pass | | 11 | An empty or unrecognized stored method falls back to `client_secret_basic` | Pass | | 12 | Type-changing updates are rejected both ways, `client_secret_basic` to `client_secret_post` is allowed | Pass | | 13 | Regression sweep of the merged stack, discovery through revocation | Pass | | 14 | Every claim in the new docs paragraphs matches observed behavior | Pass | No correctness defects found. Commands and captured output for each scenario below. **Two notes for anyone re-running this.** The previous runbook for this stack asserts the opposite of scenario 12: it flips a confidential client to public with a `PUT` and records `200`. That was correct before #28043 pinned the client type. The `400` here is the fix, not a regression. Generate the PKCE verifier with enough entropy that stripping `=+/` still leaves 43 characters. The `openssl rand -base64 32 | tr -d "=+/" | cut -c -43` recipe yields 38 to 43, so most runs are rejected by the token endpoint under RFC 7636 section 4.1 with an error that looks unrelated. Same trap noted in #28045. <details> <summary>Shell helpers used throughout</summary> ```bash export BASE_URL=http://localhost:3000 export AUTH_HEADER="Coder-Session-Token: $(cat ./.coderv2/session)" export PGURL="postgres://coder@localhost:$(cat ./.coderv2/postgres/port)/coder?sslmode=disable&password=$(cat ./.coderv2/postgres/password)" # Enable DCR; both guards are unreachable without it. curl -s -X PUT "$BASE_URL/api/v2/oauth2-provider/settings" -H "$AUTH_HEADER" \ -H "Content-Type: application/json" \ -d '{"dynamic_client_registration_enabled": true}' # Print "HTTP <code>" then the body. show() { local out; out=$(curl -s -w '\n%{http_code}' "$@") echo "HTTP $(printf '%s\n' "$out" | tail -n1)" printf '%s\n' "$out" | sed '$d' | jq . 2>/dev/null || printf '%s\n' "$out" | sed '$d' } # $1=name, $2=auth method ("" to omit the field). register() { local body if [ -z "$2" ]; then body=$(jq -nc --arg n "$1" '{client_name:$n,redirect_uris:["http://localhost:9876/callback"]}') else body=$(jq -nc --arg n "$1" --arg m "$2" '{client_name:$n,redirect_uris:["http://localhost:9876/callback"],token_endpoint_auth_method:$m}') fi curl -s -X POST "$BASE_URL/oauth2/register" -H "Content-Type: application/json" -d "$body" } # The stored row, as opposed to what the API reports. stored() { psql "$PGURL" -At -c "select client_type || ' | ' || coalesce(token_endpoint_auth_method,'<NULL>') from oauth2_provider_apps where id = '$1';" } # 43 characters from the unreserved set, always. See the note above. gen_verifier() { openssl rand -base64 96 | tr -d "=+/\n" | cut -c -43; } challenge_for() { printf '%s' "$1" | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_'; } # Full PKCE authorize plus exchange. $1=client_id, $2=client_secret ("" for none). exchange() { local verifier challenge redirect code args verifier=$(gen_verifier); challenge=$(challenge_for "$verifier") redirect=$(curl -s -X POST \ "$BASE_URL/oauth2/authorize?client_id=$1&response_type=code&redirect_uri=http://localhost:9876/callback&state=$(openssl rand -hex 16)&code_challenge=$challenge&code_challenge_method=S256" \ -H "$AUTH_HEADER" -w '\n%{redirect_url}' -o /dev/null) code=$(printf '%s' "$redirect" | grep -oE 'code=[^&]+' | sed 's/code=//') args=(-d "grant_type=authorization_code" -d "code=$code" -d "client_id=$1" -d "redirect_uri=http://localhost:9876/callback" -d "code_verifier=$verifier") [ -n "$2" ] && args+=(-d "client_secret=$2") curl -s -X POST "$BASE_URL/oauth2/tokens" -H "Content-Type: application/x-www-form-urlencoded" "${args[@]}" } ``` </details> <details> <summary>1. Secrets API refuses a public client</summary> A bare non-`201` would also be produced by a request that failed on authentication, routing, or a malformed UUID before reaching the guard, so the status and the message are both asserted. ```bash PUB=$(register manual-28097-public none) PUB_ID=$(echo "$PUB" | jq -r '.client_id') stored "$PUB_ID" show -X POST "$BASE_URL/api/v2/oauth2-provider/apps/$PUB_ID/secrets" -H "$AUTH_HEADER" ``` ```text public | none HTTP 400 { "message": "Cannot create a client secret for a public OAuth2 app.", "detail": "Public clients authenticate with PKCE and have no client secret. The client type is fixed at registration, so register a new confidential client instead." } ``` The message is the guard's own, so execution reached `CreateAppSecret` and returned at the `IsPublic()` check. The detail points at registering a new confidential client, which scenario 12 confirms is the only remedy. </details> <details> <summary>2. No partial secret row is left behind</summary> The guard returns before `GenerateSecret()`, so nothing should exist at either layer. The count was already `0` at registration, and a confidential app is queried the same way as a control, so `0` is not merely what this query always returns. ```bash curl -s "$BASE_URL/api/v2/oauth2-provider/apps/$PUB_ID/secrets" -H "$AUTH_HEADER" | jq -c . psql "$PGURL" -At -c "select count(*) from oauth2_provider_app_secrets where app_id = '$PUB_ID';" psql "$PGURL" -At -c "select count(*) from oauth2_provider_app_secrets where app_id = '$CONF_ID';" ``` ```text [] 0 1 ``` </details> <details> <summary>3. A confidential app's secret lifecycle is unchanged</summary> ```bash CONF=$(register manual-28097-confidential "") CONF_ID=$(echo "$CONF" | jq -r '.client_id') show -X POST "$BASE_URL/api/v2/oauth2-provider/apps/$CONF_ID/secrets" -H "$AUTH_HEADER" curl -s "$BASE_URL/api/v2/oauth2-provider/apps/$CONF_ID/secrets" -H "$AUTH_HEADER" | jq -c '[.[].id]' curl -s -X DELETE "$BASE_URL/api/v2/oauth2-provider/apps/$CONF_ID/secrets/$SECRET_ID" \ -H "$AUTH_HEADER" -o /dev/null -w "delete: HTTP %{http_code}\n" ``` ```text HTTP 201 { "id": "da7a1029-47c4-435e-988a-557ffa4aeacf", "client_secret_full": "coder_POUtoElODM_<redacted>" } ["b2fc55b6-...","da7a1029-...","24f8f8c2-..."] delete: HTTP 204 ["b2fc55b6-...","da7a1029-..."] ``` Create, list, and delete all behave as before. The guard keys on `client_type` and nothing else on this path changed. Unrelated observation, not a finding against this PR: the listing returns two different `client_secret_truncated` formats. Secrets issued at registration are asterisk-padded (`***...TzqEOt`, from `createDisplaySecret`), while secrets issued through the admin API show the bare last six characters (`Rixs60`). Both write the same column and surface through the same field. Cosmetic and pre-existing, but a UI listing both kinds together would render them inconsistently. </details> <details> <summary>4. Admin-created apps are always confidential</summary> `postOAuth2ProviderApp` hardcodes the client type, so the guard is unreachable through the admin create path. ```bash ADMIN_APP=$(curl -s -X POST "$BASE_URL/api/v2/oauth2-provider/apps" -H "$AUTH_HEADER" \ -H "Content-Type: application/json" \ -d '{"name":"manual-28097-admin","callback_url":"http://localhost:9876/callback"}') ADMIN_ID=$(echo "$ADMIN_APP" | jq -r '.id') stored "$ADMIN_ID" show -X POST "$BASE_URL/api/v2/oauth2-provider/apps/$ADMIN_ID/secrets" -H "$AUTH_HEADER" ``` ```text confidential | client_secret_post HTTP 201 { "id": "a77ab276-ea74-4711-ba8b-955a8cb97951", "client_secret_full": "coder_eGlt04LhYg_<redacted>" } ``` Together with scenario 1 this brackets the guard: it fires for a dynamically registered public client and for nothing else. Note the admin API stores `client_secret_post`, not the RFC 7591 section 2 default, so every admin-created app is an agreement case for scenario 5. Such apps carry no registration access token, so they cannot reach the RFC 7592 endpoints where the reporting change applies at all. </details> <details> <summary>5. Agreement cases report the stored value unchanged</summary> The `client_secret_post` case is the one that matters. An implementation that substituted the type default on every call would still pass the public case, since `none` is the public default, but would rewrite this client to `client_secret_basic` and tell it to send its secret in the wrong place. ```bash curl -s "$BASE_URL/oauth2/clients/$PUB_ID" -H "Authorization: Bearer $PUB_RAT" \ | jq -c '{reported: .token_endpoint_auth_method}' POST_REG=$(register manual-28097-post client_secret_post) POST_ID=$(echo "$POST_REG" | jq -r '.client_id') echo "$POST_REG" | jq -c '{reported_on_register: .token_endpoint_auth_method}' curl -s "$BASE_URL/oauth2/clients/$POST_ID" -H "Authorization: Bearer $POST_RAT" \ | jq -c '.token_endpoint_auth_method' stored "$POST_ID" ``` ```text {"reported":"none"} {"reported_on_register":"client_secret_post"} "client_secret_post" confidential | client_secret_post ``` Registration and read both report the stored value. The update site is covered by scenarios 8 and 9. </details> <details> <summary>6. A legacy confidential row storing `none` reports `client_secret_basic`</summary> Seeded from a real confidential client so it genuinely holds the secret its exchange requires, with only the method column rewritten. This is the pre-#28043 shape and cannot be produced through the API any more. ```bash LEGACY=$(register manual-28097-legacy "") LEGACY_ID=$(echo "$LEGACY" | jq -r '.client_id') LEGACY_SECRET=$(echo "$LEGACY" | jq -r '.client_secret') stored "$LEGACY_ID" psql "$PGURL" -c "update oauth2_provider_apps set token_endpoint_auth_method = 'none' where id = '$LEGACY_ID';" stored "$LEGACY_ID" psql "$PGURL" -At -c "select count(*) from oauth2_provider_app_secrets where app_id = '$LEGACY_ID';" curl -s "$BASE_URL/oauth2/clients/$LEGACY_ID" -H "Authorization: Bearer $LEGACY_RAT" \ | jq -c '{reported: .token_endpoint_auth_method}' stored "$LEGACY_ID" ``` ```text confidential | client_secret_basic UPDATE 1 confidential | none 1 {"reported":"client_secret_basic"} confidential | none ``` The response substitutes and the row is deliberately not rewritten. A read has no side effects. </details> <details> <summary>7. The reported method is the one actually enforced</summary> The token endpoint enforces on `client_type`, which is still confidential, so the secret is required regardless of what the method column says. ```bash exchange "$LEGACY_ID" "$LEGACY_SECRET" | jq -c '{has_access: has("access_token"), error, error_description}' exchange "$LEGACY_ID" "" | jq -c '{has_access: has("access_token"), error, error_description}' ``` ```text {"has_access":true,"error":null,"error_description":null} {"has_access":false,"error":"invalid_request","error_description":"Missing required parameter: client_secret"} ``` The second line is the pre-fix breakage reproduced. A client that read `"token_endpoint_auth_method": "none"` from `GET` and dropped its secret accordingly, which is what RFC 7592 tells it to do, would have hit exactly this on its next exchange. The old response was not internally inconsistent, it was actionable and wrong. Scenario 10 shows the same property in the opposite direction. </details> <details> <summary>8. An update resending the stored value is accepted and changes nothing</summary> ```bash show -X PUT "$BASE_URL/oauth2/clients/$LEGACY_ID" \ -H "Authorization: Bearer $LEGACY_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-legacy","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"none"}' stored "$LEGACY_ID" ``` ```text HTTP 200 "token_endpoint_auth_method": "client_secret_basic", confidential | none ``` Three things at once. The update is accepted, which is why the type-change guard requires the auth method to actually change: the requested `none` implies a public client while the row is confidential, so a guard keyed on the type mismatch alone would reject this and lock a legacy client out of managing its own registration. The `PUT` reports the same substituted value as the `GET`, covering the third call site. The row keeps what the client sent, so the divergence persists until scenario 9. </details> <details> <summary>9. An update resending the reported value heals the row</summary> ```bash show -X PUT "$BASE_URL/oauth2/clients/$LEGACY_ID" \ -H "Authorization: Bearer $LEGACY_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-legacy","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"client_secret_basic"}' stored "$LEGACY_ID" curl -s "$BASE_URL/oauth2/clients/$LEGACY_ID" -H "Authorization: Bearer $LEGACY_RAT" \ | jq -c '{reported: .token_endpoint_auth_method}' exchange "$LEGACY_ID" "$LEGACY_SECRET" | jq -c '{has_access: has("access_token")}' ``` ```text HTTP 200 "token_endpoint_auth_method": "client_secret_basic", confidential | client_secret_basic {"reported":"client_secret_basic"} {"has_access":true} ``` The row healed through an ordinary read-modify-write cycle, with no migration, backfill, or admin action. That works only because scenario 8 keeps the door open and this response gives the client a correct value to echo back; remove either and the row stays inconsistent indefinitely. </details> <details> <summary>10. The reverse mismatch reports `none`</summary> Registration cannot produce this direction either, but `reportedAuthMethod` branches on the general disagreement rather than the one shape known to exist, so it is covered. ```bash REVERSE=$(register manual-28097-reverse none) REVERSE_ID=$(echo "$REVERSE" | jq -r '.client_id') psql "$PGURL" -c "update oauth2_provider_apps set token_endpoint_auth_method = 'client_secret_basic' where id = '$REVERSE_ID';" stored "$REVERSE_ID" curl -s "$BASE_URL/oauth2/clients/$REVERSE_ID" -H "Authorization: Bearer $REVERSE_RAT" \ | jq -c '{reported: .token_endpoint_auth_method}' exchange "$REVERSE_ID" "" | jq -c '{has_access: has("access_token"), error}' ``` ```text public | client_secret_basic {"reported":"none"} {"has_access":true,"error":null} ``` This client has no secret row at all and the token endpoint does not ask for one, so `none` is what the response should say. Reporting the stored `client_secret_basic` would have instructed it to send a secret that does not exist. Both resend shapes behave symmetrically with scenarios 8 and 9: ```text resend stored (client_secret_basic) -> reported "none", row unchanged resend reported (none) -> reported "none", row now public | none ``` </details> <details> <summary>11. Empty or unrecognized stored methods fall back to the default</summary> `NULL` and `''` are indistinguishable once read into `sql.NullString`. A third case was added because the function gates on validity rather than emptiness. ```bash for v in "null" "''" "'private_key_jwt'"; do psql "$PGURL" -At -c "update oauth2_provider_apps set token_endpoint_auth_method = $v where id = '$CONF_ID';" stored "$CONF_ID" curl -s "$BASE_URL/oauth2/clients/$CONF_ID" -H "Authorization: Bearer $CONF_RAT" \ | jq -c '.token_endpoint_auth_method' done ``` ```text confidential | <NULL> "client_secret_basic" confidential | "client_secret_basic" confidential | private_key_jwt "client_secret_basic" ``` `private_key_jwt` is a real RFC 7591 method that Coder does not implement. Reporting it back would advertise an authentication scheme the token endpoint cannot honor, which is the same class of problem as the legacy `none` report in scenario 7. </details> <details> <summary>12. Type-changing updates are rejected both ways</summary> ```bash show -X PUT "$BASE_URL/oauth2/clients/$CONF_ID" \ -H "Authorization: Bearer $CONF_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-confidential","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"none"}' stored "$CONF_ID" show -X PUT "$BASE_URL/oauth2/clients/$PUB_ID" \ -H "Authorization: Bearer $PUB_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-public","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"client_secret_basic"}' stored "$PUB_ID" show -X PUT "$BASE_URL/oauth2/clients/$CONF_ID" \ -H "Authorization: Bearer $CONF_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-confidential","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"client_secret_post"}' stored "$CONF_ID" ``` ```text HTTP 400 { "error": "invalid_client_metadata", "error_description": "token_endpoint_auth_method cannot move an existing client between public and confidential (stored \"client_secret_basic\", requested \"none\"); the client type is fixed at registration, so register a new client instead" } confidential | client_secret_basic HTTP 400 { "error": "invalid_client_metadata", "error_description": "token_endpoint_auth_method cannot move an existing client between public and confidential (stored \"none\", requested \"client_secret_basic\"); the client type is fixed at registration, so register a new client instead" } public | none HTTP 200 "token_endpoint_auth_method": "client_secret_post", confidential | client_secret_post ``` Both rejections leave the row and its secrets untouched. A partial application would be the dangerous outcome here, a client left confidential while its caller believes it went public. The third case is the control. Two `400`s alone would be equally consistent with a guard that rejects every `token_endpoint_auth_method` change, a considerably more disruptive rule than the documented one. The `200` pins the guard to the derived client type rather than the method string. The second case is also what backs scenario 1. If public to confidential were permitted, the workaround to a refused secret request would be to flip the type and ask again. The two guards close that loop, which is why the same remedy appears in both error messages. </details> <details> <summary>13. Regression sweep of the merged stack</summary> Nothing here is new in this PR, but the guards sit on top of it. ```text discovery token_endpoint_auth_methods_supported : ["client_secret_basic","client_secret_post","none"] public registration, client_secret key present : false confidential registration, secret issued : true, method defaulted to client_secret_basic PKCE-only exchange, no client_secret sent : access + refresh issued access token against /api/v2/users/me : 200 token row app_id / app_secret_id : populated / NULL RFC 7009 revoke of own refresh token : 200, access token then 401 authorized-apps listing while a token is live : [{"name":"manual-28097-public"}] bulk DELETE /oauth2/tokens : 204, listing then [] ``` The NULL `app_secret_id` is what makes the secrets guard necessary. That column is `ON DELETE CASCADE`, so deleting a confidential app's secret takes its tokens with it, while a public client's tokens would survive. An admin who deleted such a secret would believe access was cut off when it was not. PKCE failure modes were checked separately, since the token endpoint distinguishes two the previous runbook treated as one: ```text verifier omitted : invalid_request, RFC 7636 section 4.1 bounds well-formed 43 chars but wrong : invalid_grant, "The PKCE code verifier is invalid" correct verifier, replaying that code: invalid_grant, "invalid or expired" ``` The third confirms the failed check destroys the code, so a leaked code cannot absorb repeated verifier guesses. </details> <details> <summary>14. The new docs paragraphs match observed behavior</summary> `pnpm run lint-docs` reports 0 errors across 501 files, `pnpm run format-docs` rewrites nothing, and Vale reports 0 errors with 2 warnings, both on pre-existing gerund headings that the same file on `main` also produces. The new prose is one sentence per line. | Documented claim | Verified by | |---|---| | A client's type is fixed when it registers | 12 | | A type-changing update is rejected with `invalid_client_metadata` | 12, both directions | | The client either holds a secret that would stop being required or has none and no way to be issued one | 12 (2 secrets), 1 and 12 (0 secrets, issuance refused) | | Switching between `client_secret_basic` and `client_secret_post` is allowed | 12 | | To change type, register a new client | matches the remedy in both errors and in scenario 1's detail | | Legacy `none` clients are stored as confidential and still require their `client_secret` | 6, 7 | | Coder reports `client_secret_basic` for those clients | 6 | | So that what it reports matches what it enforces | 7, both directions | | The mismatch clears itself the next time the client updates its registration | 9 | One wording note. Scenario 8 shows the mismatch does not clear on an update that resends the stored value, so strictly it clears on the next update carrying the reported value. A read-modify-write client, which is what RFC 7592 section 2.2 prescribes, always carries the reported value, so the sentence holds for the client it describes. A client that hardcodes its own metadata would not self-heal, but it would also not be following RFC 7592. </details> <details> <summary>Cleanup</summary> ```bash for id in "$PUB_ID" "$CONF_ID" "$POST_ID" "$LEGACY_ID" "$REVERSE_ID" "$ADMIN_ID"; do curl -s -X DELETE "$BASE_URL/api/v2/oauth2-provider/apps/$id" -H "$AUTH_HEADER" \ -o /dev/null -w "%{http_code}\n" done curl -s -X PUT "$BASE_URL/api/v2/oauth2-provider/settings" -H "$AUTH_HEADER" \ -H "Content-Type: application/json" -d '{"dynamic_client_registration_enabled": false}' show -X POST "$BASE_URL/oauth2/register" -H "Content-Type: application/json" \ -d '{"client_name":"should-fail","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"none"}' ``` ```text 204 (x6) {"dynamic_client_registration_enabled":false} HTTP 403 {"error":"invalid_request","error_description":"Dynamic client registration is disabled on this deployment"} ``` All six test apps deleted with no orphaned secret or token rows. Both seeded rows had already healed to a consistent state before deletion. DCR is back off, confirmed by a registration attempt rather than by reading the flag back. </details>

Split out of #27873 to make that PR smaller to review. First in the stack; the rest of the public-client work builds on this.
isValidCustomSchemerequired a literal.in the scheme for a public client's redirect URI, sovscode://,jetbrains://, andcursor://all 400'd while the identical schemes passed for a confidential client through the separate, more permissivevalidateScheme. Native and CLI apps, the population public clients exist for, register those exact schemes with their OS.Removed the extra restriction:
validateSchemealready blocks the schemes that are actually dangerous in a redirect context, and RFC 8252 section 7.1 only recommends reverse-domain notation rather than requiring it. PKCE, not the scheme's spelling, is what secures a public client's redirect.That removal also stopped rejecting
mailto,tel, andsmsfor public clients specifically, sincevalidateScheme's dangerous-scheme blocklist never covered them either. Those three hand off to a mail client, dialer, or SMS app rather than returning control to the application that started the flow, so a public client registered with one of them could never actually complete authorization. They are rejected again here, scoped to public clients only because that is how custom-scheme validation was already scoped before this change, not because they are known to be safe for a confidential client's redirect; confidential clients were never subject to any scheme-shape check beyondvalidateSchemeand remain so here.Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client