Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions internal/api/oauthserver/authorize.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
Expand All @@ -468,18 +468,24 @@ 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 in the authorization code flow
// For confidential clients, it is optional.
if codeChallenge == "" || codeChallengeMethod == "" {
return errors.New("PKCE flow requires both code_challenge and code_challenge_method")
if client.IsPublic() {
return errors.New("PKCE flow requires both code_challenge and code_challenge_method for public clients")
}
if codeChallenge != "" || codeChallengeMethod != "" {
return errors.New("PKCE flow requires both code_challenge and code_challenge_method")
}
return nil
}

// Validate code challenge method (case-insensitive)
Expand Down
49 changes: 49 additions & 0 deletions internal/api/oauthserver/authorize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -666,3 +666,52 @@ func (ts *OAuthAuthorizeTestSuite) TestConsent_InvalidActionRejected() {
err := ts.Server.OAuthServerConsent(w, req)
ts.assertHTTPError(err, http.StatusBadRequest, apierrors.ErrorCodeValidationFailed)
}

func (ts *OAuthAuthorizeTestSuite) TestAuthorize_ConfidentialClientWithoutPKCE() {
client := ts.createClient() // confidential by default
q := url.Values{
"client_id": []string{client.ID.String()},
"redirect_uri": []string{"https://example.com/callback"},
"scope": []string{"openid"},
"response_type": []string{"code"},
}
req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+q.Encode(), nil)
w := httptest.NewRecorder()
require.NoError(ts.T(), ts.Server.OAuthServerAuthorize(w, req))
assert.Equal(ts.T(), http.StatusFound, w.Code)

loc, err := url.Parse(w.Header().Get("Location"))
require.NoError(ts.T(), err)
authID := loc.Query().Get("authorization_id")
assert.NotEmpty(ts.T(), authID)
}

func (ts *OAuthAuthorizeTestSuite) TestAuthorize_PublicClientWithoutPKCE() {
// Create a public client
params := &OAuthServerClientRegisterParams{
ClientName: "Test Public Client",
RedirectURIs: []string{"https://example.com/callback"},
RegistrationType: "dynamic",
ClientType: models.OAuthServerClientTypePublic,
TokenEndpointAuthMethod: models.TokenEndpointAuthMethodNone,
}
client, _, err := ts.Server.registerOAuthServerClient(context.Background(), params)
require.NoError(ts.T(), err)

q := url.Values{
"client_id": []string{client.ID.String()},
"redirect_uri": []string{"https://example.com/callback"},
"scope": []string{"openid"},
"response_type": []string{"code"},
}
req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+q.Encode(), nil)
w := httptest.NewRecorder()
require.NoError(ts.T(), ts.Server.OAuthServerAuthorize(w, req))
assert.Equal(ts.T(), http.StatusFound, w.Code)

loc, err := url.Parse(w.Header().Get("Location"))
require.NoError(ts.T(), err)
assert.Equal(ts.T(), oAuth2ErrorInvalidRequest, loc.Query().Get("error"))
assert.Contains(ts.T(), loc.Query().Get("error_description"), "PKCE flow requires both code_challenge and code_challenge_method")
}