mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
Bobby Ho
209d1ca498
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>
60 lines
2.1 KiB
Go
60 lines
2.1 KiB
Go
package oauth2provider
|
|
|
|
import (
|
|
"crypto/sha256"
|
|
"crypto/subtle"
|
|
"encoding/base64"
|
|
)
|
|
|
|
// PKCE code verifier bounds from RFC 7636 §4.1.
|
|
const (
|
|
pkceVerifierMinLength = 43
|
|
pkceVerifierMaxLength = 128
|
|
)
|
|
|
|
// ValidPKCEFormat reports whether s meets RFC 7636 §4.1: 43 to 128 characters
|
|
// of the unreserved set [A-Za-z0-9-._~]. RFC 7636 gives code_verifier and
|
|
// code_challenge the same ABNF, so this check applies to both: a code_verifier
|
|
// directly, and a code_challenge because the S256 method that produces it
|
|
// (base64url(SHA256(verifier))) always yields a string within these bounds.
|
|
//
|
|
// The length floor matters because the challenge and code both travel
|
|
// through the authorization URL and redirect, landing in browser history,
|
|
// referrer headers, and proxy logs. An attacker who recovers either one
|
|
// brute-forces the verifier offline at whatever entropy the client chose,
|
|
// with no server-side rate limit to slow them down. A client secret also
|
|
// authenticates the token request today, but public clients (#27873) will
|
|
// rely on this bound alone, so it must hold on its own merit. The same
|
|
// bound on code_challenge keeps a malformed value from being persisted
|
|
// verbatim and failing late, at token exchange, instead of at the
|
|
// authorization request where RFC 7636 §4.4.1 expects it to be rejected.
|
|
func ValidPKCEFormat(s string) bool {
|
|
if len(s) < pkceVerifierMinLength || len(s) > pkceVerifierMaxLength {
|
|
return false
|
|
}
|
|
for _, r := range s {
|
|
switch {
|
|
case r >= 'A' && r <= 'Z',
|
|
r >= 'a' && r <= 'z',
|
|
r >= '0' && r <= '9',
|
|
r == '-', r == '.', r == '_', r == '~':
|
|
default:
|
|
return false
|
|
}
|
|
}
|
|
return true
|
|
}
|
|
|
|
// VerifyPKCE verifies that the code_verifier matches the code_challenge
|
|
// using the S256 method as specified in RFC 7636.
|
|
func VerifyPKCE(challenge, verifier string) bool {
|
|
if challenge == "" || verifier == "" {
|
|
return false
|
|
}
|
|
|
|
// S256: BASE64URL-ENCODE(SHA256(ASCII(code_verifier))) == code_challenge
|
|
h := sha256.Sum256([]byte(verifier))
|
|
computed := base64.RawURLEncoding.EncodeToString(h[:])
|
|
return subtle.ConstantTimeCompare([]byte(challenge), []byte(computed)) == 1
|
|
}
|