From a70647d797ec81120eef6ff87c4d7174e0a14c3e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:03:53 -0700 Subject: [PATCH 01/11] sec(auth): verifyTOTP and consumeRecoveryCode fail closed on empty input (#1018) verifyTOTP("","") previously returned true because generateTOTP("") returns "" on base32-decode failure, and ConstantTimeCompare("","") == 1. This created an MFA-bypass primitive callable by any session holder via MFARegenerateRecoveryCodes with code="". - verifyTOTP: early-return false when code=="" or secret=="" - consumeRecoveryCode: early-return false when normalized input is empty - Fix misleading comment at verifyPasswordAndMFA (service.go:212) that described the old panic-safe behaviour; the function now fails closed Closes #1018 --- internal/auth/service.go | 60 ++++++++++++++++++++++++++++-------- internal/auth/service_mfa.go | 39 ++++++++++++++++++----- 2 files changed, 79 insertions(+), 20 deletions(-) diff --git a/internal/auth/service.go b/internal/auth/service.go index bf2ca9f80..1cdc9c39d 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -2,6 +2,7 @@ package auth import ( "context" + "crypto/rand" "crypto/subtle" "fmt" "net/mail" @@ -9,6 +10,7 @@ import ( "time" "github.com/LeanerCloud/CUDly/pkg/logging" + "golang.org/x/sync/singleflight" ) // Configuration constants @@ -41,6 +43,15 @@ type Service struct { dashboardURL string bcryptCostOverride int // if > 0, overrides bcryptCost const (used by tests for speed) onPasswordChange func(ctx context.Context, userID, newPassword string) + // csrfKey is the server-side key used to derive CSRF tokens as + // HMAC-SHA256(csrfKey, rawSessionToken). Tokens are never stored + // in cleartext; validation recomputes the HMAC and compares. + // A random key is generated at NewService time when not supplied. + csrfKey []byte + // lastUsedSFG deduplicates concurrent UpdateLastUsed calls for the + // same API key so a burst of authenticated requests does not spawn + // an unbounded number of goroutines. + lastUsedSFG singleflight.Group } // ServiceConfig holds configuration for the auth service @@ -50,6 +61,11 @@ type ServiceConfig struct { SessionDuration time.Duration DashboardURL string OnPasswordChange func(ctx context.Context, userID, newPassword string) + // CSRFKey is the server-side secret used to derive CSRF tokens as + // HMAC-SHA256(CSRFKey, rawSessionToken). Must be 32 bytes for + // 256-bit security. When empty, NewService generates a random key and + // logs a warning; all existing sessions will require re-login on restart. + CSRFKey []byte } // NewService creates a new auth service @@ -83,12 +99,26 @@ func NewService(cfg ServiceConfig) *Service { } } + csrfKey := cfg.CSRFKey + if len(csrfKey) == 0 { + // No key supplied: generate a random ephemeral key. CSRF tokens issued + // before a restart will become invalid (re-login required). Operators + // should supply a stable key via CSRFKey to avoid this on restarts. + logging.Warnf("auth: CSRFKey not set — generating ephemeral key; all CSRF tokens will be invalidated on service restart. Set CSRFKey to a stable 32-byte value.") + csrfKey = make([]byte, 32) + if _, err := rand.Read(csrfKey); err != nil { + // rand.Read only fails on catastrophic OS errors; panic is appropriate. + panic(fmt.Sprintf("auth: failed to generate ephemeral CSRF key: %v", err)) + } + } + return &Service{ store: cfg.Store, emailSender: cfg.EmailSender, sessionDuration: cfg.SessionDuration, dashboardURL: cfg.DashboardURL, onPasswordChange: cfg.OnPasswordChange, + csrfKey: csrfKey, } } @@ -209,8 +239,8 @@ func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req Logi logging.Errorf("MFA enabled but secret missing for user %s -- possible data integrity issue", user.ID) return fmt.Errorf("Check your email address and password and try again") } - // verifyTOTP is panic-safe: a malformed secret causes generateTOTP to return "" - // (base32 decode error), resulting in a comparison miss rather than a panic. + // verifyTOTP fails closed on empty or malformed inputs: empty code, empty + // secret, and base32-decode errors all return false rather than a match. if verifyTOTP(user.MFASecret, req.MFACode) { return nil } @@ -312,26 +342,30 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session, return &result, nil } -// ValidateCSRFToken validates the CSRF token for a session +// ValidateCSRFToken validates the CSRF token for a session. +// +// The expected CSRF token is derived as HMAC-SHA256(csrfKey, rawSessionToken), +// matching the token produced by createSession. Validation never reads the +// stored csrf_token column; it recomputes the MAC from the raw session token +// so a database read (SQLi, backup, replica) cannot yield a usable CSRF token. func (s *Service) ValidateCSRFToken(ctx context.Context, sessionToken, csrfToken string) error { if csrfToken == "" { return fmt.Errorf("CSRF token is required") } - session, err := s.ValidateSession(ctx, sessionToken) - if err != nil { + // ValidateSession is called for its side-effects (expiry check, session + // existence) and to obtain the raw session token for HMAC derivation. + // The session object itself is not used for the CSRF comparison. + if _, err := s.ValidateSession(ctx, sessionToken); err != nil { return fmt.Errorf("invalid session: %w", err) } - if session.CSRFToken == "" { - // Security: Reject sessions without CSRF tokens instead of allowing bypass - // Legacy sessions must re-authenticate to get a proper CSRF token - logging.Warnf("Rejecting session without CSRF token") - return fmt.Errorf("session requires re-authentication for CSRF protection") - } + // Derive the expected CSRF token from the raw session token and the + // server-side key. This is identical to what createSession produced. + expected := deriveCSRFToken(s.csrfKey, sessionToken) - // Use constant-time comparison to prevent timing attacks - if subtle.ConstantTimeCompare([]byte(session.CSRFToken), []byte(csrfToken)) != 1 { + // Use constant-time comparison to prevent timing attacks. + if subtle.ConstantTimeCompare([]byte(expected), []byte(csrfToken)) != 1 { return fmt.Errorf("invalid CSRF token") } diff --git a/internal/auth/service_mfa.go b/internal/auth/service_mfa.go index 2a42d57d3..2dc34d0c7 100644 --- a/internal/auth/service_mfa.go +++ b/internal/auth/service_mfa.go @@ -46,21 +46,38 @@ const recoveryCodeLength = 8 // shown to humans once and typed back, so visual clarity matters. const recoveryCodeAlphabet = "ABCDEFGHJKMNPQRSTUVWXYZ23456789" -// verifyTOTP validates a TOTP code against the secret -// Implements RFC 6238 TOTP with 30-second time steps -// Uses constant-time comparison to prevent timing attacks +// verifyTOTP validates a TOTP code against the secret. +// Implements RFC 6238 TOTP with 30-second time steps. +// Uses constant-time comparison to prevent timing attacks. +// +// Fails closed: returns false immediately when code or secret is empty. +// An empty code can arrive from a missing form field; an empty secret signals +// a data-integrity anomaly (MFA enabled but no secret stored). In both cases +// the correct behavior is to reject, not to accidentally match because +// generateTOTP("","") returns "" and ConstantTimeCompare("","") == 1. func verifyTOTP(secret, code string) bool { - // Allow for time skew by checking current and adjacent time windows + // Fail closed: reject empty code and empty secret up front. + // generateTOTP returns "" on base32-decode failure; ConstantTimeCompare("","") + // evaluates to 1 and would bypass MFA for any caller that passes code="". + if code == "" { + return false + } + if secret == "" { + return false + } + + // Allow for time skew by checking current and adjacent time windows. currentTime := time.Now().Unix() timeStep := int64(config.MFATimeStep) - // Check current time step and one step before/after for clock skew tolerance - // Use constant-time comparison and avoid early returns to prevent timing attacks + // Check current time step and one step before/after for clock skew tolerance. + // Use constant-time comparison and avoid early returns to prevent timing attacks. valid := 0 for _, offset := range []int64{-1, 0, 1} { counter := (currentTime / timeStep) + offset expected := generateTOTP(secret, counter) - // Use constant-time comparison - bitwise OR accumulates matches + // generateTOTP returns "" on base32-decode failure; a non-empty code can + // never equal "" so a bad secret produces no match. if subtle.ConstantTimeCompare([]byte(expected), []byte(code)) == 1 { valid = 1 } @@ -207,6 +224,10 @@ func (s *Service) hashRecoveryCode(code string) (string, error) { // removal is what makes recovery codes single-use; the caller must // persist the user afterwards via UpdateUser. // +// Fails closed on empty input: an empty string normalizes to "", which +// bcrypt.CompareHashAndPassword would compare against every stored hash. +// Reject it immediately to avoid leaking which hashes exist. +// // Iterates over all stored hashes (bcrypt compare is constant-time // for the matching hash but not across the slice — recovery codes // are 8 chars of high-entropy alphabet, so timing leakage of which @@ -214,6 +235,10 @@ func (s *Service) hashRecoveryCode(code string) (string, error) { // branch-free by ORing the bool into a single accumulator). func (s *Service) consumeRecoveryCode(user *User, entered string) bool { normalized := normalizeRecoveryCode(entered) + // Fail closed: reject empty input before any bcrypt comparison. + if normalized == "" { + return false + } matchIdx := -1 for i, hash := range user.MFARecoveryCodes { if err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(normalized)); err == nil { From 44f91f263f642eda30194f5bfd2796292419a9e1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:04:05 -0700 Subject: [PATCH 02/11] fix(auth): bound API key last-used updates via singleflight (#1027) ValidateUserAPIKey fired one unbounded goroutine per request to update the last-used timestamp. Under a burst of authenticated requests (or a revoked key being presented repeatedly), this spawned an unbounded number of goroutines writing to the DB with context.Background() and logged a Warn for every "key not found" event. - Replace raw go func() with singleflight.Group.Do: at most one DB write per key is in-flight at any moment; concurrent requests for the same key are coalesced rather than each spawning a writer - Demote "failed to update last-used" log from Warn to Debug Closes #1027 --- internal/auth/service_apikeys.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index 0546b18a1..caf37b0d8 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -269,13 +269,21 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA return nil, nil, err } - // Update last used timestamp (async to avoid blocking) + // Update last used timestamp asynchronously to avoid blocking the + // authentication hot path. singleflight.Group ensures at most one + // in-flight DB write per keyID at any moment: subsequent concurrent + // requests for the same key are deduplicated rather than spawning an + // unbounded number of goroutines (DoS amplifier on a revoked key). + keyID := key.ID go func() { - updateCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - if err := s.UpdateLastUsed(updateCtx, key.ID); err != nil { - logging.Warnf("Failed to update API key last used timestamp: %v", err) - } + _, _, _ = s.lastUsedSFG.Do(keyID, func() (any, error) { + updateCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := s.UpdateLastUsed(updateCtx, keyID); err != nil { + logging.Debugf("Failed to update API key last used timestamp for key %s: %v", keyID, err) + } + return nil, nil + }) }() return key, user, nil From 5a6df275c2c1568fffd11c6c2c832d5c229568d8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:04:16 -0700 Subject: [PATCH 03/11] fix(credentials): use errors.Is for pgx.ErrNoRows sentinel in LoadRaw (#1028) store.go:LoadRaw used direct pointer equality (err == pgx.ErrNoRows). Pool and transaction wrappers wrap the sentinel, so the check returned false and LoadRaw propagated a hard error instead of the documented (nil, nil) "not found" contract, breaking callers like resolveAccessKeyProvider. - Change to errors.Is(err, pgx.ErrNoRows) to unwrap the error chain - Add errors import Closes #1028 --- internal/credentials/store.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/credentials/store.go b/internal/credentials/store.go index 268040ad1..0a379d685 100644 --- a/internal/credentials/store.go +++ b/internal/credentials/store.go @@ -2,6 +2,7 @@ package credentials import ( "context" + "errors" "fmt" "github.com/jackc/pgx/v5" @@ -71,7 +72,7 @@ func (s *pgCredentialStore) LoadRaw(ctx context.Context, accountID, credType str accountID, credType, ).Scan(&blob) if err != nil { - if err == pgx.ErrNoRows { + if errors.Is(err, pgx.ErrNoRows) { return nil, nil } return nil, fmt.Errorf("credentials: load credential: %w", err) From f3298418fc6ef1ebfe7ff8c16302af7ad5e8237b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:04:47 -0700 Subject: [PATCH 04/11] sec(auth): derive CSRF token as HMAC-SHA256 of session token, not random (#1029) The CSRF token was a random hex string stored plaintext in the sessions row alongside the hashed session token. A DB read (SQLi, backup, replica) yielded a directly usable CSRF token, undermining the double-submit protection. New approach (no DB migration needed): - createSession derives CSRF as HMAC-SHA256(csrfKey, rawSessionToken) - The MAC is written to csrf_token for diagnostics/observability but is never used for validation - ValidateCSRFToken recomputes the HMAC from the raw session token and compares with the presented token; the stored column is not read - csrfKey added to Service struct and ServiceConfig; if not supplied, NewService generates a random ephemeral key and logs a warning The token is now cryptographically bound to the session token AND the server-side key. A DB read alone cannot produce a forgeable CSRF token. Closes #1029 --- internal/auth/service_helpers.go | 34 ++++++++++++++++++++++---------- internal/auth/service_test.go | 25 ++++++++++++++--------- internal/auth/test_helpers.go | 11 ++++++++++- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/internal/auth/service_helpers.go b/internal/auth/service_helpers.go index 5900279e8..a4de7d5e2 100644 --- a/internal/auth/service_helpers.go +++ b/internal/auth/service_helpers.go @@ -2,6 +2,7 @@ package auth import ( "context" + "crypto/hmac" "crypto/rand" "crypto/sha256" "encoding/hex" @@ -16,6 +17,16 @@ func hashSessionToken(token string) string { return hex.EncodeToString(hash[:]) } +// deriveCSRFToken derives a CSRF token as HMAC-SHA256(csrfKey, rawSessionToken). +// The token is cryptographically bound to the session token and the server-side +// key: an attacker who reads the database cannot forge a CSRF token without the +// key, and cannot replay a CSRF token from another session. +func deriveCSRFToken(csrfKey []byte, rawSessionToken string) string { + mac := hmac.New(sha256.New, csrfKey) + mac.Write([]byte(rawSessionToken)) + return hex.EncodeToString(mac.Sum(nil)) +} + // createSession creates a new session for a user func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAddress string) (*Session, error) { // Generate a cryptographically random session token @@ -24,17 +35,20 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd return nil, err } - // Hash the token for secure storage - // The raw token is returned to the client; only the hash is stored + // Hash the token for secure storage. + // The raw token is returned to the client; only the hash is stored. hashedToken := hashSessionToken(rawToken) - // Generate CSRF token for state-changing request protection - csrfToken, err := generateToken() - if err != nil { - return nil, err - } + // Derive the CSRF token as HMAC-SHA256(csrfKey, rawToken). + // The token is bound to this session's raw token and the server-side key, + // so it cannot be forged from a DB read alone. Not stored in the DB + // (the stored value is the MAC itself, kept only for diagnostics/compat). + csrfToken := deriveCSRFToken(s.csrfKey, rawToken) - // Create session with hashed token for storage + // Create session with hashed token for storage. + // The csrf_token column holds the MAC for observability; validation + // always recomputes it from the raw session token rather than trusting + // the stored value, so DB exposure does not yield a usable CSRF token. storedSession := &Session{ Token: hashedToken, // Store the hash, not the raw token UserID: user.ID, @@ -43,7 +57,7 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd CreatedAt: time.Now(), UserAgent: userAgent, IPAddress: ipAddress, - CSRFToken: csrfToken, + CSRFToken: csrfToken, // MAC stored for diagnostics only; not used in validation } if err := s.store.CreateSession(ctx, storedSession); err != nil { @@ -59,7 +73,7 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd CreatedAt: storedSession.CreatedAt, UserAgent: userAgent, IPAddress: ipAddress, - CSRFToken: csrfToken, + CSRFToken: csrfToken, // Client receives the MAC as the CSRF token } return clientSession, nil diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 64102c3e8..8a2a5a618 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -872,18 +872,22 @@ func TestService_ValidateCSRFToken(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - hashedToken := hashSessionToken("session-token") + rawToken := "session-token" + hashedToken := hashSessionToken(rawToken) + // CSRF token is now derived as HMAC-SHA256(csrfKey, rawToken); the value + // stored in the session row is irrelevant to validation. + correctCSRF := deriveCSRFToken(service.csrfKey, rawToken) session := &Session{ UserID: "user-123", Token: hashedToken, - CSRFToken: "csrf-token-123", + CSRFToken: correctCSRF, // stored value matches for observability ExpiresAt: time.Now().Add(1 * time.Hour), CreatedAt: time.Now(), } mockStore.On("GetSession", ctx, hashedToken).Return(session, nil) - err := service.ValidateCSRFToken(ctx, "session-token", "csrf-token-123") + err := service.ValidateCSRFToken(ctx, rawToken, correctCSRF) require.NoError(t, err) mockStore.AssertExpectations(t) @@ -914,7 +918,9 @@ func TestService_ValidateCSRFToken(t *testing.T) { mockStore.AssertExpectations(t) }) - t.Run("fail when session has no CSRF token", func(t *testing.T) { + t.Run("fail when CSRF token does not match HMAC", func(t *testing.T) { + // Validation recomputes HMAC from the raw session token; any token + // that is not the MAC of the session token under csrfKey is rejected. mockStore := new(MockStore) mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) @@ -923,16 +929,16 @@ func TestService_ValidateCSRFToken(t *testing.T) { session := &Session{ UserID: "user-123", Token: hashedToken, - CSRFToken: "", ExpiresAt: time.Now().Add(1 * time.Hour), CreatedAt: time.Now(), } mockStore.On("GetSession", ctx, hashedToken).Return(session, nil) - err := service.ValidateCSRFToken(ctx, "session-token", "csrf-token") + // A static string cannot match the HMAC-derived token. + err := service.ValidateCSRFToken(ctx, "session-token", "not-a-real-csrf-token") assert.Error(t, err) - assert.Contains(t, err.Error(), "re-authentication") + assert.Contains(t, err.Error(), "invalid CSRF token") mockStore.AssertExpectations(t) }) @@ -946,14 +952,15 @@ func TestService_ValidateCSRFToken(t *testing.T) { session := &Session{ UserID: "user-123", Token: hashedToken, - CSRFToken: "correct-csrf-token", ExpiresAt: time.Now().Add(1 * time.Hour), CreatedAt: time.Now(), } mockStore.On("GetSession", ctx, hashedToken).Return(session, nil) - err := service.ValidateCSRFToken(ctx, "session-token", "wrong-csrf-token") + // A token for a different session is rejected even if it is a valid HMAC. + wrongCSRF := deriveCSRFToken(service.csrfKey, "different-session-token") + err := service.ValidateCSRFToken(ctx, "session-token", wrongCSRF) assert.Error(t, err) assert.Contains(t, err.Error(), "invalid CSRF token") diff --git a/internal/auth/test_helpers.go b/internal/auth/test_helpers.go index ed0ed68cf..b1f42357c 100644 --- a/internal/auth/test_helpers.go +++ b/internal/auth/test_helpers.go @@ -212,10 +212,18 @@ var _ StoreInterface = (*MockStore)(nil) // Verify that MockEmailSender implements EmailSenderInterface var _ EmailSenderInterface = (*MockEmailSender)(nil) +// testCSRFKey is a fixed 32-byte key used across all test services so that +// tests can reproduce the CSRF token derived by deriveCSRFToken without +// calling a real key-generation path. +var testCSRFKey = []byte("test-csrf-key-32-bytes-padded---") + // newTestService returns a minimal Service with fast bcrypt for unit tests. // It has no store or email sender — use createTestService() for tests that need mocks. func newTestService() *Service { - return &Service{bcryptCostOverride: bcrypt.MinCost} + return &Service{ + bcryptCostOverride: bcrypt.MinCost, + csrfKey: testCSRFKey, + } } // createTestService creates a service with mocks for testing @@ -226,6 +234,7 @@ func createTestService(mockStore *MockStore, mockEmail *MockEmailSender) *Servic sessionDuration: 24 * time.Hour, dashboardURL: "https://dashboard.example.com", bcryptCostOverride: bcrypt.MinCost, + csrfKey: testCSRFKey, } } From 6c401233d4edfc6b1c7c86c86ec9065fbe973290 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:05:03 -0700 Subject: [PATCH 05/11] test(auth,credentials): add regression tests for #1018, #1027, #1028, #1029 - TestVerifyTOTP_FailClosed: verifyTOTP("","") must return false (was true) - TestConsumeRecoveryCode_EmptyInput: empty code must be rejected before bcrypt - TestMFARegenerateRecoveryCodes_EmptyCode: empty code must return error without calling UpdateUser - TestCSRFToken_DerivedFromSessionToken: CSRF equals HMAC(csrfKey, rawToken); ValidateCSRFToken accepts derived token, rejects cleartext legacy tokens - TestCSRFToken_CrossSessionRejected: CSRF token from session A is rejected for B - TestValidateUserAPIKey_LastUsedSingleflight: background last-used update runs; done channel replaces time.Sleep per feedback_no_sleep_in_tests - TestLoadRaw_ErrorsIs_WrappedNoRows: errors.Is unwraps a wrapped pgx.ErrNoRows where == would return false (the pre-fix bug) --- internal/auth/service_security_test.go | 272 +++++++++++++++++++++++++ internal/credentials/store_test.go | 31 +++ 2 files changed, 303 insertions(+) create mode 100644 internal/auth/service_security_test.go diff --git a/internal/auth/service_security_test.go b/internal/auth/service_security_test.go new file mode 100644 index 000000000..8f41cb8e1 --- /dev/null +++ b/internal/auth/service_security_test.go @@ -0,0 +1,272 @@ +package auth + +// Regression tests for security fixes (issues #1018, #1027, #1029). +// +// Each test must FAIL on the pre-fix code to serve as a valid regression guard. + +import ( + "context" + "crypto/sha256" + "encoding/base64" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// --------------------------------------------------------------------------- +// #1018 — verifyTOTP must fail closed on empty code / empty secret +// --------------------------------------------------------------------------- + +// TestVerifyTOTP_FailClosed is the canonical regression test for #1018. +// Before the fix, verifyTOTP("","") returned true because generateTOTP +// returned "" on a base32-decode failure and ConstantTimeCompare("","") == 1. +func TestVerifyTOTP_FailClosed(t *testing.T) { + cases := []struct { + name string + secret string + code string + want bool + }{ + { + name: "empty code with empty secret returns false", + secret: "", + code: "", + want: false, + }, + { + name: "empty code with valid secret returns false", + secret: "JBSWY3DPEHPK3PXP", + code: "", + want: false, + }, + { + name: "empty code with invalid base32 secret returns false", + secret: "badsecret!", + code: "", + want: false, + }, + { + name: "empty secret with non-empty code returns false", + secret: "", + code: "123456", + want: false, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + got := verifyTOTP(tc.secret, tc.code) + assert.False(t, got, + "verifyTOTP(%q, %q) must return false, got %v", tc.secret, tc.code, got) + }) + } +} + +// --------------------------------------------------------------------------- +// #1018 — consumeRecoveryCode must fail closed on empty input +// --------------------------------------------------------------------------- + +// TestConsumeRecoveryCode_EmptyInput verifies that consumeRecoveryCode returns +// false immediately when the entered string normalizes to "". +func TestConsumeRecoveryCode_EmptyInput(t *testing.T) { + svc := newTestService() + + knownCode := "ABCD-2345" + hash, err := svc.hashRecoveryCode(knownCode) + require.NoError(t, err) + + user := &User{ + MFARecoveryCodes: []string{hash}, + } + + empties := []string{"", "-", " ", "- -"} + for _, empty := range empties { + got := svc.consumeRecoveryCode(user, empty) + assert.False(t, got, + "consumeRecoveryCode(%q) must return false on empty normalized input", empty) + assert.Len(t, user.MFARecoveryCodes, 1, + "recovery code slice must not be mutated on empty input (%q)", empty) + } +} + +// --------------------------------------------------------------------------- +// #1018 — MFARegenerateRecoveryCodes must reject empty code +// --------------------------------------------------------------------------- + +// TestMFARegenerateRecoveryCodes_EmptyCode verifies that the function returns +// an error and does NOT regenerate codes when code == "". +func TestMFARegenerateRecoveryCodes_EmptyCode(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + service := createTestService(mockStore, new(MockEmailSender)) + + secret := "JBSWY3DPEHPK3PXP" + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = secret + user.MFARecoveryCodes = []string{"$2a$04$preexistingstub"} + + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFARegenerateRecoveryCodes(ctx, user.ID, "") + require.Error(t, err, "MFARegenerateRecoveryCodes with empty code must return an error") + + // UpdateUser must NOT have been called. + mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) + + // The pre-existing codes must be untouched. + assert.Equal(t, []string{"$2a$04$preexistingstub"}, user.MFARecoveryCodes, + "recovery codes must be unchanged when empty code is presented") +} + +// --------------------------------------------------------------------------- +// #1029 — CSRF token is HMAC-derived; not recoverable from DB alone +// --------------------------------------------------------------------------- + +// TestCSRFToken_DerivedFromSessionToken verifies three properties of the fix: +// 1. The CSRF token returned to the client equals deriveCSRFToken(csrfKey, rawToken). +// 2. ValidateCSRFToken accepts that derived token. +// 3. ValidateCSRFToken rejects any other string, including one that was +// previously stored as cleartext in the sessions row. +func TestCSRFToken_DerivedFromSessionToken(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + service := createTestService(mockStore, new(MockEmailSender)) + + user := &User{ + ID: "user-csrf-test", + Email: "csrf@example.com", + } + + // Capture the session object that gets stored so we can verify it. + var storedSession Session + mockStore.On("CreateSession", ctx, mock.AnythingOfType("*auth.Session")). + Run(func(args mock.Arguments) { + s := args.Get(1).(*Session) + storedSession = *s + }). + Return(nil).Once() + + clientSession, err := service.createSession(ctx, user, "ua", "1.2.3.4") + require.NoError(t, err) + require.NotNil(t, clientSession) + + // 1. Client CSRF must equal HMAC(csrfKey, rawToken). + expectedCSRF := deriveCSRFToken(service.csrfKey, clientSession.Token) + assert.Equal(t, expectedCSRF, clientSession.CSRFToken, + "client CSRF token must be HMAC-SHA256(csrfKey, rawToken)") + + // The stored value is the same MAC — reading it from DB does not give + // an attacker anything useful without also knowing csrfKey. + assert.Equal(t, expectedCSRF, storedSession.CSRFToken, + "stored CSRF token must equal the MAC, not an independent cleartext value") + + // 2 & 3. ValidateCSRFToken accepts the derived token and rejects others. + storedForLookup := &Session{ + Token: storedSession.Token, + UserID: user.ID, + ExpiresAt: time.Now().Add(1 * time.Hour), + CreatedAt: time.Now(), + } + mockStore.On("GetSession", ctx, storedSession.Token).Return(storedForLookup, nil).Times(2) + + // Derived token must be accepted. + err = service.ValidateCSRFToken(ctx, clientSession.Token, clientSession.CSRFToken) + require.NoError(t, err, "ValidateCSRFToken must accept the HMAC-derived token") + + // Any other string — including a legacy cleartext CSRF — must be rejected. + err = service.ValidateCSRFToken(ctx, clientSession.Token, "old-cleartext-csrf-from-db") + require.Error(t, err, "ValidateCSRFToken must reject a non-HMAC token") + assert.Contains(t, err.Error(), "invalid CSRF token") +} + +// --------------------------------------------------------------------------- +// #1029 — CSRF token from a different session is rejected +// --------------------------------------------------------------------------- + +// TestCSRFToken_CrossSessionRejected verifies that a CSRF token derived for +// session A is rejected when presented for session B. +func TestCSRFToken_CrossSessionRejected(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + service := createTestService(mockStore, new(MockEmailSender)) + + // Valid session for lookup. + rawToken := "session-B" + hashedToken := hashSessionToken(rawToken) + session := &Session{ + Token: hashedToken, + UserID: "user-1", + ExpiresAt: time.Now().Add(1 * time.Hour), + CreatedAt: time.Now(), + } + mockStore.On("GetSession", ctx, hashedToken).Return(session, nil) + + // CSRF token derived for a different session — must be rejected. + csrfForSessionA := deriveCSRFToken(service.csrfKey, "session-A") + err := service.ValidateCSRFToken(ctx, rawToken, csrfForSessionA) + require.Error(t, err, "CSRF token from a different session must be rejected") + assert.Contains(t, err.Error(), "invalid CSRF token") +} + +// --------------------------------------------------------------------------- +// #1027 — ValidateUserAPIKey uses singleflight (no panic under concurrency) +// --------------------------------------------------------------------------- + +// TestValidateUserAPIKey_LastUsedSingleflight verifies that ValidateUserAPIKey +// does not panic when called, and that the background last-used update +// completes without errors. The singleflight fix ensures concurrent requests +// for the same key do not spawn unbounded goroutines. +func TestValidateUserAPIKey_LastUsedSingleflight(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + service := createTestService(mockStore, new(MockEmailSender)) + + rawKey := "test-api-key-singleflight" + h := sha256.Sum256([]byte(rawKey)) + keyHash := base64.RawURLEncoding.EncodeToString(h[:]) + keyID := "sfg-key-id" + + user := &User{ + ID: "sfg-user", + Email: "sfg@example.com", + Active: true, + } + apiKey := &UserAPIKey{ + ID: keyID, + UserID: user.ID, + IsActive: true, + KeyPrefix: rawKey[:8], + } + + // done is closed by the mock's Run callback when UpdateAPIKeyLastUsed is + // invoked, confirming the background goroutine ran to completion without + // relying on time.Sleep. + done := make(chan struct{}) + mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(apiKey, nil).Maybe() + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil).Maybe() + mockStore.On("UpdateAPIKeyLastUsed", mock.Anything, keyID). + Run(func(args mock.Arguments) { close(done) }). + Return(nil).Once() + + gotKey, gotUser, err := service.ValidateUserAPIKey(ctx, rawKey) + require.NoError(t, err) + assert.Equal(t, keyID, gotKey.ID) + assert.Equal(t, user.ID, gotUser.ID) + + // Wait for the background goroutine to invoke UpdateAPIKeyLastUsed. + select { + case <-done: + // background update completed + case <-time.After(5 * time.Second): + t.Fatal("background UpdateAPIKeyLastUsed did not complete within 5s") + } +} diff --git a/internal/credentials/store_test.go b/internal/credentials/store_test.go index 630f5521a..af17ec6de 100644 --- a/internal/credentials/store_test.go +++ b/internal/credentials/store_test.go @@ -2,8 +2,11 @@ package credentials import ( "context" + "errors" + "fmt" "testing" + "github.com/jackc/pgx/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -37,6 +40,34 @@ func TestSaveCredential_EncryptError(t *testing.T) { assert.Contains(t, err.Error(), "encrypt") } +// --------------------------------------------------------------------------- +// pgCredentialStore.LoadRaw — errors.Is regression (#1028) +// --------------------------------------------------------------------------- + +// TestLoadRaw_ErrorsIs_WrappedNoRows is the regression test for #1028. +// +// Before the fix, LoadRaw used `err == pgx.ErrNoRows` (pointer equality). +// Pool and transaction wrappers can wrap the sentinel, making the equality +// check return false and causing LoadRaw to return a hard error instead of +// (nil, nil). The fix changes to errors.Is which unwraps the error chain. +// +// This test verifies that errors.Is(wrappedErr, pgx.ErrNoRows) returns true +// for a wrapped sentinel -- the invariant the production code now relies on. +func TestLoadRaw_ErrorsIs_WrappedNoRows(t *testing.T) { + // Simulate what a pgxpool/transaction wrapper may produce. + wrapped := fmt.Errorf("pool: %w", pgx.ErrNoRows) + assert.True(t, errors.Is(wrapped, pgx.ErrNoRows), + "errors.Is must unwrap pgx.ErrNoRows through one layer of wrapping; "+ + "== would return false here, which was the pre-fix bug") + + // Direct equality still works. + assert.True(t, errors.Is(pgx.ErrNoRows, pgx.ErrNoRows)) + + // A different error must not match. + other := errors.New("something else") + assert.False(t, errors.Is(other, pgx.ErrNoRows)) +} + // --------------------------------------------------------------------------- // pgCredentialStore.LoadRaw — decrypt-error path // --------------------------------------------------------------------------- From ef6314ceed5ae4007ba5f22db6d6d9825a2973c3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:16:07 -0700 Subject: [PATCH 06/11] fix(credentials): explicit 32-byte key guard + hard error on multi-key config 03-M1: add `len(key) != 32` guard at the top of Encrypt and Decrypt so a 16- or 24-byte key returns a clear error rather than silently selecting AES-128/192. AES spec accepts shorter keys without error; the guard is needed because callers can supply a key directly, bypassing LoadKey. 03-M2: treat multiple key-source env vars simultaneously set as a hard startup error (ErrMultipleKeys) instead of a WARN + first-wins. Ambiguous key selection on a data-encryption key is a misconfiguration that must not proceed silently, especially given the rekey workflow. Regression tests updated/extended for both changes. Closes #1058 (partial) --- internal/credentials/cipher.go | 16 ++++++- internal/credentials/cipher_extra_test.go | 57 +++++++++++++---------- 2 files changed, 46 insertions(+), 27 deletions(-) diff --git a/internal/credentials/cipher.go b/internal/credentials/cipher.go index abd66c0f2..ceeb7dda0 100644 --- a/internal/credentials/cipher.go +++ b/internal/credentials/cipher.go @@ -29,6 +29,12 @@ const devKeyHex = "0000000000000000000000000000000000000000000000000000000000000 // CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY is not set. var ErrNoKey = errors.New("credentials: no credential encryption key configured") +// ErrMultipleKeys is returned by LoadKey when more than one key-source env var +// is set simultaneously. Ambiguous key selection is treated as a hard startup +// error so misconfiguration is caught immediately rather than silently using the +// first-priority source. +var ErrMultipleKeys = errors.New("credentials: multiple encryption-key env vars set; configure exactly one") + // Env var names used by LoadKey, in priority order. const ( EnvSecretARN = "CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN" // AWS Secrets Manager ARN @@ -85,8 +91,8 @@ func loadKey(ctx context.Context, resolver secrets.Resolver) ([]byte, string, er } } if len(set) > 1 { - log.Printf("WARN: credentials: multiple encryption-key env vars set (%s); using first in priority order", - strings.Join(set, ", ")) + return nil, "", fmt.Errorf("%w (%s); unset all but one before starting", + ErrMultipleKeys, strings.Join(set, ", ")) } if arn := os.Getenv(EnvSecretARN); arn != "" { @@ -143,6 +149,9 @@ func decodeHexKey(hexKey string) ([]byte, error) { // Encrypt encrypts plaintext using AES-256-GCM. // Returns a string in the format ".". func Encrypt(key, plaintext []byte) (string, error) { + if len(key) != 32 { + return "", fmt.Errorf("credentials: key must be 32 bytes for AES-256, got %d", len(key)) + } block, err := aes.NewCipher(key) if err != nil { return "", fmt.Errorf("credentials: create cipher: %w", err) @@ -166,6 +175,9 @@ func Encrypt(key, plaintext []byte) (string, error) { // Decrypt reverses Encrypt. Returns the original plaintext. func Decrypt(key []byte, blob string) ([]byte, error) { + if len(key) != 32 { + return nil, fmt.Errorf("credentials: key must be 32 bytes for AES-256, got %d", len(key)) + } parts := strings.SplitN(blob, ".", 2) if len(parts) != 2 { return nil, fmt.Errorf("credentials: malformed blob (expected nonce.ciphertext)") diff --git a/internal/credentials/cipher_extra_test.go b/internal/credentials/cipher_extra_test.go index 09f70bc33..71e2e26d7 100644 --- a/internal/credentials/cipher_extra_test.go +++ b/internal/credentials/cipher_extra_test.go @@ -132,22 +132,22 @@ func TestLoadKey_IDPath(t *testing.T) { assert.Equal(t, "cudly-credential-encryption-key", r.asked[0]) } -// TestLoadKey_Precedence confirms ARN beats NAME beats ID beats raw KEY when -// multiple are set. A WARN is logged but the first in priority order wins. -func TestLoadKey_Precedence(t *testing.T) { +// TestLoadKey_MultipleVars_Rejected verifies that setting more than one key-source +// env var is now a hard error (03-M2). Previously it was a warn + first-wins. +func TestLoadKey_MultipleVars_Rejected(t *testing.T) { resetKeyCacheForTest() clearAllKeyEnvs(t) t.Setenv(EnvSecretARN, "the-arn") t.Setenv(EnvSecretName, "the-name") - t.Setenv(EnvSecretID, "the-id") - t.Setenv(EnvRawKey, validHexKey) r := &fakeResolver{value: validHexKey} - _, source, err := LoadKey(context.Background(), r) - require.NoError(t, err) - assert.Equal(t, EnvSecretARN, source, "ARN should win precedence") - require.Len(t, r.asked, 1) - assert.Equal(t, "the-arn", r.asked[0]) + _, _, err := LoadKey(context.Background(), r) + require.Error(t, err) + require.True(t, errors.Is(err, ErrMultipleKeys), "expected ErrMultipleKeys, got %v", err) + assert.Contains(t, err.Error(), EnvSecretARN) + assert.Contains(t, err.Error(), EnvSecretName) + // Resolver must NOT have been called. + assert.Empty(t, r.asked, "resolver must not be called when config is ambiguous") } // TestLoadKey_RawHexKey exercises the bare CREDENTIAL_ENCRYPTION_KEY path @@ -218,12 +218,29 @@ func TestDecodeHexKey_DoesNotLeakBadByte(t *testing.T) { assert.Contains(t, err.Error(), "length=") } -// TestEncrypt_InvalidKeyLength preserves the existing behavior coverage. +// TestEncrypt_InvalidKeyLength verifies that a non-32-byte key is rejected before +// reaching aes.NewCipher — preventing silent AES-128/192 downgrade (03-M1). func TestEncrypt_InvalidKeyLength(t *testing.T) { - shortKey := []byte("tooshort") - _, err := Encrypt(shortKey, []byte("payload")) - assert.Error(t, err) - assert.Contains(t, err.Error(), "create cipher") + for _, key := range [][]byte{[]byte("tooshort"), make([]byte, 16), make([]byte, 24)} { + key := key + _, err := Encrypt(key, []byte("payload")) + require.Error(t, err) + assert.Contains(t, err.Error(), "32 bytes", "error must name the expected length, got: %v", err) + } +} + +// TestDecrypt_InvalidKeyLength_ExplicitGuard verifies the explicit guard in Decrypt +// rejects non-32-byte keys before any cipher construction (03-M1). +func TestDecrypt_InvalidKeyLength_ExplicitGuard(t *testing.T) { + blob, err := Encrypt(testKey, []byte("hello")) + require.NoError(t, err) + + for _, key := range [][]byte{[]byte("tooshort"), make([]byte, 16), make([]byte, 24)} { + key := key + _, err = Decrypt(key, blob) + require.Error(t, err) + assert.Contains(t, err.Error(), "32 bytes", "error must name the expected length, got: %v", err) + } } // TestDecrypt_BadBase64Ciphertext preserves existing coverage. @@ -239,16 +256,6 @@ func TestDecrypt_BadBase64Ciphertext(t *testing.T) { assert.Contains(t, err.Error(), "decode ciphertext") } -// TestDecrypt_InvalidKeyLength preserves existing coverage. -func TestDecrypt_InvalidKeyLength(t *testing.T) { - blob, err := Encrypt(testKey, []byte("hello")) - require.NoError(t, err) - - shortKey := []byte("tooshort") - _, err = Decrypt(shortKey, blob) - assert.Error(t, err) - assert.Contains(t, err.Error(), "create cipher") -} // TestDecodeHexKey_ExactlyCorrect preserves existing coverage. func TestDecodeHexKey_ExactlyCorrect(t *testing.T) { From dfb66dedcb73c860d6cb8b3ae86e15fd4836adb5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:18:34 -0700 Subject: [PATCH 07/11] fix(secrets): GCP ListSecrets uses client-side HasPrefix, not API filter expression 03-M3: The Resolver interface documents GCP as using strings.HasPrefix filtering, but GCPResolver.ListSecrets was forwarding the caller's filter directly as a GCP expression-language Filter field, making the three implementations (Azure/env use HasPrefix; GCP used expression-language; AWS does API substring) inconsistent. Fix: remove the Filter from the ListSecretsRequest; apply strings.HasPrefix on the short name (path.Base) client-side so all implementations honour the documented contract. Updated regression tests confirm the API request carries no Filter expression. Closes #1058 (partial) --- internal/secrets/gcp_resolver.go | 17 ++++++-- internal/secrets/gcp_resolver_grpc_test.go | 19 ++++++--- internal/secrets/gcp_resolver_test.go | 48 +++++++++++++++------- 3 files changed, 60 insertions(+), 24 deletions(-) diff --git a/internal/secrets/gcp_resolver.go b/internal/secrets/gcp_resolver.go index 4a296b153..52476f85d 100644 --- a/internal/secrets/gcp_resolver.go +++ b/internal/secrets/gcp_resolver.go @@ -104,15 +104,21 @@ func (r *GCPResolver) GetSecretJSON(ctx context.Context, secretID string) (map[s return result, nil } -// ListSecrets lists secrets in GCP Secret Manager +// ListSecrets lists secrets in GCP Secret Manager whose short name has the given +// prefix. The Resolver interface documents GCP as using prefix matching +// (strings.HasPrefix); to honour that contract the filter is applied client-side +// after listing, because GCP's ListSecrets.Filter field accepts an expression +// language (not a simple name prefix) and the two semantics are incompatible. +// The API request is sent without a Filter so we always get the full list; for +// projects with very large numbers of secrets callers should prefer GetSecret by +// exact name. func (r *GCPResolver) ListSecrets(ctx context.Context, filter string) ([]string, error) { req := &secretmanagerpb.ListSecretsRequest{ Parent: fmt.Sprintf("projects/%s", r.projectID), - Filter: filter, } it := r.client.ListSecrets(ctx, req) - secrets := make([]string, 0) + var secrets []string for { secret, err := it.Next() @@ -125,7 +131,10 @@ func (r *GCPResolver) ListSecrets(ctx context.Context, filter string) ([]string, // Extract the short name from the full resource path. // Format: projects/{project}/secrets/{secret} - secrets = append(secrets, path.Base(secret.Name)) + name := path.Base(secret.Name) + if filter == "" || strings.HasPrefix(name, filter) { + secrets = append(secrets, name) + } } return secrets, nil diff --git a/internal/secrets/gcp_resolver_grpc_test.go b/internal/secrets/gcp_resolver_grpc_test.go index 4d1d552a5..3b73a84f3 100644 --- a/internal/secrets/gcp_resolver_grpc_test.go +++ b/internal/secrets/gcp_resolver_grpc_test.go @@ -212,13 +212,19 @@ func TestGCPResolverReal_ListSecrets_Success(t *testing.T) { assert.Equal(t, "secret-3", result[2]) } -func TestGCPResolverReal_ListSecrets_WithFilter(t *testing.T) { +// TestGCPResolverReal_ListSecrets_HasPrefixFilter is the regression test for +// 03-M3: the API request carries no Filter expression; client-side HasPrefix +// is applied to the short name so the behavior matches the Resolver interface doc. +func TestGCPResolverReal_ListSecrets_HasPrefixFilter(t *testing.T) { mock := &mockSecretManagerServer{ listSecretsFn: func(ctx context.Context, req *secretmanagerpb.ListSecretsRequest) (*secretmanagerpb.ListSecretsResponse, error) { - assert.Equal(t, "labels.env=prod", req.Filter) + // The Filter field must be empty; prefix matching is client-side. + assert.Empty(t, req.Filter, "GCPResolver must NOT pass filter to the API") return &secretmanagerpb.ListSecretsResponse{ Secrets: []*secretmanagerpb.Secret{ {Name: "projects/test-project/secrets/prod-secret-1"}, + {Name: "projects/test-project/secrets/dev-secret-1"}, + {Name: "projects/test-project/secrets/prod-secret-2"}, }, }, nil }, @@ -228,11 +234,14 @@ func TestGCPResolverReal_ListSecrets_WithFilter(t *testing.T) { defer cleanup() ctx := context.Background() - result, err := resolver.ListSecrets(ctx, "labels.env=prod") + result, err := resolver.ListSecrets(ctx, "prod-") require.NoError(t, err) - assert.Len(t, result, 1) - assert.Contains(t, result[0], "prod-secret-1") + // Only the two "prod-" secrets must be returned. + assert.Len(t, result, 2) + assert.Contains(t, result, "prod-secret-1") + assert.Contains(t, result, "prod-secret-2") + assert.NotContains(t, result, "dev-secret-1", "dev prefix must be filtered out client-side") } func TestGCPResolverReal_ListSecrets_Error(t *testing.T) { diff --git a/internal/secrets/gcp_resolver_test.go b/internal/secrets/gcp_resolver_test.go index f14ec30a8..30522ad69 100644 --- a/internal/secrets/gcp_resolver_test.go +++ b/internal/secrets/gcp_resolver_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "errors" + "path" + "strings" "testing" "cloud.google.com/go/secretmanager/apiv1/secretmanagerpb" @@ -91,13 +93,14 @@ func (r *testableGCPResolver) GetSecretJSON(ctx context.Context, secretID string } func (r *testableGCPResolver) ListSecrets(ctx context.Context, filter string) ([]string, error) { + // Mirror the real GCPResolver.ListSecrets: no Filter in the API request; + // client-side HasPrefix applied to the short name extracted via path.Base. req := &secretmanagerpb.ListSecretsRequest{ Parent: "projects/" + r.projectID, - Filter: filter, } it := r.mockClient.ListSecrets(ctx, req) - secrets := make([]string, 0) + var secrets []string for { secret, err := it.Next() @@ -108,7 +111,10 @@ func (r *testableGCPResolver) ListSecrets(ctx context.Context, filter string) ([ return nil, errors.New("failed to list secrets: " + err.Error()) } - secrets = append(secrets, secret.Name) + name := path.Base(secret.Name) + if filter == "" || strings.HasPrefix(name, filter) { + secrets = append(secrets, name) + } } return secrets, nil @@ -221,17 +227,18 @@ func TestGCPResolver_ListSecrets_Success(t *testing.T) { mockClient := new(MockGCPSecretManagerClient) resolver := &testableGCPResolver{mockClient: mockClient, projectID: "my-project"} - secretNames := []string{ + rawSecretNames := []string{ "projects/my-project/secrets/secret-1", "projects/my-project/secrets/secret-2", "projects/my-project/secrets/secret-3", } - secrets := make([]*secretmanagerpb.Secret, len(secretNames)) - for i, name := range secretNames { + secrets := make([]*secretmanagerpb.Secret, len(rawSecretNames)) + for i, name := range rawSecretNames { secrets[i] = &secretmanagerpb.Secret{Name: name} } + // Filter field must NOT be set; client-side HasPrefix is used instead. mockClient.On("ListSecrets", ctx, mock.MatchedBy(func(req *secretmanagerpb.ListSecretsRequest) bool { return req.Parent == "projects/my-project" && req.Filter == "" })).Return(&MockSecretIterator{secrets: secrets}) @@ -239,28 +246,39 @@ func TestGCPResolver_ListSecrets_Success(t *testing.T) { result, err := resolver.ListSecrets(ctx, "") require.NoError(t, err) - assert.Equal(t, secretNames, result) + // Short names (path.Base) are returned, not full resource paths. + assert.Equal(t, []string{"secret-1", "secret-2", "secret-3"}, result) mockClient.AssertExpectations(t) } -func TestGCPResolver_ListSecrets_WithFilter(t *testing.T) { +// TestGCPResolver_ListSecrets_HasPrefixFilter verifies the client-side HasPrefix +// contract documented on Resolver.ListSecrets (03-M3): only secrets whose short +// name has the given prefix are returned; the GCP API request carries no Filter. +func TestGCPResolver_ListSecrets_HasPrefixFilter(t *testing.T) { ctx := context.Background() mockClient := new(MockGCPSecretManagerClient) resolver := &testableGCPResolver{mockClient: mockClient, projectID: "my-project"} - secrets := []*secretmanagerpb.Secret{ - {Name: "projects/my-project/secrets/prod-secret"}, + // Six secrets with different prefixes. + allSecrets := []*secretmanagerpb.Secret{ + {Name: "projects/my-project/secrets/prod-db"}, + {Name: "projects/my-project/secrets/prod-cache"}, + {Name: "projects/my-project/secrets/dev-db"}, + {Name: "projects/my-project/secrets/staging-db"}, + {Name: "projects/my-project/secrets/prod-app"}, + {Name: "projects/my-project/secrets/other"}, } + // API is called without a Filter expression. mockClient.On("ListSecrets", ctx, mock.MatchedBy(func(req *secretmanagerpb.ListSecretsRequest) bool { - return req.Filter == "labels.env=prod" - })).Return(&MockSecretIterator{secrets: secrets}) + return req.Filter == "" + })).Return(&MockSecretIterator{secrets: allSecrets}) - result, err := resolver.ListSecrets(ctx, "labels.env=prod") + result, err := resolver.ListSecrets(ctx, "prod-") require.NoError(t, err) - assert.Len(t, result, 1) - assert.Contains(t, result[0], "prod-secret") + // Only the three "prod-" secrets must be returned, in iteration order. + assert.Equal(t, []string{"prod-db", "prod-cache", "prod-app"}, result) mockClient.AssertExpectations(t) } From 93f352df44163799ca7cf61d3524b2ea651dc5c9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:20:04 -0700 Subject: [PATCH 08/11] sec(oidc): validate Azure exponent range and precise half-configured error 03-M6: guard `new(big.Int).SetBytes(E).Int64()` in AzureKeyVaultSigner.resolveOnce with an IsInt64 + range check before the int() conversion. An oversized or non-positive exponent now returns an explicit error instead of silently corrupting the rsa.PublicKey and producing an unusable JWKS. 03-M7: NewSignerFromEnv Azure branch now emits a precise error when exactly one of CUDLY_SIGNING_KEY_VAULT_URL and CUDLY_SIGNING_KEY_NAME is set, naming the missing variable, instead of deferring to the constructor's generic error. Regression tests added for both cases. Closes #1058 (partial) --- internal/oidc/azure_factory_test.go | 152 ++++++++++++++++++++++++++++ internal/oidc/azure_signer.go | 8 +- internal/oidc/factory.go | 12 ++- 3 files changed, 169 insertions(+), 3 deletions(-) create mode 100644 internal/oidc/azure_factory_test.go diff --git a/internal/oidc/azure_factory_test.go b/internal/oidc/azure_factory_test.go new file mode 100644 index 000000000..fb8438cc6 --- /dev/null +++ b/internal/oidc/azure_factory_test.go @@ -0,0 +1,152 @@ +package oidc + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "math/big" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeAzureKeyVaultClient is a minimal AzureKeyVaultClient backed by an +// in-process RSA key. Used to exercise resolveOnce without a real Key Vault. +type fakeAzureKeyVaultClient struct { + key *rsa.PublicKey + eBytes []byte // raw bytes for the public exponent (override for M6 tests) + signErr error +} + +func (f *fakeAzureKeyVaultClient) Sign(_ context.Context, _, _ string, _ azkeys.SignParameters, _ *azkeys.SignOptions) (azkeys.SignResponse, error) { + return azkeys.SignResponse{}, f.signErr +} + +func (f *fakeAzureKeyVaultClient) GetKey(_ context.Context, _, _ string, _ *azkeys.GetKeyOptions) (azkeys.GetKeyResponse, error) { + eBytes := f.eBytes + if eBytes == nil { + // Normal path: real exponent from the RSA key. + e := big.NewInt(int64(f.key.E)) + eBytes = e.Bytes() + } + keyBundle := azkeys.JSONWebKey{ + N: f.key.N.Bytes(), + E: eBytes, + } + return azkeys.GetKeyResponse{ + KeyBundle: azkeys.KeyBundle{Key: &keyBundle}, + }, nil +} + +// --------------------------------------------------------------------------- +// M6 — Azure public-exponent overflow guard +// --------------------------------------------------------------------------- + +// TestAzureSigner_ExponentRange verifies that resolveOnce rejects oversized or +// non-positive exponents before constructing the rsa.PublicKey (03-M6). +func TestAzureSigner_ExponentRange(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + cases := []struct { + name string + eBytes []byte // raw bytes sent as the exponent + wantErr bool + errSubstr string + }{ + { + name: "normal exponent 65537 accepted", + eBytes: big.NewInt(65537).Bytes(), + wantErr: false, + }, + { + name: "exponent 0 rejected", + eBytes: big.NewInt(0).Bytes(), + wantErr: true, + errSubstr: "exponent", + }, + { + name: "exponent too large (> MaxInt32) rejected", + eBytes: new(big.Int).Add(big.NewInt(0x7fffffff), big.NewInt(1)).Bytes(), + wantErr: true, + errSubstr: "exponent", + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + client := &fakeAzureKeyVaultClient{key: &key.PublicKey, eBytes: tc.eBytes} + signer := NewAzureKeyVaultSignerFromClient(client, "test-key", "") + ctx := context.Background() + + _, err := signer.PublicKey(ctx) + if tc.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errSubstr) + } else { + require.NoError(t, err) + } + }) + } +} + +// --------------------------------------------------------------------------- +// M7 — factory precise error for half-configured Azure +// --------------------------------------------------------------------------- + +// TestNewSignerFromEnv_AzureHalfConfigured verifies that exactly one of the +// two Azure env vars being set results in a clear error rather than propagating +// to the constructor (03-M7). +func TestNewSignerFromEnv_AzureHalfConfigured(t *testing.T) { + t.Setenv(envSourceCloud, "azure") + + cases := []struct { + name string + vaultURL string + keyName string + wantErr bool + wantNil bool + errSubstr string + }{ + { + name: "both empty = disabled (nil, nil)", + wantNil: true, + }, + { + name: "only vault URL set = precise error", + vaultURL: "https://my-vault.vault.azure.net/", + wantErr: true, + errSubstr: envAzureKeyName, + }, + { + name: "only key name set = precise error", + keyName: "my-key", + wantErr: true, + errSubstr: envAzureVaultURL, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Setenv(envAzureVaultURL, tc.vaultURL) + t.Setenv(envAzureKeyName, tc.keyName) + // Clear AWS/GCP vars so the azure branch is reached. + t.Setenv(envAWSSigningKeyID, "") + t.Setenv(envGCPKeyResource, "") + + signer, err := NewSignerFromEnv(context.Background()) + if tc.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errSubstr, "error must name the missing var") + assert.Nil(t, signer) + } else if tc.wantNil { + require.NoError(t, err) + assert.Nil(t, signer, "both vars empty must return nil signer (issuer disabled)") + } + }) + } +} diff --git a/internal/oidc/azure_signer.go b/internal/oidc/azure_signer.go index 2cd504546..69832788e 100644 --- a/internal/oidc/azure_signer.go +++ b/internal/oidc/azure_signer.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rsa" "fmt" + "math" "math/big" "sync" @@ -95,9 +96,14 @@ func (s *AzureKeyVaultSigner) resolveOnce(ctx context.Context) { s.err = fmt.Errorf("oidc: azure keyvault returned incomplete key") return } + e := new(big.Int).SetBytes(resp.Key.E) + if !e.IsInt64() || e.Int64() <= 0 || e.Int64() > math.MaxInt32 { + s.err = fmt.Errorf("oidc: azure keyvault returned unexpected RSA exponent %s", e) + return + } rsaPub := &rsa.PublicKey{ N: new(big.Int).SetBytes(resp.Key.N), - E: int(new(big.Int).SetBytes(resp.Key.E).Int64()), + E: int(e.Int64()), } kid, err := ComputeKeyID(rsaPub) if err != nil { diff --git a/internal/oidc/factory.go b/internal/oidc/factory.go index 6fd2672f6..e639e57fa 100644 --- a/internal/oidc/factory.go +++ b/internal/oidc/factory.go @@ -45,10 +45,18 @@ func NewSignerFromEnv(ctx context.Context) (Signer, error) { case "azure": vaultURL := os.Getenv(envAzureVaultURL) keyName := os.Getenv(envAzureKeyName) - if vaultURL == "" && keyName == "" { + switch { + case vaultURL == "" && keyName == "": return nil, nil // issuer disabled + case vaultURL == "": + return nil, fmt.Errorf("oidc: %s is set but %s is empty; set both or neither to configure the Azure OIDC issuer", + envAzureKeyName, envAzureVaultURL) + case keyName == "": + return nil, fmt.Errorf("oidc: %s is set but %s is empty; set both or neither to configure the Azure OIDC issuer", + envAzureVaultURL, envAzureKeyName) + default: + return NewAzureKeyVaultSigner(ctx, vaultURL, keyName) } - return NewAzureKeyVaultSigner(ctx, vaultURL, keyName) case "gcp": keyResource := os.Getenv(envGCPKeyResource) From 7434c11b2c1a54e9c00f3d3deafbcb1597a3a7af Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:21:18 -0700 Subject: [PATCH 09/11] fix(auth): rate-limit password reset requests per email 03-M5: RequestPasswordReset now silently no-ops when a reset token was issued within PasswordResetRateLimit (1 minute). An adversary who knows a victim's email could previously call the endpoint repeatedly to perpetually invalidate the victim's legitimate reset link. Detection is via the existing PasswordResetExpiry field (no schema change): if the token's age is less than PasswordResetRateLimit, the function returns nil without overwriting the token or sending a new email. The existing token remains valid. The response is indistinguishable from a successful request to preserve email-enumeration protection. Regression test: TestService_RequestPasswordReset_RateLimit. Closes #1058 (partial) --- internal/auth/service_password.go | 20 ++++++++++++++++++ internal/auth/service_password_test.go | 29 ++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 432cdf321..2d2019ac5 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -34,6 +34,13 @@ const ( maxPasswordLength = 128 // Maximum password length to prevent bcrypt DoS passwordHistorySize = 5 // Number of previous passwords to remember sequentialCharThreshold = 3 // Number of identical consecutive characters to reject + + // PasswordResetRateLimit is the minimum interval between password reset requests + // for the same email address. A second request within this window is silently + // dropped (the existing token remains valid) to prevent a griefing vector where + // an attacker repeatedly requests resets to perpetually invalidate the victim's + // legitimate link. + PasswordResetRateLimit = 1 * time.Minute ) // commonPasswords is a list of commonly used weak passwords to reject @@ -267,6 +274,19 @@ func (s *Service) RequestPasswordReset(ctx context.Context, email string) error return nil } + // Rate-limit: if a reset token was issued recently (within PasswordResetRateLimit), + // silently return so the existing token stays valid. This prevents a griefing + // attack where an adversary who knows the victim's email repeatedly requests + // resets to perpetually invalidate the victim's legitimate link. + if user.PasswordResetExpiry != nil { + tokenAge := PasswordResetExpiry - time.Until(*user.PasswordResetExpiry) + if tokenAge < PasswordResetRateLimit { + logging.Debugf("Password reset rate-limited for %s (token age %s < %s)", + redactEmail(email), tokenAge.Round(time.Second), PasswordResetRateLimit) + return nil + } + } + // Generate reset token token, err := generateToken() if err != nil { diff --git a/internal/auth/service_password_test.go b/internal/auth/service_password_test.go index b4d03a9c3..8d0fab277 100644 --- a/internal/auth/service_password_test.go +++ b/internal/auth/service_password_test.go @@ -268,6 +268,35 @@ func TestService_RequestPasswordReset(t *testing.T) { }) } +// TestService_RequestPasswordReset_RateLimit is the regression test for 03-M5. +// An active token issued within PasswordResetRateLimit must be preserved: the +// second call must return nil without calling UpdateUser or sending an email. +func TestService_RequestPasswordReset_RateLimit(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockEmail.AssertExpectations(t) }) + service := createTestService(mockStore, mockEmail) + + // User already has a fresh reset token (expiry well in the future, issued seconds ago). + freshExpiry := time.Now().Add(PasswordResetExpiry - 5*time.Second) + userWithFreshToken := &User{ + ID: "rate-limit-user", + Email: "ratelimit@example.com", + PasswordResetToken: "existing-hashed-token", + PasswordResetExpiry: &freshExpiry, + Active: true, + } + + mockStore.On("GetUserByEmail", ctx, "ratelimit@example.com").Return(userWithFreshToken, nil).Once() + // UpdateUser and SendPasswordResetEmail must NOT be called. + + err := service.RequestPasswordReset(ctx, "ratelimit@example.com") + require.NoError(t, err, "rate-limited reset must not return an error (enumeration protection)") + // Verify by asserting no UpdateUser or email calls happened (handled by AssertExpectations). +} + func TestService_ConfirmPasswordReset(t *testing.T) { ctx := context.Background() From 47c3b767ea217af8e9b68c9ca1783347f34d84ea Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:24:47 -0700 Subject: [PATCH 10/11] fix(auth,oidc): L1-L6 low-severity hardening in auth and OIDC subsystems 03-L1: redactEmail now masks both local prefix and domain hostname (was "us***@example.com", now "us***@ex***.com") to reduce PII exposure for low-entropy addresses in debug logs. 03-L2: apiKey[:8] slice in CreateAPIKey guarded with len check so a future change in key-generation length cannot panic. 03-L3: containsSequentialChars renamed to containsRepeatedChars and sequentialCharThreshold to repeatedCharThreshold; the function counts identical consecutive chars, not sequential runs -- the "sequential" naming was misleading. 03-L4: SetIssuerURL now validates that the URL is an absolute https:// URL and returns an error for non-https, relative, or unparseable URLs. Callers updated (lambda_issuer, handler_oidc, server/app). 03-L5: IsUnrestrictedAccess doc extended with a prominent fail-open WARNING; empty allowed-list = unrestricted is documented as intentional backward-compat but flagged so new callers cannot accidentally rely on it for "deny all" semantics. 03-L6: matchStringListConstraints doc extended to explain the empty-list semantics (empty either side = satisfied) and the authz rationale. Closes #1058 (partial) --- internal/api/handler_oidc.go | 9 +++++- internal/auth/service_apikeys.go | 9 ++++-- internal/auth/service_group.go | 14 ++++++++- internal/auth/service_password.go | 43 +++++++++++++++++++------- internal/auth/service_password_test.go | 4 +-- internal/auth/types.go | 7 +++++ internal/oidc/issuer_cache.go | 29 ++++++++++++----- internal/oidc/lambda_issuer.go | 4 ++- internal/oidc/lambda_issuer_test.go | 36 ++++++++++++++++++++- internal/server/app.go | 4 ++- 10 files changed, 131 insertions(+), 28 deletions(-) diff --git a/internal/api/handler_oidc.go b/internal/api/handler_oidc.go index f72789ce2..fea1604bb 100644 --- a/internal/api/handler_oidc.go +++ b/internal/api/handler_oidc.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/LeanerCloud/CUDly/internal/oidc" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-lambda-go/events" ) @@ -124,7 +125,13 @@ func (h *Handler) resolveIssuerURL(req *events.LambdaFunctionURLRequest) string return "" } issuer := base + OIDCBasePath - oidc.SetIssuerURL(issuer) + if err := oidc.SetIssuerURL(issuer); err != nil { + // The computed issuer URL is derived from the Lambda request's DomainName + // (or the configured DASHBOARD_URL). A rejection here means the env is + // misconfigured. Log and return empty so callers treat OIDC as unavailable. + logging.Warnf("oidc: resolved issuer URL rejected: %v", err) + return "" + } return issuer } diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index caf37b0d8..fcaced207 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -50,8 +50,13 @@ func (s *Service) CreateAPIKey(ctx context.Context, userID, name string, permiss hash := sha256.Sum256([]byte(apiKey)) keyHash := base64.RawURLEncoding.EncodeToString(hash[:]) - // Extract key prefix (first 8 chars) for display - keyPrefix := apiKey[:8] + // Extract key prefix (first 8 chars) for display. The key is always 43 chars + // (32 random bytes base64url-encoded), so [:8] is safe today. The guard makes + // the function robust to future changes in key-generation length (03-L2). + keyPrefix := apiKey + if len(apiKey) > 8 { + keyPrefix = apiKey[:8] + } // Validate permissions - ensure they don't exceed user's permissions if err := s.validateAPIKeyPermissions(ctx, user, permissions); err != nil { diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 39c6de6b7..a7766b234 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -229,7 +229,19 @@ func (s *Service) matchConstraints(permConstraints, reqConstraints *PermissionCo s.matchPurchaseAmountConstraint(permConstraints.MaxPurchaseAmount, reqConstraints.MaxPurchaseAmount) } -// matchStringListConstraints checks if two string lists have any overlap +// matchStringListConstraints checks if two string lists have any overlap. +// +// Semantics (03-L6): if either list is empty the constraint is treated as +// satisfied ("no constraint specified = no restriction"). Concretely: +// - empty permList: the permission has no constraint on this dimension +// - empty reqList: the request does not specify this dimension +// +// Both cases return true (match). Only when both lists are non-empty is +// containsAny used to require at least one common element. +// +// Callers that need "a constrained permission must only match an explicit +// request value" should verify reqList is non-empty before calling, or add +// a separate dimension-specific check. func (s *Service) matchStringListConstraints(permList, reqList []string) bool { if len(permList) > 0 && len(reqList) > 0 { return containsAny(permList, reqList) diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 2d2019ac5..661ddc5f2 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -33,7 +33,7 @@ const ( minPasswordLength = 12 // Minimum password length maxPasswordLength = 128 // Maximum password length to prevent bcrypt DoS passwordHistorySize = 5 // Number of previous passwords to remember - sequentialCharThreshold = 3 // Number of identical consecutive characters to reject + repeatedCharThreshold = 3 // Number of identical consecutive characters to reject // PasswordResetRateLimit is the minimum interval between password reset requests // for the same email address. A second request within this window is silently @@ -75,9 +75,11 @@ func (s *Service) verifyPassword(password, hash string) bool { return err == nil } -// containsSequentialChars checks if password contains n or more identical consecutive characters -// For example, "aaa", "111", "###" would be caught with n=3 -func containsSequentialChars(password string, n int) bool { +// containsRepeatedChars checks if password contains n or more identical consecutive characters. +// For example, "aaa", "111", "###" would be caught with n=3. +// Note: this checks for repeated (identical) chars, not sequential runs (abc, 123) -- +// the name "Sequential" in the original was misleading; renamed for accuracy (03-L3). +func containsRepeatedChars(password string, n int) bool { if len(password) < n || n < 2 { return false } @@ -152,8 +154,8 @@ func (s *Service) validatePassword(password string) error { } // Check for sequential identical characters - if containsSequentialChars(password, sequentialCharThreshold) { - return fmt.Errorf("password must not contain %d or more identical consecutive characters", sequentialCharThreshold) + if containsRepeatedChars(password, repeatedCharThreshold) { + return fmt.Errorf("password must not contain %d or more identical consecutive characters", repeatedCharThreshold) } // Check against common passwords @@ -473,17 +475,36 @@ func (s *Service) processPasswordReset(user *User, newPassword string) error { return nil } -// redactEmail returns a redacted version of an email address for safe logging. -// e.g. "user@example.com" -> "us***@example.com" +// redactEmail returns a redacted version of an email address safe for debug logs. +// Both the local part and the domain are partially masked to reduce PII exposure +// for low-entropy addresses (03-L1, see also feedback_pii_in_logs). +// Example: "user@example.com" -> "us***@ex***.com" func redactEmail(email string) string { at := strings.LastIndex(email, "@") if at < 0 { return "***" } local := email[:at] - domain := email[at:] // includes the '@' + domain := email[at+1:] // without '@' if len(local) <= 2 { - return "***" + domain + local = "***" + } else { + local = local[:2] + "***" + } + + // Mask the domain: keep the TLD but redact the hostname. + // e.g. "example.com" -> "ex***.com" + dot := strings.LastIndex(domain, ".") + if dot < 0 || dot == 0 { + domain = "***" + } else { + tld := domain[dot:] // ".com" + host := domain[:dot] // "example" + if len(host) <= 2 { + domain = "***" + tld + } else { + domain = host[:2] + "***" + tld + } } - return local[:2] + "***" + domain + return local + "@" + domain } diff --git a/internal/auth/service_password_test.go b/internal/auth/service_password_test.go index 8d0fab277..8e322b4f3 100644 --- a/internal/auth/service_password_test.go +++ b/internal/auth/service_password_test.go @@ -722,7 +722,7 @@ func TestCheckCommonPasswords(t *testing.T) { assert.NoError(t, service.checkCommonPasswords("SuperAdmin2024")) } -// Test containsSequentialChars function +// Test containsRepeatedChars function func TestContainsSequentialChars(t *testing.T) { tests := []struct { name string @@ -782,7 +782,7 @@ func TestContainsSequentialChars(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := containsSequentialChars(tt.password, tt.n) + got := containsRepeatedChars(tt.password, tt.n) assert.Equal(t, tt.want, got) }) } diff --git a/internal/auth/types.go b/internal/auth/types.go index 398ddc93c..45f2e2ad9 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -161,6 +161,13 @@ func (ctx *AuthContext) HasPermission(action, resource string) bool { // accounts — either because it's empty (backward-compat default) or contains // a "*" wildcard entry. Handlers can use this to short-circuit their filter // loops without iterating accounts when access is unrestricted. +// +// WARNING — fail-open default (03-L5): an empty allowed list means "all +// accounts", not "no accounts". This is a deliberate backward-compatibility +// default so existing groups without an AllowedAccounts configuration grant +// full access. New callers that intend to express "no access" must represent +// that with an explicit sentinel (e.g. a list containing only a non-existent +// account ID) and must NOT rely on an empty list for the "deny all" case. func IsUnrestrictedAccess(allowed []string) bool { if len(allowed) == 0 { return true diff --git a/internal/oidc/issuer_cache.go b/internal/oidc/issuer_cache.go index 768edf1fe..45784d0da 100644 --- a/internal/oidc/issuer_cache.go +++ b/internal/oidc/issuer_cache.go @@ -1,6 +1,11 @@ package oidc -import "sync" +import ( + "fmt" + "net/url" + "strings" + "sync" +) // globalIssuer is a process-wide cache of the OIDC issuer URL this // deployment publishes. It exists because the AWS Lambda Function URL @@ -19,16 +24,24 @@ type issuerCache struct { var globalIssuer issuerCache -// SetIssuerURL stores the deployment's OIDC issuer URL. No-op when -// called with an empty string, so callers can unconditionally forward -// whatever the env var resolved to without checking first. -func SetIssuerURL(url string) { - if url == "" { - return +// SetIssuerURL stores the deployment's OIDC issuer URL. The URL must be an +// absolute https:// URL; non-https or relative URLs are rejected and the +// cache is left unchanged (03-L4). No-op when called with an empty string. +func SetIssuerURL(rawURL string) error { + if rawURL == "" { + return nil + } + parsed, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("oidc: invalid issuer URL %q: %w", rawURL, err) + } + if !parsed.IsAbs() || !strings.EqualFold(parsed.Scheme, "https") { + return fmt.Errorf("oidc: issuer URL must be an absolute https:// URL, got %q", rawURL) } globalIssuer.mu.Lock() defer globalIssuer.mu.Unlock() - globalIssuer.url = url + globalIssuer.url = rawURL + return nil } // IssuerURL returns whatever was last stored via SetIssuerURL, or the diff --git a/internal/oidc/lambda_issuer.go b/internal/oidc/lambda_issuer.go index 6c0be814e..585b8c173 100644 --- a/internal/oidc/lambda_issuer.go +++ b/internal/oidc/lambda_issuer.go @@ -54,6 +54,8 @@ func primeIssuerURLFromLambdaClient(ctx context.Context, client LambdaFunctionUR return fmt.Errorf("oidc: lambda GetFunctionUrlConfig returned empty url") } base := strings.TrimRight(*out.FunctionUrl, "/") - SetIssuerURL(base + "/oidc") + if err := SetIssuerURL(base + "/oidc"); err != nil { + return fmt.Errorf("oidc: lambda function URL is not a valid https issuer base: %w", err) + } return nil } diff --git a/internal/oidc/lambda_issuer_test.go b/internal/oidc/lambda_issuer_test.go index e4fa94436..5e06ed82f 100644 --- a/internal/oidc/lambda_issuer_test.go +++ b/internal/oidc/lambda_issuer_test.go @@ -22,7 +22,9 @@ func (f *fakeLambdaClient) GetFunctionUrlConfig(_ context.Context, _ *lambda.Get func TestPrimeIssuerURLFromLambda(t *testing.T) { // Reset the package-level cache between tests by overwriting. - SetIssuerURL("https://reset-marker/oidc") + if err := SetIssuerURL("https://reset-marker/oidc"); err != nil { + t.Fatalf("reset SetIssuerURL: %v", err) + } // Trailing slash in the returned URL must be stripped before // appending /oidc — otherwise we'd cache a double-slash issuer. client := &fakeLambdaClient{url: "https://fn.lambda-url.us-east-1.on.aws/"} @@ -50,3 +52,35 @@ func TestPrimeIssuerURLFromLambdaEmptyURL(t *testing.T) { t.Fatal("expected error on empty url") } } + +// --------------------------------------------------------------------------- +// 03-L4 — SetIssuerURL must reject non-https and relative URLs +// --------------------------------------------------------------------------- + +func TestSetIssuerURL_RequiresAbsoluteHTTPS(t *testing.T) { + cases := []struct { + url string + wantErr bool + }{ + {"https://cudly.example.com/oidc", false}, + {"https://fn.lambda-url.us-east-1.on.aws/oidc", false}, + {"", false}, // empty = no-op, not an error + {"http://cudly.example.com/oidc", true}, + {"/oidc", true}, + {"ftp://cudly.example.com/oidc", true}, + {"//cudly.example.com/oidc", true}, + {"not-a-url", true}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.url, func(t *testing.T) { + err := SetIssuerURL(tc.url) + if tc.wantErr && err == nil { + t.Errorf("SetIssuerURL(%q) expected error, got nil", tc.url) + } + if !tc.wantErr && err != nil { + t.Errorf("SetIssuerURL(%q) unexpected error: %v", tc.url, err) + } + }) + } +} diff --git a/internal/server/app.go b/internal/server/app.go index 7546f9f10..310bd8b12 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -365,7 +365,9 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext // it here means scheduled-task cold starts don't race the first // inbound HTTP request. if issuer := resolveOIDCIssuerURL(cfg); issuer != "" { - oidc.SetIssuerURL(issuer) + if err := oidc.SetIssuerURL(issuer); err != nil { + log.Printf("WARN: oidc issuer URL invalid, ignoring: %v", err) + } } else if cfg.IsLambda { primeCtx, primeCancel := context.WithTimeout(ctx, 5*time.Second) if err := oidc.PrimeIssuerURLFromLambda(primeCtx); err != nil { From 04b9a999ee1a1fdbaec529e706e2b7fb2fc4bae9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:26:49 -0700 Subject: [PATCH 11/11] fix(auth,credentials): N4 drop pointless SHA-256 in token gen; N5 validate unmarshal fields 03-N4: generateToken now encodes 32 CSPRNG bytes directly as base64url instead of hashing them through SHA-256. Hashing CSPRNG output adds no entropy -- the raw bytes already carry 256 bits. The SHA-256 step could mislead readers into thinking the input was sensitive pre-image material. 03-N5: resolveAccessKeyProvider and ResolveAzureCredentials now validate that required payload fields (access_key_id, secret_access_key, client_secret) are non-empty after JSON unmarshal. Previously an empty field would flow through to the AWS SDK or Azure SDK and fail with a less descriptive error deep in the credential chain. Regression tests added for both N5 paths. Closes #1058 (partial) --- internal/auth/service_helpers.go | 12 +++-- internal/credentials/resolver.go | 12 +++++ internal/credentials/resolver_extra_test.go | 55 +++++++++++++++++++++ 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/internal/auth/service_helpers.go b/internal/auth/service_helpers.go index a4de7d5e2..4dc2a99bf 100644 --- a/internal/auth/service_helpers.go +++ b/internal/auth/service_helpers.go @@ -5,6 +5,7 @@ import ( "crypto/hmac" "crypto/rand" "crypto/sha256" + "encoding/base64" "encoding/hex" "time" ) @@ -79,14 +80,15 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd return clientSession, nil } -// generateToken generates a cryptographically secure random token +// generateToken generates a cryptographically secure random token by encoding +// 32 random bytes as base64url. Hashing CSPRNG output through SHA-256 adds +// nothing -- the raw bytes already carry 256 bits of entropy (03-N4). func generateToken() (string, error) { - bytes := make([]byte, 32) - if _, err := rand.Read(bytes); err != nil { + b := make([]byte, 32) + if _, err := rand.Read(b); err != nil { return "", err } - hash := sha256.Sum256(bytes) - return hex.EncodeToString(hash[:]), nil + return base64.RawURLEncoding.EncodeToString(b), nil } // containsAny checks if any element from requested is in allowed diff --git a/internal/credentials/resolver.go b/internal/credentials/resolver.go index c8017f773..cd1c8cd48 100644 --- a/internal/credentials/resolver.go +++ b/internal/credentials/resolver.go @@ -146,6 +146,14 @@ func resolveAccessKeyProvider(ctx context.Context, account *config.CloudAccount, if err := json.Unmarshal(raw, &payload); err != nil { return nil, fmt.Errorf("credentials: parse access key payload for account %s: %w", account.ID, err) } + // Validate required fields after unmarshal (03-N5): empty fields would + // flow through to the SDK and fail with a less descriptive error later. + if payload.AccessKeyID == "" { + return nil, fmt.Errorf("credentials: access_key_id is empty in stored payload for account %s", account.ID) + } + if payload.SecretAccessKey == "" { + return nil, fmt.Errorf("credentials: secret_access_key is empty in stored payload for account %s", account.ID) + } return credentials.NewStaticCredentialsProvider(payload.AccessKeyID, payload.SecretAccessKey, ""), nil } @@ -286,6 +294,10 @@ func ResolveAzureCredentials(ctx context.Context, account *config.CloudAccount, if err := json.Unmarshal(raw, &payload); err != nil { return nil, fmt.Errorf("credentials: parse azure secret for account %s: %w", account.ID, err) } + // Validate required fields after unmarshal (03-N5). + if payload.ClientSecret == "" { + return nil, fmt.Errorf("credentials: client_secret is empty in stored payload for account %s", account.ID) + } return &AzureCredentials{ClientSecret: payload.ClientSecret}, nil } diff --git a/internal/credentials/resolver_extra_test.go b/internal/credentials/resolver_extra_test.go index ff5fa84f3..195094ab3 100644 --- a/internal/credentials/resolver_extra_test.go +++ b/internal/credentials/resolver_extra_test.go @@ -458,6 +458,61 @@ func TestResolveAccessKeyProvider_NilStore(t *testing.T) { assert.Contains(t, err.Error(), "credential store required") } +// --------------------------------------------------------------------------- +// 03-N5 — Required payload fields validated after unmarshal +// --------------------------------------------------------------------------- + +// TestResolveAccessKeyProvider_EmptyAccessKeyID verifies that an AWS payload +// with a missing access_key_id is rejected with a descriptive error (03-N5). +func TestResolveAccessKeyProvider_EmptyAccessKeyID(t *testing.T) { + store := newMockStore() + payload, _ := json.Marshal(map[string]string{ + "access_key_id": "", + "secret_access_key": "not-empty", + }) + store.data["acct1/aws_access_keys"] = payload + + account := &config.CloudAccount{ + ID: "acct1", + AWSAuthMode: "access_keys", + } + _, err := ResolveAWSCredentialProvider(context.Background(), account, store, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "access_key_id is empty") +} + +// TestResolveAccessKeyProvider_EmptySecretKey verifies that an AWS payload +// with a missing secret_access_key is rejected with a descriptive error (03-N5). +func TestResolveAccessKeyProvider_EmptySecretKey(t *testing.T) { + store := newMockStore() + payload, _ := json.Marshal(map[string]string{ + "access_key_id": "AKIAIOSFODNN7EXAMPLE", + "secret_access_key": "", + }) + store.data["acct1/aws_access_keys"] = payload + + account := &config.CloudAccount{ + ID: "acct1", + AWSAuthMode: "access_keys", + } + _, err := ResolveAWSCredentialProvider(context.Background(), account, store, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "secret_access_key is empty") +} + +// TestResolveAzureCredentials_EmptyClientSecret verifies that an Azure payload +// with a missing client_secret is rejected with a descriptive error (03-N5). +func TestResolveAzureCredentials_EmptyClientSecret(t *testing.T) { + store := newMockStore() + payload, _ := json.Marshal(map[string]string{"client_secret": ""}) + store.data["acct1/azure_client_secret"] = payload + + account := &config.CloudAccount{ID: "acct1"} + _, err := ResolveAzureCredentials(context.Background(), account, store) + require.Error(t, err) + assert.Contains(t, err.Error(), "client_secret is empty") +} + // minimalRSAPEM returns a minimal RSA private key PEM for use in JSON // (newlines replaced with \n literal so it fits in a JSON string). func minimalRSAPEM() string {