diff --git a/internal/api/openapi_403_test.go b/internal/api/openapi_403_test.go index e1007c064..5b7a5aca4 100644 --- a/internal/api/openapi_403_test.go +++ b/internal/api/openapi_403_test.go @@ -197,55 +197,68 @@ func parseRouterEntries(routerPath string) ([]routeHandlerEntry, error) { return entries, nil } -// handlerFuncCallsRequirePermission returns true if any handler_*.go file in -// srcDir contains a function whose name is fnName and whose body calls +// permissionGatedHandlers scans every handler_*.go file in srcDir once and +// returns the set of *Handler method names whose body calls // requirePermission. -func handlerFuncCallsRequirePermission(srcDir, fnName string) (bool, error) { +// +// The scan reads each file a single time and records every gated method, +// instead of re-reading all handler files for each route (which, with ~130 +// routes and ~75k handler lines, took about a minute under -race). The +// per-method rule is the same: a method is gated when a line containing +// requirePermission appears after its `func (h *Handler) name(` declaration +// and before the next such declaration, or before its brace depth goes +// negative. +func permissionGatedHandlers(srcDir string) (map[string]bool, error) { pattern := filepath.Join(srcDir, "handler_*.go") files, err := filepath.Glob(pattern) if err != nil { - return false, err + return nil, err } // Match the exact function name as declared on *Handler. reFunc := regexp.MustCompile(`^func \(h \*Handler\) (\w+)\(`) + gated := make(map[string]bool) for _, fpath := range files { - f, err := os.Open(fpath) - if err != nil { - return false, err + if err := scanHandlerFileForPermission(fpath, reFunc, gated); err != nil { + return nil, err } + } + return gated, nil +} - scanner := bufio.NewScanner(f) - inTarget := false - braceDepth := 0 +// scanHandlerFileForPermission adds to gated the name of every *Handler +// method in fpath whose body calls requirePermission. +func scanHandlerFileForPermission(fpath string, reFunc *regexp.Regexp, gated map[string]bool) error { + f, err := os.Open(fpath) + if err != nil { + return err + } + defer f.Close() - for scanner.Scan() { - line := scanner.Text() + scanner := bufio.NewScanner(f) + current := "" + braceDepth := 0 - if m := reFunc.FindStringSubmatch(line); m != nil { - inTarget = m[1] == fnName - braceDepth = 0 - } + for scanner.Scan() { + line := scanner.Text() - if inTarget { - braceDepth += strings.Count(line, "{") - strings.Count(line, "}") - if strings.Contains(line, "requirePermission") { - f.Close() - return true, nil - } - if braceDepth < 0 { - inTarget = false - } - } + if m := reFunc.FindStringSubmatch(line); m != nil { + current = m[1] + braceDepth = 0 } - if err := scanner.Err(); err != nil { - f.Close() - return false, err + + if current != "" { + braceDepth += strings.Count(line, "{") - strings.Count(line, "}") + if strings.Contains(line, "requirePermission") { + gated[current] = true + } + if braceDepth < 0 { + current = "" + } } - f.Close() } - return false, nil + return scanner.Err() } // specPathsForRoute returns the set of spec routeKeys that the given router @@ -299,6 +312,11 @@ func TestOpenAPI403OnPermissionGatedRoutes(t *testing.T) { require.NoError(t, err, "failed to parse router.go") require.NotEmpty(t, routerEntries, "parsed zero route entries from router.go") + // Index the permission-gated handlers once for all routes. + gatedHandlers, err := permissionGatedHandlers(pkgDir) + require.NoError(t, err, "failed to scan handler_*.go files") + require.NotEmpty(t, gatedHandlers, "found no handler that calls requirePermission") + var failures []string for _, entry := range routerEntries { @@ -308,12 +326,7 @@ func TestOpenAPI403OnPermissionGatedRoutes(t *testing.T) { } // Check if the delegate handler calls requirePermission. - callsPerm, err := handlerFuncCallsRequirePermission(pkgDir, entry.delegateFnName) - if err != nil { - t.Logf("warning: could not check handler %s: %v", entry.delegateFnName, err) - continue - } - if !callsPerm { + if !gatedHandlers[entry.delegateFnName] { continue } diff --git a/internal/auth/service.go b/internal/auth/service.go index edaa3795d..ea8f587ed 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -71,6 +71,10 @@ type ServiceConfig struct { DashboardURL string CSRFKey []byte SessionDuration time.Duration + // BcryptCost lowers the bcrypt work factor for tests that build the + // service through other packages. Zero, the only value production + // uses, keeps bcryptCost (12) and the fixed cost-12 timing sentinel. + BcryptCost int } // NewService creates a new auth service. @@ -124,6 +128,8 @@ func NewService(cfg ServiceConfig) *Service { dashboardURL: cfg.DashboardURL, onPasswordChange: cfg.OnPasswordChange, csrfKey: csrfKey, + // Zero leaves the production cost in hashPassword and Login. + bcryptCostOverride: cfg.BcryptCost, } } @@ -162,7 +168,7 @@ func (s *Service) Login(ctx context.Context, req LoginRequest) (*LoginResponse, // user is indistinguishable from a wrong-password attempt on a real // account. Without this, the missing-row path returns immediately // (no bcrypt), leaking account existence via timing (issue #416). - s.verifyPassword(req.Password, dummyPasswordHash) + s.verifyPassword(req.Password, s.dummyHashForCompare()) return nil, err } diff --git a/internal/auth/service_lockout_test.go b/internal/auth/service_lockout_test.go index 5efc859b4..5874ee924 100644 --- a/internal/auth/service_lockout_test.go +++ b/internal/auth/service_lockout_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" ) func TestLogin_AccountLockout_Bookkeeping(t *testing.T) { @@ -114,6 +115,26 @@ func TestLogin_BookkeepingErrors(t *testing.T) { } } +// Production (NewService with no BcryptCost) compares against the fixed +// cost-12 sentinel. Only a test cost override switches the timing-equalizing +// compare to a hash at that cost. +func TestDummyHashForCompare_MatchesHashCost(t *testing.T) { + prod := NewService(ServiceConfig{DashboardURL: "https://dashboard.example.com", CSRFKey: testCSRFKey}).dummyHashForCompare() + assert.Equal(t, dummyPasswordHash, prod) + prodCost, err := bcrypt.Cost([]byte(prod)) + require.NoError(t, err) + assert.Equal(t, bcryptCost, prodCost) + + for name, svc := range map[string]*Service{ + "override": newTestService(), + "config": NewService(ServiceConfig{DashboardURL: "https://dashboard.example.com", CSRFKey: testCSRFKey, BcryptCost: bcrypt.MinCost}), + } { + testCost, err := bcrypt.Cost([]byte(svc.dummyHashForCompare())) + require.NoError(t, err, name) + assert.Equal(t, bcrypt.MinCost, testCost, name) + } +} + // TestLogin_OWASPEnumerationInvariant guards against regression that re-introduces // distinct error messages for the 6 authentication failure modes (OWASP ASVS V3.3.4 // / V14.2). All 6 paths MUST return an identical string so the login endpoint cannot diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 3be2d3027..30f7075bd 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -71,6 +71,22 @@ func (s *Service) hashPassword(password string) (string, error) { return string(hash), nil } +// dummyHashForCompare returns the hash Login compares against when the +// account cannot log in. Production always uses dummyPasswordHash, at the +// same cost as real hashes. When a test lowers the cost with +// bcryptCostOverride, the dummy hash uses that cost too, so the compare still +// takes as long as a real one in that test instead of a full cost-12 compare. +func (s *Service) dummyHashForCompare() string { + if s.bcryptCostOverride <= 0 { + return dummyPasswordHash + } + hash, err := bcrypt.GenerateFromPassword([]byte("timing-equalizer"), s.bcryptCostOverride) + if err != nil { + return dummyPasswordHash + } + return string(hash) +} + // verifyPassword verifies a password against a bcrypt hash. func (s *Service) verifyPassword(password, hash string) bool { err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(password)) diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go index 2207ab298..f38c71f52 100644 --- a/internal/auth/service_user_test.go +++ b/internal/auth/service_user_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" - "golang.org/x/crypto/bcrypt" ) func TestService_SetupAdmin(t *testing.T) { @@ -1191,18 +1190,19 @@ func TestService_SetupAdmin_EdgeCases(t *testing.T) { // Test TOTP functions. func TestService_UpdateUserProfile(t *testing.T) { ctx := context.Background() + // Hashed once at MinCost: the subtests only need a hash that + // verifyPassword accepts, not the production work factor. + oldHash := TestPasswordHash(t, "OldPassword123") t.Run("update email and password successfully", func(t *testing.T) { mockStore := new(MockStore) mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - // Create user with bcrypt hash for UpdateUserProfile test - hash, _ := bcrypt.GenerateFromPassword([]byte("OldPassword123"), bcrypt.DefaultCost) testUser := &User{ ID: "user-123", Email: "old@example.com", - PasswordHash: string(hash), + PasswordHash: oldHash, Active: true, CreatedAt: time.Now(), } @@ -1224,11 +1224,10 @@ func TestService_UpdateUserProfile(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - hash, _ := bcrypt.GenerateFromPassword([]byte("OldPassword123"), bcrypt.DefaultCost) testUser := &User{ ID: "user-123", Email: "old@example.com", - PasswordHash: string(hash), + PasswordHash: oldHash, Active: true, } @@ -1252,11 +1251,10 @@ func TestService_UpdateUserProfile(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - hash, _ := bcrypt.GenerateFromPassword([]byte("OldPassword123"), bcrypt.DefaultCost) testUser := &User{ ID: "user-123", Email: "old@example.com", - PasswordHash: string(hash), + PasswordHash: oldHash, Active: true, } otherUser := &User{ @@ -1280,11 +1278,10 @@ func TestService_UpdateUserProfile(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - hash, _ := bcrypt.GenerateFromPassword([]byte("OldPassword123"), bcrypt.DefaultCost) testUser := &User{ ID: "user-123", Email: "old@example.com", - PasswordHash: string(hash), + PasswordHash: oldHash, Active: true, } @@ -1302,11 +1299,10 @@ func TestService_UpdateUserProfile(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - hash, _ := bcrypt.GenerateFromPassword([]byte("OldPassword123"), bcrypt.DefaultCost) testUser := &User{ ID: "user-123", Email: "old@example.com", - PasswordHash: string(hash), + PasswordHash: oldHash, Active: true, } @@ -1337,11 +1333,10 @@ func TestService_UpdateUserProfile(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - hash, _ := bcrypt.GenerateFromPassword([]byte("OldPassword123"), bcrypt.DefaultCost) testUser := &User{ ID: "user-123", Email: "old@example.com", - PasswordHash: string(hash), + PasswordHash: oldHash, Active: true, } diff --git a/internal/purchase/execute_detached_test.go b/internal/purchase/execute_detached_test.go index 43f58083f..28f3ecfed 100644 --- a/internal/purchase/execute_detached_test.go +++ b/internal/purchase/execute_detached_test.go @@ -78,9 +78,19 @@ func newDetachFixture(t *testing.T) *detachFixture { return f } +// A run longer than the terminal save budget must still give the save its +// full budget, and each provider call gets its own per-rec budget rather than +// a share of one. The budgets are scaled down (production: 30s each, with 16s +// calls) so the test takes about 2.5s instead of 32s; the production values +// are pinned with literal durations in the ExpiredCallerDeadline test below. func TestExecuteAndFinalize_SlowRunGetsFreshTerminalSaveBudget(t *testing.T) { + const ( + budget = 2 * time.Second // terminal save and per-rec cap + callTime = 1200 * time.Millisecond // each call fits its cap; two exceed the save budget + ) t.Setenv("CUDLY_MAX_ACCOUNT_PARALLELISM", "1") f := newDetachFixture(t) + f.manager.budgets = purchaseBudgets{terminalSave: budget, perRec: budget} f.exec.Recommendations = append(f.exec.Recommendations, f.exec.Recommendations[0]) f.exec.Recommendations[1].ResourceType = "m5.xlarge" var active atomic.Int32 @@ -93,7 +103,7 @@ func TestExecuteAndFinalize_SlowRunGetsFreshTerminalSaveBudget(t *testing.T) { } defer active.Add(-1) ctx := args.Get(0).(context.Context) - timer := time.NewTimer(16 * time.Second) + timer := time.NewTimer(callTime) defer timer.Stop() select { case <-timer.C: @@ -103,15 +113,15 @@ func TestExecuteAndFinalize_SlowRunGetsFreshTerminalSaveBudget(t *testing.T) { }).Return(common.PurchaseResult{Success: true, CommitmentID: "ri-slow"}, nil).Twice() started := time.Now() err := f.manager.executeAndFinalize(context.Background(), f.exec) - assert.Greater(t, time.Since(started), 30*time.Second) + assert.Greater(t, time.Since(started), budget) assert.False(t, overlapped.Load()) assert.False(t, providerCanceled.Load(), "provider call must fit its own budget") f.svc.AssertNumberOfCalls(t, "PurchaseCommitment", 2) assert.Equal(t, 2, f.history) require.NoError(t, err) assert.Equal(t, []string{"completed"}, f.saved) - assert.Greater(t, f.saveRemaining, 29*time.Second) - assert.LessOrEqual(t, f.saveRemaining, 30*time.Second) + assert.Greater(t, f.saveRemaining, budget-time.Second) + assert.LessOrEqual(t, f.saveRemaining, budget) } var stsOut = sts.GetCallerIdentityOutput{Account: aws.String("123456789012")} @@ -153,6 +163,7 @@ func TestExecuteAndFinalize_ExpiredCallerDeadline_UsesOwnBudgets(t *testing.T) { // per-rec cap (30s) still bounds the provider call inside the 4m run budget. assert.WithinDuration(t, time.Now(), provDeadline, 30*time.Second) + assert.Greater(t, time.Until(provDeadline), 29*time.Second) _, ok := f.saveCtx.Deadline() require.True(t, ok, "terminal save needs a deadline") // Literal durations on purpose: comparing against the constants would diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 4e80d4671..5696de36d 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1102,7 +1102,7 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen // The parent ctx governs the overall Lambda budget; this per-rec deadline // is always the shorter of the two. recTuple := fmt.Sprintf("%s/%s/%s/%s", rec.Provider, rec.Service, rec.Region, rec.ResourceType) - recCtx, recCancel := context.WithTimeout(ctx, 30*time.Second) + recCtx, recCancel := context.WithTimeout(ctx, m.budgets.perRecTimeout()) defer recCancel() // Create the provider diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index adba19c9e..a299a190a 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -59,6 +59,9 @@ type Manager struct { oidcIssuerURL string notifyDays int resolver stepResolver + // budgets lets tests shorten purchase deadlines. Production leaves it + // zero, which means the default durations below. + budgets purchaseBudgets } // PurchaseDefaults holds default purchase settings. @@ -233,8 +236,32 @@ const ( // #913). purchaseExecutionTimeout = 4 * time.Minute terminalSaveTimeout = 30 * time.Second + // perRecPurchaseTimeout caps one recommendation's provider calls; see + // executeSinglePurchase. + perRecPurchaseTimeout = 30 * time.Second ) +// purchaseBudgets overrides deadlines for tests. A zero field means the +// production default, so a Manager built by NewManager uses the constants. +type purchaseBudgets struct { + terminalSave time.Duration + perRec time.Duration +} + +func (b purchaseBudgets) terminalSaveTimeout() time.Duration { + if b.terminalSave > 0 { + return b.terminalSave + } + return terminalSaveTimeout +} + +func (b purchaseBudgets) perRecTimeout() time.Duration { + if b.perRec > 0 { + return b.perRec + } + return perRecPurchaseTimeout +} + // executeAndFinalize runs a purchase and handles status updates, record saving, and progress. // // The root execution row is ALWAYS saved with its finalized status — including @@ -261,7 +288,7 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE if execErr != nil { logging.Errorf("Failed to execute purchase %s: %v", exec.ExecutionID, execErr) } - saveCtx, cancelSave := ctxutil.DetachedWithTimeout(ctx, terminalSaveTimeout) + saveCtx, cancelSave := ctxutil.DetachedWithTimeout(ctx, m.budgets.terminalSaveTimeout()) defer cancelSave() // Operators hear about every failed or partial row (issue #714), even // when the terminal save below fails. A fan-out root is skipped: each diff --git a/internal/secrets/azure_resolver_httptest_test.go b/internal/secrets/azure_resolver_httptest_test.go index 543b931d6..7ced375df 100644 --- a/internal/secrets/azure_resolver_httptest_test.go +++ b/internal/secrets/azure_resolver_httptest_test.go @@ -333,7 +333,14 @@ func TestNewAzureResolver_GetSecret_BlocksMetadata(t *testing.T) { resolver, err := NewAzureResolver(ctx, vaultURL) require.NoError(t, err) - secret, err := resolver.GetSecret(ctx, "test-secret") + // The SDK retries the blocked dial 3 times with exponential + // backoff (about 9s). Keep the retries but shrink the delay for + // this call only, via azcore's per-call override. + callCtx := policy.WithRetryOptions(ctx, policy.RetryOptions{ + RetryDelay: time.Millisecond, + MaxRetryDelay: time.Millisecond, + }) + secret, err := resolver.GetSecret(callCtx, "test-secret") require.Error(t, err) assert.Empty(t, secret) assert.Contains(t, err.Error(), "connection to metadata endpoint 169.254.169.254 is blocked") diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index a1dfd4fee..2a90b0184 100644 --- a/internal/server/adapter_test.go +++ b/internal/server/adapter_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" ) // hashSessionTokenForTest mirrors auth.hashSessionToken (private). Session @@ -36,6 +37,9 @@ func createMockAuthService(t *testing.T) (*authServiceAdapter, *auth.MockStore) // random ephemeral key, so any test asserting a specific derived CSRF // token (TestAuthServiceAdapter_ValidateCSRFToken) cannot reproduce it. CSRFKey: auth.TestCSRFKey(), + // Hashing and Login's timing-equalizing compare run at the test + // cost; cost 12 takes seconds per call under -race. + BcryptCost: bcrypt.MinCost, }) adapter := newAuthServiceAdapter(service) return adapter, mockStore @@ -582,6 +586,7 @@ func TestSetupAdminThenLogin_RoundTrip(t *testing.T) { SessionDuration: time.Hour, CSRFKey: auth.TestCSRFKey(), DashboardURL: "http://localhost:3000", + BcryptCost: bcrypt.MinCost, }) adapter := newAuthServiceAdapter(realAuth) handler := api.NewHandler(api.HandlerConfig{AuthService: adapter}) diff --git a/internal/server/app.go b/internal/server/app.go index fd92d96bf..f0f1eabb6 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -145,6 +145,9 @@ type ApplicationConfig struct { // SQS event source mapping's function_response_types actually includes // "ReportBatchItemFailures" (issue #108 follow-up). ReportSQSBatchItemFailures bool + // authBcryptCost is passed to auth.ServiceConfig.BcryptCost. Only tests + // in this package set it; zero keeps the production bcrypt cost. + authBcryptCost int } // ExternalDeps holds pre-built external dependencies that require infrastructure. @@ -516,6 +519,7 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext EmailSender: deps.EmailSender, SessionDuration: 24 * time.Hour, DashboardURL: cfg.DashboardURL, + BcryptCost: cfg.authBcryptCost, }) // Sensitive requests pass ensureDB, which replaces this temporary limiter @@ -764,6 +768,7 @@ func (app *Application) initAuthService(authStore *auth.PostgresStore, encKey [] DashboardURL: app.appConfig.DashboardURL, OnPasswordChange: buildAdminPasswordSyncCallback(authStore, app.secretResolver), CSRFKey: csrfKey, + BcryptCost: app.appConfig.authBcryptCost, }) if svc == nil { return nil, fmt.Errorf("failed to create auth service") diff --git a/internal/server/app_rate_limiter_integration_test.go b/internal/server/app_rate_limiter_integration_test.go index 2e344cfb6..d1ee54998 100644 --- a/internal/server/app_rate_limiter_integration_test.go +++ b/internal/server/app_rate_limiter_integration_test.go @@ -15,6 +15,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" ) func TestApplicationRateLimiter(t *testing.T) { @@ -44,6 +45,10 @@ func TestApplicationRateLimiter(t *testing.T) { cfg := ApplicationConfig{ IsLambda: isLambda, DashboardURL: "https://rate-limit.example.test", DefaultTerm: 3, DefaultCoverage: 80, + // Each 401 login runs Login's timing-equalizing bcrypt + // compare, about 5s at cost 12 under -race. The limiter + // does not depend on the cost. + authBcryptCost: bcrypt.MinCost, } newApp := func() *Application { app, appErr := NewApplicationFromDeps(ctx, cfg, ExternalDeps{