From 53f780dab8af82620ee13b0bdd377aa00d294302 Mon Sep 17 00:00:00 2001 From: tsushanth <78000697+tsushanth@users.noreply.github.com> Date: Mon, 22 Jun 2026 18:12:43 -0700 Subject: [PATCH] fix(oauthserver): make PKCE optional for confidential clients on /oauth/authorize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validatePKCEParams` unconditionally required `code_challenge` + `code_challenge_method` on the authorization endpoint, so confidential clients whose OAuth consumers don't send PKCE were rejected with `invalid_request — PKCE flow requires both code_challenge and code_challenge_method`. Reporter @gormster hit this with Microsoft Power Platform Connectors; the original discussion (#44326) was about Shopify. Both connector-style consumers authenticate to the token endpoint with the registered client_secret and never include PKCE parameters on the authorize redirect — by design. OAuth 2.1 §4.1.1 only mandates PKCE for public clients. For confidential clients the client_secret presented at /oauth/token already authenticates the code exchange, so PKCE is OPTIONAL. The fix threads the resolved `client` through `validateRemainingAuthorizeParams` so `validatePKCEParams` can branch on `client.ClientType`: - Public client, no PKCE → reject (unchanged, OAuth 2.1 mandate). - Confidential client, no PKCE → accept (the bug being fixed). - Either client type, *partial* PKCE (only one of the two fields) → reject. Half-supplied PKCE is always wrong. - Either client type, full PKCE → validate format as before (S256/plain, 43–128 char challenge). `/oauth/token`'s `OAuthServerAuthorization.VerifyPKCE` already handles the "no challenge stored" case (returns `nil`), so the token exchange side needs no changes — a confidential client that opted out at /authorize will simply skip the code_verifier check at /token. Adds three regression tests on the OAuth server authorize suite: - `TestAuthorize_ConfidentialClientWithoutPKCEAccepted` — the reporter's exact failure shape now redirects to `?authorization_id=…` instead of `?error=invalid_request&error_description=PKCE…`. - `TestAuthorize_PublicClientWithoutPKCERejected` — verifies the OAuth 2.1 mandate is still enforced for public clients. - `TestAuthorize_ConfidentialClientWithPartialPKCERejected` — verifies the half-supplied PKCE guard. Fixes #2585 --- internal/api/oauthserver/authorize.go | 28 ++++++-- internal/api/oauthserver/authorize_test.go | 83 ++++++++++++++++++++++ 2 files changed, 105 insertions(+), 6 deletions(-) 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")) +}