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

@@ -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
 
 

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