diff --git a/.env.example b/.env.example index 7cf4508..3708d34 100644 --- a/.env.example +++ b/.env.example @@ -86,7 +86,16 @@ POOL_ADMIN_ADDRESSES= # === Global Rate Limiting === # Per-IP rate limiting for all endpoints (skipped when x402 is enabled). -# RATE_LIMIT_ENABLED=true # Master switch (default: true) +# # Number of proxies between the internet and this application, used to locate +# the caller in X-Forwarded-For. 1 is correct behind a single reverse proxy. +# Set 0 if exposed directly, in which case forwarding headers are ignored. +# +# This value decides who the per-IP rate limits and the daily spend budget are +# applied to. Too high groups callers together; too low lets a caller choose +# their own identity by sending the header themselves. +TRUSTED_PROXY_HOPS=1 + +RATE_LIMIT_ENABLED=true # Master switch (default: true) # RATE_LIMIT_PER_MINUTE=60 # Requests per minute per IP (default: 60) # RATE_LIMIT_BURST=10 # Extra burst capacity (default: 10) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index a48ce6d..ffc53bc 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -64,6 +64,7 @@ jobs: X402_ALLOW_TESTNET_PAID_BYPASS=${{ vars.X402_ALLOW_TESTNET_PAID_BYPASS || 'false' }} X402_MAX_STAMP_BZZ=${{ vars.X402_MAX_STAMP_BZZ || '5.0' }} STAMP_DAILY_BZZ_PER_CALLER=${{ vars.STAMP_DAILY_BZZ_PER_CALLER || '0.5' }} + TRUSTED_PROXY_HOPS=${{ vars.TRUSTED_PROXY_HOPS || '1' }} STAMP_POOL_CHECK_INTERVAL_SECONDS=${{ vars.STAMP_POOL_CHECK_INTERVAL_SECONDS || '900' }} STAMP_POOL_MIN_TTL_HOURS=${{ vars.STAMP_POOL_MIN_TTL_HOURS || '24' }} STAMP_POOL_TOPUP_HOURS=${{ vars.STAMP_POOL_TOPUP_HOURS || '168' }} @@ -129,6 +130,7 @@ jobs: X402_ALLOW_TESTNET_PAID_BYPASS=${{ vars.X402_ALLOW_TESTNET_PAID_BYPASS || 'false' }} X402_MAX_STAMP_BZZ=${{ vars.X402_MAX_STAMP_BZZ || '5.0' }} STAMP_DAILY_BZZ_PER_CALLER=${{ vars.STAMP_DAILY_BZZ_PER_CALLER || '0.5' }} + TRUSTED_PROXY_HOPS=${{ vars.TRUSTED_PROXY_HOPS || '1' }} STAMP_POOL_CHECK_INTERVAL_SECONDS=${{ vars.STAMP_POOL_CHECK_INTERVAL_SECONDS || '900' }} STAMP_POOL_MIN_TTL_HOURS=${{ vars.STAMP_POOL_MIN_TTL_HOURS || '24' }} STAMP_POOL_TOPUP_HOURS=${{ vars.STAMP_POOL_TOPUP_HOURS || '168' }} diff --git a/app/core/client_ip.py b/app/core/client_ip.py new file mode 100644 index 0000000..814dfd6 --- /dev/null +++ b/app/core/client_ip.py @@ -0,0 +1,106 @@ +# app/core/client_ip.py +"""Identify the calling client, for limits that are keyed on who is calling. + +Three controls key on this value: the global per-IP rate limit, the x402 +free-tier rate limit, and the daily spend budget (#102). A caller who can change +the value at will is not limited by any of them. + +## Why the previous approach was a trap rather than a bug + +There were two near-identical copies of this function, in `app/x402/middleware.py` +and `app/middleware/rate_limit.py`. Both took the FIRST entry of +`X-Forwarded-For` and trusted it: + + return forwarded_for.split(",")[0].strip() + +`X-Forwarded-For` is a list, appended to by each proxy it passes through, so the +leftmost entry is the one FURTHEST from us — and if any proxy appends rather than +replaces, that entry is whatever the client sent. A caller rotating the header +would then get a fresh rate-limit window and a fresh spend budget on every +request. That is the same failure the pool allowance had when every distinct +`Origin` minted a new budget. + +It was not exploitable: Caddy is configured to replace the header rather than +append, verified on 2026-09-09 by exhausting the free-tier limit and retrying +with three different forged values — all still refused. But the safety lived +entirely in the proxy's configuration, nothing in this repository recorded the +dependency, and no test pinned it. Setting `trusted_proxies` in Caddy, putting a +CDN in front, or moving to a load balancer that appends would have made all three +controls bypassable with nothing failing to say so. + +## What this does instead + +Count back from the RIGHT by the number of proxies we actually have in front of +us. The rightmost entry is the one our own proxy added and is therefore the only +entry we know to be truthful; each further step left is one hop further out. + +With `TRUSTED_PROXY_HOPS = 1` this is correct whether the proxy replaces or +appends, which is the property worth having — it stops depending on a behaviour +nobody here controls: + + header "1.2.3.4" (Caddy replaced) -> 1.2.3.4 + header "9.9.9.9, 1.2.3.4" (Caddy appended) -> 1.2.3.4 + header "9.9.9.9, 8.8.8.8, 1.2.3.4" (forged chain) -> 1.2.3.4 + +A forged chain cannot reach past the hop count, because entries to the right of +the client are added by infrastructure we control. +""" +import logging +from typing import Optional + +from fastapi import Request + +from app.core.config import settings + +logger = logging.getLogger(__name__) + +UNKNOWN = "unknown" + + +def _peer(request: Request) -> str: + """The direct TCP peer — never forgeable, but it is the proxy when behind one.""" + if request.client and request.client.host: + return request.client.host + return UNKNOWN + + +def get_client_ip(request: Request) -> str: + """The calling client's address, as far as the proxy configuration allows. + + Reads `TRUSTED_PROXY_HOPS`: the number of proxies between the internet and + the application. 0 means the application is directly exposed, in which case + forwarding headers are ignored entirely rather than half-trusted. + """ + hops = settings.TRUSTED_PROXY_HOPS + + # Directly exposed: the headers carry no authority at all, so reading them + # would be strictly worse than using the connection we actually have. + if hops <= 0: + return _peer(request) + + forwarded_for = request.headers.get("X-Forwarded-For") + if forwarded_for: + parts = [p.strip() for p in forwarded_for.split(",") if p.strip()] + if parts: + index = len(parts) - hops + if index >= 0: + return parts[index] + # Fewer entries than configured hops: the header disagrees with the + # deployment. Falling back to the peer is the safe direction — it may + # group callers together behind a proxy, which over-limits, rather + # than trusting a value that is short by a hop, which under-limits. + logger.warning( + "X-Forwarded-For has %d entries but TRUSTED_PROXY_HOPS is %d; " + "using the direct peer instead. Check the setting against the " + "actual number of proxies.", + len(parts), hops, + ) + return _peer(request) + + # Single-valued alternative set by some proxies. Equally forgeable, so it is + # read only where a proxy is expected to be setting it. + real_ip = request.headers.get("X-Real-IP") + if real_ip and real_ip.strip(): + return real_ip.strip() + + return _peer(request) diff --git a/app/core/config.py b/app/core/config.py index 309befb..de208ad 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -280,6 +280,19 @@ class Settings(BaseSettings): MAX_JSON_DEPTH: int = 20 # Maximum JSON nesting depth # === Global Rate Limiting === + # Number of proxies between the internet and this application, used to + # locate the caller in X-Forwarded-For. See app/core/client_ip.py. + # + # 1 is correct for the standard deployment: Caddy terminates TLS and + # forwards to the gateway on loopback. Raise it if a CDN or load balancer is + # added in front, and set it to 0 if the application is exposed directly, in + # which case forwarding headers are ignored rather than half-trusted. + # + # Getting this too HIGH groups callers together, which over-limits and is + # visible as complaints. Too LOW lets a caller pick their own identity, which + # under-limits and is visible as nothing at all. + TRUSTED_PROXY_HOPS: int = 1 + RATE_LIMIT_ENABLED: bool = True # Enable global rate limiting RATE_LIMIT_PER_MINUTE: int = 60 # Requests per minute per IP RATE_LIMIT_BURST: int = 10 # Extra burst capacity above per-minute limit diff --git a/app/middleware/rate_limit.py b/app/middleware/rate_limit.py index 50ca806..a29c97c 100644 --- a/app/middleware/rate_limit.py +++ b/app/middleware/rate_limit.py @@ -97,20 +97,11 @@ def cleanup_stale(self, max_age: float = 300): _counter = SlidingWindowCounter() -def get_client_ip(request: Request) -> str: - """Extract client IP from request, handling proxies.""" - forwarded_for = request.headers.get("X-Forwarded-For") - if forwarded_for: - return forwarded_for.split(",")[0].strip() - - real_ip = request.headers.get("X-Real-IP") - if real_ip: - return real_ip.strip() - - if request.client: - return request.client.host - - return "unknown" +# Single implementation in app/core/client_ip. Re-exported here because +# callers and tests import it from this module. Both copies used to take the +# FIRST X-Forwarded-For entry, which is the one furthest from us and is +# caller-controlled if any proxy appends rather than replaces. +from app.core.client_ip import get_client_ip # noqa: F401,E402 class RateLimitMiddleware(BaseHTTPMiddleware): diff --git a/app/x402/middleware.py b/app/x402/middleware.py index b45d8d8..e6390d5 100644 --- a/app/x402/middleware.py +++ b/app/x402/middleware.py @@ -82,23 +82,11 @@ def is_protected_endpoint(method: str, path: str) -> bool: return False -def get_client_ip(request: Request) -> str: - """Extract client IP from request, handling proxies.""" - # Check for forwarded headers first - forwarded_for = request.headers.get("X-Forwarded-For") - if forwarded_for: - # Take the first IP in the chain - return forwarded_for.split(",")[0].strip() - - real_ip = request.headers.get("X-Real-IP") - if real_ip: - return real_ip.strip() - - # Fall back to direct connection - if request.client: - return request.client.host - - return "unknown" +# Single implementation in app/core/client_ip. Re-exported here because +# callers and tests import it from this module. Both copies used to take the +# FIRST X-Forwarded-For entry, which is the one furthest from us and is +# caller-controlled if any proxy appends rather than replaces. +from app.core.client_ip import get_client_ip # noqa: F401,E402 def create_payment_requirements( diff --git a/deploy/Caddyfile b/deploy/Caddyfile index 0906053..a179e2a 100644 --- a/deploy/Caddyfile +++ b/deploy/Caddyfile @@ -24,10 +24,18 @@ } {$GATEWAY_HOST} { - # Caddy sets X-Forwarded-For by default. Both get_client_ip() - # implementations read it, and without it every caller collapses into one - # bucket — per-IP rate limiting and the x402 free-tier allowance would then - # be shared globally rather than per client. + # Caddy sets X-Forwarded-For by default. The gateway reads it to identify + # the caller, and without it every caller collapses into one bucket — + # per-IP rate limiting, the x402 free-tier allowance and the daily spend + # budget would all be shared globally rather than per client. + # + # The gateway counts back from the RIGHT of that header by + # TRUSTED_PROXY_HOPS (default 1), so it is correct whether Caddy replaces + # the header or appends to it. IF YOU ADD A PROXY OR CDN IN FRONT OF THIS + # ONE, raise TRUSTED_PROXY_HOPS to match, or the gateway will read the new + # proxy's address as the caller and every request will share one budget. + # Lowering it below the real number of hops is worse: the caller then gets + # to pick their own identity by sending the header themselves. # Prometheus metrics are NOT public (#188). The endpoint exposed 146 series to # anyone who asked: wallet addresses, BZZ and xDAI balances, gateway version, # which features are enabled, and request volumes — a funding position, in @@ -66,6 +74,9 @@ } {$GATEWAY_DEV_HOST} { + # The X-Forwarded-For note on the site above applies here too: the gateway + # identifies callers from that header, and TRUSTED_PROXY_HOPS must match the + # real number of proxies in front of it. # Prometheus metrics are NOT public (#188). The endpoint exposed 146 series to # anyone who asked: wallet addresses, BZZ and xDAI balances, gateway version, # which features are enabled, and request volumes — a funding position, in diff --git a/tests/test_client_ip.py b/tests/test_client_ip.py new file mode 100644 index 0000000..8786eb0 --- /dev/null +++ b/tests/test_client_ip.py @@ -0,0 +1,215 @@ +"""Identifying the caller, for limits that are keyed on who is calling. + +Three controls depend on this: the global per-IP rate limit, the x402 free-tier +rate limit, and the daily spend budget (#102). A caller who can choose this value +is not limited by any of them. + +The previous implementation took the FIRST entry of `X-Forwarded-For`, which is +the one furthest from us and is caller-supplied if any proxy in the chain appends +rather than replaces. It was not exploitable in the deployed configuration — +verified against staging on 2026-09-09 by exhausting the free-tier limit and +retrying with forged values, all still refused — but the safety lived entirely in +the proxy's configuration, nothing recorded that dependency, and no test pinned +it. These tests pin it. +""" +from unittest.mock import MagicMock + +import pytest +from fastapi import Request + +from app.core.client_ip import UNKNOWN, get_client_ip +from app.core.config import settings + +CLIENT = "203.0.113.50" +FORGED = "9.9.9.9" + + +def _request(headers=None, peer=None): + r = MagicMock(spec=Request) + r.headers = headers or {} + if peer is None: + r.client = None + else: + r.client = MagicMock() + r.client.host = peer + return r + + +class TestForgingTheHeader: + """The property the old implementation did not have.""" + + def test_a_forged_prefix_cannot_change_the_answer(self, monkeypatch): + """A caller prepending entries must not be able to pick their identity. + + With one proxy in front, everything left of the last entry came from + outside and carries no authority. + """ + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + for forged_chain in ( + f"{FORGED}, {CLIENT}", + f"{FORGED}, 8.8.8.8, {CLIENT}", + f"{FORGED},{FORGED},{FORGED}, {CLIENT}", + ): + got = get_client_ip(_request({"X-Forwarded-For": forged_chain})) + assert got == CLIENT, f"{forged_chain!r} yielded {got}" + + def test_rotating_the_header_does_not_produce_a_new_identity(self, monkeypatch): + """This is the failure mode in full: a caller changing the header on + every request would get a fresh rate-limit window and a fresh spend + budget each time.""" + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + seen = { + get_client_ip(_request({"X-Forwarded-For": f"10.0.0.{i}, {CLIENT}"})) + for i in range(20) + } + assert seen == {CLIENT}, f"rotation produced {len(seen)} identities" + + def test_the_old_behaviour_is_gone(self, monkeypatch): + """Explicit, because this is what the previous code and its test did.""" + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + assert get_client_ip( + _request({"X-Forwarded-For": f"{FORGED}, {CLIENT}"}) + ) != FORGED + + +class TestProxyBehaviourIndependence: + """The point of counting hops is not depending on something we do not control. + + Caddy currently replaces the header. If it were configured with + trusted_proxies, or replaced by a load balancer that appends, the answer must + not change. + """ + + def test_a_replacing_proxy_and_an_appending_proxy_agree(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + replaced = get_client_ip(_request({"X-Forwarded-For": CLIENT})) + appended = get_client_ip(_request({"X-Forwarded-For": f"{FORGED}, {CLIENT}"})) + assert replaced == appended == CLIENT + + def test_two_hops_looks_one_further_out(self, monkeypatch): + """A CDN in front of Caddy: the last entry is Caddy's view of the CDN, + and the caller is one step further left.""" + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 2) + header = f"{CLIENT}, 172.16.0.1" + assert get_client_ip(_request({"X-Forwarded-For": header})) == CLIENT + + def test_a_forged_prefix_still_cannot_reach_past_two_hops(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 2) + header = f"{FORGED}, {CLIENT}, 172.16.0.1" + assert get_client_ip(_request({"X-Forwarded-For": header})) == CLIENT + + +class TestDirectExposure: + def test_zero_hops_ignores_the_headers_entirely(self, monkeypatch): + """Half-trusting a header when nothing is in front is worse than using + the connection we actually have.""" + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 0) + r = _request({"X-Forwarded-For": FORGED, "X-Real-IP": FORGED}, peer="192.0.2.7") + assert get_client_ip(r) == "192.0.2.7" + + def test_zero_hops_with_no_peer_is_unknown(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 0) + assert get_client_ip(_request({"X-Forwarded-For": FORGED})) == UNKNOWN + + +class TestMisconfiguration: + """Getting the setting wrong should fail in the safe direction. + + Too high groups callers together, which over-limits and shows up as + complaints. Too low lets a caller choose their own identity, which + under-limits and shows up as nothing. + """ + + def test_a_chain_shorter_than_the_hop_count_falls_back_to_the_peer(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 3) + r = _request({"X-Forwarded-For": f"{FORGED}, {CLIENT}"}, peer="192.0.2.7") + assert get_client_ip(r) == "192.0.2.7", \ + "a short chain must not fall through to a caller-supplied entry" + + def test_the_fallback_never_returns_a_header_value(self, monkeypatch): + """The dangerous version of the above would clamp to index 0 and hand + back the leftmost entry — exactly the value an attacker controls.""" + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 5) + r = _request({"X-Forwarded-For": f"{FORGED}, {CLIENT}"}, peer="192.0.2.7") + assert get_client_ip(r) not in (FORGED, CLIENT) + + +class TestOrdinaryCases: + def test_a_single_entry_is_the_caller(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + assert get_client_ip(_request({"X-Forwarded-For": CLIENT})) == CLIENT + + def test_whitespace_and_empty_entries_are_ignored(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + assert get_client_ip(_request({"X-Forwarded-For": f" {FORGED} , , {CLIENT} "})) == CLIENT + + def test_real_ip_is_used_when_forwarded_for_is_absent(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + assert get_client_ip(_request({"X-Real-IP": CLIENT})) == CLIENT + + def test_forwarded_for_wins_over_real_ip(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + r = _request({"X-Forwarded-For": CLIENT, "X-Real-IP": "10.0.0.1"}, peer="192.0.2.7") + assert get_client_ip(r) == CLIENT + + def test_no_headers_falls_back_to_the_peer(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + assert get_client_ip(_request({}, peer="192.0.2.7")) == "192.0.2.7" + + def test_nothing_at_all_is_unknown(self, monkeypatch): + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + assert get_client_ip(_request({})) == UNKNOWN + + +class TestOneImplementation: + """Two copies of this existed and both had the same flaw. If they diverge + again, one control gets fixed and another silently does not.""" + + def test_every_module_uses_the_same_function(self): + from app.core.client_ip import get_client_ip as canonical + from app.middleware.rate_limit import get_client_ip as rate_limit_copy + from app.x402.middleware import get_client_ip as x402_copy + from app.api.endpoints.stamps import get_client_ip as stamps_copy + + assert rate_limit_copy is canonical + assert x402_copy is canonical + assert stamps_copy is canonical + + +class TestTheLimitsUseIt: + """A correct helper nothing calls would fix nothing.""" + + @pytest.mark.parametrize("chain,expected", [ + (f"{FORGED}, {CLIENT}", CLIENT), + (f"8.8.8.8, 1.1.1.1, {CLIENT}", CLIENT), + ]) + def test_the_spend_budget_charges_the_real_caller(self, monkeypatch, chain, expected): + from unittest.mock import AsyncMock, patch + from fastapi.testclient import TestClient + from app.main import app + from app.services.spend_budget import SpendBudgetTracker + + monkeypatch.setattr(settings, "TRUSTED_PROXY_HOPS", 1) + monkeypatch.setattr(settings, "X402_MAX_STAMP_BZZ", 0.0) + monkeypatch.setattr(settings, "STAMP_DAILY_BZZ_PER_CALLER", -1.0) + + import app.api.endpoints.stamps as stamps_ep + tracker = SpendBudgetTracker(state_file=None) + tracker._state_file = "/dev/null" + monkeypatch.setattr(stamps_ep, "spend_budget_tracker", tracker) + + with patch("app.services.swarm_api.get_chainstate", + new=AsyncMock(return_value={"currentPrice": "24000"})), \ + patch("app.services.swarm_api.check_sufficient_funds", + new=AsyncMock(return_value={"sufficient": True, "required_bzz": 0.01, + "wallet_balance_bzz": 100.0, + "shortfall_bzz": 0.0})), \ + patch("app.services.swarm_api.purchase_postage_stamp", + new=AsyncMock(return_value="b" * 64)): + r = TestClient(app).post("/api/v1/stamps/", + json={"depth": 17, "duration_hours": 24}, + headers={"X-Forwarded-For": chain}) + + assert r.status_code == 201, r.text + assert list(tracker.snapshot()["spent"]) == [expected], \ + "the spend was charged to a caller-supplied address" diff --git a/tests/test_x402_middleware.py b/tests/test_x402_middleware.py index bca9d99..13c133f 100644 --- a/tests/test_x402_middleware.py +++ b/tests/test_x402_middleware.py @@ -111,10 +111,18 @@ class TestGetClientIP: """Test client IP extraction.""" def test_forwarded_for_header(self): + """The caller is counted from the RIGHT, by TRUSTED_PROXY_HOPS. + + This test previously asserted "203.0.113.50" — the leftmost entry. That + is the one furthest from us, and it is whatever the client sent if any + proxy in the chain appends rather than replaces. With one proxy in front, + the rightmost entry is the one our own proxy added, and the entry it + added is the address it saw. + """ request = MagicMock(spec=Request) request.headers = {"X-Forwarded-For": "203.0.113.50, 70.41.3.18"} request.client = None - assert get_client_ip(request) == "203.0.113.50" + assert get_client_ip(request) == "70.41.3.18" def test_real_ip_header(self): request = MagicMock(spec=Request)