From 6820efd3e21c45f72e0aefdb95dba00420e1c50f Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Sat, 10 Oct 2026 01:48:42 +0200 Subject: [PATCH 1/2] fix(oauth2): validate PKCE verifier and challenge syntax MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PKCE checked the transformation but never the RFC 7636 syntax: pkce.Verify hashed whatever it was given, and /authorize checked the presence and method of a code_challenge but not its shape. A client could pair a one-character code_verifier with its S256 challenge and have the code exchanged for tokens under every profile, and /authorize stored challenges that no S256 transformation can produce. The pkce package now owns the syntax. ValidVerifier encodes RFC 7636 §4.1: 43 to 128 bytes, each one unreserved, so whitespace and every non-ASCII byte are refused. ValidChallenge encodes §4.2: a plain challenge is a verifier, and an S256 challenge must survive a round trip through the unpadded base64url encoding of a SHA-256 digest. Decoding alone is not enough: encoding/base64 skips CR and LF even in strict mode, and lax decoding ignores non-zero trailing bits. Verify and VerifyS256 refuse a malformed verifier before the constant-time comparison. /authorize still checks presence first, then the method against the profile (an absent method means plain, §4.3), and only then the syntax, so a request breaking several rules keeps its current error; a malformed challenge is redirected as invalid_request before the ConsentFunc runs. /token answers invalid_grant "malformed code_verifier" after the profile checks: §4.6 maps a failed proof to invalid_grant, and the code, consumed before PKCE runs, stays burned. The PKCE error descriptions no longer carry the double quote RFC 6749 §4.1.2.1 and §5.2 forbid, nor echo an unsupported method. Tests that relied on non-compliant PKCE values now use well-formed ones, and the security considerations and Profile docs no longer misstate which profiles accept plain. Refs #79. --- CHANGELOG.md | 14 + docs/security-considerations.md | 14 +- oauth2/authorize_endpoint.go | 28 +- oauth2/authorize_endpoint_test.go | 2 +- oauth2/authorize_pkce_test.go | 352 +++++++++++++++++++ oauth2/grant/authorization_code.go | 9 +- oauth2/grant/authorization_code_pkce_test.go | 204 +++++++++++ oauth2/grant/grant_more_test.go | 23 +- oauth2/grant/grant_test.go | 3 +- oauth2/pkce/pkce.go | 88 ++++- oauth2/pkce/pkce_test.go | 14 +- oauth2/pkce/syntax_test.go | 224 ++++++++++++ oauth2/profile.go | 4 +- 13 files changed, 948 insertions(+), 31 deletions(-) create mode 100644 oauth2/authorize_pkce_test.go create mode 100644 oauth2/grant/authorization_code_pkce_test.go create mode 100644 oauth2/pkce/syntax_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d5750770..8dad9d01 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/docs/security-considerations.md b/docs/security-considerations.md index e0b0f810..a20c5c6e 100644 --- a/docs/security-considerations.md +++ b/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 diff --git a/oauth2/authorize_endpoint.go b/oauth2/authorize_endpoint.go index 0f557062..f8e4afe1 100644 --- a/oauth2/authorize_endpoint.go +++ b/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 diff --git a/oauth2/authorize_endpoint_test.go b/oauth2/authorize_endpoint_test.go index 574e0d95..d154f2f6 100644 --- a/oauth2/authorize_endpoint_test.go +++ b/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) diff --git a/oauth2/authorize_pkce_test.go b/oauth2/authorize_pkce_test.go new file mode 100644 index 00000000..04ea51bb --- /dev/null +++ b/oauth2/authorize_pkce_test.go @@ -0,0 +1,352 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oauth2_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/pkce" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// unreservedAlphabet is the RFC 3986 §2.3 unreserved set PKCE values are +// drawn from (RFC 7636 §4.1). +const unreservedAlphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~" + +// errorDescriptionCharset is the error_description syntax of RFC 6749 +// §4.1.2.1 and §5.2: %x20-21 / %x23-5B / %x5D-7E, so no '"' and no '\'. +var errorDescriptionCharset = regexp.MustCompile(`^[\x20\x21\x23-\x5B\x5D-\x7E]*$`) + +// s256 derives the S256 code_challenge of verifier. +func s256(t *testing.T, verifier string) string { + t.Helper() + + c, ok := pkce.Challenge(pkce.MethodS256, verifier) + require.True(t, ok) + + return c +} + +// authorizePKCE runs /authorize (authorizeQuery defaults) with the given +// PKCE parameters — an empty challenge or method omits the parameter — and +// returns the redirect target. +func authorizePKCE(t *testing.T, srv *oauth2.Server, challenge, method string, consent oauth2.ConsentFunc) *url.URL { + t.Helper() + + q := authorizeQuery() + q.Del("code_challenge") + q.Del("code_challenge_method") + + if challenge != "" { + q.Set("code_challenge", challenge) + } + + if method != "" { + q.Set("code_challenge_method", method) + } + + rec := runAuthorize(srv, q, consent) + require.Equal(t, http.StatusFound, rec.Code) + + return mustLocation(t, rec) +} + +// authorizeError checks that loc is an RFC 6749 §4.1.2.1 error redirect to +// the registered redirect URI and returns its error code and description. +func authorizeError(t *testing.T, loc *url.URL) (code, desc string) { + t.Helper() + + q := loc.Query() + + assert.Equal(t, redirectURI, loc.Scheme+"://"+loc.Host+loc.Path) + assert.Equal(t, "xyz-state", q.Get("state")) + assert.False(t, q.Has("code"), "an error redirect carries no code") + + desc = q.Get("error_description") + assert.Regexp(t, errorDescriptionCharset, desc) + + return q.Get("error"), desc +} + +// exchangeCode redeems code at /token (client_secret_basic) with verifier. +func exchangeCode(srv *oauth2.Server, code, verifier string) *httptest.ResponseRecorder { + form := url.Values{ + "grant_type": {"authorization_code"}, + "code": {code}, + "redirect_uri": {redirectURI}, + "code_verifier": {verifier}, + } + + rec := httptest.NewRecorder() + srv.TokenHandler().ServeHTTP(rec, formRequest("/oauth2/token", form, true)) + + return rec +} + +// tokenError decodes an RFC 6749 §5.2 error body into its error code and +// description. +func tokenError(t *testing.T, rec *httptest.ResponseRecorder) (code, desc string) { + t.Helper() + + var body struct { + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + } + + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + assert.Regexp(t, errorDescriptionCharset, body.ErrorDescription) + + return body.Error, body.ErrorDescription +} + +// TestAuthorizeRejectsMalformedS256Challenge covers challenges no S256 +// transformation can produce (RFC 7636 §4.2, Appendix A): a code bound to +// one could never be redeemed, so /authorize refuses it before consent. +func TestAuthorizeRejectsMalformedS256Challenge(t *testing.T) { + t.Parallel() + + srv := newAuthorizeServer(t, oauth2.Profile20BCP) + c := pkceChallenge + + cases := []struct { + name string + challenge string + }{ + {"42 chars", c[:42]}, + {"44 chars", c + "A"}, + {"padded", c + "="}, + {"standard alphabet plus", strings.Replace(c, "-", "+", 1)}, + {"standard alphabet slash", c[:10] + "/" + c[11:]}, + {"LF in place of a character", c[:20] + "\n" + c[21:]}, + {"trailing CRLF in place of a character", c[:41] + "\r\n"}, + {"trailing LF", c + "\n"}, + {"tilde", c[:10] + "~" + c[11:]}, + {"dot", c[:10] + "." + c[11:]}, + {"non-canonical trailing bits", c[:42] + "N"}, + {"non-ASCII", c[:41] + "é"}, + {"64 chars", strings.Repeat("a", 64)}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + consented := false + loc := authorizePKCE(t, srv, tc.challenge, "S256", + func(w http.ResponseWriter, r *http.Request, ar *oauth2.AuthorizeRequest) (*oauth2.Consent, error) { + consented = true + + return approve(w, r, ar) + }) + + code, desc := authorizeError(t, loc) + assert.Equal(t, oauth2.CodeInvalidRequest, code) + assert.Equal(t, "malformed S256 code_challenge", desc) + assert.False(t, consented, "the ConsentFunc MUST NOT see a malformed challenge") + }) + } +} + +// TestAuthorizeRejectsMalformedPlainChallenge covers plain challenges that +// are not a valid code verifier (RFC 7636 §4.2), with the method explicit +// and absent (RFC 7636 §4.3 defaults it to plain). +func TestAuthorizeRejectsMalformedPlainChallenge(t *testing.T) { + t.Parallel() + + srv := newAuthorizeServer(t, oauth2.Profile20) + + methods := []struct { + name string + method string + }{ + {"plain", "plain"}, + {"absent method", ""}, + } + + challenges := []struct { + name string + challenge string + }{ + {"42 chars", strings.Repeat("a", 42)}, + {"129 chars", strings.Repeat("a", 129)}, + {"plus", strings.Repeat("a", 42) + "+"}, + {"embedded space", pkceVerifier[:20] + " " + pkceVerifier[20:]}, + {"trailing LF", pkceVerifier + "\n"}, + {"non-ASCII", strings.Repeat("a", 41) + "é"}, + } + + for _, m := range methods { + for _, tc := range challenges { + t.Run(m.name+"/"+tc.name, func(t *testing.T) { + t.Parallel() + + code, desc := authorizeError(t, authorizePKCE(t, srv, tc.challenge, m.method, approve)) + assert.Equal(t, oauth2.CodeInvalidRequest, code) + assert.Equal(t, "malformed plain code_challenge", desc) + }) + } + } +} + +// TestAuthorizeAcceptsWellFormedChallenges is a regression guard: valid +// challenges still reach the ConsentFunc unchanged, the method as sent. +func TestAuthorizeAcceptsWellFormedChallenges(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + profile oauth2.Profile + challenge string + method string + }{ + {"plain, 43 chars", oauth2.Profile20, strings.Repeat("a", 43), "plain"}, + {"plain, 128 chars", oauth2.Profile20, strings.Repeat("a", 128), "plain"}, + {"plain, the whole unreserved alphabet", oauth2.Profile20, unreservedAlphabet, "plain"}, + {"absent method", oauth2.Profile20, pkceVerifier, ""}, + {"S256 under Profile20", oauth2.Profile20, pkceChallenge, "S256"}, + // /authorize cannot see the verifier: S256("x") is well-formed. + {"S256 of a one-character verifier", oauth2.Profile20BCP, s256(t, "x"), "S256"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var seen *oauth2.AuthorizeRequest + loc := authorizePKCE(t, newAuthorizeServer(t, tc.profile), tc.challenge, tc.method, + func(w http.ResponseWriter, r *http.Request, ar *oauth2.AuthorizeRequest) (*oauth2.Consent, error) { + seen = ar + + return approve(w, r, ar) + }) + + assert.NotEmpty(t, loc.Query().Get("code")) + assert.Empty(t, loc.Query().Get("error")) + require.NotNil(t, seen) + assert.Equal(t, tc.challenge, seen.CodeChallenge) + assert.Equal(t, tc.method, seen.CodeChallengeMethod) + }) + } +} + +// TestAuthorizePKCEChecksKeepTheirPrecedence: presence, method and profile +// are checked before the challenge syntax, so a request breaking several +// rules keeps the error of the first one. The descriptions are pinned +// verbatim: they hold no '"' or '\' and never echo the method. +func TestAuthorizePKCEChecksKeepTheirPrecedence(t *testing.T) { + t.Parallel() + + srv := newAuthorizeServer(t, oauth2.Profile20BCP) + + cases := []struct { + name string + challenge string + method string + want string + }{ + {"missing challenge", "", "S256", "code_challenge is required"}, + {"plain with a malformed challenge", "short", "plain", "code_challenge_method plain is refused by the active profile"}, + {"absent method with a malformed challenge", "short", "", "code_challenge_method plain is refused by the active profile"}, + {"hostile unsupported method with a malformed challenge", "short", `S512"\`, "unsupported code_challenge_method"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + code, desc := authorizeError(t, authorizePKCE(t, srv, tc.challenge, tc.method, approve)) + assert.Equal(t, oauth2.CodeInvalidRequest, code) + assert.Equal(t, tc.want, desc) + assert.NotContains(t, desc, "S512", "the method is never echoed") + }) + } +} + +// TestAuthorizeCodeFlowRejectsOneCharacterVerifier reproduces issue #79 +// through the real handlers. S256("x") is a well-formed challenge, so only +// /token can refuse the one-character verifier behind it. +func TestAuthorizeCodeFlowRejectsOneCharacterVerifier(t *testing.T) { + t.Parallel() + + srv := newAuthorizeServer(t, oauth2.Profile20BCP) + + code := authorizePKCE(t, srv, s256(t, "x"), "S256", approve).Query().Get("code") + require.NotEmpty(t, code) + + rec := exchangeCode(srv, code, "x") + require.Equal(t, http.StatusBadRequest, rec.Code) + + errCode, desc := tokenError(t, rec) + assert.Equal(t, oauth2.CodeInvalidGrant, errCode) + assert.Equal(t, "malformed code_verifier", desc) +} + +// TestAuthorizeCodeFlowMalformedVerifierBurnsTheCode: the code is consumed +// before the verifier is checked, so a malformed verifier burns it like a +// wrong one. Retrying the same code with the right verifier is refused; it +// would succeed had the code survived. +func TestAuthorizeCodeFlowMalformedVerifierBurnsTheCode(t *testing.T) { + t.Parallel() + + srv := newAuthorizeServer(t, oauth2.Profile20BCP) + + rec := runAuthorize(srv, authorizeQuery(), approve) + require.Equal(t, http.StatusFound, rec.Code) + + code := mustLocation(t, rec).Query().Get("code") + require.NotEmpty(t, code) + + malformed := exchangeCode(srv, code, pkceVerifier+" ") + require.Equal(t, http.StatusBadRequest, malformed.Code) + + errCode, desc := tokenError(t, malformed) + assert.Equal(t, oauth2.CodeInvalidGrant, errCode) + assert.Equal(t, "malformed code_verifier", desc) + + retry := exchangeCode(srv, code, pkceVerifier) + require.Equal(t, http.StatusBadRequest, retry.Code) + assert.Equal(t, oauth2.CodeInvalidGrant, decodeError(t, retry)) +} + +// TestAuthorizeCodeFlowEnforcesVerifierSyntax sends the verifiers whose form +// encoding matters — a space travels as "+", a literal "+" as "%2B" — bound +// to their own S256 challenge, so only the syntax check can refuse them. +func TestAuthorizeCodeFlowEnforcesVerifierSyntax(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + verifier string + }{ + {"trailing space", pkceVerifier + " "}, + {"standard base64 plus", strings.Repeat("a", 42) + "+"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + srv := newAuthorizeServer(t, oauth2.Profile20BCP) + + code := authorizePKCE(t, srv, s256(t, tc.verifier), "S256", approve).Query().Get("code") + require.NotEmpty(t, code) + + rec := exchangeCode(srv, code, tc.verifier) + require.Equal(t, http.StatusBadRequest, rec.Code) + + errCode, desc := tokenError(t, rec) + assert.Equal(t, oauth2.CodeInvalidGrant, errCode) + assert.Equal(t, "malformed code_verifier", desc) + }) + } +} diff --git a/oauth2/grant/authorization_code.go b/oauth2/grant/authorization_code.go index a1b36001..c8ae6082 100644 --- a/oauth2/grant/authorization_code.go +++ b/oauth2/grant/authorization_code.go @@ -105,7 +105,14 @@ func (g *AuthorizationCode) verifyPKCE(req Request, code *oauth2.AuthorizationCo // The "plain" transformation is accepted only when the profile tolerates // it (Profile20). BCP and OAuth 2.1 mandate S256. if method == pkce.MethodPlain && !req.Profile.AllowsPKCEPlain() { - return oauth2.ErrInvalidGrant.WithDescription(`PKCE method "plain" is refused by the active profile`) + return oauth2.ErrInvalidGrant.WithDescription("PKCE method plain is refused by the active profile") + } + + // RFC 7636 §4.1: a code_verifier is 43-128 unreserved characters. A + // malformed one cannot be a valid proof, and §4.6 answers a failed + // proof with invalid_grant. The code stays consumed. + if !pkce.ValidVerifier(verifier) { + return oauth2.ErrInvalidGrant.WithDescription("malformed code_verifier") } if !pkce.Verify(method, verifier, code.CodeChallenge) { diff --git a/oauth2/grant/authorization_code_pkce_test.go b/oauth2/grant/authorization_code_pkce_test.go new file mode 100644 index 00000000..4a0ed205 --- /dev/null +++ b/oauth2/grant/authorization_code_pkce_test.go @@ -0,0 +1,204 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package grant_test + +import ( + "context" + "net/url" + "strings" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/grant" + "github.com/hyperscale-stack/security/oauth2/pkce" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// wellFormedWrongVerifier is RFC 7636 §4.1 compliant but does not match +// codeChallenge, so only the PKCE comparison can refuse it. +const wellFormedWrongVerifier = "a-well-formed-verifier-that-does-not-match-the-challenge" + +// unreservedAlphabet is the RFC 3986 §2.3 unreserved set a code verifier is +// drawn from (RFC 7636 §4.1). +const unreservedAlphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~" + +// seedPKCECode stores an authorization code bound to challenge / method and +// returns the /token request presenting verifier under profile. +func seedPKCECode( + t *testing.T, + store oauth2.Storage, + method pkce.Method, + challenge, verifier string, + profile oauth2.Profile, +) grant.Request { + t.Helper() + + raw := "raw-pkce-code" + require.NoError(t, store.SaveAuthorizationCode(context.Background(), &oauth2.AuthorizationCode{ + Code: raw, CodeHash: oauth2.HashToken(nil, raw), + ClientID: clientID, Subject: subject, RedirectURI: redirectURI, Scope: "read:mail", + CodeChallenge: challenge, CodeChallengeMethod: string(method), + IssuedAt: time.Date(2026, 5, 20, 12, 0, 0, 0, time.UTC), + ExpiresAt: time.Date(2026, 5, 20, 12, 10, 0, 0, time.UTC), + })) + + form := url.Values{} + form.Set("code", raw) + form.Set("redirect_uri", redirectURI) + form.Set("code_verifier", verifier) + + return grant.Request{ + Client: newClient(), Form: form, Profile: profile, + Issuer: "https://auth.example", Audience: "api", + Now: time.Date(2026, 5, 20, 12, 5, 0, 0, time.UTC), + } +} + +// newPKCEGrant builds an authorization_code grant that does not force PKCE +// itself: the profile of each request decides. +func newPKCEGrant(store oauth2.Storage) *grant.AuthorizationCode { + return grant.NewAuthorizationCode(grant.Config{ + Storage: store, AccessTokens: newAccessGen(), AccessTTL: time.Hour, + }) +} + +// mustS256 derives the S256 challenge of verifier. +func mustS256(t *testing.T, verifier string) string { + t.Helper() + + c, ok := pkce.Challenge(pkce.MethodS256, verifier) + require.True(t, ok) + + return c +} + +// TestAuthorizationCodeRejectsMalformedVerifier stores the S256 challenge of +// each malformed verifier, so the transformation matches and only the RFC +// 7636 §4.1 syntax check can refuse the exchange. +func TestAuthorizationCodeRejectsMalformedVerifier(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + verifier string + }{ + {"one character (issue #79)", "x"}, + {"42 chars", strings.Repeat("a", 42)}, + {"129 chars", strings.Repeat("a", 129)}, + {"non-ASCII", strings.Repeat("a", 41) + "é"}, + {"trailing LF", codeVerifier + "\n"}, + {"embedded space", codeVerifier[:20] + " " + codeVerifier[20:]}, + {"standard base64 plus", strings.Repeat("a", 42) + "+"}, + {"standard base64 slash", strings.Repeat("a", 42) + "/"}, + {"base64 padding", strings.Repeat("a", 42) + "="}, + {"percent-encoded tilde", strings.Repeat("a", 40) + "%7E"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store := newStore() + req := seedPKCECode(t, store, pkce.MethodS256, mustS256(t, tc.verifier), tc.verifier, oauth2.Profile20BCP) + + resp, err := newPKCEGrant(store).Handle(context.Background(), req) + require.Error(t, err) + assert.Nil(t, resp) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.ErrorContains(t, err, "malformed code_verifier") + }) + } +} + +// TestAuthorizationCodeAcceptsWellFormedVerifiers is a regression guard for +// the RFC 7636 §4.1 bounds and alphabet. +func TestAuthorizationCodeAcceptsWellFormedVerifiers(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + verifier string + }{ + {"43 chars (the minimum)", strings.Repeat("a", 43)}, + {"128 chars (the maximum)", strings.Repeat("a", 128)}, + {"the whole unreserved alphabet", unreservedAlphabet}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store := newStore() + req := seedPKCECode(t, store, pkce.MethodS256, mustS256(t, tc.verifier), tc.verifier, oauth2.Profile20BCP) + + resp, err := newPKCEGrant(store).Handle(context.Background(), req) + require.NoError(t, err) + assert.NotEmpty(t, resp.Pair.Access.Token) + }) + } +} + +// TestAuthorizationCodeProfile20WithoutPKCE is a regression guard: PKCE +// stays optional under Profile20 when the grant does not require it. +func TestAuthorizationCodeProfile20WithoutPKCE(t *testing.T) { + t.Parallel() + + store := newStore() + req := seedPKCECode(t, store, "", "", "", oauth2.Profile20) + + resp, err := newPKCEGrant(store).Handle(context.Background(), req) + require.NoError(t, err) + assert.NotEmpty(t, resp.Pair.Access.Token) +} + +// TestAuthorizationCodePlainRejectsMalformedVerifier covers a plain pair +// that matches verbatim but is too short to be a code verifier. The empty +// method defaults to plain (RFC 7636 §4.3), which Profile20 tolerates. +func TestAuthorizationCodePlainRejectsMalformedVerifier(t *testing.T) { + t.Parallel() + + store := newStore() + req := seedPKCECode(t, store, "", "shared-plain-secret", "shared-plain-secret", oauth2.Profile20) + + _, err := newPKCEGrant(store).Handle(context.Background(), req) + require.Error(t, err) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.ErrorContains(t, err, "malformed code_verifier") +} + +// TestAuthorizationCodeMalformedStoredChallengeNeverMatches documents codes +// stored before /authorize validated the challenge: a well-formed verifier +// can never match a malformed challenge, so the exchange fails as a plain +// mismatch. +func TestAuthorizationCodeMalformedStoredChallengeNeverMatches(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + method pkce.Method + challenge string + profile oauth2.Profile + }{ + {"padded S256 challenge", pkce.MethodS256, codeChallenge + "=", oauth2.Profile20BCP}, + {"short S256 challenge", pkce.MethodS256, "abc", oauth2.Profile20BCP}, + {"short plain challenge", pkce.MethodPlain, "shared-plain-secret", oauth2.Profile20}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store := newStore() + req := seedPKCECode(t, store, tc.method, tc.challenge, codeVerifier, tc.profile) + + _, err := newPKCEGrant(store).Handle(context.Background(), req) + require.Error(t, err) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.ErrorContains(t, err, "PKCE verification failed") + }) + } +} diff --git a/oauth2/grant/grant_more_test.go b/oauth2/grant/grant_more_test.go index 10b05e76..93f31ea0 100644 --- a/oauth2/grant/grant_more_test.go +++ b/oauth2/grant/grant_more_test.go @@ -103,17 +103,21 @@ func TestAuthorizationCodeMissingVerifier(t *testing.T) { assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) } +// plainVerifier is a well-formed RFC 7636 §4.1 verifier (52 characters, +// including ".", "~" and "_") used as a plain PKCE pair. +const plainVerifier = "shared-plain-secret.with~unreserved_chars-0123456789" + // plainPKCECode seeds a code whose challenge method is empty (the grant // defaults to "plain", where the verifier equals the challenge verbatim) -// and returns the matching /token form. -func plainPKCECode(t *testing.T, store oauth2.Storage) url.Values { +// bound to verifier, and returns the matching /token form. +func plainPKCECode(t *testing.T, store oauth2.Storage, verifier string) url.Values { t.Helper() raw := "raw-plain-code" require.NoError(t, store.SaveAuthorizationCode(context.Background(), &oauth2.AuthorizationCode{ Code: raw, CodeHash: oauth2.HashToken(nil, raw), ClientID: clientID, Subject: subject, RedirectURI: redirectURI, Scope: "read:mail", - CodeChallenge: "shared-plain-secret", CodeChallengeMethod: "", + CodeChallenge: verifier, CodeChallengeMethod: "", IssuedAt: time.Date(2026, 5, 20, 12, 0, 0, 0, time.UTC), ExpiresAt: time.Date(2026, 5, 20, 12, 10, 0, 0, time.UTC), })) @@ -121,7 +125,7 @@ func plainPKCECode(t *testing.T, store oauth2.Storage) url.Values { form := url.Values{} form.Set("code", raw) form.Set("redirect_uri", redirectURI) - form.Set("code_verifier", "shared-plain-secret") + form.Set("code_verifier", verifier) return form } @@ -130,7 +134,7 @@ func TestAuthorizationCodePlainPKCEAcceptedUnderProfile20(t *testing.T) { t.Parallel() store := newStore() - form := plainPKCECode(t, store) + form := plainPKCECode(t, store, plainVerifier) g := grant.NewAuthorizationCode(grant.Config{ Storage: store, AccessTokens: newAccessGen(), AccessTTL: time.Hour, @@ -148,7 +152,9 @@ func TestAuthorizationCodePlainPKCERefusedUnderBCP(t *testing.T) { t.Parallel() store := newStore() - form := plainPKCECode(t, store) + // A malformed (too short) plain pair: the profile ban must answer + // before the RFC 7636 syntax check. + form := plainPKCECode(t, store, "shared-plain-secret") g := grant.NewAuthorizationCode(grant.Config{ Storage: store, AccessTokens: newAccessGen(), AccessTTL: time.Hour, @@ -161,6 +167,11 @@ func TestAuthorizationCodePlainPKCERefusedUnderBCP(t *testing.T) { }) require.Error(t, err) assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + + // RFC 6749 §5.2: the description carries no double quote. + var oe *oauth2.Error + require.ErrorAs(t, err, &oe) + assert.Equal(t, "PKCE method plain is refused by the active profile", oe.Description) } func TestAuthorizationCodeProfileRequiresPKCE(t *testing.T) { diff --git a/oauth2/grant/grant_test.go b/oauth2/grant/grant_test.go index 6aaf9156..a6b8b7fe 100644 --- a/oauth2/grant/grant_test.go +++ b/oauth2/grant/grant_test.go @@ -131,11 +131,12 @@ func TestAuthorizationCodePKCEMismatch(t *testing.T) { store := newStore() g, req := newAuthCodeReq(context.Background(), store, true) - req.Form.Set("code_verifier", "wrong-verifier") + req.Form.Set("code_verifier", wellFormedWrongVerifier) _, err := g.Handle(context.Background(), req) require.Error(t, err) assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.ErrorContains(t, err, "PKCE verification failed") } func TestAuthorizationCodeRedirectMismatch(t *testing.T) { diff --git a/oauth2/pkce/pkce.go b/oauth2/pkce/pkce.go index ac2dfec0..76bdb94d 100644 --- a/oauth2/pkce/pkce.go +++ b/oauth2/pkce/pkce.go @@ -9,6 +9,11 @@ // clients (OAuth 2.0 BCP §2.1.1 / OAuth 2.1 draft §1.7). The "plain" // method is supported for backwards compatibility but its use is refused // when the server profile is OAuth 2.0 BCP or OAuth 2.1 draft. +// +// Both ends enforce the RFC 7636 syntax, not just the transformation: +// [ValidChallenge] checks a code_challenge at the authorization endpoint, +// and [Verify] refuses at the token endpoint a code_verifier that +// [ValidVerifier] rejects. package pkce import ( @@ -17,6 +22,12 @@ import ( "encoding/base64" ) +// RFC 7636 §4.1 bounds on the code_verifier length, in characters. +const ( + minVerifierLength = 43 + maxVerifierLength = 128 +) + // Method identifies the PKCE transformation used to derive the challenge // from the verifier (RFC 7636 §4.2). type Method string @@ -32,10 +43,52 @@ const ( // String makes Method satisfy fmt.Stringer. func (m Method) String() string { return string(m) } -// Verify computes the challenge from verifier per method and compares it -// constant-time against expected. Returns false on length mismatch, on -// unsupported method, or on plain-vs-S256 mismatch. +// ValidVerifier reports whether verifier is a well-formed RFC 7636 §4.1 +// code_verifier: 43 to 128 characters from the unreserved set +// [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~". Any other byte, including +// whitespace and every non-ASCII byte, makes it malformed. +func ValidVerifier(verifier string) bool { + if len(verifier) < minVerifierLength || len(verifier) > maxVerifierLength { + return false + } + + for i := range len(verifier) { + if !isUnreserved(verifier[i]) { + return false + } + } + + return true +} + +// ValidChallenge reports whether challenge is well-formed for method, as the +// authorization endpoint checks it (RFC 7636 §4.2): a plain challenge is the +// verifier itself (see [ValidVerifier]); an S256 challenge is exactly the +// unpadded base64url encoding of a SHA-256 digest (43 characters, Appendix +// A). Any other method — including the empty one, which callers resolve to +// [MethodPlain] first (§4.3) — is never valid. +func ValidChallenge(method Method, challenge string) bool { + switch method { + case MethodS256: + return validS256Challenge(challenge) + case MethodPlain: + return ValidVerifier(challenge) + default: + return false + } +} + +// Verify reports whether verifier matches the expected challenge under +// method (RFC 7636 §4.6). It returns false for a malformed verifier (see +// [ValidVerifier]), an unsupported method or a mismatch, so a malformed +// stored challenge never matches either. The syntax check reads only the +// verifier; a well-formed verifier is then compared with expected in +// constant time. func Verify(method Method, verifier, expected string) bool { + if !ValidVerifier(verifier) { + return false + } + switch method { case MethodS256: return s256Equal(verifier, expected) @@ -46,8 +99,8 @@ func Verify(method Method, verifier, expected string) bool { } } -// VerifyS256 is a convenience for the recommended S256 method. -func VerifyS256(verifier, expected string) bool { return s256Equal(verifier, expected) } +// VerifyS256 is [Verify] with the recommended S256 method. +func VerifyS256(verifier, expected string) bool { return Verify(MethodS256, verifier, expected) } func s256Equal(verifier, expected string) bool { sum := sha256.Sum256([]byte(verifier)) @@ -59,7 +112,8 @@ func s256Equal(verifier, expected string) bool { // Challenge derives the challenge for a given verifier and method. Useful // in test helpers and client-side libraries; not used by the server during // verification (the server only consumes the challenge stored alongside -// the code). +// the code). Challenge does not validate verifier; check untrusted input +// with [ValidVerifier] first. func Challenge(method Method, verifier string) (string, bool) { switch method { case MethodS256: @@ -72,3 +126,25 @@ func Challenge(method Method, verifier string) (string, bool) { return "", false } } + +// validS256Challenge reports whether challenge is exactly what +// BASE64URL-ENCODE (RFC 7636 Appendix A) yields for a SHA-256 digest. +// Re-encoding the decoded bytes also refuses what a decode alone lets +// through: the CR and LF encoding/base64 skips, even in strict mode, and +// non-zero trailing bits. +func validS256Challenge(challenge string) bool { + if len(challenge) != base64.RawURLEncoding.EncodedLen(sha256.Size) { + return false + } + + digest, err := base64.RawURLEncoding.DecodeString(challenge) + + return err == nil && base64.RawURLEncoding.EncodeToString(digest) == challenge +} + +// isUnreserved reports whether c is an RFC 3986 §2.3 unreserved character. +// It inspects bytes, not runes, so any multi-byte UTF-8 sequence is refused. +func isUnreserved(c byte) bool { + return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || + c == '-' || c == '.' || c == '_' || c == '~' +} diff --git a/oauth2/pkce/pkce_test.go b/oauth2/pkce/pkce_test.go index 0f4de0a7..fa95a517 100644 --- a/oauth2/pkce/pkce_test.go +++ b/oauth2/pkce/pkce_test.go @@ -18,6 +18,10 @@ const ( // challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM rfc7636Verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" rfc7636Challenge = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" + + // wellFormedVerifier is RFC 7636 §4.1 compliant but differs from the + // Appendix B verifier, so only a mismatch can refuse it. + wellFormedVerifier = "a-well-formed-verifier-that-does-not-match-the-challenge" ) func TestVerifyS256AcceptsRFC7636Vector(t *testing.T) { @@ -30,20 +34,22 @@ func TestVerifyS256AcceptsRFC7636Vector(t *testing.T) { func TestVerifyS256RejectsBadVerifier(t *testing.T) { t.Parallel() - assert.False(t, pkce.VerifyS256("wrong-verifier", rfc7636Challenge)) + require.True(t, pkce.ValidVerifier(wellFormedVerifier), "the mismatch, not the syntax, must refuse it") + assert.False(t, pkce.VerifyS256(wellFormedVerifier, rfc7636Challenge)) } func TestVerifyPlainAcceptsExactMatch(t *testing.T) { t.Parallel() - assert.True(t, pkce.Verify(pkce.MethodPlain, "verifier", "verifier")) - assert.False(t, pkce.Verify(pkce.MethodPlain, "verifier", "other")) + assert.True(t, pkce.Verify(pkce.MethodPlain, rfc7636Verifier, rfc7636Verifier)) + assert.False(t, pkce.Verify(pkce.MethodPlain, rfc7636Verifier, wellFormedVerifier)) } func TestVerifyUnknownMethodReturnsFalse(t *testing.T) { t.Parallel() - assert.False(t, pkce.Verify("MD5", "verifier", "challenge")) + // A well-formed verifier, so the unknown method is what refuses it. + assert.False(t, pkce.Verify("MD5", rfc7636Verifier, rfc7636Challenge)) } func TestChallengeMatchesVerification(t *testing.T) { diff --git a/oauth2/pkce/syntax_test.go b/oauth2/pkce/syntax_test.go new file mode 100644 index 00000000..1c8c562c --- /dev/null +++ b/oauth2/pkce/syntax_test.go @@ -0,0 +1,224 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package pkce_test + +import ( + "strings" + "testing" + + "github.com/hyperscale-stack/security/oauth2/pkce" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// unreservedAlphabet is the RFC 3986 §2.3 unreserved set a code verifier is +// drawn from (RFC 7636 §4.1). +const unreservedAlphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~" + +// base64URLAlphabet is the RFC 4648 §5 alphabet, in index order. +const base64URLAlphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_" + +// as returns n "a" characters. +func as(n int) string { return strings.Repeat("a", n) } + +// mustChallenge derives the method challenge of verifier. +func mustChallenge(t *testing.T, method pkce.Method, verifier string) string { + t.Helper() + + c, ok := pkce.Challenge(method, verifier) + require.True(t, ok) + + return c +} + +func TestValidVerifier(t *testing.T) { + t.Parallel() + + v := rfc7636Verifier + + cases := []struct { + name string + verifier string + want bool + }{ + {"RFC 7636 Appendix B vector (43 chars, the minimum)", v, true}, + {"128 chars (the maximum)", as(128), true}, + {"the whole unreserved alphabet", unreservedAlphabet, true}, + {"empty", "", false}, + {"one character (issue #79)", "x", false}, + {"42 chars", as(42), false}, + {"129 chars", as(129), false}, + {"43 bytes but 42 runes", as(41) + "é", false}, + {"43 runes", as(42) + "é", false}, + {"43 bytes with a 3-byte rune", as(40) + "~", false}, + {"leading space", " " + v, false}, + {"trailing LF", v + "\n", false}, + {"embedded tab", v[:20] + "\t" + v[20:], false}, + {"trailing CRLF", v + "\r\n", false}, + {"embedded space", v[:20] + " " + v[20:], false}, + {"standard base64 plus", as(42) + "+", false}, + {"standard base64 slash", as(42) + "/", false}, + {"base64 padding", as(42) + "=", false}, + {"percent-encoded tilde", as(40) + "%7E", false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tc.want, pkce.ValidVerifier(tc.verifier)) + }) + } +} + +// TestValidVerifierRejectsEveryNonUnreservedByte checks each of the 256 +// byte values in an otherwise valid 43-byte verifier: whitespace, control +// characters, every forbidden ASCII character and every non-ASCII byte are +// refused, the 66 unreserved characters are accepted. +func TestValidVerifierRejectsEveryNonUnreservedByte(t *testing.T) { + t.Parallel() + + require.Len(t, unreservedAlphabet, 66) + + accepted := 0 + + for b := range 256 { + c := byte(b) + got := pkce.ValidVerifier(as(42) + string([]byte{c})) + + assert.Equal(t, strings.IndexByte(unreservedAlphabet, c) >= 0, got, "byte 0x%02X", c) + + if got { + accepted++ + } + } + + assert.Equal(t, len(unreservedAlphabet), accepted) +} + +func TestValidChallenge(t *testing.T) { + t.Parallel() + + c := rfc7636Challenge + + cases := []struct { + name string + method pkce.Method + challenge string + want bool + }{ + {"S256: RFC 7636 Appendix B challenge", pkce.MethodS256, c, true}, + {"S256: challenge of a one-character verifier", pkce.MethodS256, mustChallenge(t, pkce.MethodS256, "x"), true}, + {"S256: 42 chars", pkce.MethodS256, c[:42], false}, + {"S256: 44 chars", pkce.MethodS256, c + "A", false}, + {"S256: padded", pkce.MethodS256, c + "=", false}, + {"S256: standard alphabet plus", pkce.MethodS256, strings.Replace(c, "-", "+", 1), false}, + {"S256: standard alphabet slash", pkce.MethodS256, c[:10] + "/" + c[11:], false}, + {"S256: LF in place of a character", pkce.MethodS256, c[:20] + "\n" + c[21:], false}, + {"S256: trailing CRLF in place of a character", pkce.MethodS256, c[:41] + "\r\n", false}, + {"S256: trailing LF", pkce.MethodS256, c + "\n", false}, + {"S256: tilde", pkce.MethodS256, c[:10] + "~" + c[11:], false}, + {"S256: dot", pkce.MethodS256, c[:10] + "." + c[11:], false}, + {"S256: non-canonical trailing bits", pkce.MethodS256, c[:42] + "N", false}, + {"S256: non-ASCII", pkce.MethodS256, c[:41] + "é", false}, + {"S256: 64 chars", pkce.MethodS256, as(64), false}, + {"S256: empty", pkce.MethodS256, "", false}, + {"plain: RFC 7636 Appendix B verifier", pkce.MethodPlain, rfc7636Verifier, true}, + {"plain: 43 chars", pkce.MethodPlain, as(43), true}, + {"plain: 128 chars", pkce.MethodPlain, as(128), true}, + {"plain: the whole unreserved alphabet", pkce.MethodPlain, unreservedAlphabet, true}, + {"plain: an S256-shaped string", pkce.MethodPlain, c, true}, + {"plain: 42 chars", pkce.MethodPlain, as(42), false}, + {"plain: 129 chars", pkce.MethodPlain, as(129), false}, + {"plain: plus", pkce.MethodPlain, as(42) + "+", false}, + {"plain: trailing space", pkce.MethodPlain, rfc7636Verifier + " ", false}, + {"plain: non-ASCII", pkce.MethodPlain, as(41) + "é", false}, + {"empty method", "", rfc7636Verifier, false}, + {"unknown method", "S512", c, false}, + {"methods are case-sensitive", "s256", c, false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tc.want, pkce.ValidChallenge(tc.method, tc.challenge)) + }) + } +} + +// TestValidChallengeS256RequiresCanonicalTrailingBits varies the last of the +// 43 characters: it carries 4 bits of the digest and 2 zero bits, so only +// the 16 characters whose index is a multiple of 4 are canonical. The others +// decode to the same digest but are not what BASE64URL-ENCODE produces. +func TestValidChallengeS256RequiresCanonicalTrailingBits(t *testing.T) { + t.Parallel() + + var accepted []byte + + for i := range len(base64URLAlphabet) { + ch := base64URLAlphabet[i] + got := pkce.ValidChallenge(pkce.MethodS256, rfc7636Challenge[:42]+string(ch)) + + assert.Equal(t, i%4 == 0, got, "final character %q", ch) + + if got { + accepted = append(accepted, ch) + } + } + + assert.Equal(t, "AEIMQUYcgkosw048", string(accepted)) +} + +// TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches pins the +// issue #79 fix at the primitive level: the challenge is derived from the +// malformed verifier itself, so only the syntax check can refuse it. +func TestVerifyRejectsMalformedVerifierEvenWhenTransformMatches(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + verifier string + }{ + {"one character (issue #79)", "x"}, + {"42 chars", as(42)}, + {"129 chars", as(129)}, + {"non-ASCII", as(41) + "é"}, + {"leading space", " " + rfc7636Verifier}, + {"trailing LF", rfc7636Verifier + "\n"}, + {"standard base64 plus", as(42) + "+"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + challenge := mustChallenge(t, pkce.MethodS256, tc.verifier) + + assert.False(t, pkce.Verify(pkce.MethodS256, tc.verifier, challenge)) + assert.False(t, pkce.VerifyS256(tc.verifier, challenge)) + assert.False(t, pkce.Verify(pkce.MethodPlain, tc.verifier, tc.verifier)) + }) + } +} + +// TestVerifyAcceptsEveryLegalVerifierLength is a regression guard: every +// length RFC 7636 §4.1 allows round-trips through both methods. +func TestVerifyAcceptsEveryLegalVerifierLength(t *testing.T) { + t.Parallel() + + source := strings.Repeat(unreservedAlphabet, 2) + + for n := 43; n <= 128; n++ { + v := source[:n] + challenge := mustChallenge(t, pkce.MethodS256, v) + + assert.True(t, pkce.ValidVerifier(v), "length %d", n) + assert.True(t, pkce.ValidChallenge(pkce.MethodS256, challenge), "length %d", n) + assert.True(t, pkce.Verify(pkce.MethodS256, v, challenge), "length %d", n) + assert.True(t, pkce.ValidChallenge(pkce.MethodPlain, v), "length %d", n) + assert.True(t, pkce.Verify(pkce.MethodPlain, v, v), "length %d", n) + } +} diff --git a/oauth2/profile.go b/oauth2/profile.go index 4557a54c..8dc73546 100644 --- a/oauth2/profile.go +++ b/oauth2/profile.go @@ -15,8 +15,8 @@ package oauth2 // and password grants outright; mandates PKCE on // authorization_code; mandates refresh-token // rotation. -// - Profile21Draft — draft-ietf-oauth-v2-1. Same constraints as BCP plus -// an explicit prohibition of "plain" PKCE. +// - Profile21Draft — draft-ietf-oauth-v2-1. Same constraints as BCP, +// which already refuses "plain" PKCE (S256 only). // // The recommended default is [Profile20BCP]. type Profile int From 48a5b6d3c46e6466725449a50a63ecacc1104e04 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Sat, 10 Oct 2026 04:04:52 +0200 Subject: [PATCH 2/2] test(oauth2): pin the missing code_verifier description TestAuthorizationCodeMissingVerifier only asserted invalid_grant, so moving the missing-verifier check after the RFC 7636 syntax check went unnoticed: an absent code_verifier then reads "malformed code_verifier" instead of "missing code_verifier", with the same error code. Mutation testing of the PKCE syntax fix left that mutant alive. The test now pins the description, so the presence check keeps answering before the syntax check. Refs #79. --- oauth2/grant/grant_more_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/oauth2/grant/grant_more_test.go b/oauth2/grant/grant_more_test.go index 93f31ea0..bc818bb7 100644 --- a/oauth2/grant/grant_more_test.go +++ b/oauth2/grant/grant_more_test.go @@ -101,6 +101,8 @@ func TestAuthorizationCodeMissingVerifier(t *testing.T) { _, err := g.Handle(context.Background(), req) require.Error(t, err) assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + // The presence check answers before the RFC 7636 syntax check. + assert.ErrorContains(t, err, "missing code_verifier") } // plainVerifier is a well-formed RFC 7636 §4.1 verifier (52 characters,