sec(auth): constant-time token validation via subtle.ConstantTimeCompare (closes #392) (#837)

* 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.

* 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.

* 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).

* 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.

Author: cristim

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")
 	}

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

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) {

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")
 	}

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.

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],
 	}
 

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,
 		}

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),