…ngth

tr -d "=+/" deleted every '+' and '/' character that happened to appear
in the base64 output instead of translating them to the URL-safe
alphabet, so cut -c -43 truncated a string that was often already
short. Roughly 70% of runs produced a verifier below the 43-character
floor coderd/oauth2provider now enforces (#28003), so the manual and
scripted OAuth2 flows these scripts drive failed token exchange
intermittently.

Use tr '+/' '-_' | tr -d '=' instead: translating first and then
stripping the single padding character is deterministic, since 32
random bytes always base64-encode to a fixed length. This always
yields exactly 43 characters, so the cut is no longer needed.

extractAuthorizeParams only checked code_challenge for non-emptiness, so
a malformed value (wrong length, disallowed characters, an arbitrarily
large blob) was persisted verbatim and only surfaced as a failure at
token exchange, with an error that misleadingly names code_verifier
instead of the parameter that was actually invalid.

RFC 7636 gives code_verifier and code_challenge the same ABNF, so reuse
the existing bounds check rather than adding a second one: rename
ValidPKCEVerifier to ValidPKCEFormat and validate code_challenge against
it in extractAuthorizeParams, rejecting a malformed value with
invalid_request at the authorization request per RFC 7636 §4.4.1.

TestExtractAuthorizeParams_Scopes used a 14-character placeholder
code_challenge that the new check now correctly rejects; lengthened it
to a valid value since that test only exercises scope parsing.

…_verifier

A malformed code_verifier (wrong length or disallowed characters) and a
well-formed verifier that simply fails the PKCE hash comparison both
returned the same error: invalid_grant, "The PKCE code verifier is
invalid." A client that sent a too-short verifier had no way to tell
that apart from a genuine hash mismatch, would re-check its SHA-256
computation, find nothing wrong, and retry the same bad verifier
indefinitely since invalid_grant conventionally signals "retry."

RFC 6749 §5.2 assigns a malformed parameter to invalid_request; RFC 7636
§4.6 reserves invalid_grant for the comparison failure specifically. Move
the code_verifier format check out of authorizationCodeGrant and into
extractTokenRequest, which already owns syntax validation for this grant
type, so the two failure modes return distinct, spec-accurate errors.

Several existing tests sent an empty or placeholder code_verifier
incidental to what they were actually testing (client_secret
requirements, scope parsing, malformed-code handling); updated them to
use a valid-length value so they still reach the behavior under test.

…f PKCE hash mismatch

InvalidCodeVerifier ("wrong-verifier", 14 chars) was rejected on length
before VerifyPKCE ever ran, so no test exercised the token endpoint's
hash-comparison branch end to end; TestVerifyPKCE unit-tests the
function, but nothing proved the endpoint still calls it.

Lengthen InvalidCodeVerifier to a well-formed but wrong 43-character
value so it again reaches the hash comparison. Add MalformedCodeVerifier
and a new test asserting the length-rejection path returns
invalid_request, now that the previous commit gives it a distinct error
from the hash-mismatch invalid_grant case.

The code was deleted only inside the success-path transaction, so every
PKCE rejection (errInvalidPKCE) left it live in the database. RFC 6749
§10.5 requires authorization codes to be single-use; without that, an
attacker holding a leaked code (the exact threat PKCE defends against,
since codes and challenges land in browser history, referrer headers, and
proxy logs) could retry the token endpoint with different code_verifier
guesses for the entire 10-minute code lifetime, unthrottled. The
43-character length floor bounds guess format, not entropy.

Add revokeOAuth2CodeOnPKCEFailure, called from both PKCE rejection paths
in authorizationCodeGrant. It deletes the code using the same system
authz context already used for reads in this function; a deletion
failure is noted on the request's log line rather than changing the
response, since surfacing it as a different error would let a caller
distinguish delete success from failure, itself a new oracle.

Added TestOAuth2PKCEFailureConsumesCode to verify the code is
unredeemable, even with the correct verifier, once a PKCE mismatch has
occurred.

Tighten the ValidPKCEFormat doc comment and correct a false claim
(CRF-8, CRF-10). The rationale restated the same threat model across
three separate rhetorical framings, and claimed PKCE is the only
client authentication some clients have, which is false today since
authorizationCodeGrant validates a client secret before PKCE ever
runs; that claim only becomes true once #27873 adds public clients.
Trim the paragraph to a single concrete why and note the caveat.

Delete four boundary-case comments in pkce_test.go (CRF-9). Each one
restated the case name and the strings.Repeat literal beside it; the
RFC provenance already lives on ValidPKCEFormat's doc comment and the
pkceVerifierMinLength/pkceVerifierMaxLength constants, so the comments
carried no information and would drift if either constant changed.

Replace an em-dash with a comma in a comment inside the block this
PR's PKCE-failure handling touches (CRF-2), per the repo's no-emdash
rule. It survived lint because the check scans only changed lines by
default, and this comment was pre-existing context rather than a line
this PR added.

Fix the PKCE example in docs/admin/integrations/oauth2-provider.md
(CRF-7). tr -d "=+/" deleted reserved base64 characters instead of
translating them to the URL-safe alphabet, so the example computed a
code_challenge that failed to verify roughly 74% of the time. Also
strip the newline openssl base64 inserts at its default 64-column
wrap, which the 96-byte verifier example crosses; the prior
cut -c1-128 never merged the wrapped lines back together either.

The PKCE Flow section showed how to generate a code_verifier and
code_challenge but never stated the bound now enforced server-side:
43 to 128 characters from the unreserved set [A-Za-z0-9-._~] (RFC
7636 §4.1). A value outside these bounds returns invalid_request,
at the token endpoint for code_verifier and at the authorization
endpoint for code_challenge.

isValidCustomScheme required a literal "." in the scheme for a public
client's redirect URI, so vscode://, jetbrains://, and cursor:// all
400'd while the identical schemes passed for a confidential client
through the separate, more permissive validateScheme. Native and CLI
apps, the population public clients exist for, register those exact
schemes with their OS.

Removed the extra restriction: validateScheme already blocks the
schemes that are actually dangerous in a redirect context, and RFC
8252 section 7.1 only recommends reverse-domain notation rather than
requiring it. PKCE, not the scheme's spelling, is what secures a
public client's redirect.

That removal also stopped rejecting mailto, tel, and sms for public
clients specifically, since validateScheme's dangerous-scheme
blocklist never covered them either. Those three hand off to a mail
client, dialer, or SMS app rather than returning control to the
client, so unlike vscode:// or jetbrains://, none of them can deliver
an authorization code. A public client's redirect URI scheme is its
only mechanism for regaining control, so they are rejected again here,
scoped specifically to public clients rather than folded into
validateScheme's blocklist, since they are harmless for a confidential
client's redirect.

…/tel/sms scope, not an invented one

The previous comment claimed mailto, tel, and sms are harmless for a
confidential client's redirect specifically. That is not true: the
client_secret only matters at token exchange, not at redirect
delivery, so nothing about being confidential changes what happens
when the browser is sent to one of these schemes. The actual reason
they are checked only in the isPublicClient branch is that
custom-scheme validation was already scoped there before this PR;
confidential clients were never subject to any scheme-shape check
here, independent of any judgment about these three schemes.

Split out of #27873 to make that PR smaller to review. Second in the
stack; adds the vocabulary the rest of the public-client work is built
on, with no behavioral change beyond what it stores.

RFC 7591 §2 / OAuth 2.1 §2.1 define two client types: a confidential
client authenticates with a secret, a public client authenticates with
PKCE alone. DetermineClientType() previously hardcoded "confidential"
regardless of the requested token_endpoint_auth_method. It now derives
the type via the new ClientTypeFor() mapping, which is the single owner
of the auth-method-to-client-type relationship: registration derives
the stored client_type from it, and redirect URI validation uses it to
pick which RFC 8252 rules apply, so the two cannot disagree about what
"public" means.

OAuth2ProviderApp.IsPublic() is the reader for the stored client_type
column, added alongside matching database constants so the value
registration writes and the value IsPublic reads back cannot drift.
An unset or unrecognized client type reads as confidential, so an app
can never skip client authentication by accident.

AllOAuth2TokenEndpointAuthMethods() is the single source Valid() reads
from, so what registration accepts is defined in one place. Discovery
metadata does not yet derive from it and still hardcodes its own list
without "none"; a follow-up PR wires the token endpoint to honor
"none", and only then should discovery advertise it too.

registration.go and app registration itself do not yet skip secret
issuance for a public client; that follows in the next PR in the
stack.

Split out of #27873 to make that PR smaller to review. First in the
stack; the rest of the public-client work builds on this.

`isValidCustomScheme` required a literal `.` in the scheme for a public
client's redirect URI, so `vscode://`, `jetbrains://`, and `cursor://`
all 400'd while the identical schemes passed for a confidential client
through the separate, more permissive `validateScheme`. Native and CLI
apps, the population public clients exist for, register those exact
schemes with their OS.

Removed the extra restriction: `validateScheme` already blocks the
schemes that are actually dangerous in a redirect context, and RFC 8252
section 7.1 only recommends reverse-domain notation rather than
requiring it. PKCE, not the scheme's spelling, is what secures a public
client's redirect.

That removal also stopped rejecting `mailto`, `tel`, and `sms` for
public clients specifically, since `validateScheme`'s dangerous-scheme
blocklist never covered them either. Those three hand off to a mail
client, dialer, or SMS app rather than returning control to the
application that started the flow, so a public client registered with
one of them could never actually complete authorization. They are
rejected again here, scoped to public clients only because that is how
custom-scheme validation was already scoped before this change, not
because they are known to be safe for a confidential client's redirect;
confidential clients were never subject to any scheme-shape check beyond
`validateScheme` and remain so here.

Refs
https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client

…-vocabulary

UpdateClientConfiguration wrote ClientType: string(req.DetermineClientType())
on every PUT, recomputed from the request instead of read from storage.
ApplyDefaults() fills an omitted token_endpoint_auth_method with
client_secret_basic, so a public client's PUT that only touched an
unrelated field (e.g. redirect_uris) silently converted it to
confidential, since DetermineClientType() can now return "public" where
it previously always returned "confidential".

A client's type is fixed at registration; RFC 7592 §2.2 permits
rejecting metadata the server will not accept. UpdateClientConfiguration
now rejects a PUT that would move a client between public and
confidential with 400 invalid_client_metadata, and carries the stored
client_type through verbatim rather than re-deriving it. A legacy row
whose stored client_type and token_endpoint_auth_method already
disagree can still manage itself, as long as the update does not also
ask to change the auth method.

ClientTypeFor(), extracted as its own function in the previous commit,
had exactly one caller and no second one materialized, so it is
inlined back into DetermineClientType().

…-vocabulary

Resolves a conflict in coderd/oauth2provider/tokens.go: #28003 hardened
revokeOAuth2CodeOnPKCEFailure on main (detaches the delete from the
request context, treats sql.ErrNoRows as non-error) after this branch's
own copy of that function predated the hardening. Took main's version
in full; this branch made no independent edits to it.

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.

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.

…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.

The token endpoint accepts a PKCE-only exchange from a public client as of
this branch, but token_endpoint_auth_methods_supported still omitted "none",
so a client doing RFC 8414 discovery could not learn that secretless
registration was available.

AdvertisedOAuth2TokenEndpointAuthMethods now returns everything registration
accepts. It stays separate from AllOAuth2TokenEndpointAuthMethods because the
two bound different things: what Valid() enforces on a registration request,
versus what the token endpoint honors at exchange time.

The existing ElementsMatch assertion compares the handler against the
function, so it holds for whatever the function returns. Added a Contains
assertion against the literal value, which is what fails if "none" stops
being advertised.

TestClientConfiguration_ReportedAuthMethod seeded no confidential row
storing client_secret_post, the only stored value whose reported method
differs from the client type default. Every case passed whether
reportedAuthMethod returned the stored method or switched on client type
alone, so collapsing it to a two-value switch would have silently
reported client_secret_basic for those rows and shipped green.

Verified by short-circuiting the "return stored" branch: the whole
coderd/oauth2provider package passed before this case and fails on it
after.

The rejection stated the constraint but not the next step, while the
sibling client type error in registration.go already ends with "register
a new client instead". Match it.

RejectsPublicClient asserted only require.Error, which the client returns
for any non-201, so a routing or middleware failure that never reached
the guard passed too. Pin the status code and the reason instead.

The handler comment, its test comment, and the test godoc each restated
the rationale that belongs on the code they cover. Keep the reasoning at
one site and let the others say what they verify.

Also soften the self-healing claim on reportedAuthMethod: the row heals
only when a client echoes back the reported method, which is what the
resending-reported cases show, not a guarantee for a client that resends
the stored value.

…c-clients-guards

Remove the function headers from the public-client token exchange and
client-secret requirement tests, whose names and subtest names already say
what they cover, and cut the inline explanations to the facts that are not
in the assertion messages.

Comments only, no behavior change.

The two call sites explained the same code_verifier length floor at
different lengths.

Comments only, no behavior change.

Comments only, no behavior change.

…tests

Remove the headers from the client type and reported auth method tests,
whose names and table case names already say what they cover, and cut the
case comments to what those names cannot carry. The reason the fixtures are
seeded through dbgen moves to the call it explains.

Comments only, no behavior change.

…c-clients-guards

Creating a secret for a public client is rejected, but the generated
reference listed only the 200 response, so integrators had no documented
explanation for the failure. Annotate the endpoint and regenerate.