chore: add rate limiter and redis#283
Conversation
6cb37d4 to
cf77460
Compare
alagoa
left a comment
There was a problem hiding this comment.
Looking good, just some suggestions particularly on cross container interaction.
Also, should we consider adding rate limiting to the sign-in/login routes as well?
We could extend it to sign-in/login and others but I want to get this one route working first (and merged in), then we can do that in a follow-up |
94fa605 to
71dfc2b
Compare
alagoa
left a comment
There was a problem hiding this comment.
Looks good, so we test this out with the validation email endpoint, then roll it out to the other public routes?
|
We could test a bit on the PR env |
47ceb61 to
d01b3ef
Compare
d01b3ef to
eab0464
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces Redis-backed request rate limiting (via fastapi-limiter) and wires Redis into local/dev/test environments, plus adds scheduled background jobs in the FastAPI lifespan to run team monitoring and hard-delete maintenance tasks.
Changes:
- Add Redis service/configuration and wire
REDIS_URLthrough settings, docker-compose, and test Makefile targets. - Initialize
FastAPILimiteron app startup and apply aRateLimiterdependency to/auth/validate-email. - Add APScheduler-based recurring jobs in
app.mainformonitor_teamsandhard_delete_expired_teamswith DB-based locking.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
app/main.py |
Initializes Redis rate limiter; adds APScheduler cron jobs with DB locks during lifespan. |
app/api/auth.py |
Applies rate limiting to the /auth/validate-email endpoint. |
app/core/config.py |
Adds REDIS_URL and RATE_LIMIT_VALIDATE_EMAIL settings. |
docker-compose.yml |
Adds a Redis service and makes backend depend on it; sets backend REDIS_URL. |
Makefile |
Adds a Redis container for backend test targets and exports REDIS_URL into test runs. |
requirements.txt |
Adds fastapi-limiter, redis, and pyrate-limiter dependencies. |
tests/conftest.py |
Adds an autouse mock to bypass Redis rate limiting in tests; adds an httpx.AsyncClient patch fixture. |
tests/test_team_keys_policy.py |
Minor test file import/whitespace changes. |
tests/test_auth.py |
Minor whitespace-only change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…imit threshold - Add lock_acquired flag to monitor_teams_job and hard_delete_teams_job to ensure release_lock() is only called in the outer exception handler when this process actually acquired the lock, preventing accidental release of another process's active lock - Replace hard-coded > 5 threshold in conftest.py rate limiter mock with > times to dynamically use the configured limit from evalsha arguments Agent-Logs-Url: https://github.com/amazeeio/amazee.ai/sessions/95fc923c-8caf-4072-a62d-e924affb1b01 Co-authored-by: dan2k3k4 <158704+dan2k3k4@users.noreply.github.com>
- Fix mock_evalsha to use int(times) comparison (fastapi_limiter passes times as str) - Replace AsyncMock http_callback with real coroutine that raises HTTPException(429) - Add test_validate_email_rate_limit to verify HTTP 429 after exceeding limit Agent-Logs-Url: https://github.com/amazeeio/amazee.ai/sessions/a09f1e03-703d-41b5-ba70-d919cb6e5705 Co-authored-by: dan2k3k4 <158704+dan2k3k4@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _: None = Depends( | ||
| RateLimiter(times=settings.RATE_LIMIT_VALIDATE_EMAIL, seconds=60) | ||
| ), |
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
The test test_validate_email_rate_limit was already added in commit 188c3ee. It makes settings.RATE_LIMIT_VALIDATE_EMAIL (default: 5) requests and asserts the next one returns HTTP 429. The conftest mock enforces the threshold dynamically via int(times) from the evalsha call, so it stays in sync with any configured value.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Use separate sessions for lock acquisition, job execution, and lock release - matching the monitor_teams_job pattern. This prevents partial job mutations from being committed if the job fails and release_lock() is called on the same dirty session. Agent-Logs-Url: https://github.com/amazeeio/amazee.ai/sessions/0ef5f678-aa06-4c14-b6a9-7421c39fcbcb Co-authored-by: dan2k3k4 <158704+dan2k3k4@users.noreply.github.com>
smlx
left a comment
There was a problem hiding this comment.
To confirm: is redis planned to be used for purposes other than rate limiting in future? The reason I ask is that the email validation is not expected to be a high-throughput endpoint, so maybe using database backing for the rate limiter and avoiding adding the complexity of redis just for this feature might be an option?
From what I can tell, if redis goes down this will currently fail closed which means that email validation will not be available for some period if redis restarts. Is that right?
|
@dan2k3k4 please address failing tests |
|
@copilot check and fix backend-tests |
…oundError Agent-Logs-Url: https://github.com/amazeeio/amazee.ai/sessions/f01c5274-cda5-4686-b905-7d3133ffa6d1 Co-authored-by: dan2k3k4 <158704+dan2k3k4@users.noreply.github.com>
... Fixed in commit |
| _: None = Depends( | ||
| RateLimiter(times=settings.RATE_LIMIT_VALIDATE_EMAIL, seconds=60) | ||
| ), |
There was a problem hiding this comment.
Rate limit bypassable via
X-Forwarded-For spoofing
The fastapi-limiter default identifier reads X-Forwarded-For first and takes split(",")[0]. If the reverse proxy appends to (rather than replaces) this header, an attacker can pre-set X-Forwarded-For: 1.2.3.4 on each request and appear as a new IP every time, effectively bypassing the rate limit and enabling unlimited validation-code emails to arbitrary addresses. A custom identifier should be used that either strips the header or uses a trusted proxy-only value.
| # Get cookie domain from LAGOON_ROUTES | ||
| cookie_domain = get_cookie_domain() | ||
|
|
||
| # Set cookie expiration based on user role |
There was a problem hiding this comment.
The comment
# Get cookie domain from LAGOON_ROUTES is now orphaned — the get_cookie_domain() call it referenced was refactored into the walrus-operator expression below. The blank line and stale comment are misleading.
| # Get cookie domain from LAGOON_ROUTES | |
| cookie_domain = get_cookie_domain() | |
| # Set cookie expiration based on user role | |
| # Set cookie expiration based on user role |
| async def lifespan(app: FastAPI): | ||
| # Initialize rate limiter | ||
| await FastAPILimiter.init( | ||
| redis=AsyncRedis.from_url(settings.REDIS_URL, decode_responses=True) | ||
| ) |
There was a problem hiding this comment.
Redis unavailability now prevents app startup
FastAPILimiter.init() calls redis.script_load(lua_script), which requires an active Redis connection. If Redis is unavailable when the app starts, the lifespan raises an exception and the app cannot serve any traffic — even endpoints with no rate limiting. The docker-compose.yml guards against this with condition: service_healthy, but this constraint doesn't exist in every deployment path (e.g., CI pipelines, bare-metal deployments, or Kubernetes rolling restarts where Redis may be transiently unreachable). Consider wrapping the init in a try/except with a warning log so the app can start degraded without rate limiting.
Greptile Summary
This PR introduces Redis-backed rate limiting via
fastapi-limiteron the/auth/validate-emailendpoint (5 requests/60 s) and migrates the existingmonitor_teams/hard_delete_teamsbackground work from ad-hoc execution into an APScheduler with distributed locking. Redis is wired in throughdocker-compose.yml, Makefile test targets, and a newREDIS_URLconfig setting./auth/validate-email:RateLimiter(times=5, seconds=60)is added as a FastAPI dependency; tests are mocked via a newautousefixture that emulates the Redis Lua-script return value and enforces limits in test names containingrate_limit.monitor_teams_jobandhard_delete_teams_jobare scheduled with environment-aware cron triggers and distributed DB locks to prevent duplicate execution across replicas.docker-compose.ymlwith healthcheck;backendnow declaresdepends_on: redis: condition: service_healthy.Confidence Score: 3/5
The rate limiting on /auth/validate-email can be bypassed by any client that spoofs X-Forwarded-For, defeating the primary protection the feature is meant to provide.
The rate limit identifier trusts a client-controlled header, making it trivially bypassable. Redis is also now a hard startup dependency with no degraded-mode fallback.
app/api/auth.py and app/main.py need the most attention — the rate-limiter identifier and the hard Redis startup dependency.
Security Review
X-Forwarded-Forspoofing on rate-limited endpoint (app/api/auth.py): Thefastapi-limiterdefault identifier readsX-Forwarded-Forand usessplit(\",\")[0]. If the upstream proxy appends rather than replaces this header, an attacker can pre-set an arbitrary IP value on each request and bypass the 5-request-per-minute limit on/auth/validate-email, enabling unlimited validation-code email delivery to arbitrary addresses or email enumeration.Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Proxy participant FastAPI participant RateLimiter participant Redis participant ValidateEmail Client->>Proxy: POST /auth/validate-email Proxy->>FastAPI: forward request (X-Forwarded-For: client_ip) FastAPI->>RateLimiter: resolve dependency RateLimiter->>Redis: EVALSHA lua_sha 1 ip:path times ms alt under limit Redis-->>RateLimiter: 0 (allowed) RateLimiter-->>FastAPI: continue FastAPI->>ValidateEmail: execute handler ValidateEmail-->>Client: 200 OK else limit exceeded Redis-->>RateLimiter: pexpire ms remaining RateLimiter-->>Client: 429 Too Many Requests end Note over FastAPI: lifespan startup FastAPI->>Redis: FastAPILimiter.init script_load FastAPI->>APScheduler: scheduler.start APScheduler-->>FastAPI: jobs registered monitor_teams hard_delete_teamsReviews (1): Last reviewed commit: "fix: add apscheduler==3.11.2 to requirem..." | Re-trigger Greptile