From d3621e507298a5374fd180a3c9d60ea12a6c6dd2 Mon Sep 17 00:00:00 2001 From: George Lazarica Date: Sun, 2 Aug 2026 23:42:22 +0200 Subject: [PATCH 1/3] fix(oauth): reject plain PKCE method per OAuth 2.1 spec OAuth 2.1 (draft-ietf-oauth-v2-1-12, Section 4.1.1) explicitly removes support for the plain code_challenge_method. Only S256 is permitted. With plain, code_challenge == code_verifier, and code_challenge is transmitted in the authorization request URL, which routinely appears in server access logs, browser history, Referer headers, and CDN logs. An attacker with read access to any of those can immediately replay the authorization code using code_verifier = code_challenge, defeating PKCE's proof-of-possession guarantee entirely. Changes: - validatePKCEParams(): reject any method other than S256 - VerifyPKCEChallenge(): remove the plain case (falls through to default which returns PKCEInvalidCodeMethodError) - pkce_test.go: update plain cases to expect errors - authorize_test.go: add TestValidatePKCEParams_OAuth21 Ref: https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-12#section-4.1.1 --- internal/api/oauthserver/authorize.go | 13 ++-- internal/api/oauthserver/authorize_test.go | 83 ++++++++++++++++++++++ internal/security/pkce.go | 11 ++- internal/security/pkce_test.go | 33 +++++---- 4 files changed, 115 insertions(+), 25 deletions(-) diff --git a/internal/api/oauthserver/authorize.go b/internal/api/oauthserver/authorize.go index 2addacfdaa..4ce97d41d6 100644 --- a/internal/api/oauthserver/authorize.go +++ b/internal/api/oauthserver/authorize.go @@ -476,15 +476,18 @@ func (s *Server) validateRemainingAuthorizeParams(params *AuthorizeParams) error } func (s *Server) validatePKCEParams(codeChallengeMethod, codeChallenge string) error { - // PKCE is mandatory for the authorization code flow OAuth2.1 - // Both code_challenge and code_challenge_method must be provided together + // PKCE is mandatory for the authorization code flow per OAuth 2.1. + // Both code_challenge and code_challenge_method must be provided together. if codeChallenge == "" || codeChallengeMethod == "" { return errors.New("PKCE flow requires both code_challenge and code_challenge_method") } - // Validate code challenge method (case-insensitive) - if strings.ToLower(codeChallengeMethod) != "s256" && strings.ToLower(codeChallengeMethod) != "plain" { - return errors.New("code_challenge_method must be 'S256' or 'plain'") + // OAuth 2.1 (draft-ietf-oauth-v2-1-12, Section 4.1.1) permits only S256. + // The plain method is excluded: code_challenge is transmitted in the + // authorization request URL and may appear in server logs, browser history, + // or Referer headers — with plain, that directly exposes the code_verifier. + if strings.ToLower(codeChallengeMethod) != "s256" { + return errors.New("code_challenge_method must be 'S256'") } // Validate code challenge format and length (per OAuth2 spec) diff --git a/internal/api/oauthserver/authorize_test.go b/internal/api/oauthserver/authorize_test.go index c7ec7f458c..9934a20ad8 100644 --- a/internal/api/oauthserver/authorize_test.go +++ b/internal/api/oauthserver/authorize_test.go @@ -666,3 +666,86 @@ func (ts *OAuthAuthorizeTestSuite) TestConsent_InvalidActionRejected() { err := ts.Server.OAuthServerConsent(w, req) ts.assertHTTPError(err, http.StatusBadRequest, apierrors.ErrorCodeValidationFailed) } + +// TestValidatePKCEParams_OAuth21 verifies that the plain PKCE method is rejected +// per OAuth 2.1 (draft-ietf-oauth-v2-1-12, Section 4.1.1) and that S256 is accepted. +func TestValidatePKCEParams_OAuth21(t *testing.T) { + globalConfig, err := confload.LoadGlobal(oauthServerTestConfig) + require.NoError(t, err) + + conn, err := test.SetupDBConnection(globalConfig) + require.NoError(t, err) + defer conn.Close() + + hooksMgr := &v0hooks.Manager{} + tokenService := tokens.NewService(globalConfig, hooksMgr) + server := NewServer(globalConfig, conn, tokenService) + + tests := []struct { + name string + codeChallengeMethod string + codeChallenge string + wantErr bool + errContains string + }{ + { + name: "S256 accepted", + codeChallengeMethod: "S256", + codeChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + wantErr: false, + }, + { + name: "s256 lowercase accepted", + codeChallengeMethod: "s256", + codeChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + wantErr: false, + }, + { + name: "plain rejected", + codeChallengeMethod: "plain", + codeChallenge: "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG", + wantErr: true, + errContains: "S256", + }, + { + name: "PLAIN rejected case-insensitively", + codeChallengeMethod: "PLAIN", + codeChallenge: "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG", + wantErr: true, + errContains: "S256", + }, + { + name: "unknown method rejected", + codeChallengeMethod: "rs256", + codeChallenge: "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG", + wantErr: true, + errContains: "S256", + }, + { + name: "missing method rejected", + codeChallengeMethod: "", + codeChallenge: "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG", + wantErr: true, + errContains: "requires both", + }, + { + name: "missing challenge rejected", + codeChallengeMethod: "S256", + codeChallenge: "", + wantErr: true, + errContains: "requires both", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := server.validatePKCEParams(tt.codeChallengeMethod, tt.codeChallenge) + if tt.wantErr { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.errContains) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/internal/security/pkce.go b/internal/security/pkce.go index 7e4ccf41dd..1496b90706 100644 --- a/internal/security/pkce.go +++ b/internal/security/pkce.go @@ -11,8 +11,11 @@ import ( const PKCEInvalidCodeChallengeError = "code challenge does not match previously saved code verifier" const PKCEInvalidCodeMethodError = "code challenge method not supported" -// VerifyPKCEChallenge performs PKCE verification using the provided challenge, method, and verifier -// This is a shared utility function used by both FlowState and OAuthServerAuthorization +// VerifyPKCEChallenge performs PKCE verification using the provided challenge, method, and verifier. +// Only S256 is supported per OAuth 2.1 (draft-ietf-oauth-v2-1-12, Section 4.1.1). +// The plain method is explicitly excluded: the code_challenge is transmitted in +// the authorization URL and may appear in server logs, browser history, or Referer +// headers — with plain, that leaks the code_verifier directly. func VerifyPKCEChallenge(codeChallenge, codeChallengeMethod, codeVerifier string) error { switch strings.ToLower(codeChallengeMethod) { case "s256": @@ -21,10 +24,6 @@ func VerifyPKCEChallenge(codeChallenge, codeChallengeMethod, codeVerifier string if subtle.ConstantTimeCompare([]byte(codeChallenge), []byte(encodedCodeVerifier)) != 1 { return errors.New(PKCEInvalidCodeChallengeError) } - case "plain": - if subtle.ConstantTimeCompare([]byte(codeChallenge), []byte(codeVerifier)) != 1 { - return errors.New(PKCEInvalidCodeChallengeError) - } default: return errors.New(PKCEInvalidCodeMethodError) } diff --git a/internal/security/pkce_test.go b/internal/security/pkce_test.go index 795b067781..0041c3b9c9 100644 --- a/internal/security/pkce_test.go +++ b/internal/security/pkce_test.go @@ -17,17 +17,19 @@ func TestVerifyPKCEChallenge(t *testing.T) { }{ { name: "valid S256 PKCE", - codeChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", // S256 of "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + codeChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", codeChallengeMethod: "S256", codeVerifier: "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk", wantErr: false, }, { - name: "valid plain PKCE", + // OAuth 2.1 Section 4.1.1: plain is not supported. + name: "plain method rejected", codeChallenge: "test-challenge", codeChallengeMethod: "plain", codeVerifier: "test-challenge", - wantErr: false, + wantErr: true, + errMsg: PKCEInvalidCodeMethodError, }, { name: "invalid S256 verifier", @@ -38,12 +40,13 @@ func TestVerifyPKCEChallenge(t *testing.T) { errMsg: "code challenge does not match", }, { - name: "invalid plain verifier", + // plain is rejected at method level, not verifier level. + name: "plain method rejected even with wrong verifier", codeChallenge: "test-challenge", codeChallengeMethod: "plain", codeVerifier: "wrong-challenge", wantErr: true, - errMsg: "code challenge does not match", + errMsg: PKCEInvalidCodeMethodError, }, { name: "invalid challenge method", @@ -61,11 +64,13 @@ func TestVerifyPKCEChallenge(t *testing.T) { wantErr: false, }, { - name: "case insensitive plain method", + // Case-insensitive rejection. + name: "PLAIN rejected case-insensitively", codeChallenge: "test-challenge", codeChallengeMethod: "PLAIN", codeVerifier: "test-challenge", - wantErr: false, + wantErr: true, + errMsg: PKCEInvalidCodeMethodError, }, { name: "empty verifier with S256", @@ -76,12 +81,12 @@ func TestVerifyPKCEChallenge(t *testing.T) { errMsg: "code challenge does not match", }, { - name: "empty verifier with plain", + name: "plain method rejected with empty verifier", codeChallenge: "test-challenge", codeChallengeMethod: "plain", codeVerifier: "", wantErr: true, - errMsg: "code challenge does not match", + errMsg: PKCEInvalidCodeMethodError, }, { name: "empty challenge with S256", @@ -92,26 +97,26 @@ func TestVerifyPKCEChallenge(t *testing.T) { errMsg: "code challenge does not match", }, { - name: "empty challenge with plain", + name: "plain method rejected with empty challenge", codeChallenge: "", codeChallengeMethod: "plain", codeVerifier: "test-challenge", wantErr: true, - errMsg: "code challenge does not match", + errMsg: PKCEInvalidCodeMethodError, }, { - name: "both empty with plain", + name: "plain method rejected when both empty", codeChallenge: "", codeChallengeMethod: "plain", codeVerifier: "", - wantErr: false, + wantErr: true, + errMsg: PKCEInvalidCodeMethodError, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := VerifyPKCEChallenge(tt.codeChallenge, tt.codeChallengeMethod, tt.codeVerifier) - if tt.wantErr { assert.Error(t, err) assert.Contains(t, err.Error(), tt.errMsg) From 98e2a68c3271e7e90472f63f3ad2125f8b208e0e Mon Sep 17 00:00:00 2001 From: George Lazarica Date: Mon, 3 Aug 2026 23:34:58 +0200 Subject: [PATCH 2/3] fix(oauth): scope plain PKCE rejection to OAuth 2.1 server endpoint only The initial implementation incorrectly removed plain support from the shared VerifyPKCEChallenge function, breaking legacy PKCE tests that use the plain method via the social login flow (e.g. external_github_test.go). The plain PKCE method rejection per OAuth 2.1 Section 4.1.1 is correctly enforced in validatePKCEParams (oauthserver package only). The shared VerifyPKCEChallenge function retains plain support for the legacy flow. Also: - Remove 'plain' from CodeChallengeMethodsSupported in the OAuth 2.1 discovery document (jwks.go) since the server no longer accepts it - Restore pkce_test.go to reflect the preserved plain behaviour in the shared function - Update oauth_authorization.go comment for accuracy Signed-off-by: georgelzrc Signed-off-by: George Lazarica --- internal/api/jwks.go | 2 +- internal/models/oauth_authorization.go | 5 ++-- internal/security/pkce.go | 11 +++++---- internal/security/pkce_test.go | 33 +++++++++++--------------- 4 files changed, 24 insertions(+), 27 deletions(-) diff --git a/internal/api/jwks.go b/internal/api/jwks.go index 0ca0bb4b65..0bddc98414 100644 --- a/internal/api/jwks.go +++ b/internal/api/jwks.go @@ -86,7 +86,7 @@ func (a *API) WellKnownOpenID(w http.ResponseWriter, r *http.Request) error { SubjectTypesSupported: []string{"public"}, IDTokenSigningAlgValuesSupported: []string{"RS256", "HS256", "ES256"}, // TODO :: should create this based on signing key config? TokenEndpointAuthMethodsSupported: []string{"client_secret_basic", "client_secret_post", "none"}, - CodeChallengeMethodsSupported: []string{"S256", "plain"}, + CodeChallengeMethodsSupported: []string{"S256"}, ScopesSupported: models.SupportedOAuthScopes, // OIDC Standard Claims diff --git a/internal/models/oauth_authorization.go b/internal/models/oauth_authorization.go index 2b7fab2579..5313beb9d2 100644 --- a/internal/models/oauth_authorization.go +++ b/internal/models/oauth_authorization.go @@ -111,8 +111,9 @@ func NewOAuthServerAuthorization(params NewOAuthServerAuthorizationParams) *OAut auth.CodeChallenge = ¶ms.CodeChallenge } if params.CodeChallengeMethod != "" { - // Normalize code challenge method to lowercase for database storage - // Database enum expects 's256' and 'plain' (lowercase) + // Normalize code challenge method to lowercase for database storage. + // The OAuth 2.1 server only accepts 's256'; the legacy flow may still + // pass 'plain', which is handled at the validation layer. normalizedMethod := strings.ToLower(params.CodeChallengeMethod) auth.CodeChallengeMethod = &normalizedMethod } diff --git a/internal/security/pkce.go b/internal/security/pkce.go index 1496b90706..7e4ccf41dd 100644 --- a/internal/security/pkce.go +++ b/internal/security/pkce.go @@ -11,11 +11,8 @@ import ( const PKCEInvalidCodeChallengeError = "code challenge does not match previously saved code verifier" const PKCEInvalidCodeMethodError = "code challenge method not supported" -// VerifyPKCEChallenge performs PKCE verification using the provided challenge, method, and verifier. -// Only S256 is supported per OAuth 2.1 (draft-ietf-oauth-v2-1-12, Section 4.1.1). -// The plain method is explicitly excluded: the code_challenge is transmitted in -// the authorization URL and may appear in server logs, browser history, or Referer -// headers — with plain, that leaks the code_verifier directly. +// VerifyPKCEChallenge performs PKCE verification using the provided challenge, method, and verifier +// This is a shared utility function used by both FlowState and OAuthServerAuthorization func VerifyPKCEChallenge(codeChallenge, codeChallengeMethod, codeVerifier string) error { switch strings.ToLower(codeChallengeMethod) { case "s256": @@ -24,6 +21,10 @@ func VerifyPKCEChallenge(codeChallenge, codeChallengeMethod, codeVerifier string if subtle.ConstantTimeCompare([]byte(codeChallenge), []byte(encodedCodeVerifier)) != 1 { return errors.New(PKCEInvalidCodeChallengeError) } + case "plain": + if subtle.ConstantTimeCompare([]byte(codeChallenge), []byte(codeVerifier)) != 1 { + return errors.New(PKCEInvalidCodeChallengeError) + } default: return errors.New(PKCEInvalidCodeMethodError) } diff --git a/internal/security/pkce_test.go b/internal/security/pkce_test.go index 0041c3b9c9..795b067781 100644 --- a/internal/security/pkce_test.go +++ b/internal/security/pkce_test.go @@ -17,19 +17,17 @@ func TestVerifyPKCEChallenge(t *testing.T) { }{ { name: "valid S256 PKCE", - codeChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + codeChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", // S256 of "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" codeChallengeMethod: "S256", codeVerifier: "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk", wantErr: false, }, { - // OAuth 2.1 Section 4.1.1: plain is not supported. - name: "plain method rejected", + name: "valid plain PKCE", codeChallenge: "test-challenge", codeChallengeMethod: "plain", codeVerifier: "test-challenge", - wantErr: true, - errMsg: PKCEInvalidCodeMethodError, + wantErr: false, }, { name: "invalid S256 verifier", @@ -40,13 +38,12 @@ func TestVerifyPKCEChallenge(t *testing.T) { errMsg: "code challenge does not match", }, { - // plain is rejected at method level, not verifier level. - name: "plain method rejected even with wrong verifier", + name: "invalid plain verifier", codeChallenge: "test-challenge", codeChallengeMethod: "plain", codeVerifier: "wrong-challenge", wantErr: true, - errMsg: PKCEInvalidCodeMethodError, + errMsg: "code challenge does not match", }, { name: "invalid challenge method", @@ -64,13 +61,11 @@ func TestVerifyPKCEChallenge(t *testing.T) { wantErr: false, }, { - // Case-insensitive rejection. - name: "PLAIN rejected case-insensitively", + name: "case insensitive plain method", codeChallenge: "test-challenge", codeChallengeMethod: "PLAIN", codeVerifier: "test-challenge", - wantErr: true, - errMsg: PKCEInvalidCodeMethodError, + wantErr: false, }, { name: "empty verifier with S256", @@ -81,12 +76,12 @@ func TestVerifyPKCEChallenge(t *testing.T) { errMsg: "code challenge does not match", }, { - name: "plain method rejected with empty verifier", + name: "empty verifier with plain", codeChallenge: "test-challenge", codeChallengeMethod: "plain", codeVerifier: "", wantErr: true, - errMsg: PKCEInvalidCodeMethodError, + errMsg: "code challenge does not match", }, { name: "empty challenge with S256", @@ -97,26 +92,26 @@ func TestVerifyPKCEChallenge(t *testing.T) { errMsg: "code challenge does not match", }, { - name: "plain method rejected with empty challenge", + name: "empty challenge with plain", codeChallenge: "", codeChallengeMethod: "plain", codeVerifier: "test-challenge", wantErr: true, - errMsg: PKCEInvalidCodeMethodError, + errMsg: "code challenge does not match", }, { - name: "plain method rejected when both empty", + name: "both empty with plain", codeChallenge: "", codeChallengeMethod: "plain", codeVerifier: "", - wantErr: true, - errMsg: PKCEInvalidCodeMethodError, + wantErr: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := VerifyPKCEChallenge(tt.codeChallenge, tt.codeChallengeMethod, tt.codeVerifier) + if tt.wantErr { assert.Error(t, err) assert.Contains(t, err.Error(), tt.errMsg) From 8c1356473011c2c0ae20f06c4bb847d40706c804 Mon Sep 17 00:00:00 2001 From: George Lazarica Date: Mon, 3 Aug 2026 23:57:16 +0200 Subject: [PATCH 3/3] ci: trigger CI re-run