mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix: reject PKCE code_verifier below RFC 7636 length floor (#28003)
The token endpoint accepted any non-empty `code_verifier`, so a one-character verifier was enough to authenticate. RFC 7636 §4.1 requires 43 to 128 characters from the unreserved set. That fix plus the related gaps review surfaced in the same path: - Enforce the length and charset floor on the verifier before the S256 comparison runs. - Validate the challenge at the authorize endpoint too. It was only checked for non-emptiness, so a malformed challenge was stored and then failed late at token exchange, blaming the wrong parameter. - A malformed verifier now returns `invalid_request` (RFC 6749 §5.2); a well-formed but wrong one still returns `invalid_grant` (RFC 7636 §4.6). Both looked identical before, so a client had no way to tell a syntax error from a hash mismatch and would retry the same bad verifier forever. - Revoke the authorization code when a PKCE check fails. Without that, a leaked code could be replayed with unlimited verifier guesses for its remaining lifetime, and RFC 6749 §10.5 requires codes to be single use. - Fix verifier generation in `scripts/oauth2/*.sh` and the docs example. They deleted reserved base64 characters instead of translating them to the URL-safe alphabet, so most runs produced verifiers under the new floor. Also carries #28041, which merged into this branch: public clients may register bare custom schemes such as `vscode://` again, with `mailto`, `tel`, and `sms` rejected. Split out of #27873 (public OAuth2 client support). PKCE is already mandatory for every client, so this stands on its own. <details> <summary>Manual verification</summary> Ran against a local dev server on this branch, using a session token and a throwaway app from `scripts/oauth2/setup-test-app.sh`. 1. Happy path unchanged: HTTP 200, verifier length 43. 2. `code_verifier=short`, and a 43-character verifier ending in `!`: both HTTP 400 `invalid_request`, so charset is enforced and not just length. 3. `code_challenge=tooshort` at authorize: HTTP 400 `invalid_request`, no code issued. An empty challenge still hits the older "required and cannot be empty" message. 4. Well-formed but wrong verifier: HTTP 400 `invalid_grant`, distinct from the cases above. 5. Retrying that same code with the correct verifier: HTTP 400, code already revoked by the failed check. 6. `generate-pkce.sh` produces a 43-character verifier (20 out of 20 runs); the docs example produces 128. 7. `scripts/oauth2/test-mcp-oauth2.sh` passes end to end. The two bearer-token failures in its output are a pre-existing script bug (`09c50559f3`, July 2025) that reuses a resource-scoped token against the real API, not a regression here. </details>
This commit is contained in:
@@ -182,11 +182,18 @@ PKCE is **required** for all OAuth2 authorization code flows. Coder enforces
|
||||
PKCE in compliance with the OAuth 2.1 specification. Both public and
|
||||
confidential clients must include PKCE parameters:
|
||||
|
||||
> [!NOTE]
|
||||
> `code_verifier` and `code_challenge` must each be 43-128 characters from
|
||||
> the unreserved character set `[A-Za-z0-9-._~]` (RFC 7636 §4.1). A value
|
||||
> outside these bounds is rejected with an `invalid_request` error, at the
|
||||
> token endpoint for `code_verifier` and at the authorization endpoint for
|
||||
> `code_challenge`.
|
||||
|
||||
1. Generate a code verifier and challenge:
|
||||
|
||||
```sh
|
||||
CODE_VERIFIER=$(openssl rand -base64 96 | tr -d "=+/" | cut -c1-128)
|
||||
CODE_CHALLENGE=$(echo -n $CODE_VERIFIER | openssl dgst -sha256 -binary | base64 | tr -d "=+/" | cut -c1-43)
|
||||
CODE_VERIFIER=$(openssl rand -base64 96 | tr -d '\n' | tr '+/' '-_' | tr -d '=')
|
||||
CODE_CHALLENGE=$(echo -n $CODE_VERIFIER | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_')
|
||||
```
|
||||
|
||||
2. Include PKCE parameters in the authorization request:
|
||||
@@ -326,12 +333,25 @@ application's callback URL to a valid scheme (see
|
||||
|
||||
Verify that the `code_verifier` used in the token request matches the one used to generate the `code_challenge`.
|
||||
|
||||
### "public clients may not use the mailto/tel/sms scheme"
|
||||
|
||||
This error appears during client registration when a public client
|
||||
(`token_endpoint_auth_method: none`) registers a redirect URI using the
|
||||
`mailto:`, `tel:`, or `sms:` scheme. These schemes hand off to a mail
|
||||
client, dialer, or SMS app instead of returning control to the
|
||||
application that started the flow, so a public client registered with
|
||||
one of them could never complete authorization. Register a redirect URI
|
||||
the client can actually receive control on instead, such as a custom
|
||||
scheme (`myapp://callback`) or a loopback HTTP address.
|
||||
|
||||
## Callback URL schemes
|
||||
|
||||
Custom URI schemes (`myapp://`, `vscode://`, `jetbrains://`, etc.) are fully supported for native and desktop applications. The OS routes the redirect back to the registered application without requiring a running HTTP server.
|
||||
|
||||
The following schemes are blocked for security reasons: `javascript:`, `data:`, `file:`, `ftp:`.
|
||||
|
||||
Public clients (`token_endpoint_auth_method: none`) additionally cannot register `mailto:`, `tel:`, or `sms:` redirect URIs, since those schemes hand off to another app rather than returning an authorization code to the client. Confidential clients are not subject to this restriction.
|
||||
|
||||
## Security Considerations
|
||||
|
||||
- **Use HTTPS**: Always use HTTPS in production to protect tokens in transit
|
||||
@@ -339,7 +359,8 @@ The following schemes are blocked for security reasons: `javascript:`, `data:`,
|
||||
(public and confidential)
|
||||
- **Validate redirect URLs**: Only register trusted redirect URIs. Dangerous
|
||||
schemes (`javascript:`, `data:`, `file:`, `ftp:`) are blocked by the server,
|
||||
but custom URI schemes for native apps (`myapp://`) are permitted
|
||||
custom URI schemes for native apps (`myapp://`) are permitted, and public
|
||||
clients additionally cannot use `mailto:`, `tel:`, or `sms:`
|
||||
- **Rotate secrets**: Periodically rotate client secrets using the management API
|
||||
|
||||
## Limitations
|
||||
|
||||
Reference in New Issue
Block a user