diff --git a/docs/public-hosting.md b/docs/public-hosting.md index 2400c72..df8e7fe 100644 --- a/docs/public-hosting.md +++ b/docs/public-hosting.md @@ -60,7 +60,10 @@ traverses, so the rightmost entries are the ones your own infrastructure wrote a anything a caller forged sits to the left of them. Counting in from the right by the number of proxies you actually run means a forged prefix can never be selected. Set it too HIGH and it starts trusting entries no proxy of yours wrote; too low and several -callers share one rate-limit bucket, which is merely inconvenient. +callers share one rate-limit bucket, which is merely inconvenient. A request that +passed fewer proxies than the count — a LAN visitor reaching your proxy directly while +it is set to `2` for the CDN — is read from the header's leftmost entry, which your +first proxy wrote. **4. Stop reaching the app except through the proxy.** The default compose binds `0.0.0.0:5000` so LAN clients can reach it directly — which is right for a LAN install diff --git a/src/inkwell/proxy.py b/src/inkwell/proxy.py index 21b1c0e..a4c7b16 100644 --- a/src/inkwell/proxy.py +++ b/src/inkwell/proxy.py @@ -9,7 +9,10 @@ right by the **Trusted proxy hops** setting (default 1), and a forged prefix is selected. Too high a count is the dangerous direction: it believes entries no proxy of ours -wrote. When the header is shorter than configured, the socket address is used. +wrote. A header shorter than the count is read from its leftmost entry: the request +passed fewer proxies (a LAN visitor straight to the reverse proxy, behind a count set +for the CDN road), and a caller's own entries could only have made it longer. That +holds while the first proxy drops a forwarding header from a client it doesn't trust. """ from __future__ import annotations @@ -19,16 +22,17 @@ from .settings import live def trusted_entry(header: str, hops: int) -> str | None: - """The nth-from-the-right entry of a forwarding header, or None if there isn't one. + """The nth-from-the-right entry of a forwarding header, its leftmost if it is + shorter than that, or None when no proxy is trusted or there is no header. Pure, so the trust boundary is testable without a request context. """ if hops <= 0: return None entries = [part.strip() for part in header.split(",") if part.strip()] - if len(entries) < hops: + if not entries: return None - return entries[-hops] + return entries[-min(hops, len(entries))] def forwarded_for(header: str, remote_addr: str | None, hops: int) -> str: diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 4a77290..b645e51 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -41,13 +41,21 @@ def test_two_hops_sees_past_a_cdn(): assert forwarded_for("1.2.3.4, 203.0.113.7, 172.16.0.5", PEER, 2) == "203.0.113.7" -def test_a_short_header_falls_back_rather_than_reaching_left(): - # Fewer proxies than configured: the socket address, rather than an entry no - # proxy of ours wrote. - assert forwarded_for("203.0.113.7", PEER, 2) == PEER +def test_a_short_header_is_read_from_its_leftmost_entry(): + # Fewer proxies than configured — a LAN visitor straight to our proxy, behind a + # count set for the CDN road. The leftmost entry is what the first proxy wrote; a + # caller's own entries could only have made the header longer. Falling back to the + # socket address here would file every LAN visitor under the proxy's address. + assert forwarded_for("192.168.0.20", PEER, 2) == "192.168.0.20" assert forwarded_for("", PEER, 1) == PEER +def test_a_single_https_from_the_cdn_counts_at_two_hops(): + # Cloudflare sends one `X-Forwarded-Proto: https` and our proxy passes it on + # without appending, so at hops 2 it is a one-entry header — and still HTTPS. + assert trusted_entry("https", 2) == "https" + + def test_malformed_headers_do_not_crash_or_leak_empties(): assert forwarded_for(",,,", PEER, 1) == PEER assert forwarded_for(" , 203.0.113.7 , ", PEER, 1) == "203.0.113.7"