From f8012dc1755e5195fd94cf0264f21817bb99fc1e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 21:38:12 +0200 Subject: [PATCH 1/4] sec(auth): constant-time token check after DB fetch (closes #392) SQL equality in PostgreSQL is not constant-time; an attacker on the same VPC can use response-time differences to learn bytes of a SHA-256 hash. The admin API key path already uses subtle.ConstantTimeCompare; align the user API key and password-reset token paths to match. No schema changes: the stored hash format is unchanged. After the DB row is fetched, compare the expected hash against the stored hash via crypto/subtle.ConstantTimeCompare in ValidateUserAPIKey, validateResetToken, and ResetTokenStatus. A mismatch returns the same error as a missing row to avoid timing oracles via differing code paths. Update all test fixtures that set PasswordResetToken to literal stub values to instead use hashSessionToken(token) so they match the hash the service now verifies. Add targeted mismatch test cases to both the API-key and reset-token validation paths. --- internal/auth/service_apikeys.go | 8 +++ internal/auth/service_apikeys_test.go | 28 ++++++++++ internal/auth/service_password.go | 11 ++++ internal/auth/service_password_test.go | 73 +++++++++++++++++++++++--- internal/auth/service_test.go | 2 +- 5 files changed, 113 insertions(+), 9 deletions(-) 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_test.go b/internal/auth/service_test.go index 3e1691b0d..eb63b10be 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -631,7 +631,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, } From 28e4634c6d00c9b4b6fe68c2c332aafda400f295 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 09:01:43 +0200 Subject: [PATCH 2/4] sec(auth): constant-time session token check (extends #392 PR #837) ValidateSession had the same SQL-equality timing oracle as the user API key and password-reset token paths fixed in this PR: GetSession runs `WHERE token = $1` in PostgreSQL, which short-circuits on first mismatching byte. Session tokens are the hottest auth surface (every authenticated API request hits ValidateSession via handler.go), so the oracle is even more exploitable here than on the API key path. Add a `subtle.ConstantTimeCompare` of `session.Token` against the locally-derived `hashedToken` immediately after the store fetch, returning the same "session not found" error to keep the response indistinguishable from a missing-row outcome. `crypto/subtle` is already imported in service.go. Tests: - New `TestService_ValidateSession/constant-time mismatch rejects session even when DB row exists` asserts the guard fires when the stored token differs from the derived hash. - Fix `internal/server/adapter_test.go` fixtures that used literal stub tokens (`"hashed-token"`, `"hashed-session-token"`) to use a local `hashSessionTokenForTest` helper mirroring auth.hashSessionToken (private), so the new guard passes for the positive cases. `go test ./internal/...` -- 4223 passed, 0 failures. --- internal/auth/service.go | 7 +++++++ internal/auth/service_test.go | 29 +++++++++++++++++++++++++++++ internal/server/adapter_test.go | 21 +++++++++++++++++---- 3 files changed, 53 insertions(+), 4 deletions(-) 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_test.go b/internal/auth/service_test.go index eb63b10be..164283a31 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -190,6 +190,35 @@ 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", + Role: RoleUser, + 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) { 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), From ea9613cf67a692db245747ee24f26b2a7744b6f5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 14:42:54 -0700 Subject: [PATCH 3/4] fix(auth): drop stale Role field from Session test literal Session.Role was removed from the base branch; the constant-time mismatch test in service_test.go still referenced it, breaking go vet. Remove the field from the struct literal; the test logic is unaffected (it verifies token-hash rejection, not role checks). --- internal/auth/service_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 164283a31..3ea7d25df 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -206,7 +206,6 @@ func TestService_ValidateSession(t *testing.T) { Token: "completely-different-hash", UserID: "user-999", Email: "mismatch@example.com", - Role: RoleUser, ExpiresAt: time.Now().Add(time.Hour), } From a7702544d44b7019d7fa6d69c6af767843c59897 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 10 Jul 2026 17:02:50 +0200 Subject: [PATCH 4/4] test(auth): set KeyHash in singleflight fixture for constant-time check ValidateUserAPIKey now runs subtle.ConstantTimeCompare(key.KeyHash, keyHash) after the DB fetch. The TestValidateUserAPIKey_LastUsedSingleflight fixture omitted KeyHash, so the empty stored hash failed the compare and the function returned early before the background UpdateAPIKeyLastUsed ran, breaking the mock's Once expectation. Real DB rows always carry KeyHash, so the fixture must set it to represent the actual validated path. --- internal/auth/service_security_test.go | 1 + 1 file changed, 1 insertion(+) 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], }