First-pass review only. These are mechanical findings from a single reviewer; the full review panel has not looked at this PR yet and will review after these are addressed.

The change is small, correct for what it claims, and the tests are real: each refusal asserts the status, the error code, and the description, then proves the refused request consumed nothing (the refresh row survives, the session survives, the body-only retry succeeds). The guard runs before ParseForm, so nothing is read off the wire before the refusal. The docs sentence says what a client should do instead. All six hand-built requests that reach the new code path were updated, none missed.

1 P1, 2 P2, 1 P3, 2 Nits, 2 Notes.

The substantive concern is scope of the fix rather than its correctness. vals := r.Form is the merged query-plus-body map, so client_secret was never the only credential reachable from the URL: refresh_token, code, and the revocation token all still are. The PR ships a breaking change for the weakest of those four and leaves the live bearer credentials accepted. Reviewer verdict, verbatim: "a client can put a live refresh token, which mints access tokens for the length of the refresh lifetime, into a URL and every proxy and access log on the path records it. That is the harm the PR description gives as its reason for the client_secret refusal, at equal or greater blast radius." Naming the set instead of the one parameter costs one line.

One blocker is not yours, detailed below: a duplicate migration number on main stops this package's tests from running at all.

Process note: the head commit is a merge of main with no changes of its own, so the diff reviewed is the feature work only.

coderd/oauth2provider/tokens.go:172

P2 [CRF-1] The guard covers client_secret only; the token endpoint still accepts refresh_token and code from the URL query, which is the same root cause and the same leak. (Netero)

vals := r.Form is the merged query-plus-body map, so every credential the endpoint reads is reachable from the URL, not just client_secret. rejectClientSecretInQuery closes one name out of three.

Verified. I called extractTokenRequest with POST /oauth2/tokens?refresh_token=secret_from_url and a body of grant_type=refresh_token&client_id=...&client_secret=...:

err=<nil> validationErrs=[] refresh_token="secret_from_url" client_secret="coder_secret" grant=refresh_token

No duplicate-parameter rejection fires, because the token appears once. Code: p.String(vals, "", "code") on line 188 reads through the same map, so an authorization code in the URL is accepted the same way.

Fix: name the set, not the one parameter. rejectCredentialsInQuery(r, "client_secret", "refresh_token", "code"), same sentinel, same response.

Confirmed by reading the function: vals := r.Form at line 172 feeds both Code and RefreshToken. A refresh token in a URL is a live credential that mints access tokens for the length of the refresh lifetime and needs no second factor; that is a worse leak than the one this PR fixes, and it is left open by the same function that closes the weaker case. The stated justification for the breaking change ("URLs are recorded by proxies and access logs") applies unchanged to these parameters. Either extend the guard to the credential set, or say in the PR description why the stronger credentials are out of scope.

🤖

coderd/oauth2provider/revoke.go:43

P2 [CRF-2] The revocation endpoint still accepts the token it is about to revoke from the URL query. (Netero)

Same root cause as client_secret: r.Form.Get("token") reads the merged map.

Verified. extractRevocationRequest with POST /oauth2/revoke?token=secret_from_url and a body carrying only client_id and client_secret:

err=<nil> token="secret_from_url"

Confirmed at line 43. The presented token is a bearer credential. Worse than the token-endpoint case: when the revocation fails client authentication (401, nothing revoked), the token that just landed in every access log on the path is still live and replayable. Add token to the rejected set.

🤖

coderd/database/migrations/000596_template_usage_stats_session_usage.down.sql:1

P1 [CRF-3] Two migrations share number 000596, so every Postgres-backed test in coderd/oauth2provider fails before it runs, including the three tests this PR adds. (Netero) Not caused by this diff; raised because it blocks verification. Reproduced at the PR head: go test ./coderd/oauth2provider/ -run 'TestOAuth2RefreshClientAuthentication/SecretInQueryString' dies with failed to init driver with path .: duplicate migration file: 000596_template_usage_stats_session_usage.down.sql before reaching the test body. git ls-tree origin/main coderd/database/migrations/ shows both 000596_aibridge_workspace_context.* and 000596_template_usage_stats_session_usage.* at c47eb47, which is both this PR's merge base and current main, so the collision is upstream. Renumbering one pair to 000597 is the fix. Until it lands, SecretInQueryString, SecretInQueryStringAndBody, and the revocation test have never executed, so the PR's central claim is unverified by its own tests. The DB-free internal tests in the package do pass.

🤖

🤖 This review was automatically generated with Coder Agents.

Round 2. Six of eight round 1 findings are resolved: the docs now name both endpoints and gained a troubleshooting entry, the guard moved into Tokens and RevokeToken (which deleted the sentinel, both errors.Is branches, and all six URL: &url.URL{} test edits), the revocation URL-plus-body case is covered, and both duplicated error literals became consts. Keeping the method check ahead of the guard in RevokeToken so a GET still answers 405 is the right call, and mutation-testing the new revoke cases by stubbing the guard out is the kind of verification most fixes skip. The P1 migration collision is gone: this head sits on eec4915, where #29568 renumbered the duplicate, so the three integration tests can finally run.

Further review is blocked. Two P2 findings from round 1 got neither a code change nor a response:

• CRF-1, coderd/oauth2provider/tokens.go:192: refresh_token and code are still read from vals := r.Form, the merged query-plus-body map, so both are still accepted from the URL query. clientSecretInQuery checks one name out of the set.
• CRF-2, coderd/oauth2provider/revoke.go:40: extractRevocationRequest still reads the token it is about to revoke via r.Form.Get("token"), so that token is still accepted from the URL query.

This is not an accusation of ignoring them: the post tool folded both into the round 1 review body instead of creating inline threads, so there was no thread to reply to. You replied to all five threads that existed. Recording it here so the request is unambiguous.

What is needed for review to continue, either one:

1. Extend the guard to the credential set (client_secret, refresh_token, code, and the revocation token), or
2. State why the stronger credentials are out of scope for this PR, in a reply here or in the PR description, and file an issue for them.

The reason this blocks rather than trails behind: the PR's own justification for a breaking change is that URLs are recorded by proxies and access logs. A refresh token in a URL is a live credential that mints access tokens for the whole refresh lifetime with no second factor, and a presented revocation token stays replayable when the revocation itself 401s. Shipping the refusal for the weakest of the four names, while the same function leaves the strongest accepted, is the part of this change a reviewer cannot sign off on silently. If the answer is "separate PR," that is a legitimate answer; it needs to be written down, because nobody comes back to this file with the context you have right now.

No panel review this round. The panel has still not reviewed this PR, and will not until these two are answered.

🤖 This review was automatically generated with Coder Agents.

First panel round. Eighteen reviewers plus the mechanical pass. Round 1's eight findings are closed: six fixed at the root rather than patched, one fixed upstream, and the two contested ones settled below.

The guard itself survived every attack the panel brought. Seven reviewers independently established that r.URL.Query() and r.ParseForm run the same parser over the same bytes, so there is no key the guard misses that the authenticator can still read: percent-encoded spellings (client%5Fsecret) trip it, semicolon smuggling and bad escapes drop the segment from both sides and then fail the request anyway, and case variants are invisible to both. Both request-side readers of client_secret in the tree are behind it, no first-party caller sends the secret in a URL, the refusal precedes every mint and every delete, and the error body is a constant that echoes nothing. Determinism: no clock, no goroutine, no shared state, and the reject-before-reading-the-body path was driven at 1 KiB through 3 MiB without a transport error. The //nolint:gosec two reviewers went looking to delete is load-bearing, and its comment is accurate.

5 P2, 8 P3, 2 P4, 5 Nits, 1 Note.

The round's convergence is one test. Eleven reviewers independently mutated the guard and found that SecretInQueryStringAndBody at the token endpoint stays green without it, because a client_secret in both places was already a 400 through parseSingle's repeated-parameter rejection. Bisky put it best: "I mutated the guard to return false and ran all four: three went red, one stayed green. That one is glass." The revocation twin does discriminate, so the gap is one-sided and one assertion closes it. That matters more than a test nit because it sits exactly where a future refactor would land.

CRF-1 and CRF-2: contested, and the panel closes them in your favour, 11 of 11, on grounds 1 and 2. The normative asymmetry checks out, the ?access_token= precedent at coderd/httpmw/apikey.go:966 is real, the presented credentials are single-use with replay detection at tokens.go:811, and Coder's own request logger records r.URL.Path and never the raw query, so the disclosure lives in third-party infrastructure. PLAT-660 makes it a deferral rather than a drop.

Ground 3 does not survive, and six reviewers reached that independently: "every proxy already logged the URL" is equally true of client_secret, which this PR rejects. A refusal never un-logs anything; what it buys is that the client stops sending the shape. That argument, if sound, would have blocked this PR. Please keep it out of PLAT-660's rationale, because a future reader could close the ticket with it. Four facts worth adding to that ticket: for a public client refreshTokenGrant skips authenticateClient, so a logged refresh token is a complete standalone credential; code_verifier belongs in the set, and code plus code_verifier in one URL is a replayable grant; ?access_token= is a sibling in the class rather than a precedent that settles it; and two reviewers measured the structural fix (source the credential parameters from r.PostForm) green across the package and TestOAuth2*, so the deferral does not rest on cost.

Process: no code commits this round, so this review is against the same diff round 2 saw, with the description rewrite and your CRF-1/CRF-2 comment as the change. That comment is the best artifact in this PR: it reproduces the behavior it concedes, separates the normative from the practical, corrects two claims nobody asked about, and files a ticket. Two of the P2s below exist because the prose around the guard did not get the same treatment.

coderd/oauth2.go:197

P3 [CRF-14] /oauth2/revoke enumerates its invalid_request causes in swagger and the new one is missing. (Netero P3, Mafu-san P3, Chopper P3, Mafuuu Nit, Knov Nit)

The annotation reads invalid_request: a missing client_id or token, credentials in both the Authorization header and the body, or a malformed token. This PR adds a fourth cause and leaves the list alone, so the published reference (docs/reference/api/enterprise.md:5402, coderd/apidoc/swagger.json:15195) now describes an incomplete set. An enumerated list that stops being exhaustive misleads a client author writing error handling from the reference. (Netero)

Five reviewers found it; the consequence they converge on is that the prose page and the API reference now disagree about when /oauth2/revoke returns 400. AGENTS.md requires swagger annotations for public HTTP endpoints, and the two prose locations were updated while the machine-generated one was not.

/oauth2/tokens declares only 413 (coderd/oauth2.go:169), so it has no list to fall out of date; Mafu-san and Chopper both suggest adding a @Failure 400 there for symmetry, which is a judgment call rather than a correctness one. Extend line 197 and run make gen; the generated files are currently consistent, so they regenerate together.

🤖

coderd/oauth2provider/authorize.go:306

P3 [CRF-16] /oauth2/authorize still accepts client_secret in the query and answers 200, and it is the endpoint whose URL is most exposed. (Knov P3, Hisoka Note)

Both reviewers found it independently, both by probing rather than reading. Knov:

extractAuthorizeParams reads r.URL.Query() and drops anything it did not parse through ignoredParams (authorize.go:360), which logs parameter names at debug level and nothing else. A throwaway test against a seeded app sent the well-formed authorize query plus client_secret=coder_deadbeef_cafebabe:

GET  /oauth2/authorize ... &client_secret=...  -> 200 (consent page)
POST /oauth2/authorize ... &client_secret=...  -> 302 https://example.com/callback?code=coder_...

The justification this PR gives, a URL is recorded by proxies and access logs, is strictly stronger at the authorization endpoint, because that URL also lands in the end user's address bar, browser history, and the Referer the user agent sends to the callback host. A confidential client that appends client_secret to its authorization URL is the misconfiguration this PR exists to catch, and it is the one case Coder answers 200 to, so the client never learns.

The two reviewers disagree about the remedy, and I am not asking for a 400. Hisoka:

Do not "complete" the fix there. The comment at authorize.go:357 cites the rule that unrecognized authorization-request parameters MUST be ignored, and client_secret is not a parameter of the authorization request. Rejecting it would trade one compliance claim for another.

Knov reaches the same tension and names the defensible answer: a warn-level log with the client_id and the parameter name, instead of silence at debug level, plus a sentence putting the authorization endpoint out of scope and why. The PR description scopes out client_id, refresh_token, code, and the revocation token; it does not mention /oauth2/authorize at all. That is what makes this an unnoticed sibling rather than a deferral, and it is the part that is not defensible.

Root cause is placement: this rule is enforced per handler while the three OAuth2 protocol routes share a middleware that already owns the cross-endpoint body limit for exactly this reason. Kurapika and Meruem raise the same structural point for a future third client-authenticating endpoint (introspection, device code) that would have to remember.

🤖

coderd/oauth2provider/tokens.go:188

P3 [CRF-17] client_id is the other half of the §2.4.1 MUST NOT this PR rests on, and it is dropped with no ticket, on a stated blocker that points at a different route. (Razor P3, Kite P3, Zoro P3, Pariston Note)

Four reviewers, none of whom saw each other's work, independently checked the two reasons the description gives and found both weaker than stated. This is not CRF-1 or CRF-2: those rest on §3.2.2 carrying no MUST NOT, and client_id sits inside the one sentence of §2.4.1 that the entire PR rests on. The description concedes it ("Section 2.4.1's prohibition covers it too") while PLAT-660 is named only for the other three parameters.

"site/src/api/api.ts:2045 relies on it." That line is this.axios.delete('/oauth2/tokens?client_id=${appId}'), the DELETE route, Coder's own revoke-access call, registered separately behind apiKeyMiddleware at coderd/coderd.go:1284. §2.4.1 governs the token request, not that route. The only other frontend query client_id is GET /oauth2/authorize, where §2.4.1 does not apply. (Zoro, confirmed independently by Razor, Kite and Pariston)

Razor then measured what a handler-level guard would actually cost:

I added the two-line equivalent to Tokens and RevokeToken and ran the suite: everything passed except TestFormEndpointsBodyTooLarge (both subtests, 413 became 400), and that test puts client_id in the query deliberately so the middleware does not read the oversized body. So the cost is not middleware surgery; it is a second breaking change for confidential clients that put client_id in the URL.

That cost is real and it is a legitimate reason to keep it out of this PR. Kite and Zoro converge on the same minimum ask, and it is the whole finding: add client_id to PLAT-660, or state in the ticket that it is out of scope and why. As it stands, D6-04 will read as closed while half of the cited MUST NOT is unenforced and untracked, and a paragraph in a PR description is not a tracked item.

🤖

coderd/oauth2provider/tokens.go:174

P3 [CRF-21] The guard is a one-name denylist in front of a read that admits the whole query, so the enforcement lives one layer above the line that chooses the source. (Ryosuke P3, Pariston P3)

Two reviewers arrived here separately, and both measured the alternative instead of asserting it. This is not a re-raise of CRF-1 or CRF-2, whose disposition both accept; it is about the shape of what ships.

I replaced vals := r.Form with vals := r.PostForm (keeping a merged read for client_id alone, for the middleware's sake) and r.Form.Get with r.PostForm.Get for token, token_type_hint and client_secret in extractRevocationRequest. Probe, refresh grant with refresh_token only in the query: HEAD answers 200 with a minted access token, the variant answers 400. go test ./coderd/oauth2provider/ -count=1 passes, including all four new tests and the Form-literal internal tests. go test ./coderd/ -count=1 -run TestOAuth2 passes, 39 top-level tests. (Pariston)

Ryosuke ran the same experiment independently with the same result, and both reverted cleanly. Two consequences follow.

The next client authentication parameter is client_assertion (RFC 7523 private_key_jwt, already ignored by name at tokens.go:246), and §2.4.1's MUST NOT, the one the author accepts, covers it. Under the merged read it will be accepted from the query on the day it is implemented, and clientSecretInQuery will not notice, because it checks one string. (Pariston)

And in fairness to the guard, from the same reviewer: r.PostForm alone is not a replacement, because with the secret in both places ignoring the query copy would mint a token for a request that leaked its secret in the URL, where the guard refuses. The two are complementary. The ask is not to rewrite this PR; it is that PLAT-660 should carry the allowlist shape rather than a longer name list, since the measurement shows the structural fix is smaller than the deferral implies.

🤖

coderd/oauth2provider/revoke.go:100

P4 [CRF-22] Revocation's generic branch writes err.Error() straight into error_description, so client-controlled bytes come back unsanitized and uncapped. (Ryosuke)

Verified live. A temporary subtest posting to /oauth2/revoke?client_id=<uuid>&%zz=1 with the secret in the body returns:

status=400 err="invalid_request" desc="invalid form data: invalid URL escape \"%zz\""

writeTokenError (tokens.go:318) exists precisely so descriptions are sanitized and capped "at the endpoint rather than at each call site", and RFC 6749 §5.2's NQSCHAR excludes the double quote that response just emitted. The token endpoint's matching branch answers a fixed string and never echoes.

Pre-existing, and P4 because the echoed bytes are the caller's own malformed query in a JSON error body. It is on the ladder this PR edits (the constant at line 88 replaced the duplicate literal CRF-8 named), so it is a sibling of what you already fixed rather than a separate subsystem. writeRevocationError mirroring writeTokenError would close it.

🤖

coderd/oauth2provider/tokens.go:373

P4 [CRF-23] A parameter sent twice is reported as missing. (Mafuuu)

Observed while checking CRF-9: with the guard disabled, a request carrying client_secret in both the query and the body answers Missing required parameter: client_secret. The parameter was sent twice, not zero times. parseSingle writes an accurate detail ("provided more than once, found 2 times"), and the loop at tokens.go:369 discards it, keying only on the field name.

The guard closes this for client_secret specifically. It stays open for code and client_id, which the same loop covers: a client that repeats code, for instance through a retry that appends rather than replaces, is told the code is missing and will keep sending the same request.

Pre-existing and outside the diff. Raised because it is the reason CRF-9's impostor message is convincing, and because nothing else will surface it: the accurate detail already exists and is thrown away.

🤖

🤖 This review was automatically generated with Coder Agents.