sec(api): add "register" rate-limit bucket and fix CSRF prefix match (#1016, #1017) (#1038)

* 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/<uuid>/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

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

* 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

* 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

* 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

* 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

* 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

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

Author: cristim

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

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

internal/api/handler.go

@@ -180,6 +180,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
 }
 

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

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: 3 attempts per email per hour 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
 	}
 

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

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

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