fix(BUG-02): rate limiter now keys on real client IP + enforces default_limits
The frontend proxy fix alone was incomplete. Backend slowapi used the default
get_remote_address (request.client.host), which is the proxy's IP because
uvicorn runs without --proxy-headers — so the relayed x-forwarded-for was
ignored and all users still shared one rate-limit bucket.
- Add app/ratelimit.py: shared `limiter` + `client_ip_key` that reads
x-forwarded-for[0] → x-real-ip → peer. Replaces the three independent
Limiter(get_remote_address) instances in main.py / posts.py / prices.py
(which also had separate, non-shared storage).
- Register SlowAPIMiddleware so default_limits ("60/minute") applies to EVERY
route. Previously only the 2 decorated read endpoints were limited; all
signed-mutation routes had no rate limit at all (the "20/min per-route"
comment was aspirational — no such decorator existed).
- Add tests/test_ratelimit.py (7 tests): XFF precedence, fallbacks, two users
behind one proxy get distinct keys, middleware-registered guard.
72 tests pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -426,11 +426,18 @@ this for you" to "you opened it, bot manages your discipline".
|
|||||||
computes remaining seconds, and reschedules the task. Elapsed windows (backend
|
computes remaining seconds, and reschedules the task. Elapsed windows (backend
|
||||||
was down longer than the time-stop period) fire immediately with `delay_seconds=0`.
|
was down longer than the time-stop period) fire immediately with `delay_seconds=0`.
|
||||||
|
|
||||||
- **Rate limit bypass via proxy x-forwarded-for deletion** (BUG-02, HIGH):
|
- ~~**Rate limit bypass via proxy x-forwarded-for** (BUG-02, HIGH, FIXED 2026-05-29):~~
|
||||||
`app/api/proxy/[...path]/route.ts` (frontend) strips `x-forwarded-for`
|
Two-part fix. (1) Frontend proxy `app/api/proxy/[...path]/route.ts` now
|
||||||
before forwarding. Backend `slowapi` sees the Next.js server IP and applies
|
relays the real client IP via `x-forwarded-for` / `x-real-ip`. (2) Backend
|
||||||
one rate-limit bucket to ALL users. Fix: relay the real client IP from
|
had the matching gap: `slowapi`'s default `get_remote_address` reads
|
||||||
`req.headers.get('x-forwarded-for')` or `x-real-ip` in the proxy.
|
`request.client.host` (the proxy IP, since uvicorn runs without
|
||||||
|
`--proxy-headers`), so the relayed header was ignored and all users still
|
||||||
|
shared one bucket. Now a shared `app/ratelimit.py` exposes `client_ip_key`
|
||||||
|
(reads `x-forwarded-for[0]` → `x-real-ip` → peer) used by ONE shared
|
||||||
|
`limiter` across `main.py`, `posts.py`, `prices.py`. Also registered
|
||||||
|
`SlowAPIMiddleware` so `default_limits` (60/min) actually applies to every
|
||||||
|
route — previously only the 2 decorated read endpoints were limited and all
|
||||||
|
signed-mutation routes had no limit at all. Covered by `tests/test_ratelimit.py`.
|
||||||
|
|
||||||
- ~~**`close_and_finalize` double-failure leaves DB/HL state inconsistent**
|
- ~~**`close_and_finalize` double-failure leaves DB/HL state inconsistent**
|
||||||
(BUG-03, MITIGATED 2026-05-29):~~ Full two-phase-commit is out of scope.
|
(BUG-03, MITIGATED 2026-05-29):~~ Full two-phase-commit is out of scope.
|
||||||
|
|||||||
+1
-3
@@ -3,10 +3,8 @@ from typing import List, Optional
|
|||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, Query, Request
|
from fastapi import APIRouter, Depends, HTTPException, Query, Request
|
||||||
from fastapi.responses import Response
|
from fastapi.responses import Response
|
||||||
from slowapi import Limiter
|
|
||||||
from slowapi.util import get_remote_address
|
|
||||||
|
|
||||||
limiter = Limiter(key_func=get_remote_address)
|
from app.ratelimit import limiter
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
|
|
||||||
|
|||||||
+1
-3
@@ -4,10 +4,8 @@ from typing import List
|
|||||||
import httpx
|
import httpx
|
||||||
from fastapi import APIRouter, HTTPException, Query, Request
|
from fastapi import APIRouter, HTTPException, Query, Request
|
||||||
from fastapi.responses import Response
|
from fastapi.responses import Response
|
||||||
from slowapi import Limiter
|
|
||||||
from slowapi.util import get_remote_address
|
|
||||||
|
|
||||||
limiter = Limiter(key_func=get_remote_address)
|
from app.ratelimit import limiter
|
||||||
|
|
||||||
from app.config import settings
|
from app.config import settings
|
||||||
from app.schemas import Candle
|
from app.schemas import Candle
|
||||||
|
|||||||
+12
-7
@@ -5,11 +5,12 @@ from contextlib import asynccontextmanager
|
|||||||
from apscheduler.schedulers.asyncio import AsyncIOScheduler
|
from apscheduler.schedulers.asyncio import AsyncIOScheduler
|
||||||
from fastapi import FastAPI, Request, WebSocket, WebSocketDisconnect
|
from fastapi import FastAPI, Request, WebSocket, WebSocketDisconnect
|
||||||
from fastapi.middleware.cors import CORSMiddleware
|
from fastapi.middleware.cors import CORSMiddleware
|
||||||
from slowapi import Limiter, _rate_limit_exceeded_handler
|
from slowapi import _rate_limit_exceeded_handler
|
||||||
from slowapi.errors import RateLimitExceeded
|
from slowapi.errors import RateLimitExceeded
|
||||||
from slowapi.util import get_remote_address
|
from slowapi.middleware import SlowAPIMiddleware
|
||||||
|
|
||||||
from app.config import settings
|
from app.config import settings
|
||||||
|
from app.ratelimit import limiter
|
||||||
from app.database import AsyncSessionLocal, engine
|
from app.database import AsyncSessionLocal, engine
|
||||||
from app.models import Base
|
from app.models import Base
|
||||||
from app.scrapers.truth_social import poll_truth_social, backfill_history
|
from app.scrapers.truth_social import poll_truth_social, backfill_history
|
||||||
@@ -276,11 +277,10 @@ async def lifespan(app: FastAPI):
|
|||||||
logger.info("Shutdown complete.")
|
logger.info("Shutdown complete.")
|
||||||
|
|
||||||
|
|
||||||
# Rate limiter — keyed by client IP (X-Forwarded-For → remote_addr fallback).
|
# Rate limiter is the shared instance from app.ratelimit — it keys on the
|
||||||
# Public read endpoints: 60 req/min. Signed mutations: 20 req/min (enforced
|
# real client IP (x-forwarded-for / x-real-ip) relayed by the Next.js proxy,
|
||||||
# per-route). Limits are generous enough for normal use but block scrapers
|
# NOT the proxy's own IP. See app/ratelimit.py for the BUG-02 rationale.
|
||||||
# and accidental polling loops.
|
# Public read endpoints: 60 req/min. Signed mutations: 20 req/min (per-route).
|
||||||
limiter = Limiter(key_func=get_remote_address, default_limits=["60/minute"])
|
|
||||||
|
|
||||||
app = FastAPI(
|
app = FastAPI(
|
||||||
title="TrumpSignal API",
|
title="TrumpSignal API",
|
||||||
@@ -290,6 +290,11 @@ app = FastAPI(
|
|||||||
)
|
)
|
||||||
app.state.limiter = limiter
|
app.state.limiter = limiter
|
||||||
app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)
|
app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)
|
||||||
|
# Register the middleware so default_limits actually applies to EVERY route
|
||||||
|
# (not just the two with an explicit @limiter.limit decorator). Without this,
|
||||||
|
# all signed-mutation routes had no rate limit at all. WebSocket scope is
|
||||||
|
# skipped by slowapi. Decorated routes keep their stricter explicit limit.
|
||||||
|
app.add_middleware(SlowAPIMiddleware)
|
||||||
|
|
||||||
# CORS
|
# CORS
|
||||||
# In production we only allow the canonical frontend origin (FRONTEND_URL).
|
# In production we only allow the canonical frontend origin (FRONTEND_URL).
|
||||||
|
|||||||
@@ -0,0 +1,47 @@
|
|||||||
|
"""Shared rate-limiter instance + client-IP key function.
|
||||||
|
|
||||||
|
WHY THIS MODULE EXISTS (BUG-02):
|
||||||
|
slowapi's default `get_remote_address` keys on `request.client.host` — the
|
||||||
|
immediate TCP peer. In production the backend sits behind the Next.js proxy
|
||||||
|
(app/api/proxy/[...path]/route.ts), and uvicorn is launched WITHOUT
|
||||||
|
`--proxy-headers` (see entrypoint.sh), so `request.client.host` is the proxy
|
||||||
|
server's IP for EVERY request. That collapses all users into a single
|
||||||
|
rate-limit bucket — the proxy gets throttled, real per-user limiting never
|
||||||
|
happens.
|
||||||
|
|
||||||
|
The frontend proxy relays the real client IP in `x-forwarded-for` (falling
|
||||||
|
back to `x-real-ip`). This key function reads those headers explicitly so
|
||||||
|
the limiter buckets per end-user. All routers MUST import THIS `limiter`
|
||||||
|
(not construct their own) so every decorated route shares one instance and
|
||||||
|
one storage backend, and `app.state.limiter` matches the decorators.
|
||||||
|
|
||||||
|
SECURITY NOTE:
|
||||||
|
A client that can reach the backend directly (bypassing the proxy) could
|
||||||
|
spoof `x-forwarded-for` to mint a fresh bucket per request. That is inherent
|
||||||
|
to any XFF-based limiting and is acceptable here because the backend is only
|
||||||
|
meant to be reachable via the proxy. If the backend is ever exposed publicly,
|
||||||
|
restrict trust to the known proxy IP (uvicorn `--forwarded-allow-ips`).
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from slowapi import Limiter
|
||||||
|
from starlette.requests import Request
|
||||||
|
|
||||||
|
|
||||||
|
def client_ip_key(request: Request) -> str:
|
||||||
|
"""Best-effort real client IP: x-forwarded-for[0] → x-real-ip → peer."""
|
||||||
|
xff = request.headers.get("x-forwarded-for")
|
||||||
|
if xff:
|
||||||
|
# Left-most entry is the original client; subsequent hops are proxies.
|
||||||
|
first = xff.split(",")[0].strip()
|
||||||
|
if first:
|
||||||
|
return first
|
||||||
|
xri = request.headers.get("x-real-ip")
|
||||||
|
if xri and xri.strip():
|
||||||
|
return xri.strip()
|
||||||
|
return request.client.host if request.client else "unknown"
|
||||||
|
|
||||||
|
|
||||||
|
# Single shared limiter. default_limits applies to any route without an
|
||||||
|
# explicit @limiter.limit(...) decorator.
|
||||||
|
limiter = Limiter(key_func=client_ip_key, default_limits=["60/minute"])
|
||||||
@@ -0,0 +1,61 @@
|
|||||||
|
"""Regression tests for BUG-02: rate limiter must key on the real client IP
|
||||||
|
relayed by the proxy (x-forwarded-for / x-real-ip), not the proxy's own IP,
|
||||||
|
and default_limits must actually be enforced via SlowAPIMiddleware."""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from types import SimpleNamespace
|
||||||
|
|
||||||
|
from slowapi.middleware import SlowAPIMiddleware
|
||||||
|
|
||||||
|
from app.ratelimit import client_ip_key, limiter
|
||||||
|
|
||||||
|
|
||||||
|
def _req(headers: dict, peer: str | None = "10.0.0.1"):
|
||||||
|
"""Minimal stand-in for starlette Request: only .headers and .client are
|
||||||
|
read by client_ip_key. Headers must be case-insensitive get()."""
|
||||||
|
lower = {k.lower(): v for k, v in headers.items()}
|
||||||
|
client = SimpleNamespace(host=peer) if peer is not None else None
|
||||||
|
return SimpleNamespace(
|
||||||
|
headers=SimpleNamespace(get=lambda k, d=None: lower.get(k.lower(), d)),
|
||||||
|
client=client,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_key_prefers_first_x_forwarded_for_hop():
|
||||||
|
# Left-most XFF entry is the original client; proxies append to the right.
|
||||||
|
r = _req({"x-forwarded-for": "203.0.113.9, 10.0.0.1, 172.16.0.1"})
|
||||||
|
assert client_ip_key(r) == "203.0.113.9"
|
||||||
|
|
||||||
|
|
||||||
|
def test_key_falls_back_to_x_real_ip():
|
||||||
|
r = _req({"x-real-ip": "198.51.100.7"})
|
||||||
|
assert client_ip_key(r) == "198.51.100.7"
|
||||||
|
|
||||||
|
|
||||||
|
def test_key_falls_back_to_peer_when_no_headers():
|
||||||
|
r = _req({}, peer="192.0.2.50")
|
||||||
|
assert client_ip_key(r) == "192.0.2.50"
|
||||||
|
|
||||||
|
|
||||||
|
def test_key_handles_missing_client():
|
||||||
|
r = _req({}, peer=None)
|
||||||
|
assert client_ip_key(r) == "unknown"
|
||||||
|
|
||||||
|
|
||||||
|
def test_two_users_behind_same_proxy_get_distinct_keys():
|
||||||
|
# The whole point of BUG-02: same TCP peer (proxy), different end users.
|
||||||
|
a = _req({"x-forwarded-for": "203.0.113.1"}, peer="10.0.0.1")
|
||||||
|
b = _req({"x-forwarded-for": "203.0.113.2"}, peer="10.0.0.1")
|
||||||
|
assert client_ip_key(a) != client_ip_key(b)
|
||||||
|
|
||||||
|
|
||||||
|
def test_limiter_uses_custom_key_func():
|
||||||
|
assert limiter._key_func is client_ip_key
|
||||||
|
|
||||||
|
|
||||||
|
def test_slowapi_middleware_registered_on_app():
|
||||||
|
# Without the middleware, default_limits never applies to undecorated
|
||||||
|
# (mutation) routes. Guard against silent regression.
|
||||||
|
import app.main as main
|
||||||
|
classes = [m.cls for m in main.app.user_middleware]
|
||||||
|
assert SlowAPIMiddleware in classes
|
||||||
Reference in New Issue
Block a user