fix(security): rate-limit /auth/jwt/login and /auth/register (#644)

[frankbria]

Summary

Closes #644 (P0 beta blocker, security).

The SlowAPI Limiter is constructed without default_limits/application_limits, so it only throttles routes that carry an explicit @limiter.limit(...) decorator. The fastapi-users auth routes (/auth/jwt/login, /auth/register) are mounted via library factory functions and were never decorated, so the existing rate_limit_auth() (10/min) never applied to them — leaving login and registration open to unthrottled credential brute-force.

Fix

• codeframe/lib/rate_limiter.py — new enforce_auth_rate_limit, a FastAPI dependency that runs the auth-tier rate-limit check at request time (the @limiter.limit decorator can't be used here because it binds at import time and must wrap an endpoint we own). Buckets are namespaced per-endpoint path + per-client, so login and register throttle independently.
• New get_auth_rate_limit_key — an auth-specific key that fails closed for undeterminable client IPs (one stable ip:unknown bucket) instead of the per-request-unique bucket get_rate_limit_key assigns for general routes. This prevents bypassing the limit by stripping the source IP. General-route behavior is unchanged.
• codeframe/auth/router.py — attach the dependency to the /auth/jwt and /auth/register mounts (mirroring the existing allow_registration dependency). On the register mount it runs first so throttling precedes the bootstrap 403 check.
• Reuses the existing app-level RateLimitExceeded handler → 429 + headers + audit log.

Acceptance criteria

• /auth/jwt/login returns 429 after exceeding the auth-tier limit.
• /auth/register is likewise limited.
• Test asserts 429 under repeated attempts.

Tests

tests/ui/test_auth_rate_limit.py (4 tests, @pytest.mark.v2):

• test_login_is_rate_limited — login returns 429 once the limit is exceeded.
• test_register_is_rate_limited — register likewise.
• test_unknown_client_fails_closed — undeterminable-IP clients share a stable bucket and are still throttled.
• test_no_throttle_when_disabled — dependency is a strict no-op when rate limiting is off.

Verification: full tests/ui/ suite (398) + the new tests pass; ruff clean. Cross-family review (codex) raised one Major (auth fail-open for unknown IPs) which is fixed here.

Known limitations

• Reset-password/verify routers are currently commented out in auth/router.py; when enabled they should receive the same dependency (out of scope here).
• With in-memory storage, multi-worker deployments keep per-process buckets — an existing limitation shared by the whole rate limiter; the Redis storage path is unchanged.

Summary by CodeRabbit

• New Features

  • Added rate limiting to authentication endpoints, including login and registration, returning HTTP 429 when exceeded.
  • Requests with an undetectable client IP are handled safely (“fail closed”) using a shared throttling bucket.
  • Rate limiting can be disabled without triggering throttling responses.

• Tests

  • Added UI/integration tests covering rate-limit enforcement on login and registration, unknown-client behavior, and the disabled-rate-limiting scenario.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 02ff1d63-0d4a-45b4-9b23-fa0de852086f

📥 Commits

Reviewing files that changed from the base of the PR and between 210b34f and 855853c.

📒 Files selected for processing (2)

• codeframe/lib/rate_limiter.py
• tests/ui/test_auth_rate_limit.py

🚧 Files skipped from review as they are similar to previous changes (2)

• codeframe/lib/rate_limiter.py
• tests/ui/test_auth_rate_limit.py

---

Walkthrough

Adds auth-tier rate limiting for the fastapi-users-mounted /auth/jwt and /auth/register routes. A new enforce_auth_rate_limit FastAPI dependency in rate_limiter.py calls slowapi's internal .hit(...) method directly and raises RateLimitExceeded on breach. The auth router is updated to inject this dependency, and four new tests verify throttling, fail-closed IP fallback, and disabled-mode behavior.

Changes

Auth Endpoint Rate Limiting

Layer / File(s)	Summary
Auth-tier rate limit key and dependency codeframe/lib/rate_limiter.py	Adds parse_rate_limit import, get_auth_rate_limit_key() for stable per-IP bucket generation (falls back to ip:unknown), and enforce_auth_rate_limit() async dependency that calls limiter.limiter.hit(...) and raises RateLimitExceeded when the configured auth limit is exceeded.
Auth router dependency wiring codeframe/auth/router.py	Imports enforce_auth_rate_limit and injects it as a Depends(...) on both the /auth/jwt JWT router and the /auth/register registration router, ensuring throttling runs before the bootstrap-only registration gate.
Auth rate limit tests tests/ui/test_auth_rate_limit.py	Adds _reset_rate_limit_state fixture, _build_client/_statuses helpers, and four tests covering login throttling, register throttling, fail-closed unknown-IP behavior, and no-throttle when rate limiting is disabled.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 Hippity-hop, the login door now knocks,
Too many tries? A 429 it blocks!
Unknown IP? We share one stable box,
No sneaking past with per-request magic tricks.
The rabbit guards the gate — no brute-force shocks! 🔒

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main security fix: adding rate-limiting to authentication endpoints to prevent credential brute-force attacks.
Linked Issues check	✅ Passed	The PR implementation meets all acceptance criteria from issue #644: /auth/jwt/login and /auth/register are rate-limited via the new enforce_auth_rate_limit dependency, and comprehensive tests verify 429 responses under repeated attempts.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to addressing the #644 security vulnerability; issue #39 (Playwright tests) is unrelated but does not represent out-of-scope code changes in this PR.
Docstring Coverage	✅ Passed	Docstring coverage is 90.91% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/644-rate-limit-auth-endpoints

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review — fix(security): rate-limit /auth/jwt/login and /auth/register (#644)

Overall: Approve with minor notes. This is a well-scoped, correctly reasoned security fix. The core logic is sound, the fail-closed behavior for unknown IPs is the right call, and the four tests cover the important cases. Notes below are improvements rather than blockers.

---

What the PR does

The SlowAPI @limiter.limit decorator can only be applied to endpoints the project owns. Since /auth/jwt/login and /auth/register are mounted via fastapi-users factory functions, they were never decorated and were open to unthrottled brute-force. The fix adds enforce_auth_rate_limit — a FastAPI dependency that manually calls the SlowAPI backend — and attaches it to both mounts.

---

Security — correct

• Fail-closed for unknown IPs: get_auth_rate_limit_key returns a shared stable bucket (ip:unknown) when the IP is undeterminable, preventing the bypass present in the general-route key function.
• Dependency ordering on register: enforce_auth_rate_limit runs before allow_registration, so throttling fires before the bootstrap 403 check. Correct — do not leak state to an unthrottled prober.
• Per-endpoint scoping: using request.url.path as scope means login and register maintain independent buckets.

---

Code quality — minor issues

1. Private API surface (limiter.limiter.hit)

This accesses the inner limits library object via SlowAPI's private attribute. It works, and it's the only viable path given the decorator constraint, but it is fragile against SlowAPI upgrades. A short comment referencing the constraint would help future maintainers:

# SlowAPI's public @limiter.limit API can only decorate functions we own.
# For library-mounted routes we must call the inner limits backend directly.
if not limiter.limiter.hit(limit.limit, limit.scope, key):

2. Limit object reconstructed per request

parse_rate_limit(config.auth_limit) and the resulting Limit object are created on every call. The parsed limit never changes at runtime, so this is safe but slightly wasteful. Not a blocker — the storage hit dominates — but worth noting if this becomes hot.

3. Docstring mismatch in get_auth_rate_limit_key

The Returns block says "ip:{address}" but the function can return "ip:unknown" via get_client_ip. Small documentation debt: the docstring should mention the "ip:unknown" case explicitly.

---

Tests — good coverage, one clarification worth adding

• Four tests cover the four meaningful cases (login throttled, register throttled, unknown-IP fail-closed, disabled no-op). That is exactly the right set.
• The _reset_rate_limit_state fixture's pre+post reset is correct — prevents contamination in either direction.
• importlib.reload(server) after env changes is the right pattern for these integration tests.
• The secret-scanner comment explaining the split password literals is a nice touch.

One edge case worth clarifying: test_register_is_rate_limited sends payload as form data (data=payload), but FastAPI-Users register expects JSON. Requests within the limit would return 422, and requests beyond the limit would return 429. The assertions hold either way, but a comment or switching to json=payload for the register case would make intent clearer.

---

Known limitations (documented — acceptable)

• In-memory storage with per-process buckets in multi-worker deployments: existing limitation, out of scope.
• Reset-password/verify routes commented out: noted in PR description, tracked for when enabled.

---

Checklist

• Closes P0 security issue
• Fail-closed for unknown IPs
• Tests pass (tests/ui/ + new tests)
• Ruff clean
• CLAUDE.md conventions followed (auth logic in correct layer, no core imports in lib)
• Minor docstring fix (non-blocking)

[claude]

Follow-up Review

All three notes from my previous review have been addressed in the docs commit:

• Private API comment — the explanation for why limiter.limiter.hit is called directly is now in the code.
• Docstring — get_auth_rate_limit_key Returns block now explicitly documents the ip:unknown fallback.
• Register test intent — the comment clarifying the form-data-vs-JSON tradeoff is in place.

Approve. Security fix is correct, fail-closed behavior is verified, and the code is now self-documenting. Ready to merge.