fix(auth): key the compose login lockout by client, not by nginx

[hyeonsang010716]

Why

The local login endpoint locks a client out after auth.local.max_login_attempts failures (default 5) for lockout_seconds (default 300), counted per client IP. _get_client_ip takes the TCP peer and honors X-Real-IP only when that peer is listed in AUTH_TRUSTED_PROXIES. That variable is not set anywhere: not in either compose file, Helm, .env.example, config.example.yaml or the README. The only mention is in backend/docs/AUTH_DESIGN.md.

In the Docker stack every browser request reaches the Gateway from the nginx container, so every login was counted under nginx's address. Five wrong passwords from anyone locked every user out of login for five minutes, and it can be repeated indefinitely. Uvicorn's own X-Forwarded-For handling does not help here: it trusts only 127.0.0.1 by default, and nginx is never 127.0.0.1 inside the compose network.

Reproduced on main with the real docker/nginx/nginx.conf in an nginx:alpine container and main's real auth router served by Uvicorn (compose's command, default proxy settings) in the Gateway image, on a separate Docker network. Each client was its own container with its own IP, going through nginx; only the password check was stubbed to always fail:

attacker 192.168.200.50, 5 wrong passwords: 401 401 401 401 401
victim   192.168.200.60, first attempt:     429

The Gateway access log shows every request from the nginx container's address.

Found during a repo-wide bug review; there is no tracking issue.

What changed

• AUTH_TRUSTED_PROXIES entries can now be hostnames as well as IPs and CIDRs. A hostname stands for every address it resolves to.
  • The lookup goes through the loop's getaddrinfo (executor), so it never blocks the event loop.
  • Results are cached for 10 s, the same validity nginx uses for its own resolver (resolver ... valid=10s), so a restarted proxy container that comes back on a new address is picked up.
  • Failed lookups are cached as "trust nothing" for the same 10 s, which is today's direct-mode behavior. Unauthenticated requests therefore cannot drive a lookup per request.
  • One lookup per hostname runs at a time. The cache check and an in-flight task registration happen without a yield between them, so concurrent misses join that task (through asyncio.shield, so a disconnecting request cannot cancel the lookup the others joined), and the task caches its own result.
  • A request without X-Real-IP never triggers a lookup.
  • An entry that is neither an IP/CIDR nor an RFC 1123 hostname is still warned about and skipped. A numeric last label such as 10.0.0 counts as invalid, so it is never handed to the resolver, which would read it as 10.0.0.0.
• _get_client_ip is now async. Its two callers, login_local and setup_status, await it.
• Both compose files pass AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-nginx} to the Gateway. A value an operator sets in .env still wins under both launchers:
  • make up (deploy.sh) calls Compose with --env-file .env.
  • make docker-start (docker.sh start) runs Compose from docker/ without --env-file, so interpolation sees only the shell, and the resolved environment: entry outranks the same key loaded through env_file. load_proxy_env_from_dotenv, which already exports the proxy variables from .env for this reason, now exports AUTH_TRUSTED_PROXIES too. A shell export still wins, as it does for Compose. It also strips a CRLF line ending before the quotes, so a quoted value in a Windows-edited .env loses its quotes (this applies to the proxy variables as well).
• :- is deliberate: an explicitly empty value also means nginx, because every browser request arrives through it, so direct mode inside the stack would only bring back the shared lockout. The compose comment and .env.example say so.

Why a hostname instead of a subnet: only docker-compose-dev.yaml pins its subnet (192.168.200.0/24). The production network is a plain bridge whose subnet Docker assigns. Pinning it would risk colliding with operators' LAN/VPN ranges and would force a network recreate on upgrade. The service name is stable in both files, and it trusts exactly the proxy instead of every container on the network (redis, frontend, provisioner).

Trusting nginx is safe only because nginx overwrites X-Real-IP with $remote_addr on every route to the Gateway, so a client cannot choose its own bucket. That holds today in all three maintained nginx configs and is now pinned by a test.

Deliberately out of scope:

• Helm. Behind the chart's Ingress, nginx's own $remote_addr is the ingress controller (or a SNAT'd node address), so trusting the nginx pods alone would still give one bucket. A correct default needs nginx's real_ip configured for each cluster's ingress, which is a separate change.
• Deployments behind another reverse proxy (BIND_HOST=0.0.0.0 behind an operator's own front door) have the same issue one hop earlier: nginx sees that proxy's address. .env.example now says to configure nginx's real_ip module for it.
• Keying the lockout by (ip, email). That would let anyone who knows an address lock out that one account on purpose, which is a design change rather than this fix.
• FORWARDED_ALLOW_IPS=*. It makes Uvicorn take the leftmost, client-supplied X-Forwarded-For entry as the peer. That is pre-existing, not used by any launcher, and unaffected by this change.
• fix(auth): trust forwarded headers only from trusted proxies #4715 gates the CSRF/OIDC Forwarded / X-Forwarded-* trust on the same variable with an IP/CIDR-only parser. If it lands after this, the compose default nginx should mean the same thing there. That's worth reconciling when one of the two rebases; this PR does not touch those headers.

Docs: .env.example (new AUTH_TRUSTED_PROXIES entry), backend/docs/AUTH_DESIGN.md, scripts/AGENTS.md (the dev launcher's dotenv export), the login-throttling comment in config.example.yaml, CHANGELOG.md, CHANGELOG_zh.md. The README does not describe the lockout or proxy trust, and with the compose default the Docker stack needs no operator action, so it remains accurate.

Surface area

• Frontend UI — page / component / setting / interaction under frontend/
• Backend API — endpoint / SSE event / request-response shape under backend/app
  • No shape change. POST /api/v1/auth/login/local and GET /api/v1/auth/setup-status key their per-IP state by the forwarded client address in the compose stack instead of nginx's.
• Agents / LangGraph — agent node, graph wiring, langgraph.json, or prompt change
• Sandbox — docker/ or sandboxed execution
  • docker/docker-compose.yaml and docker/docker-compose-dev.yaml set AUTH_TRUSTED_PROXIES on the Gateway.
  • scripts/docker.sh start exports AUTH_TRUSTED_PROXIES from .env for dev-compose interpolation.
• Skills — change under skills/
• Dependencies — new/upgraded entry in backend/pyproject.toml or frontend/package.json
• Default behavior change — changes existing behavior without the user opting in
  • Docker deployments now trust the bundled nginx's X-Real-IP for the login lockout by default. Outside Docker nothing changes unless AUTH_TRUSTED_PROXIES is set.
• Docs / tests / CI only — no runtime behavior change

Screenshots / Recording

N/A (no UI change).

Bug fix verification

• End-to-end, Docker: same setup as above with this branch's app/ and the compose default AUTH_TRUSTED_PROXIES=nginx. Rerun on the branch rebased onto fix(auth): count login lockouts in the database across Gateway replicas #6501: unset, the victim's first attempt is 429; with nginx, it is 401.
  • The victim's first attempt now returns 401, and the attacker's sixth returns 429.
  • After restarting nginx on a different address (.3 → .4) and waiting past the cache TTL, the attacker is still locked out on their own and a new client gets 401.
  • A forged X-Real-IP sent through nginx is overwritten and gets the sender no new bucket. I checked this in the control run on main with nginx's subnet trusted; the overwrite is nginx's, so it is the same here.
• Tests, backend/tests/test_auth.py:
  • test_login_lockout_is_per_client_behind_a_proxy_named_by_hostname[localhost-401|None-429] drives the real login endpoint over ASGI from a proxy peer. Five failures from one forwarded client, then a different client: 401 with a trusted hostname. The None case documents the shared bucket.
  • test_login_lockout_key_through_uvicorn_proxy_headers[uvicorn-default|aligned-with-nginx] runs the same request through Uvicorn's ProxyHeadersMiddleware, which runs before the app. With the default trusted hosts, and with FORWARDED_ALLOW_IPS aligned to nginx, the lockout is keyed by the browser's address and never by a forged X-Forwarded-For entry.
  • Hostname trust, failure caching (one lookup for two requests), re-resolution after the TTL, no lookup without X-Real-IP, and invalid entries (not an ip, 10.0.0) are warned about and not resolved.
  • test_concurrent_cache_misses_share_one_lookup gates the resolver in its executor thread and sends 20 concurrent requests: one resolver call, 20 correct answers. test_cancelled_request_does_not_cancel_the_shared_lookup cancels the request that started a gated lookup: the request that joined it still gets the answer, there is one resolver call, and the result is cached.
  • The seven existing test_get_client_ip_* tests now await the function; their assertions are unchanged.
• Test, backend/tests/blocking_io/test_login_local_throttle.py::test_trusted_proxy_hostname_resolution_does_not_block_loop[memory|sql]: the strict Blockbuster gate on the login path with a hostname entry. The forwarded client is locked through the LoginThrottleStore fixture from fix(auth): count login lockouts in the database across Gateway replicas #6501.
• Tests, backend/tests/test_compose_auth_trusted_proxies.py: both compose files carry the default and share a network with nginx. The real docker compose config renders nginx when the variable is unset or explicitly empty, and an .env override verbatim. test_make_docker_start_keeps_a_dotenv_override runs the real docker.sh start (its cwd, arguments and environment; only up ... is swapped for config) against the real dev compose file: unset and explicitly empty render nginx; a plain, and a quoted CRLF, .env value renders verbatim; a shell export beats .env. Every Gateway location in the three nginx configs overwrites X-Real-IP with $remote_addr.
• Red on main, green on this branch: yes. Against current main including fix(auth): count login lockouts in the database across Gateway replicas #6501 (separate worktree), the endpoint test's localhost case fails with assert 429 == 401, and so does the Uvicorn-default case of the proxy-middleware test. The hostname tests fail as well, and so do the compose default tests and the blocking-I/O test. The seven converted tests fail on main only because they await a sync function, which isn't meaningful.
• Mutation check, one change at a time, each caught:
  • Never recognizing hostnames: fails 9 tests.
  • Not caching failed lookups: fails the caching test.
  • No in-flight sharing: fails both concurrency tests.
  • Joining without asyncio.shield: fails the cancellation test.
  • Compose ${AUTH_TRUSTED_PROXIES-nginx} (empty value means direct mode): fails 2 tests.
  • A cache that never expires: fails the TTL test.
  • Resolving without X-Real-IP: fails 2 tests. test_get_client_ip_xff_never_honored also catches the empty-header return.
  • Calling socket.getaddrinfo on the event loop: fails 4 tests, including the blocking-I/O test.
  • Dropping the prod compose default: fails 3 tests.
  • Removing X-Real-IP $remote_addr from nginx's /api/ location: fails the nginx test.
  • docker.sh not exporting AUTH_TRUSTED_PROXIES from .env (the state the second review found): fails the dotenv-override and quoted-crlf cases.
  • Stripping the quotes before the CR again: fails quoted-crlf.
  • .env overriding a shell export: fails shell-export-wins.

Validation

cd backend && make lint
#   All checks passed!
#   1930 files already formatted

cd backend && make test-blocking-io
#   237 passed

cd backend && make test
#   4 shards: 26867 passed, 456 skipped, 0 failed

These numbers are from the docker.sh fix (830302f), on top of the maintainer's merge of main. One test was deselected: test_subagent_batch_service.py::test_cancelled_batch_stop_allows_successful_retry (from #6517) hangs in this environment. It hangs the same way on upstream main (5ecc1c2), where the file is identical, and it is unrelated to this change.

scripts/check_agent_guidance.py --base-ref main --head-ref HEAD: 0 errors, 0 warnings.

AI assistance

Tool(s) used: Claude Code

How you used it: Claude Code was used as an implementation and verification assistant. I led the investigation: I picked this finding out of a repo-wide bug review, had Claude Code trace how the client IP is derived through nginx, Uvicorn's proxy-header middleware and _get_client_ip, and accepted it as a bug only after reproducing the shared lockout with the real nginx config and auth router in Docker. That check also showed that the original finding's suggested fix, a subnet default, cannot work for the production compose file, whose subnet Docker assigns.

I then defined the regression coverage: the real login endpoint from a proxy peer, the same request through Uvicorn's proxy middleware, the hostname cache's failure and expiry behavior, a strict blocking-I/O check of the lookup, the rendered compose default and override, and an nginx-config guard on the X-Real-IP overwrite that makes trusting nginx safe. I had Claude Code write the failing tests first, then the fix, and directed and reviewed the validation: the red check against main, the mutation check of each change, the end-to-end Docker reruns including an nginx restart on a new address, the full backend suite and the agent-guidance check. Claude Code helped draft the docs and changelog entries, which I reviewed and refined.

I also made the final design decisions: naming the proxy by hostname instead of trusting a subnet, caching failed lookups, skipping the lookup without X-Real-IP, making _get_client_ip async rather than adding a separate refresh step a future caller could forget, and keeping Helm, outer reverse proxies, (ip, email) keys and FORWARDED_ALLOW_IPS=* out of this PR.

In short, Claude Code accelerated implementation and verification; I owned the investigation, technical decisions, scope, review, and final changes.

• I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

[willem-bd]

Read-only static review of pinned current revision; verified introduced findings below. No PR code or tests executed.

[willem-bd]

[P2] Coalesce concurrent trusted-proxy DNS cache misses

The cache is populated only after this await, with no per-host inflight reservation. With the new Docker default AUTH_TRUSTED_PROXIES=nginx, let the cached entry expire while DNS is slow or timing out, then send concurrent GET /api/v1/auth/setup-status requests. Every request sees the same miss and starts its own getaddrinfo before any result is cached; the endpoint's existing single-flight guard is reached only afterward. Thus an unauthenticated burst still drives one DNS lookup per request, including failing lookups, rather than one lookup per 10-second refresh. This queues redundant resolver work and stalls legitimate setup/login requests; on standard asyncio it also occupies the default executor shared with password verification and throttle policy reads. Reserve one shared resolution per hostname before yielding, have concurrent callers join it with cancellation-safe ownership, and add a gated-resolver concurrency regression rather than only the existing sequential cache test.

[hyeonsang010716]

Fixed in b29f449. The cache check and the in-flight registration now happen with no yield between them (_trusted_proxy_host_inflight); concurrent misses join that task through asyncio.shield, the task writes the cache itself, and a done-callback removes the in-flight entry only if it is still that task. So a cancelled request cannot cancel the shared lookup, and the result is kept even if every caller went away.

Two gated-resolver regressions (the fake getaddrinfo blocks in its executor thread): test_concurrent_cache_misses_share_one_lookup sends 20 concurrent requests → one resolver call, 20 correct answers; test_cancelled_request_does_not_cancel_the_shared_lookup cancels the request that started the lookup → the joiner still gets the answer, one call, result cached. Mutation-checked: dropping the in-flight lookup fails both, and joining without shield fails the cancellation test.

[willem-bd]

Reviewed head 9f18f4d. The diagnosis and the trust design look sound: keying on X-Real-IP only from a peer trusted by IP/CIDR/hostname, failing closed on unresolved hostnames, caching failed lookups, never resolving without X-Real-IP, and pinning nginx's $remote_addr overwrite for every Gateway location in all three configs is the right shape, and the compose default fixes the shared-bucket lockout. Two findings below, the first one blocking: PR #6501 (DB-backed login throttle) merged into main today and rewrites most of the same files, so this branch needs a rebase and one test needs rework against the store.

[willem-bd]

[P1] This branch predates #6501, which merged into main today and rewrote exactly these files: _login_attempts and the in-process _check_rate_limit/_record_failure_under_policy are gone (replaced by app.gateway.auth.login_throttle + deerflow.persistence.login_throttle with a store fixture), test_auth.py was heavily reshuffled around the same test_get_client_ip_* section this PR converts to async, and config.example.yaml, AUTH_DESIGN.md, and both CHANGELOGs were edited in the same regions. As written, this appended test cannot port as-is: it locks the forwarded client by writing auth_router._login_attempts[_CLIENT_IP] and drives auth_router.login_local against the old provider-stub context (_CLIENT_IP, _form()) that main's rewritten file no longer defines. Please rebase and re-lock the forwarded IP through the store fixture instead.

The good news: the two changes compose semantically. Main still has a sync _get_client_ip with the same two callers (login_local, setup_status) and keys the throttle store by the same client-IP string, so the async hostname-trust change and the compose default apply cleanly on top; only the throttle plumbing underneath differs. Note the PR's red/green and full-suite validation ran against a pre-#6501 main, so the suite should be rerun after the rebase.

[hyeonsang010716]

Rebased onto main with #6501 (now at b29f449). The blocking-I/O test now locks the forwarded client through the store fixture (record_failure × max_attempts), so it runs against both the memory and SQL stores; the two endpoint tests in test_auth.py use memory_throttle_store and assert on the store (fail_count under the browser address, nothing under nginx or the forged X-Forwarded-For entry). config.example.yaml, AUTH_DESIGN.md and both changelogs keep #6501's text with only this PR's sentences added.

Re-ran everything on the rebased head: red on current main (the endpoint localhost case fails 429 == 401, as does the Uvicorn-default proxy-middleware case), make test 26832 passed / 0 failed, make test-blocking-io 237 passed, and the Docker repro with #6501 in place (unset → victim 429, nginx → 401).

[willem-bd]

[P3] Compose's :- substitutes the default for an explicitly empty value too, so an operator who sets AUTH_TRUSTED_PROXIES= in .env — which the app and .env.example document as direct mode ("Empty / unset = no proxy is trusted") — silently gets nginx instead. There is no .env-only way to express direct mode inside the compose stack. If that is intended (the stack always has nginx in front, so direct mode is not meaningful there), consider saying so in the compose comment or .env.example; if not, ${AUTH_TRUSTED_PROXIES-nginx} (no colon) substitutes only when the variable is unset. Also worth covering in test_real_compose_renders_the_default_and_keeps_an_operator_override, which currently parametrizes only unset and non-empty values.

[hyeonsang010716]

Intended, so I kept :- and said why: inside the stack every browser request comes through nginx, so an empty value meaning direct mode would only bring the shared lockout back (and a stray AUTH_TRUSTED_PROXIES= line in .env would do that silently). The compose comment in both files and .env.example now say that an empty value falls back to nginx in Docker while it stays direct mode outside it. test_real_compose_renders_the_default_and_keeps_an_operator_override gained an explicitly-empty case (renders nginx); switching prod to ${AUTH_TRUSTED_PROXIES-nginx} fails it.

[WillemJiang]

@hyeonsang010716 please resolve the conflicts with the main branch.

[hyeonsang010716]

@WillemJiang Conflicts resolved: rebased onto main including #6501 (head b29f449, mergeable again). The review items are answered in their threads; the PR description has the re-run numbers.

[willem-bd]

Read-only static review of pinned current revision; verified introduced findings below. No PR code or tests executed.

[willem-bd]

[P2] Preserve nonempty dev dotenv proxy overrides

With a nonempty AUTH_TRUSTED_PROXIES value in the checkout's .env and no shell export, normal make docker-start now silently replaces that allowlist with nginx. scripts/docker.sh runs Compose from docker/ without --env-file ../.env and only exports HTTP proxy variables, so this interpolation does not see the checkout .env. The explicit environment entry then overrides the value loaded through the Gateway's existing service env_file. Existing operator-defined IP/CIDR policies therefore stop applying: intended proxy peers can fall back to shared peer-address lockout buckets, while nginx is trusted even when absent from the configured list. Pass the checkout dotenv file to the actual dev Compose invocation (or otherwise preserve that value before applying the default), and cover the real launcher's cwd/arguments; the new render test supplies --env-file itself and misses this path.

[hyeonsang010716]

Confirmed and fixed in 830302f. docker.sh start runs Compose from docker/ without --env-file, so a non-empty .env value never reached interpolation, and the resolved environment: entry replaced the one env_file loaded. The launcher's load_proxy_env_from_dotenv already exports the proxy variables from .env for exactly this reason, so AUTH_TRUSTED_PROXIES is now in that list. A shell export still wins, as it does for Compose. I didn't add --env-file ../.env to the dev invocation: that would change interpolation for every variable in the dev compose file (PORT, BIND_HOST, DEER_FLOW_INTERNAL_AUTH_TOKEN, ...), which is out of scope here. While there, the loader now strips a CRLF before the quotes; before, a quoted value in a Windows-edited .env kept its quotes (this affected the proxy variables too).

test_make_docker_start_keeps_a_dotenv_override runs the real start with its own cwd, arguments and exports. A docker shell function swaps only up ... for config, and the test renders the real dev compose file. Cases: unset → nginx, explicitly empty → nginx, a .env list → verbatim, a quoted CRLF value → unquoted, and a shell export beats .env. Before the fix, the .env and CRLF cases rendered nginx. Each of the three changes is mutation-checked: dropping the export, restoring the old strip order, and letting .env override the shell each fail their case. scripts/AGENTS.md and both changelog entries now describe it, and the _render docstring no longer claims docker.sh passes --env-file.