Skip to content

fix: make rate-limit windows atomic across instances - #1296

Merged
Baskarayelu merged 1 commit into
CredenceOrg:mainfrom
greatest0fallt1me:fix/1240-distributed-rate-limit
Aug 30, 2026
Merged

Baskarayelu merged 1 commit into
CredenceOrg:mainfrom
greatest0fallt1me:fix/1240-distributed-rate-limit

Conversation

@greatest0fallt1me

Copy link
Copy Markdown
Contributor

Summary

  • Execute Redis increment/expiry/TTL as one Lua operation so concurrent instances cannot race between commands or leave an unexpired bucket.
  • Include tenant identity and optional route scope in bounded, hashed bucket keys; the application-wide API limiter enables route scope while auth retains its shared tenant surface.
  • Preserve tier and tenant override behavior, including fixed-window expiry and per-key precedence.
  • Keep dependency failure explicit: fail-closed returns 503 for sensitive configurations, while opt-in fail-open remains available for non-sensitive routes.
  • Add shared multi-instance, concurrent-limit, tenant-isolation, route-scope, override/expiry, XFF, and dependency-failure coverage.
  • Document threat model, compatibility, migration, rollback, and operational checks.

Acceptance criteria

  • Limits are enforced consistently across middleware instances sharing Redis through an atomic Lua counter/expiry operation.
  • Bucket identity includes tenant/IP, API-key/tier where applicable, and route scope for the application API; user-controlled dimensions are hashed.
  • Atomic increments and fixed-window expiry prevent race-based overshoot and missing-expiry failures.
  • Redis outage behavior is explicit and configurable; fail-closed remains the safe production behavior.
  • Existing forwarded-IP, tenant override, tier, headers, metrics, fallback-adapter, and auth tests remain passing.

Security and correctness note

The client-controlled leftmost X-Forwarded-For value is never used. The limiter uses the socket peer or trusted authenticated identity, and Redis performs the counter decision atomically. A client cannot create a second tenant bucket by changing separators or route text because the complete identity is hashed before key construction.

The compatibility command path is retained only for minimal adapters/tests. The production Redis client uses EVAL; the deployment checklist documents verifying that capability. Sensitive routes should use failOpen: false so a dependency outage cannot silently remove abuse protection.

Compatibility and rollback

The public middleware factory and legacy rateLimit helper remain compatible. Existing auth behavior continues to share the tenant bucket across login and refresh. The application API opts into route scope explicitly. Keys use the existing namespace prefixes with a window ID, so old counters expire naturally and no database migration is required. Rollback is schema-safe but may restore the prior weaker race behavior and should be monitored.

Validation

  • npx vitest run tests/routes/rateLimit.test.ts tests/routes/authRateLimit.test.ts tests/routes/rateLimitDistributed.test.ts — 62 passing, 0 failing
  • npx eslint src/middleware/rateLimit.ts src/app.ts — 0 errors; 4 pre-existing logger-schema warnings in src/app.ts
  • git diff --check — passed
  • 501 lines added across implementation, distributed tests, and operational documentation
  • npm run build — blocked by pre-existing invalid-character errors in unrelated src/db/repositories/auditChainVerificationRepository.ts and src/db/repositories/baseRepository.ts

Related issue

Closes #1240

@Baskarayelu
Baskarayelu merged commit 3412325 into CredenceOrg:main Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Quality][Medium] Add distributed rate-limit correctness across instances

2 participants