diff --git a/internal/api/oauthserver/authorize.go b/internal/api/oauthserver/authorize.go index 2addacfda..8b9a20ff1 100644 --- a/internal/api/oauthserver/authorize.go +++ b/internal/api/oauthserver/authorize.go @@ -129,7 +129,7 @@ func (s *Server) OAuthServerAuthorize(w http.ResponseWriter, r *http.Request) er // From this point on, we have valid client + redirect_uri + all params, so we can redirect errors // validate all other parameters - now we can redirect errors - if err := s.validateRemainingAuthorizeParams(params); err != nil { + if err := s.validateRemainingAuthorizeParams(params, client); err != nil { errorRedirectURL := s.buildErrorRedirectURL(params.RedirectURI, oAuth2ErrorInvalidRequest, err.Error(), params.State) http.Redirect(w, r, errorRedirectURL, http.StatusFound) return nil @@ -444,7 +444,7 @@ func (s *Server) validateBasicAuthorizeParams(params *AuthorizeParams) (*Authori } // validateRemainingAuthorizeParams validates all other parameters (can redirect errors since we have valid client + redirect_uri) -func (s *Server) validateRemainingAuthorizeParams(params *AuthorizeParams) error { +func (s *Server) validateRemainingAuthorizeParams(params *AuthorizeParams, client *models.OAuthServerClient) error { if params.ResponseType == "" { params.ResponseType = models.OAuthServerResponseTypeCode.String() } @@ -468,16 +468,32 @@ func (s *Server) validateRemainingAuthorizeParams(params *AuthorizeParams) error } // PKCE validation - if err := s.validatePKCEParams(params.CodeChallengeMethod, params.CodeChallenge); err != nil { + if err := s.validatePKCEParams(params.CodeChallengeMethod, params.CodeChallenge, client); err != nil { return err } return nil } -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 +func (s *Server) validatePKCEParams(codeChallengeMethod, codeChallenge string, client *models.OAuthServerClient) error { + // PKCE is mandatory for public clients (OAuth 2.1 §4.1.1, RFC 8252 §8.1). + // For confidential clients the client_secret authenticates the token + // exchange and PKCE is OPTIONAL — many large IdP/connector consumers + // (Shopify, Microsoft Power Platform Connectors) never send a + // `code_challenge` and were rejected with `invalid_request` here. Issue #2585. + isConfidential := client != nil && client.ClientType == models.OAuthServerClientTypeConfidential + pkceProvided := codeChallenge != "" || codeChallengeMethod != "" + + if !pkceProvided { + if isConfidential { + // Confidential client opted out of PKCE — the client_secret presented + // at /oauth/token will authenticate the exchange instead. + return nil + } + return errors.New("PKCE flow requires both code_challenge and code_challenge_method") + } + + // PKCE was at least partially supplied — both halves must be present. if codeChallenge == "" || codeChallengeMethod == "" { return errors.New("PKCE flow requires both code_challenge and code_challenge_method") } diff --git a/internal/api/oauthserver/authorize_test.go b/internal/api/oauthserver/authorize_test.go index c7ec7f458..1b1bd20e4 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) } + +// Regression for https://github.com/supabase/auth/issues/2585: PKCE was +// rejected on the /oauth/authorize endpoint even for confidential clients, +// which made the OAuth server incompatible with consumers that don't send +// `code_challenge` (Microsoft Power Platform Connectors, Shopify, …) even +// though those clients authenticate with a client_secret at the token +// exchange. OAuth 2.1 §4.1.1 only mandates PKCE for public clients. +func (ts *OAuthAuthorizeTestSuite) TestAuthorize_ConfidentialClientWithoutPKCEAccepted() { + client := ts.createClient() + require.Equal(ts.T(), models.OAuthServerClientTypeConfidential, client.ClientType, + "createClient() should default to a confidential client") + + q := url.Values{ + "client_id": []string{client.ID.String()}, + "redirect_uri": []string{"https://example.com/callback"}, + "scope": []string{"openid"}, + // No code_challenge / code_challenge_method — confidential client opts out. + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+q.Encode(), nil) + w := httptest.NewRecorder() + require.NoError(ts.T(), ts.Server.OAuthServerAuthorize(w, req)) + + require.Equal(ts.T(), http.StatusFound, w.Code, "authorize: %s", w.Body.String()) + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(ts.T(), err) + require.Empty(ts.T(), loc.Query().Get("error"), + "expected no error redirect, got %s", w.Header().Get("Location")) + require.NotEmpty(ts.T(), loc.Query().Get("authorization_id"), + "expected an authorization_id, got %s", w.Header().Get("Location")) +} + +func (ts *OAuthAuthorizeTestSuite) TestAuthorize_PublicClientWithoutPKCERejected() { + publicParams := &OAuthServerClientRegisterParams{ + ClientName: "Test Public Client", + RedirectURIs: []string{"https://example.com/callback"}, + RegistrationType: "dynamic", + ClientType: models.OAuthServerClientTypePublic, + TokenEndpointAuthMethod: models.TokenEndpointAuthMethodNone, + } + publicClient, _, err := ts.Server.registerOAuthServerClient(context.Background(), publicParams) + require.NoError(ts.T(), err) + require.Equal(ts.T(), models.OAuthServerClientTypePublic, publicClient.ClientType) + + q := url.Values{ + "client_id": []string{publicClient.ID.String()}, + "redirect_uri": []string{"https://example.com/callback"}, + "scope": []string{"openid"}, + // No PKCE — must still error for public clients per OAuth 2.1. + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+q.Encode(), nil) + w := httptest.NewRecorder() + require.NoError(ts.T(), ts.Server.OAuthServerAuthorize(w, req)) + + require.Equal(ts.T(), http.StatusFound, w.Code) + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(ts.T(), err) + require.Equal(ts.T(), oAuth2ErrorInvalidRequest, loc.Query().Get("error"), + "expected invalid_request error redirect for public client missing PKCE, got %s", + w.Header().Get("Location")) + require.Contains(ts.T(), loc.Query().Get("error_description"), "PKCE flow requires") +} + +func (ts *OAuthAuthorizeTestSuite) TestAuthorize_ConfidentialClientWithPartialPKCERejected() { + client := ts.createClient() + + q := url.Values{ + "client_id": []string{client.ID.String()}, + "redirect_uri": []string{"https://example.com/callback"}, + "scope": []string{"openid"}, + "code_challenge": []string{"abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG"}, + // `code_challenge_method` deliberately omitted — half-supplied PKCE. + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+q.Encode(), nil) + w := httptest.NewRecorder() + require.NoError(ts.T(), ts.Server.OAuthServerAuthorize(w, req)) + + require.Equal(ts.T(), http.StatusFound, w.Code) + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(ts.T(), err) + require.Equal(ts.T(), oAuth2ErrorInvalidRequest, loc.Query().Get("error"), + "expected invalid_request when a confidential client provides only half of PKCE, got %s", + w.Header().Get("Location")) +}