fix: add rate limiting to auth endpoints#3398
Conversation
|
Someone is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds rate limiting to Supabase cookie-auth ChangesAuthentication Rate Limiting
Development Configuration Cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/main.py (1)
1154-1155: 🔒 Security & Privacy | 🔵 TrivialConfirm
X-Forwarded-Fortrust forget_remote_address.
get_remote_address(used askey_func) derives the client identity fromX-Forwarded-For. If this app is deployed behind a proxy without stripping/validating that header (or is directly internet-facing), an attacker can spoof it to bypass the per-IP login/signup limits, defeating the brute-force protection this PR aims to add. Slowapi's own docs recommend pairing this with something like uvicorn'sProxyHeadersMiddlewarefor robust IP resolution.Also applies to: 1175-1176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/main.py` around lines 1154 - 1155, The login/signup rate limiting is using get_remote_address as the key function, which can trust X-Forwarded-For and be spoofed if proxy headers are not validated. Update the rate-limiter setup around auth_login and the signup handler to use a client-IP source that is only trusted behind a properly configured proxy, and ensure the app is wrapped/configured with ProxyHeadersMiddleware or equivalent so X-Forwarded-For is stripped/validated before slowapi sees it. If the deployment cannot guarantee trusted proxy headers, switch the key_func to a safer identity source instead of get_remote_address.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Around line 27-30: The .gitignore rule for .vscode currently ignores the
directory itself, which blocks the later exception for extensions.json. Update
the ignore pattern to target the contents of .vscode instead, or explicitly
unignore the .vscode directory before the !.vscode/extensions.json exception so
the file can be tracked correctly.
In `@backend/main.py`:
- Around line 1154-1155: The auth handlers in main.py use Response in their
function signatures, but it is not imported, causing module import failure.
Update the FastAPI import list in backend/main.py to include Response so
auth_login and auth_signup can load without NameError.
---
Nitpick comments:
In `@backend/main.py`:
- Around line 1154-1155: The login/signup rate limiting is using
get_remote_address as the key function, which can trust X-Forwarded-For and be
spoofed if proxy headers are not validated. Update the rate-limiter setup around
auth_login and the signup handler to use a client-IP source that is only trusted
behind a properly configured proxy, and ensure the app is wrapped/configured
with ProxyHeadersMiddleware or equivalent so X-Forwarded-For is
stripped/validated before slowapi sees it. If the deployment cannot guarantee
trusted proxy headers, switch the key_func to a safer identity source instead of
get_remote_address.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aed1f5af-6bd2-4cc8-b41f-a91b1926c3b4
📒 Files selected for processing (4)
.gitignore.npm-cache/_update-notifier-last-checked.vscode/settings.jsonbackend/main.py
💤 Files with no reviewable changes (1)
- .vscode/settings.json
| .npm-cache/ | ||
| .npm/ | ||
| .vscode/ | ||
| !.vscode/extensions.json |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
git check-ignore -v .vscode/extensions.jsonRepository: ritesh-1918/HELPDESK.AI
Length of output: 208
Rework the .vscode ignore rule
Ignoring the directory itself prevents !.vscode/extensions.json from bringing that file back in. Use .vscode/* instead, or unignore .vscode/ before the exception.
Suggested fix
-.vscode/
+.vscode/*
!.vscode/extensions.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .npm-cache/ | |
| .npm/ | |
| .vscode/ | |
| !.vscode/extensions.json | |
| .npm-cache/ | |
| .npm/ | |
| .vscode/* | |
| !.vscode/extensions.json |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitignore around lines 27 - 30, The .gitignore rule for .vscode currently
ignores the directory itself, which blocks the later exception for
extensions.json. Update the ignore pattern to target the contents of .vscode
instead, or explicitly unignore the .vscode directory before the
!.vscode/extensions.json exception so the file can be tracked correctly.
| @limiter.limit("10/minute") | ||
| async def auth_login(body: LoginBody, request: Request, response: Response): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `Response` is imported anywhere in backend/main.py
rg -n 'Response' backend/main.py | head -30
rg -n 'from __future__ import annotations' backend/main.pyRepository: ritesh-1918/HELPDESK.AI
Length of output: 1799
Import Response in backend/main.py response: Response is used in multiple function signatures here, but Response is never imported. That raises NameError at module import and prevents /auth/login and /auth/signup from loading. Add Response to the FastAPI import list.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 1155-1155: Undefined name Response
(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/main.py` around lines 1154 - 1155, The auth handlers in main.py use
Response in their function signatures, but it is not imported, causing module
import failure. Update the FastAPI import list in backend/main.py to include
Response so auth_login and auth_signup can load without NameError.
Source: Linters/SAST tools
1 participant
Summary
slowapirate limits to/auth/loginand/auth/signupRequestin each endpoint signatureCloses #3344
Validation
python3 -m py_compile backend/main.pygit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Chores