diff --git a/internal/api/oauthserver/handlers.go b/internal/api/oauthserver/handlers.go index d219e5653..1644120c5 100644 --- a/internal/api/oauthserver/handlers.go +++ b/internal/api/oauthserver/handlers.go @@ -332,72 +332,79 @@ func (s *Server) handleAuthorizationCodeGrant(ctx context.Context, w http.Respon return apierrors.NewInternalServerError("Token service not available") } - // Find the OAuth authorization for this authorization code - authorization, err := models.FindOAuthServerAuthorizationByCode(db, params.Code) - if err != nil { - if models.IsNotFoundError(err) { - return apierrors.NewOAuthError("invalid_grant", "Invalid authorization code") + // Exchange the authorization code for tokens + var authorization *models.OAuthServerAuthorization + var user *models.User + var tokenResponse *tokens.AccessTokenResponse + var grantParams models.GrantParams + grantParams.FillGrantParams(r) + grantParams.OAuthClientID = &client.ID + + err := db.Transaction(func(tx *storage.Connection) error { + // Find and lock the OAuth authorization for this authorization code. + var terr error + authorization, terr = models.FindOAuthServerAuthorizationByCode(tx, params.Code, true) + if terr != nil { + if models.IsNotFoundError(terr) { + return apierrors.NewOAuthError("invalid_grant", "Invalid authorization code") + } + return apierrors.NewInternalServerError("Error finding authorization code").WithInternalError(terr) } - return apierrors.NewInternalServerError("Error finding authorization code").WithInternalError(err) - } - // Check if the authorization has expired - if authorization.IsExpired() { - return apierrors.NewOAuthError("invalid_grant", "Authorization code has expired") - } + if authorization.AuthorizationCode == nil || *authorization.AuthorizationCode != params.Code || authorization.Status != models.OAuthServerAuthorizationApproved { + return apierrors.NewOAuthError("invalid_grant", "Invalid authorization code") + } - // Validate that the authorization code was issued for this client - if authorization.ClientID != client.ID { - return apierrors.NewOAuthError("invalid_grant", "Authorization code was not issued for this client") - } + // Check if the authorization has expired + if authorization.IsExpired() { + return apierrors.NewOAuthError("invalid_grant", "Authorization code has expired") + } - // Validate that (if exists) the resource parameter matches the authorization code resource - if params.Resource != "" && params.Resource != utilities.StringValue(authorization.Resource) { - return apierrors.NewOAuthError("invalid_grant", "Authorization code resource does not match the resource parameter") - } + // Validate that the authorization code was issued for this client + if authorization.ClientID != client.ID { + return apierrors.NewOAuthError("invalid_grant", "Authorization code was not issued for this client") + } - // Validate redirect_uri if provided - must match the one used in authorization - if params.RedirectURI != "" && params.RedirectURI != authorization.RedirectURI { - return apierrors.NewOAuthError("invalid_grant", "Invalid redirect_uri") - } + // Validate that (if exists) the resource parameter matches the authorization code resource + if params.Resource != "" && params.Resource != utilities.StringValue(authorization.Resource) { + return apierrors.NewOAuthError("invalid_grant", "Authorization code resource does not match the resource parameter") + } - // Validate PKCE if used in the authorization - if err := authorization.VerifyPKCE(params.CodeVerifier); err != nil { - return apierrors.NewOAuthError("invalid_grant", "PKCE verification failed: "+err.Error()) - } + // Validate redirect_uri if provided - must match the one used in authorization + if params.RedirectURI != "" && params.RedirectURI != authorization.RedirectURI { + return apierrors.NewOAuthError("invalid_grant", "Invalid redirect_uri") + } - // Get the user for the authorization code - if authorization.UserID == nil { - return apierrors.NewOAuthError("invalid_grant", "Authorization code has no associated user") - } + // Validate PKCE if used in the authorization + if terr := authorization.VerifyPKCE(params.CodeVerifier); terr != nil { + return apierrors.NewOAuthError("invalid_grant", "PKCE verification failed: "+terr.Error()) + } - user, err := models.FindUserByID(db, *authorization.UserID) - if err != nil { - if models.IsNotFoundError(err) { - return apierrors.NewOAuthError("invalid_grant", "User not found for authorization code") + // Get the user for the authorization code + if authorization.UserID == nil { + return apierrors.NewOAuthError("invalid_grant", "Authorization code has no associated user") } - return apierrors.NewInternalServerError("Error finding user").WithInternalError(err) - } - if user.IsBanned() { - return apierrors.NewOAuthError("access_denied", "User is banned") - } + user, terr = models.FindUserByID(tx, *authorization.UserID) + if terr != nil { + if models.IsNotFoundError(terr) { + return apierrors.NewOAuthError("invalid_grant", "User not found for authorization code") + } + return apierrors.NewInternalServerError("Error finding user").WithInternalError(terr) + } - // Exchange the authorization code for tokens - var tokenResponse *tokens.AccessTokenResponse - var grantParams models.GrantParams - grantParams.FillGrantParams(r) - grantParams.OAuthClientID = &client.ID + if user.IsBanned() { + return apierrors.NewOAuthError("access_denied", "User is banned") + } - // Store scopes from authorization in session - scopes := authorization.Scope - grantParams.Scopes = &scopes + // Store scopes from authorization in session + scopes := authorization.Scope + grantParams.Scopes = &scopes - err = db.Transaction(func(tx *storage.Connection) error { authMethod := models.OAuthProviderAuthorizationCode // Create audit log entry for OAuth token exchange - if terr := models.NewAuditLogEntry(s.config.AuditLog, r, tx, user, models.LoginAction, "", map[string]interface{}{ + if terr = models.NewAuditLogEntry(s.config.AuditLog, r, tx, user, models.LoginAction, "", map[string]interface{}{ "provider_type": "oauth_provider_authorization_code", "client_id": client.ID.String(), }); terr != nil { @@ -405,7 +412,6 @@ func (s *Server) handleAuthorizationCodeGrant(ctx context.Context, w http.Respon } // Issue the refresh token and access token - var terr error tokenResponse, terr = tokenService.IssueRefreshToken(r, w.Header(), tx, user, authMethod, grantParams) if terr != nil { return terr @@ -424,6 +430,9 @@ func (s *Server) handleAuthorizationCodeGrant(ctx context.Context, w http.Respon if httpErr, ok := err.(*apierrors.HTTPError); ok { return httpErr } + if oauthErr, ok := err.(*apierrors.OAuthError); ok { + return oauthErr + } return apierrors.NewInternalServerError("Error exchanging authorization code").WithInternalError(err) } diff --git a/internal/api/oauthserver/handlers_test.go b/internal/api/oauthserver/handlers_test.go index 3aef32f54..2e60f3967 100644 --- a/internal/api/oauthserver/handlers_test.go +++ b/internal/api/oauthserver/handlers_test.go @@ -6,12 +6,16 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "net/url" + "sync" "testing" + "time" "github.com/gofrs/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/supabase/auth/internal/api/apierrors" "github.com/supabase/auth/internal/api/shared" "github.com/supabase/auth/internal/conf" "github.com/supabase/auth/internal/conf/confload" @@ -540,6 +544,89 @@ func (ts *OAuthClientTestSuite) TestHandlerValidation() { assert.Contains(ts.T(), err.Error(), "invalid redirect_uri") } +func (ts *OAuthClientTestSuite) TestAuthorizationCodeGrantConcurrentReplay() { + client, _ := ts.createTestOAuthClient() + user := ts.createTestUser("oauth-code-replay@example.com") + codeVerifier := "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG" + + authorization := models.NewOAuthServerAuthorization(models.NewOAuthServerAuthorizationParams{ + ClientID: client.ID, + RedirectURI: "https://example.com/callback", + Scope: "email", + CodeChallenge: codeVerifier, + CodeChallengeMethod: "plain", + TTL: time.Hour, + }) + require.NoError(ts.T(), models.CreateOAuthServerAuthorization(ts.DB, authorization)) + require.NoError(ts.T(), authorization.SetUser(ts.DB, user.ID)) + require.NoError(ts.T(), authorization.Approve(ts.DB)) + require.NotNil(ts.T(), authorization.AuthorizationCode) + + const attempts = 8 + start := make(chan struct{}) + type tokenResult struct { + recorder *httptest.ResponseRecorder + err error + } + results := make(chan tokenResult, attempts) + + var wg sync.WaitGroup + wg.Add(attempts) + for i := 0; i < attempts; i++ { + go func() { + defer wg.Done() + <-start + + form := url.Values{} + form.Set("grant_type", GrantTypeAuthorizationCode) + form.Set("code", *authorization.AuthorizationCode) + form.Set("redirect_uri", authorization.RedirectURI) + form.Set("code_verifier", codeVerifier) + + req := httptest.NewRequest(http.MethodPost, "/oauth/token", bytes.NewBufferString(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req = req.WithContext(shared.WithOAuthServerClient(req.Context(), client)) + + w := httptest.NewRecorder() + if err := ts.Server.OAuthToken(w, req); err != nil { + if oauthErr, ok := err.(*apierrors.OAuthError); ok { + err = shared.SendJSON(w, http.StatusBadRequest, oauthErr) + results <- tokenResult{recorder: w, err: err} + return + } + results <- tokenResult{recorder: w, err: err} + return + } + results <- tokenResult{recorder: w} + }() + } + + close(start) + wg.Wait() + close(results) + + successes := 0 + failures := 0 + for result := range results { + require.NoError(ts.T(), result.err) + w := result.recorder + switch w.Code { + case http.StatusOK: + successes++ + case http.StatusBadRequest: + failures++ + var body apierrors.OAuthError + require.NoError(ts.T(), json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(ts.T(), "invalid_grant", body.Err) + default: + ts.T().Fatalf("unexpected status %d with body %s", w.Code, w.Body.String()) + } + } + + assert.Equal(ts.T(), 1, successes) + assert.Equal(ts.T(), attempts-1, failures) +} + // Helper function to create a test user func (ts *OAuthClientTestSuite) createTestUser(email string) *models.User { user, err := models.NewUser("", email, "password123", "authenticated", nil) diff --git a/internal/models/oauth_authorization.go b/internal/models/oauth_authorization.go index 2b7fab257..9188f8e8e 100644 --- a/internal/models/oauth_authorization.go +++ b/internal/models/oauth_authorization.go @@ -269,14 +269,29 @@ func FindOAuthServerAuthorizationByIDForUpdate(tx *storage.Connection, authoriza return auth, nil } -// FindOAuthServerAuthorizationByCode finds an OAuth authorization by authorization code -func FindOAuthServerAuthorizationByCode(tx *storage.Connection, code string) (*OAuthServerAuthorization, error) { +// FindOAuthServerAuthorizationByCode finds an OAuth authorization by +// authorization code. If forUpdate is true, the row is locked with FOR UPDATE +// SKIP LOCKED and this must be called inside a transaction. +func FindOAuthServerAuthorizationByCode(tx *storage.Connection, code string, forUpdate bool) (*OAuthServerAuthorization, error) { auth := &OAuthServerAuthorization{} - if err := tx.Q().Where("authorization_code = ? AND status = ?", code, OAuthServerAuthorizationApproved).First(auth); err != nil { - if errors.Cause(err) == sql.ErrNoRows { - return nil, OAuthServerAuthorizationNotFoundError{} + if forUpdate { + if err := tx.RawQuery( + fmt.Sprintf("SELECT * FROM %q WHERE authorization_code = ? AND status = ? LIMIT 1 FOR UPDATE SKIP LOCKED", auth.TableName()), + code, + OAuthServerAuthorizationApproved, + ).First(auth); err != nil { + if errors.Cause(err) == sql.ErrNoRows { + return nil, OAuthServerAuthorizationNotFoundError{} + } + return nil, errors.Wrap(err, "error finding OAuth authorization by code") + } + } else { + if err := tx.Q().Where("authorization_code = ? AND status = ?", code, OAuthServerAuthorizationApproved).First(auth); err != nil { + if errors.Cause(err) == sql.ErrNoRows { + return nil, OAuthServerAuthorizationNotFoundError{} + } + return nil, errors.Wrap(err, "error finding OAuth authorization by code") } - return nil, errors.Wrap(err, "error finding OAuth authorization by code") } // Load client relationship (always present)