feat(ENG-27): add rate limiting and account lockout on login#11
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 (3)
tests/conftest.py (1)
23-26: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider guarding against pointing test DB derivation at a non-local host.
Deriving
TEST_DATABASE_URL/admin_urlfromsettings.database_url(previously hardcoded to localhost) means a misconfiguredDATABASE_URLenv var (e.g. pointing at a shared/staging Postgres) would cause the test suite to silently create/connect totrading_test_dband thepostgresadmin DB on that remote host instead of failing fast locally.♻️ Optional safety guard
db_url = settings.database_url.get_secret_value() parsed = urlsplit(db_url) +if parsed.hostname not in {"localhost", "127.0.0.1", "db", "postgres"}: + raise RuntimeError( + f"Refusing to run tests against non-local database host: {parsed.hostname!r}" + ) TEST_DATABASE_URL = urlunsplit(parsed._replace(path="/trading_test_db")) admin_url = urlunsplit(parsed._replace(path="/postgres"))🤖 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 `@tests/conftest.py` around lines 23 - 26, The test DB URL derivation in conftest.py now depends on settings.database_url, so add a safety check before building TEST_DATABASE_URL and admin_url to ensure the parsed host is local (for example, localhost/127.0.0.1 or equivalent). If the host is not local, fail fast with a clear error instead of silently rewriting the path in the urlsplit/urlunsplit flow; keep the guard close to the db_url parsing logic so the behavior is enforced wherever the settings-based database URL is used.tests/test_rate_limiting.py (1)
80-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRate-limit counters aren't reset before this test — assumption of exactly 10 prior hits is fragile.
The shared
limiteruses an in-memory store keyed by client address that isn't cleared between tests. This test assumes it starts from a clean count for its key; if the store retains hits from an earlier enabled-limiter run in the same process (test retries,pytest -k, future tests hitting/auth/loginwith the limiter enabled), the 429 could trigger earlier than request#11, making this test flaky.♻️ Suggested fix: reset storage before asserting counts
from ai_trading_discipline_copilot.core.limiter import limiter # Temporarily enable the limiter limiter.enabled = True + limiter.reset() try:Please confirm
Limiter/its storage backend in slowapi 0.1.9 exposes a.reset()(or equivalent) to clear counters between test runs.🤖 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 `@tests/test_rate_limiting.py` around lines 80 - 102, The login rate-limit test is flaky because it assumes the shared Limiter starts with zero hits for the client key, but the in-memory counter may persist across tests. Update test_rate_limiting_on_login to clear or reset the limiter storage before sending requests, using the Limiter instance from ai_trading_discipline_copilot.core.limiter (or its backend equivalent) so the 11th request is the first guaranteed 429. If slowapi’s Limiter exposes a reset/clear method, call it in setup/teardown around the assertions; otherwise add the appropriate storage reset hook before enabling limiter.enabled.src/ai_trading_discipline_copilot/routers/auth.py (1)
27-27: 🔒 Security & Privacy | 🔵 TrivialPer-process rate limiting won't hold under multiple workers.
The shared
limiter(core/limiter.py) uses the default in-memory storage (nostorage_uri). If the app runs with multiple Uvicorn/Gunicorn workers or is horizontally scaled, each process tracks its own counters, so the effective global rate for/login,/register,/forgot-password, and/resend-verificationbecomes roughly N× the configured limit for N workers/replicas — weakening the brute-force protection these decorators are meant to provide. Consider a shared backend (e.g. Redis) forLimiter(storage_uri=...)before running with more than one worker in production.Also applies to: 148-148
🤖 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 `@src/ai_trading_discipline_copilot/routers/auth.py` at line 27, The auth routes rely on a process-local rate limiter import, so `/login`, `/register`, `/forgot-password`, and `/resend-verification` are only enforced per worker. Update the shared `limiter` defined in `core/limiter.py` to use a distributed storage backend via `Limiter(storage_uri=...)` (for example Redis), and make sure the decorators in `routers/auth.py` keep using that shared instance so limits remain global across multiple Uvicorn/Gunicorn workers or replicas.
🤖 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 `@src/ai_trading_discipline_copilot/core/config.py`:
- Around line 53-56: The shared default in Config.allowed_hosts should not
include the test-only host, since it is passed directly into
TrustedHostMiddleware via settings.allowed_hosts in main.py. Remove "testserver"
from the default list in config.py and keep only safe production defaults, then
add "testserver" only in test-specific configuration such as a test .env/CI
override or a direct override in tests/conftest.py.
In `@src/ai_trading_discipline_copilot/services/auth_service.py`:
- Around line 54-85: The failed-login counter update in auth_service is subject
to a lost-update race because `user.failed_login_attempts += 1` is done in
memory before `db.commit()`. Update the lockout path in the authentication flow
to use an atomic database-side increment (for example via an `UPDATE` on the
user record) so concurrent failed attempts cannot overwrite each other. Make
sure the same logic still sets `lockout_until` when
`settings.max_login_attempts` is reached and keep the existing lockout
checks/reset behavior in this auth service method.
---
Nitpick comments:
In `@src/ai_trading_discipline_copilot/routers/auth.py`:
- Line 27: The auth routes rely on a process-local rate limiter import, so
`/login`, `/register`, `/forgot-password`, and `/resend-verification` are only
enforced per worker. Update the shared `limiter` defined in `core/limiter.py` to
use a distributed storage backend via `Limiter(storage_uri=...)` (for example
Redis), and make sure the decorators in `routers/auth.py` keep using that shared
instance so limits remain global across multiple Uvicorn/Gunicorn workers or
replicas.
In `@tests/conftest.py`:
- Around line 23-26: The test DB URL derivation in conftest.py now depends on
settings.database_url, so add a safety check before building TEST_DATABASE_URL
and admin_url to ensure the parsed host is local (for example,
localhost/127.0.0.1 or equivalent). If the host is not local, fail fast with a
clear error instead of silently rewriting the path in the urlsplit/urlunsplit
flow; keep the guard close to the db_url parsing logic so the behavior is
enforced wherever the settings-based database URL is used.
In `@tests/test_rate_limiting.py`:
- Around line 80-102: The login rate-limit test is flaky because it assumes the
shared Limiter starts with zero hits for the client key, but the in-memory
counter may persist across tests. Update test_rate_limiting_on_login to clear or
reset the limiter storage before sending requests, using the Limiter instance
from ai_trading_discipline_copilot.core.limiter (or its backend equivalent) so
the 11th request is the first guaranteed 429. If slowapi’s Limiter exposes a
reset/clear method, call it in setup/teardown around the assertions; otherwise
add the appropriate storage reset hook before enabling limiter.enabled.
🪄 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 Plus
Run ID: d117936d-4c8a-4dd0-bff8-2cbe7a8ae30f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.pre-commit-config.yamlalembic/versions/54a7fb8168af_add_lockout_columns_to_users.pypyproject.tomlsrc/ai_trading_discipline_copilot/core/config.pysrc/ai_trading_discipline_copilot/core/limiter.pysrc/ai_trading_discipline_copilot/main.pysrc/ai_trading_discipline_copilot/models/user.pysrc/ai_trading_discipline_copilot/routers/auth.pysrc/ai_trading_discipline_copilot/services/auth_service.pytests/conftest.pytests/test_rate_limiting.py
2 participants
Summary by CodeRabbit
New Features
Bug Fixes