From c458b8e64d367e494a9704695fb1d46785e92cc7 Mon Sep 17 00:00:00 2001 From: Mike Wichers Date: Fri, 4 Sep 2026 17:48:15 -0400 Subject: [PATCH] client IP is the last X-Forwarded-For hop, not the first NPM sets the header with $proxy_add_x_forwarded_for, which appends the connecting address to whatever the client sent. The first hop is therefore client-controlled and the last is the one NPM vouches for. Reading the first hop would have let an outsider claim a LAN address with one header, and admin_api.client_is_lan decides on it. Found while verifying the nginx block removal; fixed before the block came out was relied on. Also tightens login throttling, which used the same helper. --- app/auth.py | 13 +++++++++++-- tests/smoke_admin.py | 4 ++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/auth.py b/app/auth.py index 87f66c8..2cede5a 100644 --- a/app/auth.py +++ b/app/auth.py @@ -42,10 +42,19 @@ def _render(title, body): def _client_ip(request): """Real client address. The app sits behind NPM, so request.client.host is - the proxy on every hit and would throttle the whole site as one address.""" + the proxy on every hit and would throttle the whole site as one address. + + The LAST hop, not the first. NPM sets the header with + $proxy_add_x_forwarded_for, which APPENDS the connecting address to + whatever the client sent, so the first hop is client-controlled and the + last is the one NPM vouches for. With exactly one trusted proxy in front + (the app is published on 127.0.0.1 and the docker network only), the + rightmost address is the real client. Reading the first hop would let an + outsider claim a LAN address with one header - and that is exactly what + admin_api.client_is_lan decides on.""" fwd = request.headers.get("x-forwarded-for", "") if fwd: - return fwd.split(",")[0].strip() + return fwd.split(",")[-1].strip() return request.client.host if request.client else None diff --git a/tests/smoke_admin.py b/tests/smoke_admin.py index bd68039..508992e 100644 --- a/tests/smoke_admin.py +++ b/tests/smoke_admin.py @@ -170,6 +170,10 @@ class _Req: for ip, want in (("10.0.0.55", True), ("10.0.1.20", True), ("127.0.0.1", True), ("172.19.0.31", True), ("108.36.248.87", False), ("192.168.1.9", False), ("", False), ("garbage", False)): check("client_is_lan(%r) is %s" % (ip, want), A.client_is_lan(_Req(ip)) == want) +check("forged first hop does not make an outsider LAN: last hop wins", + A.client_is_lan(_Req("10.0.0.1, 108.36.248.87")) is False) +check("and a forged outside hop does not make a LAN caller outside", + A.client_is_lan(_Req("203.0.113.9, 10.0.0.55")) is True) check("api_keys_from defaults to lan", I.get_setting("api_keys_from") == "lan") raises("api_keys_from rejects other values", 422, I.IdentityError, I.set_setting, "api_keys_from", "vpn") # a key from outside is refused while lan, allowed once anywhere, admin token never from outside