diff --git a/internal/api/jwks.go b/internal/api/jwks.go index 0ca0bb4b6..0bddc9841 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/api/oauthserver/authorize.go b/internal/api/oauthserver/authorize.go index 2addacfda..4ce97d41d 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 c7ec7f458..9934a20ad 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/models/oauth_authorization.go b/internal/models/oauth_authorization.go index 2b7fab257..5313beb9d 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 }