feat: register public clients without a secret - #28046

P2 [CRF-2] The public-client redirect-URI callout contradicts validateRedirectURIs on three independently checkable points. (Zoro P2, Mafu-san P2, Mafuuu P3, Razor P3, Chopper Nit)

From Zoro:

validateRedirectURIs in codersdk/oauth2_validation.go:127 accepts https:// for both client types (line 155 falls through with no further check when the scheme is https), yet the docs omit https entirely and say the only allowed shapes for a public client are loopback http, custom scheme, and urn:...:oob. A user reading this will assume a native app that redirects to a claimed https:// URL (the RFC 8252 §7.2 pattern) is unregisterable, which is not what the code enforces.

isLoopbackAddress accepts localhost, 127.0.0.1, and ::1 (line 311). The docs give only http://127.0.0.1:{port}/... as the loopback form.

The last sentence, "http redirects to any other host are rejected for public clients and are usable only by confidential ones," is wrong for the confidential side too. Line 165 rejects any confidential-client http:// URI whose host is not in isLocalhost (that adds .localhost). Non-loopback http:// is rejected for confidential clients as well.

From Mafuuu: TestCreateDynamicClientRegistration_ClientType/NoneIsPublicWithNoSecret in this PR registers a public client with RedirectURIs: []string{"https://example.com/callback"} and receives 201 Created, so the code allows exactly the shape the docs steer readers away from.

Replace the block with what the code actually enforces. Zoro proposed:

> [!IMPORTANT]
> Public clients must use one of:
> - `https://` to any host,
> - `http://` with a loopback host (`localhost`, `127.0.0.1`, `[::1]`),
> - a custom URI scheme (`myapp://callback`, `vscode://callback`), or
> - the `urn:ietf:wg:oauth:2.0:oob` out-of-band URN.
>
> `http://` to a non-loopback host is rejected. Confidential clients have
> the same restriction, except they also accept `.localhost` subdomains.

🤖

P3 [CRF-3] Discovery advertises "none" in token_endpoint_auth_methods_supported, but the token endpoint at tokens.go:96-101 still appends client_secret is required and cannot be empty for every authorization_code grant with an empty secret and never inspects client_type. (Knov P2, Kurapika P3, Meruem P3, Melody P3, Mafuuu Note, Mafu-san Note, Pariston Note, Kite Note, Luffy Note)

From Kurapika: "A conforming client that trusts .well-known/oauth-authorization-server, registers with token_endpoint_auth_method=none, gets no secret in the response (correct per RFC 7591 §3.2.1), and then attempts the token exchange will hit client_secret is required at parameter parsing, before authorizationCodeGrant even runs."

From Knov on the in-file comment's rationale ("disabling registration stops new public clients being created but does not stop existing ones exchanging tokens, so the method remains supported"): "No existing public client can exchange tokens today, so that rationale is aspirational, not a description of the tree."

The follow-up PR named in the description will make this coherent, but reviewing this PR standalone, discovery is telling a conforming client something the server will not do. DCR is off by default (GetOAuth2DCREnabled), but discovery is served unconditionally.

Options:

• Hold "none" out of AllOAuth2TokenEndpointAuthMethods() until the token endpoint accepts it. Registration then rejects "none" (consistent with the token endpoint's current behavior) and this becomes a coherent no-op until the follow-up.
• Introduce a separate AdvertisedTokenEndpointAuthMethods() filtered by what the token endpoint executes.
• Land the discovery advertisement in the same PR as the token-endpoint change so the pair agrees at merge.

Also: revise the comment at metadata.go:39-42. As written, it justifies the choice with a state the tree does not currently produce.

🤖

P3 [CRF-5] The fmt.Sprintf → accessURL.JoinPath fix has no regression test. (Mafu-san P3, Chopper Nit, Meruem Nit, Ryosuke Note)

From Mafu-san: "None of the three new tests uses a trailing-slash accessURL... Revert accessURL.JoinPath(\"/oauth2/clients\", clientID.String()) to the old fmt.Sprintf and every test still passes. The fix is real (verified against the code) but the regression guard is not."

The PR description names this fix explicitly ("a latent bug where an access URL configured with a trailing slash would mint //oauth2/clients/{id} as the client's management endpoint"), so it is worth pinning. Add one subtest row that sets accessURL to https://example.com/ (trailing slash) and asserts the response's RegistrationClientURI contains exactly one slash between authority and /oauth2/clients/. Same pattern would fail against path.Join as well.

🤖

P3 [CRF-6] // Create client secret - parse the formatted secret to get components mislocates the operation. (Gon P2)

From Gon: "'Create client secret' is wrong at this location. The secret was already generated on line ~82 by generateClientCredentials(). The block that follows parses that string to pull the prefix out, then inserts a DB row for it. It creates a row, not a secret. 'parse the formatted secret to get components' is what the function name ParseFormattedSecret says."

A future maintainer reading this hunts for a secret-minting call that is not there. Delete or replace with one line naming what the parse is for (extracting Prefix for InsertOAuth2ProviderAppSecretParams.SecretPrefix).

🤖

Nit [CRF-7] The em-dash → comma cleanup is applied to the two //nolint:gocritic comments this change touched but not to the ten sibling instances in the same file and package. (Kite Nit)

From Kite: "Six identical // OAuth2 system context — RFC 7592 client configuration endpoint comments remain in registration.go at lines 227, 313, 338, 421, 445, 497, and four more in tokens.go at lines 261, 289, 451, 488. scripts/check_emdash.sh only scans added lines in the diff, so these siblings sit under the linter's radar but violate the same rule the change is enforcing on the lines it touched."

One-character change each. Doing them now stops the class re-appearing in a future diff against those lines.

🤖

Nit [CRF-12] The trailing sentence "The docs promise the field is absent, so that is what to pin." restates the opening "client_secret is omitted entirely for a client that was not issued one." (Gon P2)

From Gon: "Both sentences deliver the same fact (RFC 7591 §3.2.1 requires absence, not empty). Drop the last sentence; keep the middle two, which teach the actual why (decoded struct can't distinguish absent from zero)."

🤖

Nit [CRF-13] // Generate client credentials. narrates the mechanism of the two lines that follow it. (Gon Nit)

From Gon: "The payload of the block is the second sentence ('Public clients authenticate with PKCE alone and never receive a secret'). Drop the leading section-header sentence and keep the rationale:

// Public clients skip secret generation and authenticate with PKCE
// alone (RFC 7591 §2, OAuth 2.1 §2.1).
```"

> 🤖

Note [CRF-14] The transaction test's mock InsertOAuth2ProviderApp return hardcodes ClientType: OAuth2ProviderAppClientTypeConfidential regardless of what the handler asked for. (Ryosuke Note)

From Ryosuke: "Not a bug in this PR, it happens to line up with the confidential request the test sends, but it means the mock is not honoring the parameters. If a later change reuses this test scaffolding for a public request without noticing, the assertion drifts silently. Echoing back params.ClientType in the DoAndReturn would keep the mock and the handler in step."

🤖

Note [CRF-15] now := dbtime.Now() and uuid.New() for the secret ID are captured outside the InTx closure. (Pariston Note)

From Pariston: "Not currently a bug: InTx(fn, nil) runs at the driver's default isolation and does not retry on serialization failure, so the captured values are used exactly once. Worth knowing because if anyone later passes a TxOptions with retry semantics, or wraps this in a retry loop, the same secret ID would be reused across attempts and the second attempt would fail on uniqueness. Moving the uuid.New() and dbtime.Now() calls inside the closure would remove that footgun for the cost of two lines."

🤖

Note [CRF-16] TestCreateDynamicClientRegistration_PublicClientSkipsSecretInsert overlaps TestCreateDynamicClientRegistration_ClientType/NoneIsPublicWithNoSecret on the same behavior. (Luffy Note, Zoro Note)

From Zoro: "The real-DB test at line 137 already asserts require.Empty(t, secrets) after registering with none, which fails if the handler calls InsertOAuth2ProviderAppSecret at all (an insert with the empty prefix/hash still produces a row that GetOAuth2ProviderAppSecretsByAppID returns). The mock test adds one increment: it fails at the call site rather than at the read side."

From Luffy: "The mock test pins the mechanism (no call was made). The parametric test pins the outcome (no row exists). Users see the outcome. Drop the mock test, one behavior test is enough."

Call out whether the two tests are meant to hold different invariants. If not, drop the mock one; if yes, name the invariant in the mock test's comment so a later reader does not merge them.

🤖