fix(oauth2): validate PKCE verifier and challenge syntax

CHANGELOG.md

@@ -262,3 +262,17 @@ legacy packages (`authentication/`, `authorization/`, the in-tree
   and `/revoke` are unchanged. The `none` method's `error_description` for
   a non-public client no longer contains a double quote, which RFC 6749
   §5.2 forbids. See [MIGRATION.md](MIGRATION.md).
+- The OAuth2 server now enforces the RFC 7636 PKCE syntax, not just the
+  transformation. `/authorize` answers `invalid_request` to a
+  `code_challenge` that is not 43–128 unreserved characters (`plain`) or
+  not the unpadded base64url SHA-256 digest (`S256`); for a code bound to a
+  `code_challenge`, `/token` answers `invalid_grant` to a `code_verifier`
+  that is not 43–128 characters of `A-Z a-z 0-9 - . _ ~`, and the code
+  stays consumed — a one-character verifier whose S256 challenge matched
+  used to be exchanged for tokens.
+  `pkce.Verify`/`VerifyS256` return false for a malformed verifier, and
+  `pkce.ValidVerifier`/`pkce.ValidChallenge` expose the checks. Missing or
+  unsupported methods and the profile ban on `plain` keep their error
+  codes; their `error_description` no longer contains a double quote,
+  which RFC 6749 §4.1.2.1 and §5.2 forbid, and no longer echoes an
+  unsupported `code_challenge_method`.

docs/security-considerations.md

@@ -46,9 +46,17 @@ configurable clock skew, and resolves keys by `kid` against a JWKS provider
 
 ## OAuth2
 
-- **PKCE** — `authorization_code` requires PKCE. `S256` is the only method
-  allowed under `Profile21Draft`; `plain` is accepted (with a warning) only
-  under the looser profiles.
+- **PKCE** — `authorization_code` requires PKCE under `Profile20BCP` and
+  `Profile21Draft`; under `Profile20` it is optional unless
+  `grant.Config.RequirePKCE` is set. `S256` is the only method accepted
+  under BCP / 2.1; `plain` is tolerated only under `Profile20`. Both ends
+  enforce the RFC 7636 syntax: `/authorize` answers `invalid_request` to a
+  `code_challenge` that is not 43–128 unreserved characters (`plain`) or
+  not the unpadded base64url SHA-256 digest (`S256`), and, for a code
+  bound to a `code_challenge`, `/token` answers `invalid_grant` to a
+  `code_verifier` that is not 43–128 unreserved characters. The code is
+  consumed whether the verifier is malformed or wrong. Well-formed
+  verifiers are compared in constant time.
 - **Refresh-token rotation** — mandatory under `Profile20BCP` (the default)
   and `Profile21Draft`, whatever `grant.Config.RotateRefreshTokens` says:
   every refresh exchange consumes the presented token and issues a new one

oauth2/authorize_endpoint.go

@@ -71,7 +71,9 @@ type AuthorizeRequest struct {
 	State string
 	// CodeChallenge / CodeChallengeMethod carry the PKCE parameters
 	// (RFC 7636). Empty when the request carries no PKCE; unused by the
-	// implicit flow.
+	// implicit flow. For the code flow they are validated before the
+	// ConsentFunc runs: the method is allowed by the profile and the
+	// challenge is well-formed for it (RFC 7636 §4.2).
 	CodeChallenge       string
 	CodeChallengeMethod string
 	// Nonce echoes the OIDC nonce parameter, when present.
@@ -304,8 +306,10 @@ func (s *Server) parseAuthorizeRequest(r *http.Request, client Client, redirectU
 	}, nil
 }
 
-// validateAuthorizePKCE enforces the profile's PKCE policy on the
-// /authorize parameters.
+// validateAuthorizePKCE enforces the profile's PKCE policy and the RFC 7636
+// §4.2 challenge syntax on the /authorize parameters. Presence, method and
+// profile are checked first, so they keep their errors when a request
+// breaks several rules. The descriptions never echo the input.
 func (s *Server) validateAuthorizePKCE(challenge, method string) error {
 	if challenge == "" {
 		if s.cfg.Profile.RequiresPKCE() {
@@ -315,15 +319,25 @@ func (s *Server) validateAuthorizePKCE(challenge, method string) error {
 		return nil
 	}
 
-	switch pkce.Method(method) {
-	case "", pkce.MethodPlain:
+	// RFC 7636 §4.3: an absent code_challenge_method means "plain".
+	m := pkce.Method(method)
+	if m == "" {
+		m = pkce.MethodPlain
+	}
+
+	switch m {
+	case pkce.MethodPlain:
 		if !s.cfg.Profile.AllowsPKCEPlain() {
-			return errors.New(`code_challenge_method "plain" is refused by the active profile`)
+			return errors.New("code_challenge_method plain is refused by the active profile")
 		}
 	case pkce.MethodS256:
 		// S256 is always acceptable.
 	default:
-		return fmt.Errorf("unsupported code_challenge_method %q", method)
+		return errors.New("unsupported code_challenge_method")
+	}
+
+	if !pkce.ValidChallenge(m, challenge) {
+		return fmt.Errorf("malformed %s code_challenge", m)
 	}
 
 	return nil

oauth2/authorize_endpoint_test.go

@@ -278,7 +278,7 @@ func TestAuthorizeProfile20AllowsNoPKCEAndPlain(t *testing.T) {
 		t.Parallel()
 
 		q := authorizeQuery()
-		q.Set("code_challenge", "a-plain-verifier")
+		q.Set("code_challenge", pkceVerifier) // a plain challenge is the verifier itself
 		q.Set("code_challenge_method", "plain")
 
 		rec := runAuthorize(srv, q, approve)