diff --git a/internal/auth/service.go b/internal/auth/service.go index 119fc0b86..be9a47419 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -319,6 +319,13 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session, return nil, fmt.Errorf("session not found") } + // Constant-time comparison after fetch to close the SQL-equality timing oracle. + // GetSession does `WHERE token = $1` which is not constant-time in PostgreSQL; + // align with ValidateUserAPIKey / validateResetToken (issue #392 PR #837). + if subtle.ConstantTimeCompare([]byte(session.Token), []byte(hashedToken)) != 1 { + return nil, fmt.Errorf("session not found") + } + if session.ExpiresAt.IsZero() { return nil, fmt.Errorf("session has no expiry: data integrity error") } diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index 789c01fc6..3f36f5f39 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rand" "crypto/sha256" + "crypto/subtle" "encoding/base64" "errors" "fmt" @@ -265,6 +266,13 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA return nil, nil, fmt.Errorf("invalid API key") } + // Constant-time comparison after fetch to close the SQL-equality timing oracle. + // The DB lookup already found the row by hash; this redundant check ensures the + // hot path does not leak timing information through early return differences. + if subtle.ConstantTimeCompare([]byte(key.KeyHash), []byte(keyHash)) != 1 { + return nil, nil, fmt.Errorf("invalid API key") + } + err = validateAPIKeyStatus(key) if err != nil { return nil, nil, err diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index 401e718d3..d8fc2f5f7 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -651,6 +651,34 @@ func TestService_ValidateUserAPIKey(t *testing.T) { assert.Nil(t, resultKey) mockStore.AssertExpectations(t) }) + + t.Run("reject when stored hash does not match derived hash", func(t *testing.T) { + // Simulates a DB row whose key_hash field was tampered or + // belongs to a different key -- the constant-time check must reject it. + mockStore := new(MockStore) + service := &Service{store: mockStore} + + apiKey := "test-api-key-123456" + hash := sha256.Sum256([]byte(apiKey)) + keyHash := base64.RawURLEncoding.EncodeToString(hash[:]) + + tamperedRecord := &UserAPIKey{ + ID: "key-1", + UserID: "user-123", + KeyHash: keyHash + "tampered", + IsActive: true, + } + + mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(tamperedRecord, nil) + + resultKey, resultUser, err := service.ValidateUserAPIKey(ctx, apiKey) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid API key") + assert.Nil(t, resultUser) + assert.Nil(t, resultKey) + mockStore.AssertExpectations(t) + }) } func TestService_UpdateLastUsed(t *testing.T) { diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index def996e78..9bc5993b5 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -2,6 +2,7 @@ package auth import ( "context" + "crypto/subtle" "errors" "fmt" "strings" @@ -419,6 +420,11 @@ func (s *Service) ResetTokenStatus(ctx context.Context, token string) (ResetToke return ResetTokenStateUsed, ResetTokenFlowReset, nil } + // Constant-time comparison after fetch to close the SQL-equality timing oracle. + if subtle.ConstantTimeCompare([]byte(user.PasswordResetToken), []byte(tokenHash)) != 1 { + return ResetTokenStateUsed, ResetTokenFlowReset, nil + } + if user.PasswordResetExpiry == nil || time.Now().After(*user.PasswordResetExpiry) { flow := ResetTokenFlowReset if !user.Active { @@ -448,6 +454,11 @@ func (s *Service) validateResetToken(ctx context.Context, token string) (*User, return nil, fmt.Errorf("invalid or expired reset token") } + // Constant-time comparison after fetch to close the SQL-equality timing oracle. + if subtle.ConstantTimeCompare([]byte(user.PasswordResetToken), []byte(tokenHash)) != 1 { + return nil, fmt.Errorf("invalid or expired reset token") + } + if user.PasswordResetExpiry == nil || time.Now().After(*user.PasswordResetExpiry) { return nil, fmt.Errorf("reset token has expired") } diff --git a/internal/auth/service_password_test.go b/internal/auth/service_password_test.go index d8ec3d878..b325ae4ab 100644 --- a/internal/auth/service_password_test.go +++ b/internal/auth/service_password_test.go @@ -309,7 +309,7 @@ func TestService_ConfirmPasswordReset(t *testing.T) { testUser := &User{ ID: "user-123", Email: "test@example.com", - PasswordResetToken: "hashed-token", // Now stores the hash + PasswordResetToken: hashSessionToken("valid-reset-token"), PasswordResetExpiry: &expiry, Active: true, } @@ -360,7 +360,7 @@ func TestService_ConfirmPasswordReset(t *testing.T) { expiredUser := &User{ ID: "user-456", Email: "expired@example.com", - PasswordResetToken: "hashed-expired-token", + PasswordResetToken: hashSessionToken("expired-reset-token"), PasswordResetExpiry: &expiry, Active: true, } @@ -389,7 +389,7 @@ func TestService_ConfirmPasswordReset(t *testing.T) { testUser := &User{ ID: "user-123", Email: "test@example.com", - PasswordResetToken: "hashed-token", + PasswordResetToken: hashSessionToken("valid-reset-token"), PasswordResetExpiry: &expiry, Active: true, } @@ -422,7 +422,7 @@ func TestService_ConfirmPasswordReset(t *testing.T) { testUser := &User{ ID: "user-123", Email: "test@example.com", - PasswordResetToken: "hashed-token", + PasswordResetToken: hashSessionToken("valid-reset-token"), PasswordResetExpiry: &expiry, PasswordHistory: []string{hash}, Active: true, @@ -462,7 +462,7 @@ func TestService_ConfirmPasswordReset(t *testing.T) { ID: "user-123", Email: "test@example.com", PasswordHash: currentHash, - PasswordResetToken: "hashed-token", + PasswordResetToken: hashSessionToken("valid-reset-token"), PasswordResetExpiry: &expiry, Active: true, } @@ -483,6 +483,37 @@ func TestService_ConfirmPasswordReset(t *testing.T) { mockStore.AssertExpectations(t) }) + + t.Run("constant-time mismatch rejects token even when DB row exists", func(t *testing.T) { + // The DB returned a user but the stored reset-token hash does not match + // the hash we derived from the supplied token string. The constant-time + // guard must reject the request to avoid timing-based disclosure. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + expiry := time.Now().Add(time.Hour) + mismatchUser := &User{ + ID: "user-999", + Email: "mismatch@example.com", + PasswordResetToken: "completely-different-hash", + PasswordResetExpiry: &expiry, + Active: true, + } + + mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")).Return(mismatchUser, nil).Once() + + req := PasswordResetConfirm{ + Token: "some-token", + NewPassword: "SecureT3st@789", + } + + err := service.ConfirmPasswordReset(ctx, req) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid or expired reset token") + + mockStore.AssertExpectations(t) + }) } // TestService_ResetTokenStatus covers the read-only token-status probe @@ -498,7 +529,7 @@ func TestService_ResetTokenStatus(t *testing.T) { expiry := time.Now().Add(time.Hour) mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")). - Return(&User{ID: "u1", Active: true, PasswordResetExpiry: &expiry}, nil).Once() + Return(&User{ID: "u1", Active: true, PasswordResetToken: hashSessionToken("valid-token"), PasswordResetExpiry: &expiry}, nil).Once() state, flow, err := service.ResetTokenStatus(ctx, "valid-token") require.NoError(t, err) @@ -515,7 +546,7 @@ func TestService_ResetTokenStatus(t *testing.T) { expiry := time.Now().Add(time.Hour) mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")). - Return(&User{ID: "u2", Active: false, PasswordResetExpiry: &expiry}, nil).Once() + Return(&User{ID: "u2", Active: false, PasswordResetToken: hashSessionToken("valid-invite-token"), PasswordResetExpiry: &expiry}, nil).Once() state, flow, err := service.ResetTokenStatus(ctx, "valid-invite-token") require.NoError(t, err) @@ -532,7 +563,7 @@ func TestService_ResetTokenStatus(t *testing.T) { expiry := time.Now().Add(-time.Hour) mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")). - Return(&User{ID: "u3", Active: true, PasswordResetExpiry: &expiry}, nil).Once() + Return(&User{ID: "u3", Active: true, PasswordResetToken: hashSessionToken("expired-token"), PasswordResetExpiry: &expiry}, nil).Once() state, flow, err := service.ResetTokenStatus(ctx, "expired-token") require.NoError(t, err) @@ -589,6 +620,32 @@ func TestService_ResetTokenStatus(t *testing.T) { mockStore.AssertExpectations(t) }) + + t.Run("constant-time mismatch returns used even if DB row exists", func(t *testing.T) { + // The DB returned a row but the stored token hash does not match the + // hash derived from the supplied token. The constant-time check must + // treat this as a missing/consumed token rather than branching on + // expiry state, to avoid leaking information through timing. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + expiry := time.Now().Add(time.Hour) + mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")). + Return(&User{ + ID: "u9", + Active: true, + PasswordResetToken: "different-hash-entirely", + PasswordResetExpiry: &expiry, + }, nil).Once() + + state, flow, err := service.ResetTokenStatus(ctx, "some-token") + require.NoError(t, err) + assert.Equal(t, ResetTokenStateUsed, state) + assert.Equal(t, ResetTokenFlowReset, flow) + + mockStore.AssertExpectations(t) + }) } // Test password validation rules. diff --git a/internal/auth/service_security_test.go b/internal/auth/service_security_test.go index a807b0c93..6cfa3792a 100644 --- a/internal/auth/service_security_test.go +++ b/internal/auth/service_security_test.go @@ -244,6 +244,7 @@ func TestValidateUserAPIKey_LastUsedSingleflight(t *testing.T) { ID: keyID, UserID: user.ID, IsActive: true, + KeyHash: keyHash, KeyPrefix: rawKey[:8], } diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 3e1691b0d..3ea7d25df 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -190,6 +190,34 @@ func TestService_ValidateSession(t *testing.T) { mockStore.AssertExpectations(t) }) + + t.Run("constant-time mismatch rejects session even when DB row exists", func(t *testing.T) { + // The DB returned a session row but its stored token hash does not + // match the hash we derived from the supplied token string. The + // constant-time guard (added alongside the API-key and reset-token + // equivalents in PR #837) must reject the request to avoid leaking + // timing information through the SQL equality oracle. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + hashedToken := hashSessionToken("some-token") + mismatchSession := &Session{ + Token: "completely-different-hash", + UserID: "user-999", + Email: "mismatch@example.com", + ExpiresAt: time.Now().Add(time.Hour), + } + + mockStore.On("GetSession", ctx, hashedToken).Return(mismatchSession, nil).Once() + + session, err := service.ValidateSession(ctx, "some-token") + assert.Error(t, err) + assert.Nil(t, session) + assert.Contains(t, err.Error(), "session not found") + + mockStore.AssertExpectations(t) + }) } func TestService_Logout(t *testing.T) { @@ -631,7 +659,7 @@ func TestService_ErrorPaths(t *testing.T) { testUser := &User{ ID: "user-123", Email: "test@example.com", - PasswordResetToken: "hashed-token", + PasswordResetToken: hashSessionToken("valid-reset-token"), PasswordResetExpiry: &expiry, Active: true, } diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index 1e8b54dbb..877b90f64 100644 --- a/internal/server/adapter_test.go +++ b/internal/server/adapter_test.go @@ -2,6 +2,8 @@ package server import ( "context" + "crypto/sha256" + "encoding/hex" "testing" "time" @@ -12,6 +14,15 @@ import ( "github.com/stretchr/testify/require" ) +// hashSessionTokenForTest mirrors auth.hashSessionToken (private). Session +// tokens are stored as sha256+hex so the constant-time guard in +// auth.Service.ValidateSession matches when both sides are derived from the +// same raw token. +func hashSessionTokenForTest(token string) string { + h := sha256.Sum256([]byte(token)) + return hex.EncodeToString(h[:]) +} + func createMockAuthService(t *testing.T) (*authServiceAdapter, *auth.MockStore) { t.Helper() mockStore := new(auth.MockStore) @@ -62,9 +73,10 @@ func TestAuthServiceAdapter_ValidateSession(t *testing.T) { adapter, mockStore := createMockAuthService(t) ctx := context.Background() - // Token gets hashed, use mock.Anything + // Token gets hashed; the stored Token must match hashSessionToken("valid-token") + // so the constant-time guard in ValidateSession (PR #837 follow-up) succeeds. mockStore.On("GetSession", ctx, mock.AnythingOfType("string")).Return(&auth.Session{ - Token: "hashed-token", + Token: hashSessionTokenForTest("valid-token"), UserID: "user-1", Email: "test@example.com", ExpiresAt: time.Now().Add(time.Hour), @@ -235,9 +247,10 @@ func TestAuthServiceAdapter_ValidateCSRFToken(t *testing.T) { adapter, mockStore := createMockAuthService(t) ctx := context.Background() - // Token gets hashed, so use mock.Anything + // Token gets hashed; the stored Token must match hashSessionToken("session-token") + // so the constant-time guard in ValidateSession (PR #837 follow-up) succeeds. mockStore.On("GetSession", ctx, mock.AnythingOfType("string")).Return(&auth.Session{ - Token: "hashed-session-token", + Token: hashSessionTokenForTest("session-token"), UserID: "user-1", CSRFToken: "csrf-token", ExpiresAt: time.Now().Add(time.Hour),