From 520bd7243d6f1aff4bd1b48a5d17d9e976899ffa Mon Sep 17 00:00:00 2001 From: k Date: Fri, 29 May 2026 13:55:00 +0800 Subject: [PATCH] fix(BUG-02): rate limiter now keys on real client IP + enforces default_limits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CLAUDE.md | 17 ++++++++---- app/api/posts.py | 4 +-- app/api/prices.py | 4 +-- app/main.py | 19 ++++++++----- app/ratelimit.py | 47 +++++++++++++++++++++++++++++++ tests/test_ratelimit.py | 61 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 134 insertions(+), 18 deletions(-) create mode 100644 app/ratelimit.py create mode 100644 tests/test_ratelimit.py diff --git a/CLAUDE.md b/CLAUDE.md index 1402977..37615a8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 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): - `app/api/proxy/[...path]/route.ts` (frontend) strips `x-forwarded-for` - before forwarding. Backend `slowapi` sees the Next.js server IP and applies - one rate-limit bucket to ALL users. Fix: relay the real client IP from - `req.headers.get('x-forwarded-for')` or `x-real-ip` in the proxy. +- ~~**Rate limit bypass via proxy x-forwarded-for** (BUG-02, HIGH, FIXED 2026-05-29):~~ + Two-part fix. (1) Frontend proxy `app/api/proxy/[...path]/route.ts` now + relays the real client IP via `x-forwarded-for` / `x-real-ip`. (2) Backend + had the matching gap: `slowapi`'s default `get_remote_address` reads + `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** (BUG-03, MITIGATED 2026-05-29):~~ Full two-phase-commit is out of scope. diff --git a/app/api/posts.py b/app/api/posts.py index a3fe140..0d4be94 100644 --- a/app/api/posts.py +++ b/app/api/posts.py @@ -3,10 +3,8 @@ from typing import List, Optional from fastapi import APIRouter, Depends, HTTPException, Query, Request 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.ext.asyncio import AsyncSession diff --git a/app/api/prices.py b/app/api/prices.py index 2fd1e22..9069e9e 100644 --- a/app/api/prices.py +++ b/app/api/prices.py @@ -4,10 +4,8 @@ from typing import List import httpx from fastapi import APIRouter, HTTPException, Query, Request 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.schemas import Candle diff --git a/app/main.py b/app/main.py index 300dca7..70ff960 100644 --- a/app/main.py +++ b/app/main.py @@ -5,11 +5,12 @@ from contextlib import asynccontextmanager from apscheduler.schedulers.asyncio import AsyncIOScheduler from fastapi import FastAPI, Request, WebSocket, WebSocketDisconnect 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.util import get_remote_address +from slowapi.middleware import SlowAPIMiddleware from app.config import settings +from app.ratelimit import limiter from app.database import AsyncSessionLocal, engine from app.models import Base from app.scrapers.truth_social import poll_truth_social, backfill_history @@ -276,11 +277,10 @@ async def lifespan(app: FastAPI): logger.info("Shutdown complete.") -# Rate limiter — keyed by client IP (X-Forwarded-For → remote_addr fallback). -# Public read endpoints: 60 req/min. Signed mutations: 20 req/min (enforced -# per-route). Limits are generous enough for normal use but block scrapers -# and accidental polling loops. -limiter = Limiter(key_func=get_remote_address, default_limits=["60/minute"]) +# Rate limiter is the shared instance from app.ratelimit — it keys on the +# real client IP (x-forwarded-for / x-real-ip) relayed by the Next.js proxy, +# NOT the proxy's own IP. See app/ratelimit.py for the BUG-02 rationale. +# Public read endpoints: 60 req/min. Signed mutations: 20 req/min (per-route). app = FastAPI( title="TrumpSignal API", @@ -290,6 +290,11 @@ app = FastAPI( ) app.state.limiter = limiter 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 # In production we only allow the canonical frontend origin (FRONTEND_URL). diff --git a/app/ratelimit.py b/app/ratelimit.py new file mode 100644 index 0000000..d880774 --- /dev/null +++ b/app/ratelimit.py @@ -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"]) diff --git a/tests/test_ratelimit.py b/tests/test_ratelimit.py new file mode 100644 index 0000000..4a7b75c --- /dev/null +++ b/tests/test_ratelimit.py @@ -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