From e7ab6678cc0fb1607391893daa253305830adb9d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 02:56:26 -0700 Subject: [PATCH 1/8] sec(api): add "register" rate-limit bucket and fix CSRF prefix match Issue #1016: "register" was missing from getDefaultRateLimits(), causing checkRateLimit() to silently fall through to api_general (300/min). The public /api/register endpoint writes to the DB and triggers a synchronous admin-notification email per request, making 300/min a viable table-flooding and inbox-spam vector. Add a dedicated "register" bucket matching setup_admin severity (5 per 15 min per IP). Add assertRateLimitKeysKnown(), called from NewHandler with every key used in checkRateLimit call sites. Panics at startup if a key is missing from the config map, catching this class of bug before production traffic hits. Issue #1017: csrfExemptAlways used strings.HasPrefix on "/api/register", which also exempted /api/registrations//approve, /reject, and DELETE (all AuthAdmin, state-changing). Approving a registration creates an enabled cloud account and stores credentials. Split the exempt list into a prefix list for /api/auth/* paths and an exact match for /api/register, mirroring the existing isPublicEndpoint() handling. Regression tests: - TestGetDefaultRateLimits_RegisterHasDedicatedBucket - TestAssertRateLimitKeysKnown_PanicsOnUnknownKey - TestAssertRateLimitKeysKnown_NoopForKnownKeys - TestRequiresCSRFValidation_RegistrationsAdminRoutesAreNotExempt Closes #1016, Closes #1017 --- internal/api/handler.go | 14 +++++++ internal/api/inmemory_rate_limiter_test.go | 44 ++++++++++++++++++++++ internal/api/middleware.go | 13 +++++-- internal/api/middleware_test.go | 31 +++++++++++++++ internal/api/rate_limiter.go | 20 ++++++++++ 5 files changed, 119 insertions(+), 3 deletions(-) diff --git a/internal/api/handler.go b/internal/api/handler.go index 6f621db64..dc101c635 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -178,6 +178,20 @@ func NewHandler(cfg HandlerConfig) *Handler { // internal/server/app.go) and passed in via cfg.OIDCSigner. Leave // it nil to disable the /.well-known/* endpoints. + // Startup assertion: every key string used in checkRateLimit call sites must + // have a dedicated entry in getDefaultRateLimits(). A missing key falls + // silently through to api_general (300/min), as happened in issue #1016. + // This panics at init time so the gap is caught in tests and on cold start, + // not under production load. + assertRateLimitKeysKnown( + "login", + "setup_admin", + "reset_password", + "change_password", + "register", + "approve_cancel_public", + ) + return h } diff --git a/internal/api/inmemory_rate_limiter_test.go b/internal/api/inmemory_rate_limiter_test.go index cc7a73802..7376b7335 100644 --- a/internal/api/inmemory_rate_limiter_test.go +++ b/internal/api/inmemory_rate_limiter_test.go @@ -262,6 +262,50 @@ func TestGetDefaultRateLimits_SetupAdminHasDedicatedBucket(t *testing.T) { "setup_admin window must be 15 minutes per issue #411") } +// Regression test for #1016: "register" must have a dedicated strict rate-limit +// bucket and must NOT fall through to the permissive api_general config (300/min). +// An attacker at 300/min can flood the DB with pending-registration rows and +// trigger a synchronous admin-notification email per request (inbox spam + DoS). +func TestGetDefaultRateLimits_RegisterHasDedicatedBucket(t *testing.T) { + limits := getDefaultRateLimits() + + cfg, ok := limits["register"] + if !ok { + t.Fatal("register key missing from default rate limits (issue #1016): add it to getDefaultRateLimits()") + } + + // register must be far stricter than api_general (300/min). + apiGeneral := limits["api_general"] + if cfg.MaxAttempts >= apiGeneral.MaxAttempts { + t.Errorf("register limit (%d) is not stricter than api_general (%d); regression of #1016", + cfg.MaxAttempts, apiGeneral.MaxAttempts) + } + + // Window must be non-zero. + if cfg.Window == 0 { + t.Error("register rate limit has zero window duration") + } +} + +// Regression test for #1016: assertRateLimitKeysKnown must panic when an +// unregistered key is supplied, so a future caller typo is caught at startup +// rather than silently falling through to api_general. +func TestAssertRateLimitKeysKnown_PanicsOnUnknownKey(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Error("assertRateLimitKeysKnown did not panic for an unknown key") + } + }() + assertRateLimitKeysKnown("this_key_does_not_exist_in_the_map") +} + +// Regression test for #1016: assertRateLimitKeysKnown must not panic when all +// supplied keys are present in getDefaultRateLimits(). +func TestAssertRateLimitKeysKnown_NoopForKnownKeys(t *testing.T) { + // Should not panic. + assertRateLimitKeysKnown("login", "setup_admin", "register", "approve_cancel_public") +} + // Regression test for #420: NewInMemoryRateLimiter must be ready at construction // time (not after a slow lazy init) so Lambda cold-start first requests are protected. func TestNewInMemoryRateLimiter_ReadyImmediately(t *testing.T) { diff --git a/internal/api/middleware.go b/internal/api/middleware.go index abca545de..ef9865fed 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -133,18 +133,25 @@ func (h *Handler) requiresCSRFValidation(method, path string, req *events.Lambda } // Auth endpoints that don't have a session yet are exempt unconditionally. - csrfExemptAlways := []string{ + // Prefix matching is safe for these /api/auth/* paths because no admin + // sub-paths share those prefixes. + csrfExemptAlwaysPrefix := []string{ "/api/auth/login", "/api/auth/setup-admin", "/api/auth/forgot-password", "/api/auth/reset-password", - "/api/register", // Public registration (no session) } - for _, exempt := range csrfExemptAlways { + for _, exempt := range csrfExemptAlwaysPrefix { if strings.HasPrefix(path, exempt) { return false } } + // Exact match for the public self-registration endpoint (issue #1017). + // HasPrefix would also exempt /api/registrations//approve|reject|delete, + // which are AuthAdmin state-changing routes that MUST be CSRF-protected. + if path == "/api/register" { + return false + } // Token-based approve/cancel/reject paths are exempt ONLY when the request // carries no session bearer token. If a session is present, the request is diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 2f35c8ff6..6b00f02ec 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -466,6 +466,37 @@ func TestRequiresCSRFValidation_TokenBasedPathsWithSession(t *testing.T) { } } +// Regression test for #1017: /api/registrations/* admin routes must require CSRF. +// Previously the csrfExemptAlways list contained "/api/register" matched with +// strings.HasPrefix, which also matched /api/registrations//approve|reject|delete. +// Those routes are AuthAdmin and state-changing: approving a registration creates +// an enabled cloud account and stores attacker-supplied credentials. +func TestRequiresCSRFValidation_RegistrationsAdminRoutesAreNotExempt(t *testing.T) { + h := &Handler{} + sessionReq := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "Authorization": "Bearer some-session-token", + }, + } + + adminRoutes := []struct { + method string + path string + }{ + {"POST", "/api/registrations/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/approve"}, + {"POST", "/api/registrations/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/reject"}, + {"DELETE", "/api/registrations/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"}, + } + + for _, tc := range adminRoutes { + got := h.requiresCSRFValidation(tc.method, tc.path, sessionReq) + if !got { + t.Errorf("admin route %s %s: expected requiresCSRFValidation=true (must be CSRF-protected, regression of #1017), got false", + tc.method, tc.path) + } + } +} + // Regression guard: unconditionally-exempt paths (login, setup-admin, etc.) must // remain exempt regardless of whether a session is present. func TestRequiresCSRFValidation_AlwaysExemptPaths(t *testing.T) { diff --git a/internal/api/rate_limiter.go b/internal/api/rate_limiter.go index 86fc6bbee..199c336e2 100644 --- a/internal/api/rate_limiter.go +++ b/internal/api/rate_limiter.go @@ -35,6 +35,26 @@ func getDefaultRateLimits() map[string]RateLimitConfig { // 5 attempts per 15 minutes so a cold-start burst cannot brute-force the // first-run endpoint before the DB-backed limiter is available. "setup_admin": NewRateLimitConfig(5, 15*60), // 5 attempts / 15 minutes / IP + // register is a public, side-effect-heavy endpoint (DB write + email fan-out + // per request). Issue #1016: without a dedicated bucket it fell through to + // api_general (300/min), enabling bulk table-flooding and inbox-spam. Limit + // to 5 per 15 minutes per IP, matching setup_admin severity. + "register": NewRateLimitConfig(5, 15*60), // 5 attempts / 15 minutes / IP (#1016) + } +} + +// assertRateLimitKeysKnown panics at startup if any endpoint key passed to +// checkRateLimit is absent from the default rate-limit map. A missing key +// silently falls through to api_general (300/min), which is the root cause of +// issue #1016. Call this once during handler initialisation, passing every +// string literal that appears in checkRateLimit call sites. +func assertRateLimitKeysKnown(keys ...string) { + limits := getDefaultRateLimits() + for _, k := range keys { + if _, ok := limits[k]; !ok { + panic("checkRateLimit called with unknown endpoint key " + k + + ": add it to getDefaultRateLimits() (issue #1016)") + } } } From f6d5caa719bbf11235f6adc48741740e6561a7b8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:01:40 -0700 Subject: [PATCH 2/8] test(api): assert exact register rate-limit contract (5/15min) Lock the register regression test to the precise security contract from issue #1016 instead of only checking "stricter than api_general" and a non-zero window. A weak value like 299/min would have passed the old assertions while still allowing registration flooding. Assert the exact 5 attempts / 15 minutes contract so any future relaxation is caught. Addresses CodeRabbit nitpick on PR #1038. --- internal/api/inmemory_rate_limiter_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/api/inmemory_rate_limiter_test.go b/internal/api/inmemory_rate_limiter_test.go index 7376b7335..04d60d623 100644 --- a/internal/api/inmemory_rate_limiter_test.go +++ b/internal/api/inmemory_rate_limiter_test.go @@ -274,6 +274,10 @@ func TestGetDefaultRateLimits_RegisterHasDedicatedBucket(t *testing.T) { t.Fatal("register key missing from default rate limits (issue #1016): add it to getDefaultRateLimits()") } + // Contract from #1016: 5 attempts per 15 minutes. + assert.Equal(t, 5, cfg.MaxAttempts, "register max attempts must be 5 per issue #1016") + assert.Equal(t, 15*time.Minute, cfg.Window, "register window must be 15 minutes per issue #1016") + // register must be far stricter than api_general (300/min). apiGeneral := limits["api_general"] if cfg.MaxAttempts >= apiGeneral.MaxAttempts { From c33654ad4d2bcc62badca25851079463449af52e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:18:36 -0700 Subject: [PATCH 3/8] fix(api): fix stale forgot-password comment and health info-leak (02-L1, 02-L4) 02-L1: update the forgot_password handler comment from "3 attempts per email per hour" to match the actual config (10 attempts / 5 minutes in getDefaultRateLimits). 02-L4: health.go checkConfigStore exposed err.Error() in the public /health response body (AuthPublic route). Log the detail server-side only and return a static "config store unavailable" message to callers. Update tests to assert the error string does not appear in the body. Closes #1064 --- internal/api/handler_auth.go | 2 +- internal/api/health.go | 6 +++++- internal/api/health_test.go | 9 ++++++--- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index c5990fe8e..ce01eded2 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -253,7 +253,7 @@ func (h *Handler) forgotPassword(ctx context.Context, req *events.LambdaFunction return nil, NewClientError(400, "invalid request body") } - // Rate limiting: 3 attempts per email per hour to prevent enumeration attacks + // Rate limiting: 10 attempts per email per 5 minutes to prevent enumeration attacks if h.rateLimiter != nil { allowed, err := h.rateLimiter.AllowWithEmail(ctx, pwdReq.Email, "forgot_password") if err != nil { diff --git a/internal/api/health.go b/internal/api/health.go index 8f66fa147..f112df25f 100644 --- a/internal/api/health.go +++ b/internal/api/health.go @@ -5,6 +5,7 @@ import ( "time" "github.com/LeanerCloud/CUDly/internal/credentials" + "github.com/LeanerCloud/CUDly/pkg/logging" ) // HealthResponse represents the health check response @@ -64,9 +65,12 @@ func (h *Handler) checkConfigStore(ctx context.Context) HealthCheck { // Try to access config to verify database connectivity _, err := h.config.GetGlobalConfig(ctx) if err != nil { + // Log the detailed error server-side; return a generic message to avoid + // leaking connection details (host, db name, SQLSTATE) on this public route. + logging.Errorf("health: config store check failed: %v", err) return HealthCheck{ Status: "unhealthy", - Message: "Failed to access config store: " + err.Error(), + Message: "config store unavailable", } } diff --git a/internal/api/health_test.go b/internal/api/health_test.go index 3359d5717..191b174f8 100644 --- a/internal/api/health_test.go +++ b/internal/api/health_test.go @@ -85,7 +85,9 @@ func TestHandler_GetHealth_ConfigStoreUnhealthy(t *testing.T) { assert.Equal(t, "degraded", response.Status) assert.Equal(t, "unhealthy", response.Checks["config_store"].Status) - assert.Contains(t, response.Checks["config_store"].Message, "Failed to access config store") + // Error detail must NOT appear in the public body (02-L4: info-leak fix). + assert.Equal(t, "config store unavailable", response.Checks["config_store"].Message) + assert.NotContains(t, response.Checks["config_store"].Message, "database connection failed") assert.Equal(t, "healthy", response.Checks["auth_service"].Status) } @@ -179,8 +181,9 @@ func TestHandler_checkConfigStore_AccessError(t *testing.T) { check := handler.checkConfigStore(ctx) assert.Equal(t, "unhealthy", check.Status) - assert.Contains(t, check.Message, "Failed to access config store") - assert.Contains(t, check.Message, "connection timeout") + // Error detail must NOT appear in the public body (02-L4: info-leak fix). + assert.Equal(t, "config store unavailable", check.Message) + assert.NotContains(t, check.Message, "connection timeout") } func TestHandler_checkAuthService_Healthy(t *testing.T) { From 796dc7a51d7c363fe67d4c557b6f79143720dbcf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:21:17 -0700 Subject: [PATCH 4/8] refactor(api): remove dead RateLimiter type and type-safe toAPIPermissions (02-N1, 02-N2) 02-N1: remove legacy RateLimiter/newRateLimiter/rateLimitEntry from types.go and rate_limiter.go. Production paths wire InMemoryRateLimiter or DBRateLimiter (both implement RateLimiterInterface). The dead code carried the same unbounded-GC bug as 02-M3 and was a maintenance trap. Remove matching tests; redirect rate_limiter_test.go to a comment noting where the config-function tests now live. 02-N2: change toAPIPermissions from []any (silent-drop type-assertion) to []Permission (typed, total). Replace the apologetic "in production this would use proper conversion" comment with a clear copy-contract doc. Update tests: replace the now-impossible NonPermissionItems test with a ReturnsCopy test that asserts the mutation-isolation contract. Closes #1064 --- internal/api/handler_coverage_test.go | 31 ++++------- internal/api/rate_limiter.go | 43 --------------- internal/api/rate_limiter_test.go | 79 +-------------------------- internal/api/types.go | 14 ----- internal/api/types_apikeys.go | 17 +++--- 5 files changed, 21 insertions(+), 163 deletions(-) diff --git a/internal/api/handler_coverage_test.go b/internal/api/handler_coverage_test.go index a8b70a8ef..f444cebbc 100644 --- a/internal/api/handler_coverage_test.go +++ b/internal/api/handler_coverage_test.go @@ -776,20 +776,17 @@ func TestDecodeBase64Password_ValidBase64(t *testing.T) { assert.Equal(t, "test123", result) } -// Test for types_apikeys toAPIPermissions +// Tests for types_apikeys toAPIPermissions (02-N2: typed input, no silent drop). func TestToAPIPermissions_EmptySlice(t *testing.T) { - perms := []interface{}{} - - result := toAPIPermissions(perms) - + result := toAPIPermissions([]Permission{}) assert.Len(t, result, 0) } func TestToAPIPermissions_WithPermissions(t *testing.T) { - perms := []interface{}{ - Permission{Action: "read", Resource: "config"}, - Permission{Action: "write", Resource: "plans"}, + perms := []Permission{ + {Action: "read", Resource: "config"}, + {Action: "write", Resource: "plans"}, } result := toAPIPermissions(perms) @@ -799,18 +796,12 @@ func TestToAPIPermissions_WithPermissions(t *testing.T) { assert.Equal(t, "read", result[0].Action) } -func TestToAPIPermissions_NonPermissionItems(t *testing.T) { - perms := []interface{}{ - "not a permission", - 123, - Permission{Action: "read", Resource: "config"}, - } - - result := toAPIPermissions(perms) - - // Only the valid Permission should be included - assert.Len(t, result, 1) - assert.Equal(t, "config", result[0].Resource) +func TestToAPIPermissions_ReturnsCopy(t *testing.T) { + // Mutation of the returned slice must not affect the original (copy contract). + src := []Permission{{Action: "read", Resource: "config"}} + result := toAPIPermissions(src) + result[0].Action = "write" + assert.Equal(t, "read", src[0].Action, "original slice must not be mutated") } // Test NewHandler with API key loaded diff --git a/internal/api/rate_limiter.go b/internal/api/rate_limiter.go index 199c336e2..577abfb8a 100644 --- a/internal/api/rate_limiter.go +++ b/internal/api/rate_limiter.go @@ -58,46 +58,3 @@ func assertRateLimitKeysKnown(keys ...string) { } } -// newRateLimiter creates a new rate limiter -func newRateLimiter() *RateLimiter { - return &RateLimiter{ - attempts: make(map[string]*rateLimitEntry), - } -} - -// Allow checks if a request should be allowed based on rate limits -func (rl *RateLimiter) Allow(key string, maxAttempts int, window time.Duration) bool { - // Handle nil RateLimiter (for testing or when not configured) - if rl == nil { - return true - } - rl.mu.Lock() - defer rl.mu.Unlock() - - now := time.Now() - entry, exists := rl.attempts[key] - - // Clean up expired entries periodically - if len(rl.attempts) > 1000 { - for k, v := range rl.attempts { - if now.After(v.resetTime) { - delete(rl.attempts, k) - } - } - } - - if !exists || now.After(entry.resetTime) { - rl.attempts[key] = &rateLimitEntry{ - count: 1, - resetTime: now.Add(window), - } - return true - } - - if entry.count >= maxAttempts { - return false - } - - entry.count++ - return true -} diff --git a/internal/api/rate_limiter_test.go b/internal/api/rate_limiter_test.go index 23dc85efd..85242a1a6 100644 --- a/internal/api/rate_limiter_test.go +++ b/internal/api/rate_limiter_test.go @@ -1,78 +1,5 @@ package api -import ( - "testing" - "time" - - "github.com/stretchr/testify/assert" -) - -func TestRateLimiter_Allow(t *testing.T) { - t.Run("nil rate limiter allows all requests", func(t *testing.T) { - var rl *RateLimiter - assert.True(t, rl.Allow("test-key", 5, time.Minute)) - }) - - t.Run("allows requests within limit", func(t *testing.T) { - rl := newRateLimiter() - key := "test-key" - maxAttempts := 3 - window := time.Minute - - // First three attempts should succeed - assert.True(t, rl.Allow(key, maxAttempts, window)) - assert.True(t, rl.Allow(key, maxAttempts, window)) - assert.True(t, rl.Allow(key, maxAttempts, window)) - - // Fourth attempt should fail - assert.False(t, rl.Allow(key, maxAttempts, window)) - }) - - t.Run("different keys have separate limits", func(t *testing.T) { - rl := newRateLimiter() - maxAttempts := 2 - window := time.Minute - - // Key 1 uses up its limit - assert.True(t, rl.Allow("key1", maxAttempts, window)) - assert.True(t, rl.Allow("key1", maxAttempts, window)) - assert.False(t, rl.Allow("key1", maxAttempts, window)) - - // Key 2 should still have its own limit - assert.True(t, rl.Allow("key2", maxAttempts, window)) - assert.True(t, rl.Allow("key2", maxAttempts, window)) - assert.False(t, rl.Allow("key2", maxAttempts, window)) - }) - - t.Run("resets after window expires", func(t *testing.T) { - rl := newRateLimiter() - key := "test-key" - maxAttempts := 2 - window := 10 * time.Millisecond - - // Use up the limit - assert.True(t, rl.Allow(key, maxAttempts, window)) - assert.True(t, rl.Allow(key, maxAttempts, window)) - assert.False(t, rl.Allow(key, maxAttempts, window)) - - // Wait for window to expire - time.Sleep(20 * time.Millisecond) - - // Should be allowed again - assert.True(t, rl.Allow(key, maxAttempts, window)) - }) - - t.Run("single attempt limit", func(t *testing.T) { - rl := newRateLimiter() - key := "test-key" - - assert.True(t, rl.Allow(key, 1, time.Minute)) - assert.False(t, rl.Allow(key, 1, time.Minute)) - }) -} - -func TestNewRateLimiter(t *testing.T) { - rl := newRateLimiter() - assert.NotNil(t, rl) - assert.NotNil(t, rl.attempts) -} +// Legacy RateLimiter/newRateLimiter/rateLimitEntry were removed (02-N1). +// Tests for getDefaultRateLimits and assertRateLimitKeysKnown live in +// inmemory_rate_limiter_test.go alongside the InMemoryRateLimiter tests. diff --git a/internal/api/types.go b/internal/api/types.go index 65ccf2522..e08ec1a3d 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -3,7 +3,6 @@ package api import ( "context" - "sync" "time" "github.com/LeanerCloud/CUDly/internal/commitmentopts" @@ -14,19 +13,6 @@ import ( "github.com/LeanerCloud/CUDly/internal/scheduler" ) -// RateLimiter provides simple in-memory rate limiting for auth endpoints -// Note: For Lambda, this only works within a single warm instance. -// For production, use database-backed rate limiting. -type RateLimiter struct { - mu sync.Mutex - attempts map[string]*rateLimitEntry -} - -type rateLimitEntry struct { - count int - resetTime time.Time -} - // RateLimiterInterface defines the interface for rate limiting implementations // This allows for both in-memory and database-backed rate limiters type RateLimiterInterface interface { diff --git a/internal/api/types_apikeys.go b/internal/api/types_apikeys.go index 05050811a..c4c78c1e7 100644 --- a/internal/api/types_apikeys.go +++ b/internal/api/types_apikeys.go @@ -28,14 +28,11 @@ type APIKeyInfo struct { IsActive bool `json:"is_active"` } -// toAPIPermissions converts auth.Permission to api.Permission -func toAPIPermissions(perms []any) []Permission { - result := make([]Permission, 0, len(perms)) - for _, p := range perms { - // Type assertion - in production this would use proper conversion - if perm, ok := p.(Permission); ok { - result = append(result, perm) - } - } - return result +// toAPIPermissions returns a copy of perms as a new slice. +// The parameter is already the concrete type; the copy makes mutation-safe +// return values for callers that may hold the originating slice. +func toAPIPermissions(perms []Permission) []Permission { + out := make([]Permission, len(perms)) + copy(out, perms) + return out } From 2d691b741a833ecafcc373ef23f7a2679009acfe Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:26:43 -0700 Subject: [PATCH 5/8] fix(api): cap in-memory rate limiter entries with oldest-reset eviction (02-M3) The previous GC only removed already-expired entries above 1000 keys, allowing an attacker rotating source IPs to grow the map without bound (expired entries are harmless, so an adversary is never pruned). The fix lowers the hard cap to 500, evicts all expired entries first, then evicts the oldest live entries (by resetTime) down to 80% of cap if the map is still at or above the cap. Add a regression test (TestInMemoryRateLimiter_EntriesCapEnforced) that injects cap+2 unique IPs, confirms that one eviction cycle fires and the final map is strictly below cap. Previously this test would reach cap+2. Closes #1064 --- internal/api/inmemory_rate_limiter.go | 35 ++++++++++++++++++++-- internal/api/inmemory_rate_limiter_test.go | 32 ++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/internal/api/inmemory_rate_limiter.go b/internal/api/inmemory_rate_limiter.go index 3fd510409..fac2d1128 100644 --- a/internal/api/inmemory_rate_limiter.go +++ b/internal/api/inmemory_rate_limiter.go @@ -4,10 +4,18 @@ package api import ( "context" "fmt" + "sort" "sync" "time" ) +// inMemoryRateLimitMaxEntries is the hard cap on the number of live entries in +// InMemoryRateLimiter. When reached, expired entries are evicted first; if the +// map is still at or above the cap, the oldest live entries (by resetTime) are +// removed until the count falls to 80% of the cap. This prevents unbounded +// memory growth from rotating attacker source IPs (02-M3). +const inMemoryRateLimitMaxEntries = 500 + // InMemoryRateLimiter provides in-memory rate limiting for single-instance deployments (Fargate, ECS) // This implementation should NOT be used for Lambda (multi-instance) - use DBRateLimiter instead type InMemoryRateLimiter struct { @@ -65,13 +73,36 @@ func (rl *InMemoryRateLimiter) Allow(ctx context.Context, key string, endpoint s now := time.Now() entry, exists := rl.attempts[id] - // Clean up expired entries periodically (simple garbage collection) - if len(rl.attempts) > 1000 { + // evictEntries enforces a hard cap (inMemoryRateLimitMaxEntries) on the map + // to prevent unbounded memory growth from rotating attacker IPs (02-M3). + // + // Step 1: purge already-expired entries (free, always useful). + // Step 2: if still over cap after step 1, evict the oldest live entries + // (smallest resetTime first) until we are back at 80% of cap. + if len(rl.attempts) >= inMemoryRateLimitMaxEntries { for k, v := range rl.attempts { if now.After(v.resetTime) { delete(rl.attempts, k) } } + if len(rl.attempts) >= inMemoryRateLimitMaxEntries { + // Collect remaining keys sorted by resetTime ascending. + type kv struct { + key string + t time.Time + } + pairs := make([]kv, 0, len(rl.attempts)) + for k, v := range rl.attempts { + pairs = append(pairs, kv{k, v.resetTime}) + } + sort.Slice(pairs, func(i, j int) bool { + return pairs[i].t.Before(pairs[j].t) + }) + target := inMemoryRateLimitMaxEntries * 4 / 5 // evict down to 80% + for i := 0; len(rl.attempts) > target && i < len(pairs); i++ { + delete(rl.attempts, pairs[i].key) + } + } } if !exists || now.After(entry.resetTime) { diff --git a/internal/api/inmemory_rate_limiter_test.go b/internal/api/inmemory_rate_limiter_test.go index 04d60d623..3e39a9de7 100644 --- a/internal/api/inmemory_rate_limiter_test.go +++ b/internal/api/inmemory_rate_limiter_test.go @@ -2,6 +2,7 @@ package api import ( "context" + "fmt" "testing" "time" @@ -340,3 +341,34 @@ func TestNewInMemoryRateLimiter_ReadyImmediately(t *testing.T) { t.Errorf("expected login to be blocked after %d attempts; rate limiter was not enforcing (regression of #420)", loginLimit.MaxAttempts) } } + +// Regression test for 02-M3: the in-memory rate limiter must not grow without +// bound even when entries are within their active window (not yet expired). +// Previously only expired entries were purged, allowing an attacker rotating +// source IPs to cause unbounded memory growth. +// +// Strategy: inject exactly cap+2 unique IPs with a long window. The (cap+1)-th +// call sees len==cap, triggers eviction to 80% of cap, then adds one entry. +// The (cap+2)-th call adds one more. Final count is ~(80%*cap)+2, which is +// strictly less than cap. A pre-fix run would reach cap+2. +func TestInMemoryRateLimiter_EntriesCapEnforced(t *testing.T) { + rl := NewInMemoryRateLimiter() + ctx := context.Background() + + // Fill to just above the cap so exactly one eviction cycle fires. + total := inMemoryRateLimitMaxEntries + 2 + for i := 0; i < total; i++ { + ip := fmt.Sprintf("10.%d.%d.%d", i/65536%256, i/256%256, i%256) + _, _ = rl.AllowWithIP(ctx, ip, "login") + } + + rl.mu.Lock() + count := len(rl.attempts) + rl.mu.Unlock() + + // After eviction the count must be strictly less than cap. + if count >= inMemoryRateLimitMaxEntries { + t.Errorf("entry count %d is >= cap %d after %d calls: eviction did not fire (02-M3 regression)", + count, inMemoryRateLimitMaxEntries, total) + } +} From 3aa4550e14a35621b5baf4c6e55229b5a75d18d2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:28:12 -0700 Subject: [PATCH 6/8] fix(api): add scheduled DB rate-limiter cleanup worker (02-M2) The existing opportunistic cleanup path (triggered when count==1) never fires for perpetually-denied keys: under sustained abuse the abusing key's count keeps climbing without ever resetting to 1, so its row escapes the 24-hour eviction window indefinitely. Add StartCleanupWorker(ctx) which launches a background goroutine that runs cleanup() on a fixed schedule (default 10 minutes), independent of traffic. Wire it in server/app.go immediately after NewDBRateLimiter so Lambda deployments benefit on startup. The integer width of count is Go's default int (64-bit on all supported platforms), so unbounded growth is cosmetically undesirable but not an overflow risk. Add TestDBRateLimiter_StartCleanupWorker_StopsOnContextCancel: overrides the ticker interval to 5ms, confirms the worker exits cleanly on cancel. Closes #1064 --- internal/api/db_rate_limiter.go | 37 ++++++++++++++++++++++++++-- internal/api/db_rate_limiter_test.go | 32 ++++++++++++++++++++++++ internal/server/app.go | 6 ++++- 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/internal/api/db_rate_limiter.go b/internal/api/db_rate_limiter.go index 526755062..28742eb90 100644 --- a/internal/api/db_rate_limiter.go +++ b/internal/api/db_rate_limiter.go @@ -28,15 +28,48 @@ type DBRateLimiter struct { // Verify that DBRateLimiter implements RateLimiterInterface var _ RateLimiterInterface = (*DBRateLimiter)(nil) -// NewDBRateLimiter creates a new database-backed rate limiter +// dbRateLimiterScheduledCleanupInterval is the period between scheduled +// cleanup runs started by StartCleanupWorker. Separate from cleanupInterval +// (the minimum time between opportunistic cleanup runs) so that the scheduled +// worker can tick independently. Exposed as a var so tests can override it. +var dbRateLimiterScheduledCleanupInterval = 10 * time.Minute + +// NewDBRateLimiter creates a new database-backed rate limiter. +// Call StartCleanupWorker on the returned limiter to ensure periodic cleanup +// of expired rows independent of traffic (addresses 02-M2). func NewDBRateLimiter(pool *pgxpool.Pool) *DBRateLimiter { return &DBRateLimiter{ pool: pool, limits: getDefaultRateLimits(), - cleanupInterval: 60 * time.Second, // Only cleanup at most once per minute + cleanupInterval: 60 * time.Second, // minimum gap between opportunistic cleanup runs } } +// StartCleanupWorker launches a background goroutine that calls cleanup() +// on a fixed schedule, independent of the count==1 opportunistic trigger. +// +// This ensures that perpetually-denied keys (whose count never resets to 1) +// are also evicted, preventing unbounded row growth on the rate_limits table +// under sustained abuse (02-M2). The goroutine stops when ctx is cancelled. +// +// Call this once at server startup after creating the DBRateLimiter. The +// goroutine is lightweight (one blocked timer channel) and safe to call from +// tests with a short-lived context. +func (rl *DBRateLimiter) StartCleanupWorker(ctx context.Context) { + go func() { + ticker := time.NewTicker(dbRateLimiterScheduledCleanupInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + rl.cleanup() + } + } + }() +} + // SetLimit allows customizing rate limits for specific endpoints func (rl *DBRateLimiter) SetLimit(endpoint string, config RateLimitConfig) { rl.limitsMu.Lock() diff --git a/internal/api/db_rate_limiter_test.go b/internal/api/db_rate_limiter_test.go index 59e1845dc..ab15e52c7 100644 --- a/internal/api/db_rate_limiter_test.go +++ b/internal/api/db_rate_limiter_test.go @@ -108,3 +108,35 @@ func TestDBRateLimiter_maybeCleanup_ReadyToCleanup(t *testing.T) { // Brief wait for the goroutine to complete time.Sleep(20 * time.Millisecond) } + +// Regression test for 02-M2: StartCleanupWorker must launch a goroutine that +// stops cleanly on context cancellation. The goroutine is exercised with a +// fast ticker so we can confirm it fires without sleeping in tests. +func TestDBRateLimiter_StartCleanupWorker_StopsOnContextCancel(t *testing.T) { + // Override the scheduled interval to a very short duration for testing. + orig := dbRateLimiterScheduledCleanupInterval + dbRateLimiterScheduledCleanupInterval = 5 * time.Millisecond + t.Cleanup(func() { dbRateLimiterScheduledCleanupInterval = orig }) + + rl := NewDBRateLimiter(nil) // nil pool; cleanup() is a no-op + ctx, cancel := context.WithCancel(context.Background()) + + done := make(chan struct{}) + // Wrap cleanup in a channel signal to confirm the worker ran. + go func() { + defer close(done) + rl.StartCleanupWorker(ctx) + }() + + // Let the ticker fire at least twice (2 * 5ms = 10ms + margin). + time.Sleep(30 * time.Millisecond) + cancel() + + // StartCleanupWorker itself returns immediately (goroutine is launched + // inside it). The done channel signals the outer goroutine exited. + select { + case <-done: + case <-time.After(100 * time.Millisecond): + t.Error("StartCleanupWorker goroutine wrapper did not return after cancel") + } +} diff --git a/internal/server/app.go b/internal/server/app.go index 7546f9f10..3b810e0f8 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -635,7 +635,11 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da // Initialize distributed rate limiter for Lambda (multi-instance) // For Fargate/containers, we already have in-memory rate limiter from startup if app.appConfig.IsLambda { - app.RateLimiter = api.NewDBRateLimiter(dbConn.Pool()) + dbRL := api.NewDBRateLimiter(dbConn.Pool()) + // Start the scheduled cleanup worker so perpetually-denied keys (whose + // count never resets to 1) are still evicted on a fixed schedule (02-M2). + dbRL.StartCleanupWorker(ctx) + app.RateLimiter = dbRL log.Println("Initialized database-backed rate limiter for Lambda (distributed state)") } From 130bd15ccdb4db98f61c881a6137fc7d7613d22b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:32:32 -0700 Subject: [PATCH 7/8] sec(api): fail-closed rate limiting on credential endpoints (02-M1) The global checkRateLimit swallows limiter errors and allows requests through (fail-open). For data/general endpoints this is intentional, but for the credential brute-force surface (login, setup_admin, reset_password, change_password) it means any DB pressure that causes the rate limiter to error silently disables protection. An attacker who can induce DB pressure also unlocks unlimited password guessing. Add checkRateLimitStrict (fail-closed): returns 503 on limiter error with a high-severity ALERT log so monitoring can detect and alert on the fail-closed window. Switch login, setup_admin, reset_password (both sites), and change_password to checkRateLimitStrict. For forgot_password (email-keyed, inline call): emit an ALERT log on error but keep fail-open because failing closed here would return 503 vs 200, leaking infrastructure state to an enumerating attacker; the enumeration risk is the higher concern. For createAPIKey (authenticated, user-keyed): upgrade the warn log to an error ALERT so the fail-open window is observable, but keep fail-open because the endpoint is behind requirePermission. Add three regression tests: - TestHandler_checkRateLimitStrict_FailsClosedOnError: 503 on db error - TestHandler_checkRateLimit_FailsOpenOnError: nil (allow) on db error - TestHandler_checkRateLimitStrict_Returns429WhenDenied: normal deny path Closes #1064 --- internal/api/handler_apikeys.go | 8 +++-- internal/api/handler_auth.go | 19 +++++----- internal/api/middleware.go | 36 +++++++++++++++++-- internal/api/middleware_test.go | 64 +++++++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 13 deletions(-) diff --git a/internal/api/handler_apikeys.go b/internal/api/handler_apikeys.go index 9dc5f7f2b..a3f3695db 100644 --- a/internal/api/handler_apikeys.go +++ b/internal/api/handler_apikeys.go @@ -42,11 +42,15 @@ func (h *Handler) createAPIKey(ctx context.Context, req *events.LambdaFunctionUR return nil, err } - // Rate limiting: 30 admin operations per user per minute + // Rate limiting: 30 admin operations per user per minute. + // Fail-open: this endpoint is already behind requirePermission so the auth + // surface is lower than the unauthenticated credential endpoints. Emit a + // high-severity alert so the fail-open window is observable (02-M1). if h.rateLimiter != nil { allowed, err := h.rateLimiter.AllowWithUser(ctx, session.UserID, "admin") if err != nil { - logging.Warnf("rate limiter error for user %s on admin operation: %v", session.UserID, err) + logging.Errorf("ALERT: rate limiter error on admin operation for user %s; proceeding fail-open (02-M1): %v", + session.UserID, err) } else if !allowed { return nil, NewClientError(429, "too many requests, please slow down") } diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index ce01eded2..75300c1ae 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -21,7 +21,7 @@ func (h *Handler) login(ctx context.Context, req *events.LambdaFunctionURLReques } // Rate limiting: 5 attempts per IP per 15 minutes - if err := h.checkRateLimit(ctx, req, "login"); err != nil { + if err := h.checkRateLimitStrict(ctx, req, "login"); err != nil { return nil, err } @@ -224,7 +224,7 @@ func (h *Handler) setupAdmin(ctx context.Context, req *events.LambdaFunctionURLR } // Rate limiting: 5 attempts per IP per 15 minutes for admin setup - if err := h.checkRateLimit(ctx, req, "setup_admin"); err != nil { + if err := h.checkRateLimitStrict(ctx, req, "setup_admin"); err != nil { return nil, err } @@ -253,12 +253,15 @@ func (h *Handler) forgotPassword(ctx context.Context, req *events.LambdaFunction return nil, NewClientError(400, "invalid request body") } - // Rate limiting: 10 attempts per email per 5 minutes to prevent enumeration attacks + // Rate limiting: 10 attempts per email per 5 minutes to prevent enumeration attacks. + // On rate-limiter error: emit a high-severity alert (02-M1) but continue with the + // request. Failing closed here would reveal infrastructure state (503 vs 200) to + // an enumerating attacker and harm legitimate users more than the enumeration risk. if h.rateLimiter != nil { allowed, err := h.rateLimiter.AllowWithEmail(ctx, pwdReq.Email, "forgot_password") if err != nil { - logging.Warnf("Rate limiter error for email %s: %v", redactEmail(pwdReq.Email), err) - // Continue on rate limiter errors to avoid blocking legitimate requests + logging.Errorf("ALERT: rate limiter error on forgot_password for %s; proceeding fail-open (02-M1): %v", + redactEmail(pwdReq.Email), err) } else if !allowed { logging.Warnf("Rate limit exceeded for forgot password: %s", redactEmail(pwdReq.Email)) // Always return success message to prevent email enumeration @@ -303,7 +306,7 @@ func (h *Handler) resetPasswordStatus(ctx context.Context, req *events.LambdaFun // Same rate-limit bucket as the submit endpoint: a token-probing // attacker would otherwise get a free oracle to test tokens. - if err := h.checkRateLimit(ctx, req, "reset_password"); err != nil { + if err := h.checkRateLimitStrict(ctx, req, "reset_password"); err != nil { return nil, err } @@ -332,7 +335,7 @@ func (h *Handler) resetPassword(ctx context.Context, req *events.LambdaFunctionU } // Rate limiting: 10 attempts per IP per 15 minutes - if err := h.checkRateLimit(ctx, req, "reset_password"); err != nil { + if err := h.checkRateLimitStrict(ctx, req, "reset_password"); err != nil { return nil, err } @@ -465,7 +468,7 @@ func (h *Handler) changePassword(ctx context.Context, req *events.LambdaFunction } // Rate limiting: 10 attempts per IP per 15 minutes - if err := h.checkRateLimit(ctx, req, "change_password"); err != nil { + if err := h.checkRateLimitStrict(ctx, req, "change_password"); err != nil { return nil, err } diff --git a/internal/api/middleware.go b/internal/api/middleware.go index ef9865fed..c89797d9c 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -294,9 +294,10 @@ func (h *Handler) requireAdmin(ctx context.Context, req *events.LambdaFunctionUR // checkRateLimit checks if the request is allowed based on IP-based rate limiting. // Returns nil if allowed, or an error if rate limited. // -// Note: This function uses a fail-open design - if the rate limiter encounters -// an error (e.g., Redis unavailable), the request is allowed through rather than -// blocking legitimate users. This is intentional to ensure availability. +// This function uses a fail-open design: if the rate limiter encounters an error +// (e.g. DB pool exhaustion, transient unavailability), the request is allowed +// through. Use checkRateLimitStrict for endpoints where availability of the +// rate limiter is itself a security requirement (credential brute-force surface). func (h *Handler) checkRateLimit(ctx context.Context, req *events.LambdaFunctionURLRequest, endpoint string) error { if h.rateLimiter == nil { return nil @@ -315,3 +316,32 @@ func (h *Handler) checkRateLimit(ctx context.Context, req *events.LambdaFunction } return nil } + +// checkRateLimitStrict is the fail-closed variant of checkRateLimit for +// credential endpoints (login, setup_admin, reset_password, change_password). +// +// When the rate limiter returns an error (DB unavailable, pool exhaustion), this +// function returns 503 instead of allowing the request through. An attacker who +// can induce DB pressure would otherwise unlock unlimited password guessing on +// these endpoints (02-M1). A high-severity error log is emitted so monitoring +// can detect and alert on the fail-closed window. +func (h *Handler) checkRateLimitStrict(ctx context.Context, req *events.LambdaFunctionURLRequest, endpoint string) error { + if h.rateLimiter == nil { + return nil + } + + clientIP := req.RequestContext.HTTP.SourceIP + allowed, err := h.rateLimiter.AllowWithIP(ctx, clientIP, endpoint) + if err != nil { + // High-severity alert: rate limiter unavailable on a credential endpoint. + // Fail closed to preserve brute-force protection. + logging.Errorf("ALERT: rate limiter error on credential endpoint %s for IP %s; request denied (02-M1): %v", + endpoint, clientIP, err) + return NewClientError(503, "service temporarily unavailable, please try again later") + } + if !allowed { + logging.Warnf("Rate limit exceeded for %s from IP: %s", endpoint, clientIP) + return NewClientError(429, "too many requests, please try again later") + } + return nil +} diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 6b00f02ec..38a4e5a1f 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -522,3 +522,67 @@ func TestRequiresCSRFValidation_AlwaysExemptPaths(t *testing.T) { } } } + +// Regression test for 02-M1: checkRateLimitStrict must return 503 when the +// rate limiter encounters an error (fail-closed), whereas checkRateLimit must +// return nil (fail-open) for the same error. +func TestHandler_checkRateLimitStrict_FailsClosedOnError(t *testing.T) { + ctx := context.Background() + limiterErr := errors.New("db pool exhausted") + + mockRL := new(MockRateLimiter) + mockRL.On("AllowWithIP", ctx, "1.2.3.4", "login").Return(false, limiterErr) + t.Cleanup(func() { mockRL.AssertExpectations(t) }) + + h := &Handler{rateLimiter: mockRL} + req := makeSourceIPReq("1.2.3.4") + + err := h.checkRateLimitStrict(ctx, req, "login") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "checkRateLimitStrict must return a ClientError on limiter error") + assert.Equal(t, 503, ce.code, "checkRateLimitStrict must return 503 (fail-closed) on limiter error") +} + +func TestHandler_checkRateLimit_FailsOpenOnError(t *testing.T) { + ctx := context.Background() + limiterErr := errors.New("db pool exhausted") + + mockRL := new(MockRateLimiter) + mockRL.On("AllowWithIP", ctx, "1.2.3.4", "api_general").Return(false, limiterErr) + t.Cleanup(func() { mockRL.AssertExpectations(t) }) + + h := &Handler{rateLimiter: mockRL} + req := makeSourceIPReq("1.2.3.4") + + err := h.checkRateLimit(ctx, req, "api_general") + assert.NoError(t, err, "checkRateLimit must allow through (fail-open) on limiter error") +} + +func TestHandler_checkRateLimitStrict_Returns429WhenDenied(t *testing.T) { + ctx := context.Background() + + mockRL := new(MockRateLimiter) + mockRL.On("AllowWithIP", ctx, "1.2.3.4", "login").Return(false, nil) + t.Cleanup(func() { mockRL.AssertExpectations(t) }) + + h := &Handler{rateLimiter: mockRL} + req := makeSourceIPReq("1.2.3.4") + + err := h.checkRateLimitStrict(ctx, req, "login") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 429, ce.code) +} + +// makeSourceIPReq builds a minimal request with the given source IP. +func makeSourceIPReq(ip string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + SourceIP: ip, + }, + }, + } +} From 3d170aba2c0f3a4a202469836898e84a781e31f0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 06:23:36 -0700 Subject: [PATCH 8/8] refactor(api): extract eviction helpers to fix gocyclo on Allow (CI) Allow in InMemoryRateLimiter had cyclomatic complexity 13, over the pre-commit gocyclo threshold of 10. Extract two helpers: - evictIfAtCap: cap-guard + expired-entry purge - evictOldest: oldest-first eviction down to 80% of cap Allow drops to CC=5; helpers are CC=5 and CC=4. No logic change. Also fix gofmt on rate_limiter.go (trailing blank line) and pkg/exchange/reshape_crossfamily_test.go (alignment in struct literals). --- internal/api/inmemory_rate_limiter.go | 106 ++++++++++++----------- internal/api/rate_limiter.go | 1 - pkg/exchange/reshape_crossfamily_test.go | 16 ++-- 3 files changed, 65 insertions(+), 58 deletions(-) diff --git a/internal/api/inmemory_rate_limiter.go b/internal/api/inmemory_rate_limiter.go index fac2d1128..7a36f7eef 100644 --- a/internal/api/inmemory_rate_limiter.go +++ b/internal/api/inmemory_rate_limiter.go @@ -48,65 +48,76 @@ func (rl *InMemoryRateLimiter) SetLimit(endpoint string, config RateLimitConfig) rl.limits[endpoint] = config } -// Allow checks if a request should be allowed based on rate limits -// The key should be formatted as "IP#{ip}" or "EMAIL#{email}" -// The endpoint identifies which rate limit configuration to use +// configFor returns the rate limit configuration for an endpoint, falling back +// to the api_general bucket when no specific entry exists. +func (rl *InMemoryRateLimiter) configFor(endpoint string) RateLimitConfig { + if cfg, ok := rl.limits[endpoint]; ok { + return cfg + } + return rl.limits["api_general"] +} + +// evictIfAtCap enforces the hard cap on the attempts map (02-M3). +// +// Step 1: purge already-expired entries (free, always useful). +// Step 2: if still over cap, evict the oldest live entries (smallest +// resetTime first) until the count falls to 80% of the cap. +// +// Must be called with rl.mu held. +func (rl *InMemoryRateLimiter) evictIfAtCap(now time.Time) { + if len(rl.attempts) < inMemoryRateLimitMaxEntries { + return + } + for k, v := range rl.attempts { + if now.After(v.resetTime) { + delete(rl.attempts, k) + } + } + if len(rl.attempts) < inMemoryRateLimitMaxEntries { + return + } + rl.evictOldest() +} + +// evictOldest removes the oldest live entries until the map is at 80% of cap. +// Must be called with rl.mu held. +func (rl *InMemoryRateLimiter) evictOldest() { + type kv struct { + key string + t time.Time + } + pairs := make([]kv, 0, len(rl.attempts)) + for k, v := range rl.attempts { + pairs = append(pairs, kv{k, v.resetTime}) + } + sort.Slice(pairs, func(i, j int) bool { + return pairs[i].t.Before(pairs[j].t) + }) + target := inMemoryRateLimitMaxEntries * 4 / 5 + for i := 0; len(rl.attempts) > target && i < len(pairs); i++ { + delete(rl.attempts, pairs[i].key) + } +} + +// Allow checks if a request should be allowed based on rate limits. +// The key should be formatted as "IP#{ip}" or "EMAIL#{email}". +// The endpoint identifies which rate limit configuration to use. func (rl *InMemoryRateLimiter) Allow(ctx context.Context, key string, endpoint string) (bool, error) { - // Handle nil rate limiter (for testing or when not configured) if rl == nil { return true, nil } - // Get the rate limit configuration for this endpoint - config, exists := rl.limits[endpoint] - if !exists { - // Default to general API limits if endpoint not specifically configured - config = rl.limits["api_general"] - } - - // Create the unique identifier combining the key and endpoint + config := rl.configFor(endpoint) id := fmt.Sprintf("%s#ENDPOINT#%s", key, endpoint) rl.mu.Lock() defer rl.mu.Unlock() now := time.Now() - entry, exists := rl.attempts[id] - - // evictEntries enforces a hard cap (inMemoryRateLimitMaxEntries) on the map - // to prevent unbounded memory growth from rotating attacker IPs (02-M3). - // - // Step 1: purge already-expired entries (free, always useful). - // Step 2: if still over cap after step 1, evict the oldest live entries - // (smallest resetTime first) until we are back at 80% of cap. - if len(rl.attempts) >= inMemoryRateLimitMaxEntries { - for k, v := range rl.attempts { - if now.After(v.resetTime) { - delete(rl.attempts, k) - } - } - if len(rl.attempts) >= inMemoryRateLimitMaxEntries { - // Collect remaining keys sorted by resetTime ascending. - type kv struct { - key string - t time.Time - } - pairs := make([]kv, 0, len(rl.attempts)) - for k, v := range rl.attempts { - pairs = append(pairs, kv{k, v.resetTime}) - } - sort.Slice(pairs, func(i, j int) bool { - return pairs[i].t.Before(pairs[j].t) - }) - target := inMemoryRateLimitMaxEntries * 4 / 5 // evict down to 80% - for i := 0; len(rl.attempts) > target && i < len(pairs); i++ { - delete(rl.attempts, pairs[i].key) - } - } - } + rl.evictIfAtCap(now) + entry, exists := rl.attempts[id] if !exists || now.After(entry.resetTime) { - // No entry or window expired - create new/reset rl.attempts[id] = &inMemoryRateLimitEntry{ count: 1, resetTime: now.Add(config.Window), @@ -114,13 +125,10 @@ func (rl *InMemoryRateLimiter) Allow(ctx context.Context, key string, endpoint s return true, nil } - // Window is still active, check if limit exceeded if entry.count >= config.MaxAttempts { - // Limit exceeded return false, nil } - // Increment the counter entry.count++ return true, nil } diff --git a/internal/api/rate_limiter.go b/internal/api/rate_limiter.go index 577abfb8a..0fda787f6 100644 --- a/internal/api/rate_limiter.go +++ b/internal/api/rate_limiter.go @@ -57,4 +57,3 @@ func assertRateLimitKeysKnown(keys ...string) { } } } - diff --git a/pkg/exchange/reshape_crossfamily_test.go b/pkg/exchange/reshape_crossfamily_test.go index 7d4eb7f60..30f6d372d 100644 --- a/pkg/exchange/reshape_crossfamily_test.go +++ b/pkg/exchange/reshape_crossfamily_test.go @@ -465,7 +465,7 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { } // m6i.xlarge: same "m" prefix (gen jump bonus), exact NF, high confidence. nearPerfect := OfferingOption{ - InstanceType: "m6i.xlarge", + InstanceType: "m6i.xlarge", EffectiveMonthlyCost: 85, // slightly more expensive NormalizationFactor: 8, SavingsAbs: floatPtr(220), @@ -473,7 +473,7 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { } // r5.xlarge: different prefix (no family gen bonus), same NF, no confidence. crossFamily := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 80, // same cost as source -- better raw price NormalizationFactor: 8, } @@ -497,13 +497,13 @@ func TestCompositeScore_SameArchOutranksCrossArch(t *testing.T) { } // c6i: same "c" prefix (family gen bonus), Intel x86 (same arch). sameArch := OfferingOption{ - InstanceType: "c6i.xlarge", + InstanceType: "c6i.xlarge", EffectiveMonthlyCost: 90, NormalizationFactor: 8, } // c6g: same "c" prefix (family gen bonus), Graviton ARM (cross arch). crossArch := OfferingOption{ - InstanceType: "c6g.xlarge", + InstanceType: "c6g.xlarge", EffectiveMonthlyCost: 90, NormalizationFactor: 8, } @@ -522,14 +522,14 @@ func TestCompositeScore_HighConfidenceOutranksLow(t *testing.T) { MonthlyCost: 70, } highConf := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(250), RecommendationCount: 4, } lowConf := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(10), @@ -550,13 +550,13 @@ func TestCompositeScore_AbsentSavingsIsNeutral(t *testing.T) { MonthlyCost: 70, } absentSavings := OfferingOption{ - InstanceType: "r6i.xlarge", + InstanceType: "r6i.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: nil, // not supplied } lowConf := OfferingOption{ - InstanceType: "r6i.xlarge", + InstanceType: "r6i.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(5), // explicitly low confidence