sec(server): "not Lambda" read as "single instance" pins the in-memory rate limiter on multi-replica platforms #109

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Where

• internal/runtime/runtime.go:18-20 (IsLambda(), the only environment discriminator in the codebase)
• internal/server/app.go:470-481 (the in-memory limiter and the comment asserting single-instance)
• internal/server/app.go:750-757 (the DB-backed limiter, wired only if app.appConfig.IsLambda)
• internal/api/inmemory_rate_limiter.go:19-20 (the type doc)
• internal/api/inmemory_rate_limiter.go:105-134 (Allow never returns a non-nil error)
• internal/api/rate_limiter.go:27 (login: 5 attempts / 15 min / IP)
• internal/api/middleware.go:519-546 (checkRateLimitStrict) and internal/api/middleware.go:554-560 (the fail-closed 503 branch)
• internal/api/router.go:584-607 and internal/api/router.go:858-865 (the unauthenticated approve / cancel / reject token endpoints)
• terraform/modules/compute/gcp/cloud-run/variables.tf:44-47, terraform/modules/compute/azure/container-apps/variables.tf:54-58, terraform/modules/compute/azure/aks/main.tf:615, terraform/modules/compute/gcp/gke/main.tf:654

What

IsLambda() is a single boolean over the presence of AWS_LAMBDA_RUNTIME_API, and app.go branches the rate-limiter choice on it. Lambda gets the DB-backed distributed limiter; everything else keeps the process-local in-memory one permanently.

The premise is stated outright at app.go:470-474:

// Initialize rate limiter based on runtime environment.
// Lambda: start with an in-memory limiter immediately so the first cold-start
// request is protected. ensureDB() swaps it for the DB-backed limiter once the
// database connection is established (distributed state across warm containers).
// Fargate/containers: in-memory is the permanent implementation because the
// process is long-lived and single-instance.

and the limiter's own type doc at inmemory_rate_limiter.go:19-20 encodes the same misconception:

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

The root misconception is right there in the type doc: it frames "multi-instance" as a property of Lambda. It is not. Multi-instance is a property of horizontal autoscaling, which three of the supported deployment targets do by default while being !IsLambda. Naming that explicitly is what stops this being fixed with a note; every rewording that keeps "Lambda == multi-instance" reproduces the bug.

The wiring at app.go:750-757 carries the same assumption in its comment: "Initialize distributed rate limiter for Lambda (multi-instance) / For Fargate/containers, we already have in-memory rate limiter from startup."

Failure scenario

The premise is false for three supported targets, all !IsLambda, all defaulting to 10 replicas:

Each replica holds an independent counter map, and the platform ingress spreads a single attacker's requests across all of them.

login is 5 attempts / 15 min / IP (rate_limiter.go:27). Across 10 replicas the effective ceiling becomes roughly 50 attempts / 15 min / IP. The same 10x applies to setup_admin, reset_password and change_password, which together with login are the four endpoints checkRateLimitStrict exists to protect as the credential brute-force surface (middleware.go:519-546).

Critically, the same 10x applies to approve_cancel_public, which guards the unauthenticated purchase approve / cancel / reject token endpoints (router.go:584-607, router.go:858-865). That turns it into a 10x-faster approval-token guessing budget on a money path, where a single successful guess commits a purchase with no session and no attributable identity.

Compounding it: InMemoryRateLimiter.Allow never returns a non-nil error (inmemory_rate_limiter.go:105-134). So the fail-closed 503 branch at middleware.go:554-560, the hardening whose entire purpose is to refuse the request when the limiter cannot answer, is unreachable on every non-Lambda deployment. That protection exists only on Lambda, and nothing signals its absence elsewhere.

Fix direction

Discriminate "distributed" from "single-instance" rather than inferring it from "not Lambda":

1. Add an explicit configuration flag for distribution, or simply wire the DB-backed limiter unconditionally in reinitializeAfterConnect whenever a database is present, keeping in-memory only as the pre-connect cold-start stopgap it already is on Lambda. The second option is strictly safer and removes the branch entirely.
2. Correct both comments and the type doc so they stop asserting that multi-instance is a Lambda property. Suggested framing: in-memory is safe only when the deployment is provably a single replica, which no default in this repo is.
3. Make InMemoryRateLimiter.Allow able to signal failure, or ensure the strict path treats an in-memory limiter as ineligible, so the 503 branch is not silently dead.

Related

• Filed as type/security and priority/p1: the outcome is a 10x weakening of the credential brute-force ceiling plus a 10x-faster approval-token guessing budget on an unauthenticated money path.
• The single-boolean-as-three-properties design that makes this class of bug likely is recorded as a LOW in the grouped server/scheduler issue from this pass; this issue is its first live consequence.
• The two-sources-of-truth IsLambda finding is filed separately and interacts with this: app.go:455 reads runtime.IsLambda() live while :477 reads cfg.IsLambda.