First-pass review only. These are mechanical findings from a single first-pass reviewer; the full review panel has not yet reviewed this PR and will do so once these are addressed.

The change is well built. PKCE was already mandatory, so public clients genuinely inherit it with no new code, and the PR says so rather than claiming new hardening. Test density is 79% (615 test lines to 163 production), and the tests assert persisted state and call topology rather than mock return values: TestUpdateClientConfiguration_ClientTypeIsImmutable reads rows back and checks the whole update is unapplied on rejection, and TestCreateDynamicClientRegistration_Transaction injects a mid-transaction failure. The two out-of-scope fixes are both real bugs the feature exposes, and both are explained in the description instead of smuggled in. On the rename: "DetermineClientType now actually determines the type instead of returning a constant, so the name became true rather than false."

Severity count: 1 P2, 3 P3, 3 Note.

The P2 is a regression on the claim in the description that clients already registered with token_endpoint_auth_method: "none" "keep working". Their token exchange does. Their RFC 7592 management endpoint becomes a permanent 400, verified against a real database.

CRF-2 is on docs/admin/integrations/oauth2-provider.md:117, outside the diff: the documented list of supported token endpoint auth methods names only client_secret_basic and client_secret_post, while metadata.go:39 now advertises none at /.well-known/oauth-authorization-server. Server and docs contradict each other on a capability this PR ships, so an integrator reading the docs concludes public clients are unsupported. One bullet plus one sentence in the DCR paragraph.

CRF-7 is a Note on coderd/oauth2provider/tokens.go:299, outside the diff: the files this PR edits carry pre-existing em-dashes on lines it does not touch (tokens.go:260,299,420,457, registration.go:225,311,351,434,458,510). scripts/check_emdash.sh defaults to changed-lines mode so CI is green, and this PR correctly replaced the em-dashes on the two nolint lines it moved. Not this PR's job to sweep the rest; recorded because a --all run would flag them.

docs/admin/integrations/oauth2-provider.md:117

P3 [CRF-2] The documented list of supported token endpoint auth methods still names only the two secret-based ones, while discovery now advertises none. (Netero)

The page states "Coder supports the following OAuth2 client authentication methods at the token endpoint" and enumerates client_secret_basic and client_secret_post, then explains how to request client_secret_post via DCR. After metadata.go:39, /.well-known/oauth-authorization-server advertises none, so the server and the docs contradict each other on a user-facing capability shipped in this PR. An integrator reading the docs concludes public clients are unsupported.

🤖

coderd/oauth2provider/tokens.go:299

Note [CRF-7] The files this PR edits contain pre-existing em-dashes on lines it does not touch: tokens.go:260,299,420,457 and registration.go:225,311,351,434,458,510. (Netero)

scripts/check_emdash.sh defaults to changed-lines mode, so these do not fail CI, and the PR correctly replaced the em-dashes on the two nolint lines it did move. Not this PR's job to sweep the rest; noting the cluster since a --all run would flag them.

🤖

🤖 This review was automatically generated with Coder Agents.

Panel round. 23 reviewers, first full panel on this PR.

The round 1 fixes hold up under verification rather than inspection. Both halves of the registration.go fix were mutation-checked independently: reverting either the guard condition or ClientType: existingApp.ClientType fails TestUpdateClientConfiguration_LegacyAuthMethodMismatch. TestOAuth2PublicClientTokenLifecycle asserts AppSecretID.Valid == false before it tests revocation and again after the refresh, so it proves its own precondition instead of assuming the feature produced it, and it keeps a negative control (cross-app revoke returns 200 and the session survives) next to the positive one. Four reviewers independently walked all four RFC 7592 transitions plus the legacy row and none could reach "public while holding a secret". Pushing back on the round 1 suggested fix with a database probe, rather than applying it, is the behavior this panel exists to reward.

On the change itself: "A CLI or a desktop editor can't hide a secret, so making it pretend to have one was always a lie. This PR deletes the lie."

Severity count: 4 P2, 17 P3, 3 P4, 9 Nit, 8 Note. Round 1's seven findings are all closed (four fixed, three accepted).

The four P2s, in the order I would fix them:

1. registration_test.go:253, the transaction test passes with the secret insert moved back outside the transaction, proven by mutation by two reviewers independently. It is the only test standing between this handler and the project's documented outer-store-inside-InTx failure mode.
2. app_secrets.go:64, an admin can mint a secret on a public app that the token endpoint never validates, and deleting it revokes nothing because public-client tokens carry app_secret_id = NULL. The confidential kill switch silently does not exist for public apps.
3. codersdk/oauth2_validation.go:171, the docs line this PR adds points native, mobile, and CLI apps at none, and none is the branch that rejects vscode://, jetbrains://, and cursor://. I verified the asymmetry in the code: those schemes register fine as confidential and 400 as public.
4. registration.go:353, the server reports token_endpoint_auth_method: none for legacy rows whose token exchange still requires a secret, and this PR is what makes clients likely to act on that report.

Two findings are pre-existing and need a human decision rather than an agent's: the refresh grant authenticates no client at all (CRF-15, RFC 6749 §6 for confidential clients), and oauth2_provider_app_tokens has no secondary indexes while GetOAuth2ProviderAppTokenByAPIKeyID runs on every authenticated request made with an OAuth2 app token (CRF-29). Fix here, file a ticket, or state the acceptance explicitly. "It will get an index eventually" is not a plan.

One process observation. Comment bloat is systemic in this diff rather than local: six added comments narrate what the line below them does or restate the PR description, and the legacy-row rationale is written three times in substantially the same words (registration.go:334, registration.go:371, registration_test.go:469). Filed as one Nit (CRF-33) rather than six findings, because the pattern is the point. Set against that, tokens.go:221, modelmethods.go:688, and DetermineClientType's precondition are the best kind of comment: they tell the next reader why a check they might delete as redundant is what holds the door shut.

One finding was downgraded on evidence rather than judgment. Three reviewers flagged that the token endpoint resolves client_id twice by different rules (middleware reads query-first, the parser reads the merged form where body wins), and rated it up to P3. Kurapika argued it is unreachable. I tested it: a request carrying client_id in both places returns Query param "client_id" provided more than once from parseSingle before any decision reads either value. Recorded as a Note (CRF-40) with the recommendation intact.

CRF-12 is posted as a reply on the CRF-5 thread, since five reviewers framed it as new evidence on that finding's territory; coderd/oauth2provider/app_secrets.go:64 is where the fix goes. Six other findings land on files or lines outside the diff and are folded in below with their path:line.

Last, on decomposition: Law analyzed the diff and concluded don't split. One clean independent cut exists, the registration transaction fix (~45 production lines), and landing it first would shrink the feature diff. Advisory only, not worth a rebase if the feature is otherwise ready.

codersdk/oauth2_validation.go:171

P2 [CRF-11] Registering with token_endpoint_auth_method: "none" rejects the private-use URI schemes that real native apps use, so the client class this PR exists for cannot register the redirect URI it actually owns. (Pariston P3, orchestrator raised to P2)

The docs line this PR adds says to use none for "native, mobile, and CLI applications that cannot keep a secret confidential". validateRedirectURIs disagrees. When the requested auth method is none it takes the isPublicClient branch and runs every custom scheme through isValidCustomScheme, which requires a literal . in the scheme. Pariston ran the matrix:

vscode://coder.authenticate    none                 -> 400 "custom scheme vscode should use reverse domain notation"
vscode://coder.authenticate    client_secret_basic  -> ok
jetbrains://cb                 none                 -> 400
jetbrains://cb                 client_secret_basic  -> ok
cursor://cb                    none                 -> 400
cursor://cb                    client_secret_basic  -> ok
com.example.app://cb           none                 -> ok
http://127.0.0.1:9999/cb       none                 -> ok

I re-read the branch to confirm the asymmetry: isValidCustomScheme is called only under if isPublicClient, and the confidential path waves custom schemes through. The file's own doc comment at line 83 names these as legitimate: "Legitimate custom schemes for native apps (e.g. vscode://, jetbrains://) are allowed". They are, but only if you register as confidential and take a secret you cannot protect, which is the exact trade this PR was built to remove.

Raised from P3. I tested the keep-at-P3 case first: the restriction predates this PR, loopback redirects work, and nothing existing regresses. It loses to what changed. none bought a client nothing before, so nobody had reason to choose it and eat the stricter redirect rules; this PR makes none the only path to the advertised capability and then points native apps at it. An inert restriction became a live registration failure for the feature's stated target population, and no test covers the gap: the existing custom-scheme cases at oauth2_security_test.go:254-269 pair none only with com.example.* and the OOB URN.

Two ways out, and the choice is yours. Relax isValidCustomScheme for public clients to match the doc comment above it, on the grounds that RFC 8252 §7.1 recommends reverse-domain notation rather than requiring it and the scheme is not the security boundary (PKCE is). Or keep the restriction and say so in the docs paragraph this PR already edits: public clients must use a loopback redirect or a reverse-domain scheme, and vscode://-style schemes are confidential-only. What should not ship is a docs line recommending none to native apps next to a validator that 400s the schemes those apps register with the OS.

🤖

coderd/oauth2provider/tokens.go:414

P3 [CRF-15] The refresh grant authenticates no client at all, on either side of the axis this PR just drew. (Kurapika P3, Ryosuke P3, Chopper P4, Pariston Note)

extractTokenRequest requires client_secret only inside the authorization_code branch, and refreshTokenGrant never reads req.ClientSecret. A confidential client refreshes with client_id and a refresh token, no secret. RFC 6749 §6 requires the server to authenticate a confidential client on refresh.

Proof rather than inference: TestRefreshTokenGrant_Scopes (tokens_internal_test.go:503) calls extractTokenRequest with a zero-value app, which IsPublic() reads as confidential, and a form carrying no client_id and no client_secret, then asserts require.Empty(t, validationErrs). The confidential path accepts a refresh exchange with no client credential, and the test pins it.

a refresh token belonging to a confidential client is redeemable by anyone who holds it plus the client_id. client_id is not a secret. It is returned in the registration response, echoed by RFC 7592 GET, and travels in every authorize URL.

Not a regression: base 97c4031526 has the identical structure, and the only change is the !isPublic && conjunct. Four reviewers raised it anyway for the same reason. This PR makes "does this client present a secret" a first-class property and routes exactly one of the two grants through it, so the new comment at tokens.go:221 framing the code-ownership check as load-bearing for public clients will read to the next person as if the refresh path has a secret check the auth-code path is relaxing. It does not, for anyone. /oauth2/revoke is the third instance: extractRevocationRequest parses client_id and client_secret and reads neither.

The fix is not a one-liner and it breaks any confidential client that refreshes without sending its secret today, which is why it may belong in its own change. It does not belong in nobody's: fix it here, file a ticket, or state that the gap is accepted, and if it stays, say so in a comment at this line. Silence is the one outcome that leaves the next reader to rediscover it.

🤖

coderd/oauth2.go:150

P3 [CRF-23] The published API reference still tells clients a client_secret is required for authorization_code, which this PR made false. (Hisoka)

// @Param client_secret formData string false "Client secret, required if grant_type=authorization_code" renders into docs/reference/api/enterprise.md:5103 and coderd/apidoc/docs.go:14827. The admin guide got the public-client treatment in CRF-2's fix; this annotation is its sibling and was missed. A developer reading the API reference for the endpoint they are about to call learns the opposite of what the endpoint now does.

Same annotation block, same root cause: code_verifier is not listed as a parameter at all, though it has been mandatory for every exchange and is now the sole client authentication for public clients. The token endpoint's parameter docs describe a world with only confidential clients in it.

Amend the client_secret line to say confidential clients, add a code_verifier param, run make gen.

🤖

codersdk/oauth2.go:591

P4 [CRF-28] client_secret_expires_at is never emitted, including for confidential clients that were issued a secret, where RFC 7591 §3.2.1 makes it REQUIRED. (Mafuuu P4, Chopper P4)

Both reviewers verified by dumping the raw response rather than reading the tag. ClientSecretExpiresAt: 0 combined with json:"client_secret_expires_at,omitempty" drops the field, so a client_secret_basic registration returns client_secret with no expiry field at all. Per §3.2.1, 0 is the value that means the secret never expires, so the intent is right and only the serialization loses it. A strict RFC 7591 client that requires the field on a secret-issuing response has no value to read.

coderd/oauth2_test.go:1767 asserts int64(0) after decoding, which is indistinguishable from absent, so no test constrains the wire shape. Same class as CRF-20.

Pre-existing and untouched, and the omission is now correct for public clients, which is why it is P4. It sits on a line this PR rewrote and there is no follow-up, so it is a human's call: drop omitempty and keep sending 0 when a secret is issued, or accept it explicitly.

🤖

coderd/database/dump.sql:2626

P4 [CRF-29] oauth2_provider_app_tokens has no secondary indexes, and it sits on the authenticated-request hot path. (Knuckle)

Predates this PR and changes nothing in this diff. Raised because this PR is the one whose purpose is to grow that table, and public clients are for native and MCP clients, which is precisely the population that turns this from a rounding error into a bill.

The table has exactly two indexes, pkey (id) and UNIQUE (hash_prefix). Nothing on api_key_id, app_id, user_id, or app_secret_id; Postgres does not index the referencing side of a foreign key for you. Three consequences:

1. httpmw/apikey.go:707 calls GetOAuth2ProviderAppTokenByAPIKeyID on every authenticated request made with an OAuth2 provider app token, for RFC 8707 audience validation. WHERE api_key_id = $1 with no index is a sequential scan, once per request.
2. dbpurge/dbpurge.go:227 deletes up to 10,000 expired api_keys rows every 10 minutes, and the FK is ON DELETE CASCADE, so Postgres fires a per-row referential-integrity lookup for each deleted key. Unindexed, that is up to 10,000 sequential scans inside one purge transaction. Cost is rows-deleted times table-size, and both factors grow together.
3. DeleteOAuth2ProviderAppTokensByAppAndUserID filters app_id AND user_id and GetOAuth2ProviderAppsByUserID joins on user_id. Both scan. These are the two queries #27712 moved onto app_id specifically so public clients would work, so the new access pattern arrived without the index that supports it.

Unverified at scale, and stated as such: the scans are predicted from the schema, not measured. Cheap to confirm on any deployment with real traffic with EXPLAIN (ANALYZE, BUFFERS) SELECT * FROM oauth2_provider_app_tokens WHERE api_key_id = '<id>';

CREATE INDEX idx_oauth2_provider_app_tokens_api_key_id ON oauth2_provider_app_tokens (api_key_id);
CREATE INDEX idx_oauth2_provider_app_tokens_app_id_user_id ON oauth2_provider_app_tokens (app_id, user_id);
CREATE INDEX idx_oauth2_provider_app_tokens_user_id ON oauth2_provider_app_tokens (user_id);

CREATE INDEX CONCURRENTLY in its own migration if you would rather not gamble on the deployment's table size. Leave app_secret_id alone: it is NULL for exactly the rows this PR creates. If this belongs in its own PR, it needs a ticket attached before this merges. "The tokens table will get an index eventually" is not a plan, it is the interest payment deferred.

🤖

coderd/oauth2provider/tokens.go:295

P3 [CRF-16] PKCE is now the only lock on a public client's exchange, and the server never checks the key is longer than one character. (Hisoka)

authorizationCodeGrant requires req.CodeVerifier != "" and hands it straight to VerifyPKCE. Nothing in the path enforces RFC 7636 §4.1's 43-to-128-character verifier. I grepped the tree myself to confirm: no length or charset check exists anywhere in coderd/ or codersdk/.

Reproduced at the verification boundary: a one-character verifier "a" verifies against its own 43-character S256 challenge.

For a confidential client this was the second lock. Your own comment at tokens.go:221-224 says that for a public client it is the only one. The challenge travels in the authorization request URL, which lives in browser history, referrer headers, and proxy logs; the code travels in the redirect. An attacker holding both brute-forces the verifier offline, at whatever entropy the client chose, and rate limiting cannot help because the attack never touches your server.

The gap is the client's to create and the server's to refuse. Reject a verifier outside 43-128 characters of [A-Za-z0-9-._~] next to the emptiness check that is already there. Five lines, and the class of buggy native client this feature exists to serve can no longer hand you a one-character password.

Distinct from CRF-5, which is about a stray client_secret on a field nobody reads. This is the field that is now the entire authentication.

🤖

codersdk/oauth2_validation.go:148

P3 [CRF-17] validateRedirectURIs re-derives publicness inline instead of calling DetermineClientType, so one fact now has two definitions in the same package. (Razor P3, Ryosuke P3, Robin P3, Pariston P4, Melody Nit)

Five reviewers arrived here independently. isPublicClient := tokenEndpointAuthMethod == OAuth2TokenEndpointAuthMethodNone decides which RFC 8252 rules apply to a redirect URI; DetermineClientType decides what goes in client_type and whether a secret is minted. Same field, same comparison, 400 lines apart.

Before this PR the duplication was inert, because DetermineClientType was hardcoded and had no policy in it. This PR gives it one. They agree today only because the derivation is a single comparison, and the comment you deleted named the exact way they diverge:

The day one of those lands in DetermineClientType, a client registered as application_type: native with client_secret_basic is stored public, gets no secret, and is validated against the confidential redirect rules, which permit http:// to any non-loopback host the registrant names. A secretless client with a plaintext redirect to a host it does not control is the exact failure RFC 8252 §7.3 exists to prevent.

Not a re-raise of CRF-3: that was duplicated literals, which the constants fixed. This is a duplicated derivation, which the constants leave untouched.

Fix: one owner. validateRedirectURIs(req.RedirectURIs, req.DetermineClientType()), or an exported ClientTypeFor(method) that both call. Validate() has the whole request in hand. constants.go:17 argues that two spellings of "public" should be held equal by the compiler rather than by a test after the fact; this is the same argument and the one instance left unaligned.

🤖

docs/admin/integrations/oauth2-provider.md:210

P3 [CRF-24] The page now tells people to register public clients and then shows them five token-exchange examples, every one of which needs a client secret. (Leorio)

The section header at line 184 says it out loud: "Both public and confidential clients must include PKCE parameters." Then step 3 hands the reader -u "$CLIENT_ID:$CLIENT_SECRET". Same at lines 149, 165, 238, 253. Five curl blocks, zero of them runnable by the client this PR exists for.

The CLI developer at line 121 has just read "Use this for native, mobile, and CLI applications that cannot keep a secret confidential", registered with none, and received a response with no client_secret in it. Nothing on the page tells them client_id goes in the form body and client_secret is simply absent, and the failure mode of guessing wrong is 401 The client credentials are invalid. The knowledge exists in the test file and not in the docs.

A third option under PKCE step 3:

# Public client (token_endpoint_auth_method: none), no secret
curl -X POST \
  -H "Content-Type: application/x-www-form-urlencoded" \
  -d "grant_type=authorization_code" \
  -d "code=$AUTH_CODE" \
  -d "client_id=$CLIENT_ID" \
  -d "code_verifier=$CODE_VERIFIER" \
  -d "redirect_uri=https://yourapp.example.com/callback" \
  "$CODER_URL/oauth2/tokens"

Same omission in Refresh Tokens (lines 234 to 255): both options pass a secret, and a public client refreshes with client_id plus refresh_token. Same fix, same pass.

🤖

🤖 This review was automatically generated with Coder Agents.