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

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

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

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