bvandeusen
a85c53ba2c
CI & Build / Build now, or wait for Android? (push) Successful in 2s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 6s
CI & Build / Python tests (push) Successful in 9s
CI & Build / integration (push) Successful in 12s
CI & Build / Build & push image (push) Successful in 32s
Operator, before exposing the instance: *"I'd expect that we should have a proxy hops setting for how many proxy hops we should trust a shared real-ip at… and is there any session logging."* Neither existed, and the first one was a real hole. **The address was forgeable.** `client_address()` read the LEFTMOST `X-Forwarded-For` entry — nominally "the original client", and precisely the one a caller controls, because anything they send arrives before what proxies append. So `curl -H "X-Forwarded-For: 1.2.3.4"`, rotated per request, minted a fresh rate-limit bucket every time. Concretely: stuffing ONE account stayed limited (the account key is unforgeable and that is why it exists), but spraying MANY accounts from one source was not — each account got its own budget, and the per-address cap meant to bound the total was defeated by a header. On a LAN that is nothing. It is not nothing on a public host. Now it counts in from the RIGHT by `THOUGHTSYNC_TRUSTED_PROXY_HOPS`, default 1. Each hop appends what it saw, so the rightmost entries are the ones our own infrastructure wrote and a forged prefix lands to the left of them where it can never be selected — proven for the honest, forged, padded, CDN and shorter-than-configured cases. 0 ignores the header entirely; 2 is Cloudflare in front of a proxy. Too high is the dangerous direction, so a header shorter than configured falls back to the socket address rather than reaching further left. `X-Forwarded-Proto` had the same bug and now shares the same rule. Both live in a new `proxy.py` rather than being written twice — two places holding one decision is how issue 2183 happened, and this is the same decision. Env rather than the Settings UI, against rule 25's usual pull: it is deployment topology rather than preference, and the limiter consults it BEFORE opening a database connection, which is the entire point of checking a throttle before doing expensive work. Easy to move if that reads wrong. **And there was no logging at all** — `auth.py` had no logger, and the only record of anything was `device_tokens.last_used_at`. Sign-ins, failures, throttle trips, new accounts and device-token issuance now all log, with the attempted email and the trusted address. Deliberately including the email: it is the operator's own server, and "somebody failed a login" without saying against which account is not actionable. `basicConfig` at INFO in `create_app`, because hypercorn configures its own loggers and leaves the root at WARNING — without it every line above would have gone nowhere, which is a worse failure than not writing them. This is the app log, not an audit table. Not queryable, not retained past log rotation. The table is task 2939; this is what makes the next few days observable.
69 lines
2.9 KiB
Python
69 lines
2.9 KiB
Python
"""The proxy trust boundary.
|
|
|
|
The whole security property is "a caller cannot forge their own address", and it rests
|
|
on counting in from the RIGHT of the header rather than the left. These are the cases
|
|
that tell the two apart — pure functions, no request context, no database.
|
|
"""
|
|
from thoughtsync.config import Config
|
|
from thoughtsync.proxy import forwarded_for, trusted_entry
|
|
|
|
PEER = "10.0.0.1" # the socket address: our own proxy, or the caller when unproxied
|
|
|
|
|
|
def test_default_is_one_hop():
|
|
# One reverse proxy terminating TLS — this deployment, and the only shape that is
|
|
# safe to assume. A wrong default here is a silent security bug, not a preference.
|
|
assert Config.trusted_proxy_hops() == 1
|
|
|
|
|
|
def test_no_proxy_ignores_the_header_entirely():
|
|
# hops=0 says nothing in front of us appends anything, so the header can only be
|
|
# something a caller invented.
|
|
assert forwarded_for("1.2.3.4", PEER, 0) == PEER
|
|
|
|
|
|
def test_one_hop_reads_what_our_proxy_wrote():
|
|
assert forwarded_for("203.0.113.7", PEER, 1) == "203.0.113.7"
|
|
|
|
|
|
def test_a_forged_prefix_is_never_selected():
|
|
# THE test. A caller sends `X-Forwarded-For: 1.2.3.4`; our proxy appends the
|
|
# address it actually saw. Reading from the left would hand the caller a fresh
|
|
# rate-limit bucket for every value they invent.
|
|
assert forwarded_for("1.2.3.4, 203.0.113.7", PEER, 1) == "203.0.113.7"
|
|
# …and padding it doesn't help either.
|
|
assert forwarded_for("a, b, c, d, 203.0.113.7", PEER, 1) == "203.0.113.7"
|
|
|
|
|
|
def test_two_hops_sees_past_a_cdn():
|
|
# Cloudflare appended the real client; our proxy appended Cloudflare.
|
|
assert forwarded_for("203.0.113.7, 172.16.0.5", PEER, 2) == "203.0.113.7"
|
|
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. Reaching further left would start believing
|
|
# entries no proxy of ours wrote, so the safe direction is the socket address —
|
|
# at worst several callers share one bucket.
|
|
assert forwarded_for("203.0.113.7", PEER, 2) == PEER
|
|
assert forwarded_for("", PEER, 1) == PEER
|
|
|
|
|
|
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"
|
|
|
|
|
|
def test_the_key_is_length_bounded():
|
|
# It becomes a dict key in the limiter; an unbounded header must not become an
|
|
# unbounded allocation.
|
|
assert len(forwarded_for("x" * 5000, PEER, 1)) <= 64
|
|
|
|
|
|
def test_trusted_entry_reports_absence_rather_than_guessing():
|
|
# `is_https` needs to tell "no trusted entry" apart from "an entry saying http",
|
|
# which is why this returns None rather than a default.
|
|
assert trusted_entry("", 1) is None
|
|
assert trusted_entry("https", 0) is None
|
|
assert trusted_entry("http, https", 1) == "https"
|