feat: register public clients without a secret - #28046
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
0d8a377 to
01ec6b3
Compare
872b5e3 to
7c8d3e5
Compare
01ec6b3 to
3d4b95e
Compare
7c8d3e5 to
800fda7
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 16 findings (2 P2, 4 P3, 5 Nit, 5 Note), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. Base 3d4b95e..800fda7. Netero first-pass (no P0-P2 findings; one Note on unused RegisterPublicClient), then 19-reviewer panel: Bisky, Chopper, Ging-Go, Gon, Hisoka, Kite, Knov, Komugi, Kurapika, Leorio, Mafu-san, Mafuuu, Melody, Meruem, Pariston, Razor, Robin, Ryosuke, plus wildcards Zoro and Luffy. 16 findings written to inventory: 2 P2, 5 P3, 4 Nit, 5 Note. Dominant convergent finding is CRF-1 (11 reviewers): the RFC 7592 PUT handler at registration.go:346 flips client_type without reconciling oauth2_provider_app_secrets, so the "public means no secret row" invariant this PR's InTx establishes at registration is broken by the sibling update path. 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 change stands up cleanly on the create side. Wrapping the app and secret inserts in a single InTx closes the pre-existing orphan-confidential race and does it with a two-mock-store test setup (mDB outer, mTx closure) that fails an insert issued off-transaction as an unexpected call, so the InTx contract is enforced by construction rather than by convention. Raw-body assertion on absent client_secret pins the RFC 7591 §3.2.1 wire contract rather than the decoded struct, which cannot distinguish absent from empty. Deriving discovery and registration from a single AllOAuth2TokenEndpointAuthMethods() list is the right single-owner narrowing for the advertised-vs-accepted pair. The JoinPath swap fixes a real latent double-slash bug.
From Hisoka on the dominant finding: "Bungee Gum. Pull the new InTx thread and it moves POST. It does not move PUT."
Severity: 2 P2, 5 P3, 4 Nit, 5 Note (16 findings).
Two P2 items need attention before this merges:
- [CRF-1] (11 reviewers converged, one at P1) The RFC 7592 PUT handler at
registration.go:346is the sibling of the invariant this PR just installed on the create side.UpdateClientConfigurationrecomputesclient_typefromtoken_endpoint_auth_methodand writes it straight to the app row with no touch ofoauth2_provider_app_secrets. Both directions reproduce broken states: public→confidential leaves an app withclient_type='confidential'and zero secret rows (the exact permanently-uninhabitable state the create-side transaction exists to prevent), and confidential→public leaves an orphan secret row against a nominally public client. This PR is what makes those transitions load-bearing. Treating the PR as if no follow-up will ever touch this code, this needs a decision here: rejectclient_typechanges on PUT (RFC 7592 does not require supporting them), or wire the same InTx pattern with matching secret insert/delete on the transition. If it must ship deferred, please file a linked issue rather than leaving it silent, because it cannot be agent-accepted as permanent. - [CRF-2] The new IMPORTANT callout at
docs/admin/integrations/oauth2-provider.md:130disagrees withvalidateRedirectURIson three independently checkable points: it omitshttps://to arbitrary hosts (the RFC 8252 §7.2 preferred method for native apps, which the code accepts), it lists only127.0.0.1for loopback while the validator also acceptslocalhostand[::1], and its "http to any other host" carve-out for confidential clients is wrong (confidential rejects non-loopback http too).
Two P3 items are pure regression-guard gaps that pin invariants your PR description names as goals: [CRF-5] no test sets a trailing-slash accessURL (a revert of url.JoinPath back to fmt.Sprintf passes the suite), and [CRF-8] TokenEndpointAuthMethodsSupported is asserted with require.Contains instead of require.ElementsMatch against codersdk.AllOAuth2TokenEndpointAuthMethods(). Both are one-line changes.
Stack-context observation: [CRF-3] (discovery advertises none while /oauth2/tokens rejects any exchange without client_secret) and [CRF-10] (RegisterPublicClient has no in-tree caller) both trace to the next PR in the stack. Reviewing this PR standalone, they need to be answered even if the eventual answer is "these ship together in a merge queue." Either hold "none" out of the advertised list until the token endpoint accepts it, or explicitly gate merging this PR on the follow-up landing in the same train. Same for the helper: fold into the PR that first uses it, or add a minimal exercise here.
Bundle observation: the InTx wrap and the JoinPath swap are two independent fixes riding along with the public-client feature. Each is defensible on its own reasoning and the PR description acknowledges the bundling. Called out only so the review record shows the bundle was noticed rather than missed.
coderd/oauth2provider/registration.go:346
P2 [CRF-1] The RFC 7592 PUT handler flips client_type without reconciling oauth2_provider_app_secrets, reintroducing the exact orphaned-confidential state this PR's InTx was written to prevent on the create side. (Knov P1, Hisoka P2, Chopper P2, Pariston P2, Mafuuu P2, Melody P2, Ryosuke P3, Razor P3, Kurapika P3, Meruem P3, Mafu-san P4)
From Hisoka: "Pull the new InTx thread and it moves POST. It does not move PUT. The invariant this PR just installed, 'public means no secret row', starts life legitimate on the POST side and is left undefended on the PUT side."
From Knov, the direct sequence: "1. Register a confidential client (POST); receives a client_secret and a secret row. 2. PUT the same client with token_endpoint_auth_method: \"none\". The row now has client_type = public, but the secret row still exists. Or the reverse: PUT a public client with token_endpoint_auth_method: \"client_secret_basic\". The row now has client_type = confidential, with no secret row, the exact state POST was just rewritten to prevent."
From Mafuuu on the security half of confidential→public: "Once the next PR in the stack teaches the token endpoint to accept a public client's PKCE-only exchange, that residual row is a stealth credential valid for a client that should authenticate with PKCE alone. Anyone who ever saw the original client_secret (support ticket, log, backup) can present it against a client the operator now considers public."
The PR description grounds the create-side transaction in exactly this invariant ("making a public client's 'no secret row' a legitimate state removes the ability to spot the orphaned confidential case by inspection later, so it's fixed here alongside the rest of this change"). This PR is what makes the class-of-bug reachable on the PUT side; before this PR, no dynamically-registered client was public in practice. The pattern-inheritance argument ("the update path already worked this way") does not carry: the precondition that made the pattern safe (client_type effectively constant) is exactly what this PR removes.
Narrowing options, in order of narrowness:
- Reject a PUT whose
req.DetermineClientType()differs fromexistingApp.ClientTypewithinvalid_client_metadata(RFC 7592 §2 does not require the type to be mutable). - If transitions must be supported, do them inside
InTxmatching the create-side shape: delete secret rows on confidential→public, mint and insert a fresh secret on public→confidential. The response typeOAuth2ClientConfigurationhas noclient_secretfield, so the second direction cannot return the new secret without a wider surface change; that is a design signal in favor of the reject option.
Human decision needed: fix in this PR, file a linked issue that names the two reachable states, or explicitly document why the transition is safe. A silent defer is not one of the options.
🤖
codersdk/oauth2.go:622
P3 [CRF-4] client_secret_expires_at is int64 with omitempty, so 0 (the RFC 7591 wire value for "never expires") disappears from the JSON registration response for every confidential client. (Chopper P3)
From Chopper (verified by marshalling OAuth2ClientRegistrationResponse{ClientSecret: "x", ClientSecretExpiresAt: 0}: client_secret present, client_secret_expires_at absent): "RFC 7591 §3.2.1 says client_secret_expires_at is REQUIRED when client_secret is issued, and that 0 is the on-the-wire value for 'never expires.' The recipient is any integrator whose registration client validates against the RFC schema: they see a required key missing and either fall back to a wrong default or reject the response outright."
Pre-existing but on the exact signal this PR is fixing (the registration response's RFC 7591 conformance), so it belongs in scope here. Drop omitempty from ClientSecretExpiresAt; if you need to omit the field for public clients whose response has no client_secret, do it structurally (custom MarshalJSON, or set the whole field only when ClientSecret != "").
🤖
🤖 This review was automatically generated with Coder Agents.
3d4b95e to
8c4a1c0
Compare
4bd1e82 to
cbdf0bc
Compare
Split out of #27873 to make that PR smaller to review. Third in the stack; this is the point where dynamic client registration actually produces a public client. An RFC 7591 registration requesting token_endpoint_auth_method: "none" now skips secret generation entirely: no secret is minted, and the app is persisted with the client_type the previous PR in the stack derives from that auth method. Discovery does not yet advertise "none" as a supported method. AdvertisedOAuth2TokenEndpointAuthMethods() excludes it until the token endpoint actually accepts a public client's exchange, in the next PR in the stack; advertising it earlier would tell a conforming client the server accepts an exchange it will reject. Registration now writes the app and its secret in one transaction. They were two independently committed inserts, so a failure of the second left a permanently committed app that can never authenticate while still holding a registration access token. Pre-existing, but making a public client's "no secret row" a legitimate state removes the ability to spot the orphaned confidential case by inspection later, so it is fixed here alongside the rest of this change. The registration_client_uri now uses url.JoinPath instead of fmt.Sprintf, fixing a latent bug where an access URL configured with a trailing slash would mint "//oauth2/clients/{id}" as the client's management endpoint. Pinned with a regression test against a trailing-slash access URL. The public-client redirect URI documentation is corrected to match validateRedirectURIs: https is allowed for both client types, the loopback list was incomplete, and the confidential-client restriction was misstated. RegisterPublicClient, a test helper for registering a public client end to end, is exercised in this PR instead of landing unexercised for a later PR to discover a bug in. The token endpoint does not yet accept a public client's PKCE-only exchange; that follows in the next PR in the stack, so a client registered here cannot yet obtain a token.
cbdf0bc to
7c124e7
Compare
1 participant
Split out of #27873 to make that PR smaller to review. Third in the stack (on top of #28043); this is the point where dynamic client registration actually produces a public client.
An RFC 7591 registration requesting
token_endpoint_auth_method: "none"now skips secret generation entirely: no secret is minted, and the app is persisted with theclient_typethe previous PR in the stack derives from that auth method. Discovery advertises"none"as a supported method so a client can find out Coder will accept it.Registration now writes the app and its secret in one transaction. They were two independently committed inserts, so a failure of the second left a permanently committed app that can never authenticate while still holding a registration access token. Pre-existing, but making a public client's "no secret row" a legitimate state removes the ability to spot the orphaned confidential case by inspection later, so it's fixed here alongside the rest of this change.
registration_client_urinow usesurl.JoinPathinstead offmt.Sprintf, fixing a latent bug where an access URL configured with a trailing slash would mint//oauth2/clients/{id}as the client's management endpoint.The token endpoint does not yet accept a public client's PKCE-only exchange; that follows in the next PR in the stack, so a client registered here cannot yet obtain a token. Dynamic client registration itself is off by default (
GetOAuth2DCREnabled), so this is not user-visible until the next PR lands.Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client