fix(oauth2): validate PKCE verifier and challenge syntax

[euskadi31]

Closes #79.

Problem

pkce.Verify checked the transformation and nothing else: it hashed whatever
code_verifier it was given, although RFC 7636 §4.1 defines the verifier as
43*128unreserved. Reproduced end to end through the real handlers under the
default Profile20BCP: /authorize with
code_challenge=LXEWQrcmsEQBYnyp-6wy9chTD7GQPMTbAiWHF5IaSIE (S256 of x),
then /token with code_verifier=x → 200 with tokens. It does not bypass
a properly generated challenge — a SHA-256 preimage is still needed — but it
let non-compliant, low-entropy clients through, and pkce.Verify /
VerifyS256 gave library users the same lax answer. Under Profile20, a
short plain pair (verifier = challenge, 19 characters) was exchanged too.

/authorize (validateAuthorizePKCE) checked that a code_challenge was
present and its method allowed by the profile, never the challenge's shape.
RFC 7636 §4.2 defines code_challenge = 43*128unreserved, and for S256
BASE64URL-ENCODE(SHA256(ASCII(code_verifier))) — exactly 43 unpadded
base64url characters (Appendix A). abc, a padded or standard-alphabet value,
CR/LF, or values up to the request-size limits (≈1 MB in a GET, 10 MB in a
POST form) were handed to the ConsentFunc and stored verbatim. An S256 code
bound to such a challenge can never be redeemed: the client only found out at
/token, with PKCE verification failed.

Two traps make "just decode it" the wrong S256 check:

• Go's encoding/base64 skips \r and \n, even in Strict() mode. A
  43-byte challenge with an LF in place of one character decodes without error
  (to 31 bytes); a trailing LF (44 bytes) decodes to 32 bytes even with
  Strict().
• Lax decoding ignores non-zero trailing bits. The 43rd character carries
  4 bits of the digest and 2 zero bits, so only 16 final characters
  (AEIMQUYcgkosw048) are canonical:
  E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cN decodes to the same digest as
  the RFC 7636 Appendix B challenge ending in -cM.

Finally, three PKCE error_descriptions broke the RFC 6749 §4.1.2.1 / §5.2
charset (%x20-21 / %x23-5B / %x5D-7E): code_challenge_method "plain" is refused… and PKCE method "plain" is refused… carry a ", and
unsupported code_challenge_method %q echoed the attacker-controlled method,
quotes and backslashes included.

Fix

pkce owns the syntax. Two new functions, returning bool like the rest
of the package:

func ValidVerifier(verifier string) bool                 // RFC 7636 §4.1
func ValidChallenge(method Method, challenge string) bool // RFC 7636 §4.2

• ValidVerifier: 43 to 128 bytes, every byte unreserved
  (A-Z a-z 0-9 - . _ ~). Bytes, not runes, are inspected, so whitespace,
  control characters and every non-ASCII byte are refused.

• ValidChallenge: a plain challenge is a verifier (§4.2), so
  ValidChallenge(plain, c) == ValidVerifier(c). An S256 challenge must be
  exactly what BASE64URL-ENCODE yields for a SHA-256 digest: its length must
  be RawURLEncoding.EncodedLen(sha256.Size) (43, which also bounds the
  work), then it must decode and re-encode to itself:

  digest, err := base64.RawURLEncoding.DecodeString(challenge)
  
  return err == nil && base64.RawURLEncoding.EncodeToString(digest) == challenge

  The round trip refuses padding, the standard alphabet, . and ~,
  non-ASCII, CR/LF and non-canonical trailing bits with a single rule. Any
  other method — including "" (callers resolve the §4.3 default first) and
  s256 (methods are case-sensitive) — is never valid.

• Verify refuses a malformed verifier before the constant-time comparison,
  and VerifyS256 delegates to it, so a malformed stored challenge never
  matches either. Challenge is unchanged; its godoc now says it does not
  validate.

/authorize — validateAuthorizePKCE keeps its policy checks first and
adds the syntax last, so a request breaking several rules keeps its current
error:

1. presence (code_challenge is required under BCP / 2.1);
2. RFC 7636 §4.3 default: an absent method means plain;
3. method against the profile (plain refused under BCP / 2.1, unknown
   methods refused);
4. new: pkce.ValidChallenge → malformed S256 code_challenge /
   malformed plain code_challenge.

Errors still travel as a redirected invalid_request with state (RFC 6749
§4.1.2.1; RFC 7636 §4.4.1 uses invalid_request too), before the
ConsentFunc runs; nothing is stored. AuthorizeRequest.CodeChallengeMethod
still carries the method as sent.

/token — verifyPKCE keeps its order and inserts one check: PKCE
required → missing verifier → default method → plain ban → new:
pkce.ValidVerifier → invalid_grant malformed code_verifier →
pkce.Verify. The code was consumed atomically before PKCE runs, so a
malformed verifier burns it exactly like a wrong one (fail closed). The
change is confined to verifyPKCE.

Descriptions — ASCII, inside the RFC 6749 charset, never echoing input:

Where	Before	After
/authorize, plain refused	code_challenge_method "plain" is refused by the active profile	code_challenge_method plain is refused by the active profile
/authorize, unknown method	unsupported code_challenge_method "<method as sent>"	unsupported code_challenge_method
/authorize, malformed challenge	accepted, code minted	malformed S256 code_challenge / malformed plain code_challenge
/token, plain refused	PKCE method "plain" is refused by the active profile	PKCE method plain is refused by the active profile
/token, malformed verifier	exchanged when the transformation matched	malformed code_verifier

Constant time: well-formed verifiers are compared exactly as before. The
syntax pre-check exits early, but its timing depends only on the client's own
input and the public RFC alphabet, never on the stored challenge; and codes
are single-use, so repeated probes against one challenge are impossible
anyway. No storage change.

Docs: godoc (pkce package, ValidVerifier, ValidChallenge, Verify,
VerifyS256, Challenge, the AuthorizeRequest PKCE fields,
validateAuthorizePKCE, verifyPKCE); oauth2/profile.go (the
Profile21Draft bullet claimed 2.1 alone refuses plain — BCP already
does); CHANGELOG.md (Security); docs/security-considerations.md (the
PKCE bullet said plain was "accepted (with a warning)" under the looser
profiles: it is refused under BCP, and there is no warning).

Decision: invalid_grant for a malformed code_verifier

RFC 7636 does not name an error for a malformed verifier. This PR answers
invalid_grant:

• RFC 7636 §4.6: when the transformed verifier does not equal the stored
  challenge, the token endpoint MUST answer invalid_grant. A malformed
  verifier fails that comparison against every challenge it did not produce,
  so answering invalid_request to every malformed verifier would break that
  MUST in the common case. The only malformed verifiers that "match" are those
  whose own transformation was registered — the bug fixed here.
• The code is already consumed (atomically, before PKCE runs):
  invalid_grant tells the client to restart the flow, where
  invalid_request would invite a retry that can only fail.
• Precedents: Ory fosite (handler/pkce/handler.go) and Keycloak
  (PkceUtils) answer invalid_grant to verifier length or charset errors.
  Counter-precedent: node-oidc-provider answers invalid_request
  (lib/helpers/pkce_format.js) and checks the syntax before consuming the
  code. Checking before consumption here would touch the consumption path
  oauth2: retain authorization-code replay metadata to revoke previously issued tokens #80 reworks and weaken fail-closed single use.
• Consistency (secondary): every PKCE failure at /token in this module was
  already invalid_grant, missing code_verifier included.

The counter-argument — RFC 6749 §5.2 maps an "otherwise malformed" request to
invalid_request — is acknowledged; §4.6 is the more specific rule for a
failed PKCE proof.

Behavior change

Nothing is released yet, and only input RFC 7636 already forbids is affected:

• a code_verifier shorter than 43 or longer than 128 characters, or with a
  character outside A-Z a-z 0-9 - . _ ~ (standard-base64 + / =,
  whitespace, a %-escape left encoded, non-ASCII), used to be exchanged when
  its transformation matched; /token now answers invalid_grant;
• /authorize refuses such plain challenges, and any S256 challenge that is
  not a canonical 43-character unpadded base64url SHA-256 digest;
• pkce.Verify / VerifyS256 return false for a malformed verifier
  (signatures unchanged); pkce.Challenge is unchanged;
• the descriptions in the table above (clients should match on error, not
  on error_description);
• codes in flight at deploy time that rely on malformed values fail with
  invalid_grant until they expire (CodeTTL, 10 minutes by default); no
  storage migration.

Tests

Three new files — oauth2/pkce/syntax_test.go,
oauth2/grant/authorization_code_pkce_test.go, and
oauth2/authorize_pkce_test.go (package oauth2_test: the real
AuthorizeHandler and TokenHandler with the memory store):

• TestValidVerifier (19 rows) — the RFC 7636 Appendix B verifier (43, the
  minimum), 128, the whole alphabet; empty, x, 42, 129, 43 bytes / 42 runes,
  43 runes, a 3-byte rune, leading space, trailing LF / CRLF, tab, embedded
  space, +, /, =, %7E.
• TestValidVerifierRejectsEveryNonUnreservedByte — all 256 byte values in a
  43-byte verifier: exactly the 66 unreserved characters pass.
• TestValidChallenge (29 rows) — S256: the RFC challenge and S256("x")
  (well-formed: /authorize cannot catch the issue's verifier); 42, 44 and
  64 characters, padded, +, /, LF in place of a character, trailing
  CRLF / LF, ~, ., a non-canonical final character, non-ASCII, empty.
  Plain: the verifier rows. Methods: "", S512, s256.
• TestValidChallengeS256RequiresCanonicalTrailingBits — the 64 possible
  final characters: exactly AEIMQUYcgkosw048 pass.
• TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches (7 rows) —
  Verify(S256), VerifyS256 and Verify(plain) against the malformed
  verifier's own challenge.
• TestVerifyAcceptsEveryLegalVerifierLength (guard) — lengths 43 to 128
  round-trip through both methods.
• TestAuthorizationCodeRejectsMalformedVerifier (10 rows, BCP) — each code
  is bound to the malformed verifier's own S256 challenge, so only the syntax
  check can refuse it: invalid_grant, malformed code_verifier.
• TestAuthorizationCodePlainRejectsMalformedVerifier — a matching
  19-character plain pair under Profile20.
• TestAuthorizationCodeMalformedStoredChallengeNeverMatches (pin) — codes
  stored before the fix (padded or short S256, short plain) fail with
  PKCE verification failed.
• TestAuthorizationCodeAcceptsWellFormedVerifiers and
  TestAuthorizationCodeProfile20WithoutPKCE (guards).
• TestAuthorizeRejectsMalformedS256Challenge (13 rows) and
  TestAuthorizeRejectsMalformedPlainChallenge (12 rows: explicit and absent
  method) — 302 to the registered URI, invalid_request, the exact
  description, state, no code; the S256 rows also check the ConsentFunc
  is never called.
• TestAuthorizePKCEChecksKeepTheirPrecedence (4 rows) — a missing
  challenge, plain or an absent method with a malformed challenge, and a
  hostile method S512"\: exact descriptions, nothing echoed.
• TestAuthorizeAcceptsWellFormedChallenges (guard) — the ConsentFunc
  receives the challenge and the method as sent.
• TestAuthorizeCodeFlowRejectsOneCharacterVerifier — the issue
  reproduction: S256("x") at /authorize, x at /token.
• TestAuthorizeCodeFlowMalformedVerifierBurnsTheCode — the RFC challenge,
  then /token with pkceVerifier+" " → malformed code_verifier; the same
  code with the right verifier → invalid_grant (it would be 200 had the code
  survived).
• TestAuthorizeCodeFlowEnforcesVerifierSyntax — the form-encoding cases: a
  trailing space (sent as +) and a literal + (sent as %2B).

Every error_description these tests read is also checked against the
RFC 6749 charset (^[\x20\x21\x23-\x5B\x5D-\x7E]*$).

Fixtures that relied on non-compliant PKCE values now use well-formed ones:
pkce_test.go; TestAuthorizationCodePKCEMismatch, which now also pins
PKCE verification failed; plainPKCECode(t, store, verifier), where the
Profile20 test uses a 52-character well-formed pair and the BCP test keeps
the short pair to pin that the profile ban answers before the syntax check,
with its exact description; and the plain row of
TestAuthorizeProfile20AllowsNoPKCEAndPlain. A second commit pins
missing code_verifier in TestAuthorizationCodeMissingVerifier, so the
presence check keeps answering before the syntax check.

Red phase:

• pkce first failed to compile (undefined: pkce.ValidVerifier,
  pkce.ValidChallenge). With the two functions stubbed to the pre-fix
  behavior (accept everything): all 7 rows of
  TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches failed —
  Verify and VerifyS256 returned true for x; every negative row of
  TestValidVerifier (16) and TestValidChallenge (19) failed; the byte
  sweep accepted all 256 bytes, the trailing-bits sweep all 64 characters.
• grant, against the unmodified code: all 10 rows of
  TestAuthorizationCodeRejectsMalformedVerifier issued tokens, and so
  did the short plain pair; the BCP plain test failed only on its description
  (PKCE method "plain" …).
• oauth2, against the unmodified code: the issue reproduction got 200
  with tokens, and so did both form-encoding rows; the burn test failed on
  PKCE verification failed; all 25 malformed-challenge rows got a 302
  carrying a code instead of an error; 3 of the 4 precedence rows failed on
  the " and on the echoed S512"\.
• The guards passed throughout.

Acceptance criteria:

Criterion	Tests
Valid RFC vectors and boundary-length verifiers succeed	TestValidVerifier (RFC vector = 43, 128, alphabet), TestVerifyAcceptsEveryLegalVerifierLength, TestValidChallenge (RFC challenge), TestAuthorizationCodeAcceptsWellFormedVerifiers, TestAuthorizeAcceptsWellFormedChallenges, the existing TestAuthorizeCodeFlowEndToEnd
Short, overlong, Unicode, whitespace and forbidden-character verifiers are rejected	TestValidVerifier, TestValidVerifierRejectsEveryNonUnreservedByte, TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches, TestAuthorizationCodeRejectsMalformedVerifier, TestAuthorizationCodePlainRejectsMalformedVerifier, TestAuthorizeCodeFlowEnforcesVerifierSyntax
Malformed S256 and plain challenges are rejected at /authorize	TestValidChallenge, TestValidChallengeS256RequiresCanonicalTrailingBits, TestAuthorizeRejectsMalformedS256Challenge, TestAuthorizeRejectsMalformedPlainChallenge
Missing and unsupported methods retain correct OAuth errors and profile behavior	TestAuthorizePKCEChecksKeepTheirPrecedence, TestValidChallenge (method rows), TestAuthorizationCodePlainPKCERefusedUnderBCP (exact description), TestAuthorizationCodeProfile20WithoutPKCE; the existing TestAuthorizeRedirectsProtocolErrors, TestAuthorizeRejectsUnknownPKCEMethod, TestAuthorizeProfile20AllowsNoPKCEAndPlain, TestAuthorizationCodeMissingVerifier, TestAuthorizationCodeProfileRequiresPKCE, TestAuthorizationCodeRequiresPKCEWhenConfigured, TestAuthorizationCodePlainPKCEAcceptedUnderProfile20 and the metadata tests
Helper-level and end-to-end authorization-code tests, including the one-character case	the x rows of TestValidVerifier, TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches and TestAuthorizationCodeRejectsMalformedVerifier; end to end, TestAuthorizeCodeFlowRejectsOneCharacterVerifier (the issue reproduction) and TestAuthorizeCodeFlowMalformedVerifierBurnsTheCode

An independent adversarial review fuzzed the validators with about 14 million
inputs — zero divergence from the RFC 7636 ABNF — and mutation-tested the
change before approving it. Mutation testing on a scratch copy of this branch
(18 mutants, all killed):

Mutant	Caught by
verifier minimum 43 → 42; maximum 128 → 129	TestValidVerifier, TestValidChallenge, TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches, the grant malformed-verifier test, /authorize plain
~ dropped from the unreserved set	TestValidVerifier, the byte sweep, TestValidChallenge, the every-length guard, the grant and /authorize well-formed guards, the Profile20 plain exchange
+ added to the unreserved set	TestValidVerifier, the byte sweep, TestValidChallenge, TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches, the grant malformed-verifier test, /authorize plain, the form-encoding flow
S256: decode only, no canonical re-encoding	TestValidChallenge, the trailing-bits sweep, /authorize S256
S256: length check removed	TestValidChallenge, /authorize S256
plain challenge without the syntax check	TestValidChallenge, /authorize plain
unknown method valid in ValidChallenge	TestValidChallenge
Verify without its syntax guard	TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches
/token syntax check removed	the grant malformed-verifier tests (S256 and plain), the issue reproduction, the burn test, the form-encoding flow
/token syntax check before the plain ban; plain-ban description quoted again	TestAuthorizationCodePlainPKCERefusedUnderBCP
/authorize syntax check removed	/authorize S256 and plain
/authorize syntax check before the method / profile checks; method echoed again; plain-refused description quoted again	TestAuthorizePKCEChecksKeepTheirPrecedence
/authorize §4.3 default removed	the precedence test, /authorize plain, the well-formed guard
/token missing-verifier check moved after the syntax check (an absent verifier would read malformed code_verifier, same invalid_grant)	TestAuthorizationCodeMissingVerifier, which now pins missing code_verifier (second commit)

Checks

• make build — OK.
• make test — every package passes with -race; oauth2 coverage
  93.0% → 93.1%, oauth2/grant 97.5% → 98.0%, oauth2/pkce stays at 100%.
  New and changed code is 100% covered (every function of pkce.go,
  validateAuthorizePKCE, verifyPKCE).
• make lint — 0 issues; go vet ./... on oauth2 (tests included) — OK.

Follow-ups (out of scope)

• Non-PKCE error_descriptions still echo client input, so they can carry
  characters outside the RFC 6749 §5.2 charset: token_endpoint.go
  (grant_type … not supported), grant/client_credentials.go
  (scope … not allowed for client), authorize_endpoint.go
  (response_type … is not supported, and scope %q is not allowed for this client from authorizeScope).
• Downgrade gap (RFC 9700 §4.8.2): under Profile20 without RequirePKCE, a
  code_verifier sent for a code that has no challenge is ignored; the server
  should refuse it (fosite and Keycloak answer invalid_grant). Profiles that
  mandate PKCE are not affected.
• Repeated parameters are not detected (RFC 6749 §3.1: request parameters
  MUST NOT be included more than once); FormValue takes the first value.

Notes for the maintainer

• CLAUDE.md is not edited here. Proposed addition to the "OAuth2 server"
  section, after the sub-packages list:

  pkce/ enforces the RFC 7636 syntax, not only the transformation:
  ValidChallenge at /authorize (invalid_request), ValidVerifier at
  /token (invalid_grant, after the profile checks, the code already
  consumed); Verify refuses a malformed verifier.

• MIGRATION.md is not updated: the change adds two functions and only
  refuses input RFC 7636 already forbids. An entry under "Breaking changes
  before the first v2 release" can be added if you want one.

• This also resolves the PKCE method "plain" charset breach listed as a
  follow-up in fix(oauth2): authorize token introspection callers #90, and the stale PKCE bullet noted in fix(oauth2): enforce the refresh-token rotation the profile requires #89.

[coveralls]

Coverage Report for CI Build 38015400055

Coverage increased (+0.1%) to 94.444%

Details

• Coverage increased (+0.1%) from the base build.
• Patch coverage: 46 of 46 lines across 3 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	4950
Covered Lines:	4675
Line Coverage:	94.44%
Coverage Strength:	75.03 hits per line

---

💛 - Coveralls

[coveralls]

Coverage Report for CI Build 38015644582

Coverage increased (+0.1%) to 94.444%

Details

• Coverage increased (+0.1%) from the base build.
• Patch coverage: 46 of 46 lines across 3 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	4950
Covered Lines:	4675
Line Coverage:	94.44%
Coverage Strength:	75.16 hits per line

---

💛 - Coveralls