fix(ratelimit): fail open on a Redis outage, matching login_throttle (#418)

slowapi's Limiter re-raises whatever its storage raises, and
swallow_errors=True doesn't help either since the header-injection code
that runs afterward reads request.state.view_rate_limit unconditionally,
which is only set when evaluation finishes without raising. Either way a
Redis outage 500'd every rate-limited endpoint, confirmed against a real
unreachable Redis.

Add FailOpenRedisStorage, a thin Redis storage wrapper that answers "zero
hits, resets now" instead of raising when Redis cannot answer. That reads
as "not limited" to the strategy above it, so the request goes through
with a warning logged rather than a 500 -- the same fail-open policy
login_throttle.py already documents and follows for its own Redis calls.

Refs #399

Author: haksungjang

CHANGELOG.md

@@ -313,6 +313,24 @@ and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.
   rolled back and the caller re-fetches the winner's row, so the rest of the
   scan's transaction survives intact (#398-A).
 
+- **A Redis outage could turn every rate-limited endpoint into a 500.**
+  `core/ratelimit.py` built slowapi's `Limiter` against plain `redis://`, and
+  left to its defaults slowapi re-raises whatever its storage raises;
+  `swallow_errors=True` does not save it either, since the header-injection
+  code that runs after a swallowed exception reads
+  `request.state.view_rate_limit` unconditionally, and that attribute is
+  only set once evaluation finishes without raising. Confirmed against a
+  real unreachable Redis rather than assumed from reading slowapi's source.
+  A new `FailOpenRedisStorage` answers "zero hits, resets now" instead of
+  raising when Redis cannot answer, which reads as "not limited" to the
+  strategy above it and lets the request through with a warning logged
+  instead of a 500. This makes the login rate limit and every other
+  `@limiter.limit(...)` endpoint fail open on a Redis outage, matching the
+  policy `login_throttle.py`'s per-address slowdown already documented and
+  followed. Both mechanisms are now proven independently, each with the
+  other's Redis path left healthy, so neither test's pass depends on the
+  other control also being down.
+
 - **The vulnerability drawer's response builder could drop a field silently.**
   `_detail_response` named all 41 fields of `VulnerabilityDetailResponse` as
   keyword arguments by hand, so a field the service already computed but

apps/backend/core/ratelimit.py

@@ -19,21 +19,145 @@
   - Storage is Redis (shared across uvicorn workers / Celery beats), not
     slowapi's default in-memory dict, so the 5/min budget is enforced
     correctly under multi-worker deployments.
+
+Redis failure policy (issue #399): fail open, the same policy `login_throttle.py`
+already documents for the login guesswork slowdown. A rate limit's job is to
+keep one caller from crowding out everyone else; it is not the last line of
+defence for anything it guards, and turning a Redis outage into a site-wide
+outage would make a shared cache a hard dependency of every limited endpoint,
+login included, for the sake of a control that is explicitly a courtesy
+rather than a wall.
+
+That policy is not the one slowapi ships with. Left to its defaults,
+`Limiter` re-raises whatever its storage raises: `RateLimitExceeded` first
+became a real question during this issue when a Redis restart in dev turned
+every limited endpoint into a 500 instead of the plain pass-through the name
+"rate limit" implies. `swallow_errors=True` does not fix it either -- slowapi
+only sets `request.state.view_rate_limit` once its evaluation loop finishes
+without raising, and the header-injection code that runs after the swallowed
+exception reads that attribute unconditionally, so the endpoint still 500s
+with an `AttributeError` instead of a `RedisError`. Confirmed by driving both
+configurations against an unreachable Redis directly (see
+`tests/unit/test_rate_limit.py`); neither is a shape this module can rely on.
+
+So the fix sits one layer down, in the storage `FailOpenRedisStorage`
+plugs in below. `Limiter.hit()` only ever asks its storage two things:
+"how many hits does this key have" (`incr`, `get`) and "when does it reset"
+(`get_expiry`). Answering "zero" and "now" for those during an outage is
+indistinguishable, from the strategy's point of view, from a key that has
+never been hit -- the request is allowed, the evaluation loop finishes
+normally, and `request.state.view_rate_limit` gets set exactly like the
+healthy path. Nothing upstream needs to know Redis was unreachable; slowapi
+only sees answers it already knows how to handle.
 """
 
 from __future__ import annotations
 
 import os
+import time
+from typing import Any
 
+import structlog
 from fastapi import Request
 from fastapi.responses import JSONResponse
+from limits.storage.redis import RedisStorage
+from redis.exceptions import RedisError
 from slowapi import Limiter
 from slowapi.errors import RateLimitExceeded
 from slowapi.util import get_remote_address
 
 from core.config import redis_url
 from core.errors import PROBLEM_CONTENT_TYPE
 
+log = structlog.get_logger("ratelimit")
+
+#: A blocked call here sits in front of every limited endpoint, so it gets the
+#: same short leash `login_throttle.py` gives its own Redis client: without a
+#: bound, a Redis that drops packets rather than refusing them hangs the
+#: request instead of failing it, which defeats the point of failing open.
+_SOCKET_TIMEOUT_SECONDS = 1.0
+
+#: `redis://` and `rediss://` rewritten to schemes `FailOpenRedisStorage`
+#: registers itself under, so `Limiter(storage_uri=...)` picks our subclass
+#: instead of `limits`' own `RedisStorage`. `redis+unix://` is intentionally
+#: left alone: nothing in this deployment uses it, and guessing at a
+#: transform for a scheme nobody exercises is worse than leaving it on the
+#: standard (fail-closed) storage and finding out.
+_FAIL_OPEN_SCHEMES = {"redis": "redis+failopen", "rediss": "rediss+failopen"}
+
+
+def _fail_open_storage_uri(url: str) -> str:
+    """Rewrite a `redis(s)://` URL to the scheme `FailOpenRedisStorage` owns."""
+    scheme, sep, rest = url.partition("://")
+    mapped = _FAIL_OPEN_SCHEMES.get(scheme)
+    if not sep or mapped is None:
+        return url
+    return f"{mapped}{sep}{rest}"
+
+
+def _degraded(action: str, exc: Exception) -> None:
+    """Redis could not answer a rate-limit check. Say so, then allow it.
+
+    Mirrors `login_throttle._degraded`: the warning exists so an outage is
+    visible to somebody reading logs, since the outward behaviour is the same
+    limiter quietly not counting anything until Redis answers again.
+    """
+    log.warning("ratelimit.storage_unavailable", action=action, error=str(exc))
+
+
+class FailOpenRedisStorage(RedisStorage):
+    """`limits`' Redis storage, answering "not limited" when Redis cannot answer.
+
+    Registers itself under `redis+failopen` / `rediss+failopen` (see
+    `STORAGE_SCHEME` below) rather than overriding `redis` / `rediss`
+    directly, so a reader who greps for the scheme in a stack trace finds
+    this class instead of wondering why `limits`' own `RedisStorage` grew a
+    fail-open habit it does not document.
+
+    Overrides only the three calls `FixedWindowRateLimiter` (this app's
+    strategy; see the `Limiter(...)` construction below) makes: `incr`,
+    `get`, `get_expiry`. `check()` already reports "unhealthy" rather than
+    raising in the parent class, and `reset()` / `clear()` are operator
+    tools this app does not call from the request path, so a failure there
+    is somebody's terminal, not somebody's login.
+    """
+
+    STORAGE_SCHEME = ["redis+failopen", "rediss+failopen"]
+
+    def __init__(self, uri: str, **options: Any) -> None:
+        scheme, sep, rest = uri.partition("://")
+        real_scheme = scheme.removesuffix("+failopen")
+        super().__init__(f"{real_scheme}{sep}{rest}", **options)
+
+    def incr(self, key: str, expiry: int, amount: int = 1) -> int:
+        """Zero hits recorded reads as "not limited" to every caller of this."""
+        try:
+            return super().incr(key, expiry, amount=amount)
+        except RedisError as exc:
+            _degraded("incr", exc)
+            return 0
+
+    def get(self, key: str) -> int:
+        try:
+            return super().get(key)
+        except RedisError as exc:
+            _degraded("get", exc)
+            return 0
+
+    def get_expiry(self, key: str) -> float:
+        """A key we could not read reports its expiry as now.
+
+        We do not know when it actually resets, and claiming a real deadline
+        would tell a caller building on `X-RateLimit-Reset` something we did
+        not observe.
+        """
+        try:
+            return super().get_expiry(key)
+        except RedisError as exc:
+            _degraded("get_expiry", exc)
+            return time.time()
+
+
 # Module-level constant — this is policy, not configuration. Keep it in code.
 LOGIN_RATE_LIMIT = "5/minute"
 
@@ -134,10 +258,20 @@ def _authenticated_user_key(request: Request) -> str:
 # @limiter.limit(...). storage_uri uses Redis so the 5/min budget is shared
 # across uvicorn workers (the function call is a runtime call, not a cached
 # module constant — CLAUDE.md rule #11 is about getenv, not bootstrap config).
+# The `+failopen` scheme routes construction to `FailOpenRedisStorage` above
+# instead of `limits`' own `RedisStorage`; the socket timeouts bound how long
+# a hung connection can delay a request before that fail-open policy applies.
 limiter = Limiter(
     key_func=_client_ip_for_limit,
     default_limits=[],
-    storage_uri=redis_url(),
+    storage_uri=_fail_open_storage_uri(redis_url()),
+    # slowapi types storage_options as Dict[str, str], but it forwards the
+    # values unchanged to redis.from_url(**options), which wants real floats
+    # here rather than strings it would have to parse back.
+    storage_options={
+        "socket_timeout": _SOCKET_TIMEOUT_SECONDS,  # type: ignore[dict-item]
+        "socket_connect_timeout": _SOCKET_TIMEOUT_SECONDS,  # type: ignore[dict-item]
+    },
     enabled=_limiter_enabled(),
 )
 

apps/backend/tests/integration/test_ratelimit_fail_open.py

@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: Apache-2.0
+# Copyright 2026 TRUSCA contributors
+"""A Redis outage must not turn every rate-limited endpoint into a 500 (#399).
+
+Before `FailOpenRedisStorage`, slowapi's default construction re-raised
+whatever its storage raised, and the header-injection code that ran on the
+`swallow_errors=True` path 500'd too, on an `AttributeError` this time rather
+than a `RedisError` (see `core/ratelimit.py`'s module docstring for how that
+was confirmed). Either way, a limiter meant to protect the service from one
+noisy caller became a single point of failure for the whole service instead.
+
+These tests break only the rate limiter's own storage, leaving the real
+Redis the rest of the app (including `login_throttle`) is still driven
+against untouched. `test_login_throttle.py::test_a_redis_outage_leaves_sign_in_working`
+does the mirror image: it breaks `login_throttle`'s Redis client and leaves
+the rate limiter working. Neither test's pass depends on the other
+mechanism also being broken, which is the point -- a limiter that is merely
+disabled for the test looks identical to one that fails open on its own, and
+only exercising each in isolation tells the two apart.
+"""
+
+from __future__ import annotations
+
+from collections.abc import AsyncIterator, Iterator
+
+import pytest
+from httpx import ASGITransport, AsyncClient
+
+from tests._db_required import migrate_to_head
+from tests._helpers import strong_password, unique_suffix
+
+pytestmark = pytest.mark.integration
+
+
+@pytest.fixture(scope="module", autouse=True)
+def _migrate_once() -> None:
+    migrate_to_head()
+
+
+@pytest.fixture
+async def client() -> AsyncIterator[AsyncClient]:
+    from main import app as fastapi_app
+
+    # raise_app_exceptions=False: a regression here is a 500, and the test
+    # needs to see that status code rather than have httpx re-raise it.
+    transport = ASGITransport(app=fastapi_app, raise_app_exceptions=False)
+    async with AsyncClient(transport=transport, base_url="http://testserver") as ac:
+        yield ac
+
+
+@pytest.fixture
+def unreachable_ratelimit_storage(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]:
+    """Point the live `limiter` at a Redis nothing is listening on.
+
+    Swaps the storage the strategy actually calls (`limiter._limiter.storage`)
+    as well as `limiter._storage` (read by `.check()` / `.reset()`), since
+    slowapi keeps two references to the same object and only one of them is
+    on `hit()`'s call path. `monkeypatch` restores both when the test ends,
+    so the shared singleton is never left pointed at a dead host for the
+    tests that run after this one.
+    """
+    from core.ratelimit import FailOpenRedisStorage, limiter
+
+    broken = FailOpenRedisStorage(
+        "redis+failopen://127.0.0.1:1/0",
+        socket_timeout=0.5,
+        socket_connect_timeout=0.5,
+    )
+    monkeypatch.setattr(limiter, "_storage", broken)
+    monkeypatch.setattr(limiter._limiter, "storage", broken)  # type: ignore[attr-defined]
+    yield
+
+
+async def test_registration_succeeds_when_the_rate_limiter_cannot_reach_redis(
+    client: AsyncClient, unreachable_ratelimit_storage: None
+) -> None:
+    """`/auth/register` is decorated with `@limiter.limit(...)` and nothing
+    else that talks to Redis, so a non-201 here is the rate limiter's doing.
+    """
+    email = f"ratelimit-outage-{unique_suffix()}@example.com"
+    response = await client.post(
+        "/auth/register",
+        json={"email": email, "password": strong_password(), "full_name": "Outage Test"},
+    )
+    assert response.status_code == 201, response.text
+
+
+async def test_login_is_not_blocked_by_a_rate_limiter_outage(
+    client: AsyncClient, unreachable_ratelimit_storage: None
+) -> None:
+    """`/auth/login` is decorated with the rate limiter *and* gated by
+    `login_throttle`. With only the rate limiter's storage broken, a wrong
+    password must still come back as an ordinary 401 -- proof that the
+    limiter let the request through rather than 500ing on the way in, and
+    that `login_throttle`'s own (unbroken) Redis path is what actually
+    handled the attempt.
+    """
+    email = f"ratelimit-outage-login-{unique_suffix()}@example.com"
+    password = strong_password()
+    created = await client.post(
+        "/auth/register",
+        json={"email": email, "password": password, "full_name": "Outage Test"},
+    )
+    assert created.status_code == 201, created.text
+
+    wrong = await client.post(
+        "/auth/login", json={"email": email, "password": "definitely not it 1"}
+    )
+    assert wrong.status_code == 401, wrong.text
+
+    right = await client.post("/auth/login", json={"email": email, "password": password})
+    assert right.status_code == 200, right.text