feat: accept PKCE-only token exchange for public clients - #28047
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
BobbyHo
changed the title
7c8d3e5 to
800fda7
Compare
be1eaf3 to
b4ab1ba
Compare
cbdf0bc to
7c124e7
Compare
b4ab1ba to
e19d7ea
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 23 findings (5 P2, 1 P3, 12 Nit, 5 Note), COMMENT. Review Finding inventoryFinding inventoryFindings
Contested and acknowledged(No entries yet.) Round logRound 1Panel. Netero-only pre-pass returned "No findings," so the panel ran the same round. 19 panel reviewers: Bisky, Chopper, Ging-Go, Gon, Hisoka, Kite, Knov, Knuckle, Komugi, Kurapika, Leorio, Mafu-san, Mafuuu, Meruem, Pariston, Razor, Ryosuke, Zoro (wildcard), Luffy (wildcard). Chopper, Ging-Go, Hisoka, Knuckle, Komugi, Kurapika, Mafu-san, Mafuuu, Meruem, Pariston, Zoro, Luffy returned no findings. 1 P2, 8 Nit, 6 Note. Reviewed against 7c124e7..e19d7ea. Round 2Churn guard: PROCEED. All 15 R1 findings marked Author fixed across commits aca591d, d93704d, 240db54, 577538b. Reviewed against 7c124e7..240db54. Panel round. Netero returned "No findings." 11 panel reviewers: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Meruem, Kurapika, Ryosuke, Zoro (wildcard). Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Kurapika returned no findings. 1 P3, 4 P2, 4 Nit new. Convergent structural finding (CRF-23) from Ryosuke P3, Zoro P3, Meruem Nit; higher wins per tiebreaker. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR does exactly what it claims and no more. The three production edits (skip client_secret presence, skip the secret block, mint NULL app_secret_id) each have a test that would fail without that edit, and the refresh/revoke paths that the PR asserts "already verify ownership via app_id" are now exercised end-to-end against a token row with AppSecretID.Valid == false. RFC citations sit at the exact lines they enforce (RFC 7591 §2, OAuth 2.1 §2.1, RFC 6749 §5.2 and §10.5, RFC 7636 §4.1 and §4.6, RFC 7009). The load-bearing shift, dbCode.AppID != app.ID moving from defense in depth to the sole binding between the exchange and the app named by client_id, is pinned with two public apps sharing a redirect URI, which is the exact shape the check has to hold in.
One fun quote from the panel, on the error handling: "Every new error message in this repo should read like this. This is the bar." (Leorio)
Findings: 1 P2, 8 Nit, 6 Note. Nothing structural, nothing blocking; the P2 is a godoc anchor and the notes are mostly about comment precision and one signal-hiding hazard around public clients that mis-send client_secret.
Process note (no inline anchor for a commit-metadata concern): the commit and PR scope coderd/oauth2provider does not contain coderd/oauth2.go, coderd/apidoc/*, docs/admin/integrations/oauth2-provider.md, or docs/reference/api/enterprise.md. AGENTS.md requires the scope to be a real filesystem path containing every changed file, or omitted for cross-cutting changes. Drop the scope or broaden it (coderd,docs) on the next amend.
Nothing here needs a re-review round; the notes and nits can travel with the merge or the next fix-up.
coderd/oauth2provider/tokens.go:332
Note [CRF-12] Consider a one-line comment on this check so a future reader doesn't refactor it into the !isPublic block by symmetry. (Kite)
For confidential clients, dbSecret.AppID != app.ID above pre-binds client_id to a known secret before this line runs; for a public client, client_id reaches authorizationCodeGrant completely unauthenticated (parsed from the form) and this single line is the entire binding between the exchange and the app named by client_id. The rationale comment at tokens.go:278-281 names this shift, but the load-bearing line itself carries only "same reason as the secret check above," which reads like defense in depth even when it is the last line of defense for public clients. A one-liner such as // For public clients this is the only binding between client_id and the code; do not move into the !isPublic block above. would prevent a plausible "cleanup" that quietly deletes the sole public-client bind. TestOAuth2ProviderTokenExchangePublicClientCodeBelongsToDifferentApp defends this regression; the comment makes the defense visible in the code.
🤖
🤖 This review was automatically generated with Coder Agents.
Split out of #27873 to make that PR smaller to review. Fourth in the stack; this is the half that makes the public client registered by the previous PR in the stack actually able to obtain a token. The token endpoint no longer requires a client_secret for a public client: extractTokenRequest skips the client_secret presence check, and authorizationCodeGrant skips secret validation entirely for a public client, since it has none. PKCE was already mandatory for every authorization_code flow, so public clients inherit it with no new validation code. That makes the code ownership check (dbCode.AppID != app.ID) 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. It is retained and now covered with a public client on both sides. Issued tokens for a public client carry a NULL app_secret_id rather than referencing a secret row that does not exist. The refresh and revocation paths already verify ownership directly via app_id rather than joining through app_secret_id, so they need no code change, only updated comments and coverage confirming they handle a NULL app_secret_id correctly. Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client
RFC 6749 §2.3 defines client authentication as proving client identity, and §3.2.1 says a public client is not authenticated. RFC 7636 §1 casts PKCE as a mitigation against authorization code interception, binding the token request to the entity that started authorization. Calling it "client authentication" pointed readers at the opposite of what the RFCs say, so the swagger annotation, the admin docs page, and the comments in tokens.go now call it proof of possession. The docs page also dropped "shorter values are rejected", which named one of the three RFC 7636 §4.1 failure modes and left a caller who sent a 200-character verifier expecting it to pass. extractTokenRequest's godoc claimed IsPublic was the only reader of ClientType; registration.go and apps.go read it too. It is the only decision-making reader, which is what keeps the confidential/public branch in one place.
…tests TestOAuth2ProviderTokenExchangePublicClientCodeBelongsToDifferentApp ran the PKCE probes its name did not advertise, so it is now TestOAuth2ProviderPublicClientTokenExchange, covering both checks that stand in for a client secret: code ownership and PKCE. The empty code_verifier case is gone; it hits the same length branch as the one-character case, which strictly dominates it, and pkce_test.go tables both at the unit level. TestOAuth2PublicClientTokenLifecycle ran a full register, authorize, exchange, refresh, and cross-app revoke for each of its two table rows, which differ only in which string reaches RevokeOAuth2Token. That setup moves to refreshedPublicClientSession, leaving each row with the revoke and the session check that actually diverge. Comments no longer anchor on "this PR" or "currently", and the comment above assertSecretlessToken no longer implies the raw-DB read proves the token authenticates; the session probe is what proves that. Literal 43s use pkceVerifierMinLength, and the public-client-with-secret case cites RFC 7591 §2 and OAuth 2.1 §2.1 rather than RFC 6749 §2.3.1, which does not authorize public clients to send credentials.
…s it extractTokenRequest derived isPublic at the top of the function but read it only inside the authorization_code branch, so a reader hitting the derivation had to carry it past the form parsing, the Basic auth merge, and the required-parameter checks before learning what it decides.
b6d1a46 to
240db54
Compare
|
Round 1 responses. All 15 findings are addressed across four commits; the branch was force-pushed to correct one commit message, so the SHAs below are the current ones.
|
|
/coder-agents-review |
There was a problem hiding this comment.
R2 verifies R1 cleanly. All 15 R1 findings landed at the root cause across aca591d, d93704d, 240db54, and 577538b; the churn guard classified every finding as Author fixed and spot checks confirm. Kurapika, Bisky, Hisoka, Mafuuu, Mafu-san, and Pariston returned no findings. The commit graph is now honestly scoped (refactor(coderd/oauth2provider):, test(coderd):, unscoped feat: and docs: for the cross-cutting commits); CRF-15 was internalized, not just applied once.
New findings: 1 P3, 4 P2, 4 Nit. Two patterns worth naming.
-
The R1 CRF-14 sweep landed in
extractTokenRequestbut skipped the sibling function.authorizationCodeGrantstill hoistsisPublicanddbSecretto outer scope only so a secondif !isPublicblock ~170 lines away insidedb.InTxcan readdbSecret.IDfor the token row'sAppSecretID. Three reviewers converge on this from different angles: Meruem calls the outerdbSecreta courier, Ryosuke names the concrete failure (a future edit flipping one branch mintsAppSecretID = {UUID: uuid.Nil, Valid: true}, a dangling FK pointer), Zoro proposes the same colocation. Fix collapses the two branches to one and eliminates the paired-branch invariant. -
The R1 CRF-1 / CRF-2 sweep, which was about stale-after-merge anchors in comments, closed the specific instances but left multiple siblings on the same file. Two positional / temporal anchors survived in
oauth2_test.go("the test above", "the first with a NULL app_secret_id", "the claim app_id was promoted for"), and the round's rewrites inrevoke.goandoauth2_test.goexpanded four comment blocks by re-narrating rationale that lives on the production code or on sibling comments. Gon flags all seven; Leorio echoes on two.
One fun quote from the panel: "coupled only through a function-level dbSecret used as a courier." (Meruem)
Still COMMENT, not REQUEST_CHANGES: the P3 is a class-of-bug elimination worth doing but the code works today, and the P2s are AGENTS.md fact-in-one-place violations that trim rather than change behavior. Reasonable to fold into the same amend as CRF-23.
🤖 This review was automatically generated with Coder Agents.
… the branch that reads it The confidential branch now records appSecretID directly instead of leaving the fetched secret in function scope for a second !isPublic branch inside the insert transaction to read.
…o the trap Both guard sites keep a one-line warning against joining through app_secret_id. The rationale itself lives on the app_id column comment.
Name the referenced test instead of anchoring by position, drop the PR-context narration and the ordinal claim, and rename the lifecycle test into the TestOAuth2Provider* bucket its sibling already uses.
1 participant
Split out of #27873 to make that PR smaller to review. Fourth in the stack (on top of #28046); this is the half that makes the public client registered by the previous PR in the stack actually able to obtain a token.
The token endpoint no longer requires a client_secret for a public client: extractTokenRequest skips the client_secret presence check, and authorizationCodeGrant skips secret validation entirely for a public client, since it has none. PKCE was already mandatory for every authorization_code flow, so public clients inherit it with no new validation code. That makes the code ownership check (dbCode.AppID != app.ID) 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. It is retained and now covered with a public client on both sides.
Issued tokens for a public client carry a NULL app_secret_id rather than referencing a secret row that does not exist. The refresh and revocation paths already verify ownership directly via app_id rather than joining through app_secret_id, so they need no code change, only updated comments and coverage confirming they handle a NULL app_secret_id correctly.
Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client