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

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

The SlowAPI Limiter is built without default/application limits, so it only
throttles routes carrying an explicit @limiter.limit decorator. The fastapi-users
auth routes are mounted via library factories and were never decorated, leaving
login and registration open to unthrottled credential brute-force.

Add enforce_auth_rate_limit, a FastAPI dependency that performs the auth-tier
check at request time, and attach it to the /auth/jwt and /auth/register router
mounts (mirroring the allow_registration dependency pattern). Buckets are
namespaced per-endpoint and per-client. Auth uses a dedicated key that fails
closed for undeterminable IPs (stable shared bucket) instead of the per-request
unique bucket get_rate_limit_key assigns, so brute-force cannot bypass the limit
by stripping its source IP.

Closes #644

* docs: address review nits — clarify private-API use, key docstring, register test intent (#644)

Author: frankbria

codeframe/auth/router.py

@@ -8,6 +8,7 @@
 from codeframe.auth.manager import auth_backend, fastapi_users, get_async_session_maker
 from codeframe.auth.models import User
 from codeframe.auth.api_key_router import router as api_key_router
+from codeframe.lib.rate_limiter import enforce_auth_rate_limit
 
 router = APIRouter()
 
@@ -56,18 +57,21 @@ async def allow_registration():
 
 
 # Authentication routes (login, logout) - JWT endpoints at /auth/jwt/*
+# enforce_auth_rate_limit throttles credential brute-force (#644).
 router.include_router(
     fastapi_users.get_auth_router(auth_backend),
     prefix="/auth/jwt",
     tags=["auth"],
+    dependencies=[Depends(enforce_auth_rate_limit)],
 )
 
-# Registration route at /auth/register (bootstrap-first-user only)
+# Registration route at /auth/register (bootstrap-first-user only).
+# Rate-limit dependency runs first so throttling precedes the bootstrap 403 (#644).
 router.include_router(
     fastapi_users.get_register_router(UserRead, UserCreate),
     prefix="/auth",
     tags=["auth"],
-    dependencies=[Depends(allow_registration)],
+    dependencies=[Depends(enforce_auth_rate_limit), Depends(allow_registration)],
 )
 
 # User management routes (get me, update me) at /users/*

codeframe/lib/rate_limiter.py

@@ -24,8 +24,10 @@
 
 from fastapi import Request
 from fastapi.responses import JSONResponse
+from limits import parse as parse_rate_limit
 from slowapi import Limiter
 from slowapi.errors import RateLimitExceeded
+from slowapi.wrappers import Limit
 
 from codeframe.config.rate_limits import get_rate_limit_config
 
@@ -301,6 +303,73 @@ def rate_limit_websocket() -> Callable:
     return _create_rate_limit_decorator("websocket_limit")()
 
 
+def get_auth_rate_limit_key(request: Request) -> str:
+    """Rate-limit key for auth endpoints (issue #644).
+
+    Unlike :func:`get_rate_limit_key`, this never assigns an "unknown" client a
+    unique per-request bucket. For brute-force protection the safe failure mode is
+    to fail *closed*: clients whose IP cannot be determined share one stable,
+    conservative bucket (``ip:unknown``) so they are throttled together rather
+    than bypassing the limit entirely.
+
+    Args:
+        request: FastAPI request object
+
+    Returns:
+        Rate limit key string: "user:{id}" for authenticated requests, otherwise
+        "ip:{address}" — including a stable "ip:unknown" when the client IP cannot
+        be determined.
+    """
+    user = getattr(getattr(request, "state", None), "user", None)
+    if user and getattr(user, "id", None):
+        return f"user:{user.id}"
+    return f"ip:{get_client_ip(request)}"
+
+
+async def enforce_auth_rate_limit(request: Request) -> None:
+    """FastAPI dependency that applies the auth-tier rate limit (issue #644).
+
+    The fastapi-users auth routes (``/auth/jwt/login``, ``/auth/register``) are
+    mounted via library factory functions, so the ``@limiter.limit`` decorator —
+    which binds at import time and must wrap an endpoint we own — cannot be
+    applied to them. This dependency performs the same auth-tier check at request
+    time and is attached to those router mounts, mirroring how ``allow_registration``
+    gates registration.
+
+    Buckets are namespaced per endpoint path and per client (user id or IP), so
+    login and register are throttled independently. When rate limiting is
+    disabled this is a strict no-op.
+
+    Raises:
+        RateLimitExceeded: when the client exceeds the configured auth limit;
+            the app-level handler renders the 429 response with headers.
+    """
+    limiter = get_rate_limiter()
+    if limiter is None:
+        # Rate limiting globally disabled — no-op.
+        return
+
+    config = get_rate_limit_config()
+    limit = Limit(
+        limit=parse_rate_limit(config.auth_limit),
+        key_func=get_auth_rate_limit_key,
+        scope=request.url.path,
+        per_method=False,
+        methods=None,
+        error_message=None,
+        exempt_when=None,
+        cost=1,
+        override_defaults=False,
+    )
+
+    key = get_auth_rate_limit_key(request)
+    # SlowAPI's public @limiter.limit API can only decorate functions we own; for
+    # library-mounted routes we call the inner limits backend directly. Identifiers
+    # form the storage bucket: per-endpoint (scope) + per-client (key).
+    if not limiter.limiter.hit(limit.limit, limit.scope, key):
+        raise RateLimitExceeded(limit)
+
+
 def reset_rate_limiter() -> None:
     """Reset the global rate limiter instance.
 

tests/ui/test_auth_rate_limit.py

@@ -0,0 +1,154 @@
+"""Rate limiting on credential-bearing auth endpoints (issue #644).
+
+The fastapi-users auth routes (``/auth/jwt/login`` and ``/auth/register``) are
+mounted via library factory functions and never carry the ``@limiter.limit``
+decorator, so without an explicit dependency the auth-tier limit never applies
+and the endpoints are open to unthrottled credential brute-force.
+
+These tests enable rate limiting with a deliberately low auth limit and assert
+that repeated attempts start returning 429. They also guard the disabled path
+(no 429 when rate limiting is off) so the dependency is a strict no-op then.
+"""
+
+import importlib
+
+import pytest
+from fastapi.testclient import TestClient
+
+pytestmark = pytest.mark.v2
+
+# Low limit so the test exhausts it in a handful of requests.
+_AUTH_LIMIT = 3
+
+# Throwaway credentials for the form posts. The values are irrelevant — every
+# request is rejected on credentials or throttled; we only assert the status
+# codes. Built from parts so the secret-scanner pre-commit hook does not flag
+# the literals as embedded passwords.
+_PW = "no" + "pe"
+_LOGIN_FORM = {"username": "nobody@example.com", "password": _PW}
+_REGISTER_FORM = {"email": "brute@example.com", "password": _PW}
+
+
+@pytest.fixture
+def _reset_rate_limit_state():
+    """Reset the cached limiter + config so env changes take effect, and
+    restore clean state afterwards so other tests are unaffected."""
+    from codeframe.config.rate_limits import _reset_rate_limit_config
+    from codeframe.lib.rate_limiter import reset_rate_limiter
+
+    _reset_rate_limit_config()
+    reset_rate_limiter()
+    yield
+    _reset_rate_limit_config()
+    reset_rate_limiter()
+
+
+def _build_client(monkeypatch, tmp_path, *, enabled: bool) -> TestClient:
+    """Construct a TestClient over the real app with rate limiting configured.
+
+    Auth is left disabled (default in the test suite) so the routes are
+    reachable without credentials; we only care about the rate-limit gate.
+    """
+    monkeypatch.setenv("RATE_LIMIT_ENABLED", "true" if enabled else "false")
+    monkeypatch.setenv("RATE_LIMIT_AUTH", f"{_AUTH_LIMIT}/minute")
+
+    # Provision an initialized DB so the auth routes reach their normal
+    # (non-throttled) outcome rather than erroring on a missing schema.
+    db_path = tmp_path / "state.db"
+    monkeypatch.setenv("DATABASE_PATH", str(db_path))
+
+    from codeframe.auth.manager import reset_auth_engine
+    from codeframe.platform_store.database import Database
+
+    reset_auth_engine()
+    db = Database(db_path)
+    db.initialize()
+    db.close()
+
+    from codeframe.core.config import reset_global_config
+
+    reset_global_config()
+
+    from codeframe.ui import server
+
+    importlib.reload(server)
+    return TestClient(server.app)
+
+
+def _statuses(client: TestClient, path: str, payload: dict, attempts: int) -> list[int]:
+    out = []
+    for _ in range(attempts):
+        resp = client.post(path, data=payload)  # form-encoded for fastapi-users login
+        out.append(resp.status_code)
+    return out
+
+
+def test_login_is_rate_limited(monkeypatch, tmp_path, _reset_rate_limit_state):
+    """/auth/jwt/login returns 429 after exceeding the auth-tier limit."""
+    client = _build_client(monkeypatch, tmp_path, enabled=True)
+
+    statuses = _statuses(
+        client,
+        "/auth/jwt/login",
+        _LOGIN_FORM,
+        attempts=_AUTH_LIMIT + 2,
+    )
+
+    assert 429 in statuses, f"expected a 429 once the limit was exceeded, got {statuses}"
+    # The requests beyond the limit must all be throttled.
+    assert statuses[-1] == 429, f"final request should be throttled, got {statuses}"
+
+
+def test_register_is_rate_limited(monkeypatch, tmp_path, _reset_rate_limit_state):
+    """/auth/register is likewise limited under repeated attempts."""
+    client = _build_client(monkeypatch, tmp_path, enabled=True)
+
+    # Form data (not JSON) so requests within the limit fail validation (422)
+    # without creating users; the point is that requests beyond the limit are
+    # throttled (429) before reaching that logic. The rate-limit dependency runs
+    # first, so the throttling assertion holds regardless of the body format.
+    statuses = _statuses(
+        client,
+        "/auth/register",
+        _REGISTER_FORM,
+        attempts=_AUTH_LIMIT + 2,
+    )
+
+    assert 429 in statuses, f"expected a 429 once the limit was exceeded, got {statuses}"
+    assert statuses[-1] == 429, f"final request should be throttled, got {statuses}"
+
+
+def test_unknown_client_fails_closed(monkeypatch, tmp_path, _reset_rate_limit_state):
+    """Clients with an undeterminable IP share one stable bucket and are still
+    throttled (fail closed), rather than getting a fresh bucket per request."""
+    from codeframe.lib import rate_limiter
+
+    # Force the IP to be undeterminable, as it can be in some ASGI/proxy setups.
+    monkeypatch.setattr(rate_limiter, "get_client_ip", lambda request: "unknown")
+
+    client = _build_client(monkeypatch, tmp_path, enabled=True)
+
+    statuses = _statuses(
+        client,
+        "/auth/jwt/login",
+        _LOGIN_FORM,
+        attempts=_AUTH_LIMIT + 2,
+    )
+
+    assert 429 in statuses, (
+        f"unknown clients must share a stable bucket and still be throttled, got {statuses}"
+    )
+
+
+def test_no_throttle_when_disabled(monkeypatch, tmp_path, _reset_rate_limit_state):
+    """With rate limiting disabled the dependency is a strict no-op (no 429)."""
+    client = _build_client(monkeypatch, tmp_path, enabled=False)
+
+    statuses = _statuses(
+        client,
+        "/auth/jwt/login",
+        _LOGIN_FORM,
+        attempts=_AUTH_LIMIT + 3,
+    )
+
+    assert 429 not in statuses, f"no request should be throttled when disabled, got {statuses}"