From cb886e50d03ef0dc24fada5e21121dd65d1511e2 Mon Sep 17 00:00:00 2001 From: hyeonsang010716 Date: Thu, 8 Oct 2026 23:00:21 +0900 Subject: [PATCH 1/4] fix(auth): key the compose login lockout by client, not by nginx Failed logins are counted per client IP, and the Gateway honors X-Real-IP only from a peer listed in AUTH_TRUSTED_PROXIES. The compose files never set it, and every browser request reaches the Gateway from the nginx container, so all logins shared nginx's address: five wrong passwords from anyone locked every user out for five minutes. AUTH_TRUSTED_PROXIES now also accepts hostnames, resolved off the event loop and cached for 10 seconds (failed lookups included, and only when X-Real-IP is present), and both compose files default it to the bundled nginx service, whose address Docker assigns. nginx overwrites X-Real-IP with $remote_addr on every Gateway route; a test pins that for the three maintained configs. --- .env.example | 9 + CHANGELOG.md | 12 ++ CHANGELOG_zh.md | 6 + backend/app/gateway/routers/auth.py | 100 +++++++-- backend/docs/AUTH_DESIGN.md | 2 +- .../blocking_io/test_login_local_throttle.py | 28 +++ backend/tests/test_auth.py | 203 ++++++++++++++++-- .../test_compose_auth_trusted_proxies.py | 105 +++++++++ config.example.yaml | 4 +- docker/docker-compose-dev.yaml | 5 + docker/docker-compose.yaml | 5 + 11 files changed, 438 insertions(+), 41 deletions(-) create mode 100644 backend/tests/test_compose_auth_trusted_proxies.py diff --git a/.env.example b/.env.example index c635cb02a74..c83741420da 100644 --- a/.env.example +++ b/.env.example @@ -35,6 +35,15 @@ UNBROWSE_API_KEY=your-unbrowse-api-key # BIND_HOST=0.0.0.0 # PORT=2026 +# Proxies whose X-Real-IP the Gateway trusts when it counts failed logins per +# client (comma-separated IPs, CIDRs or hostnames; a hostname is re-resolved +# every 10s). The Docker stack defaults to its bundled `nginx` service; without +# it every browser login shares nginx's address, so five wrong passwords from +# anyone lock everyone out. Behind your own front door, nginx sees that proxy's +# address instead: configure nginx's real_ip module for it as well. +# Unset outside Docker (direct mode): X-Real-IP is ignored. +# AUTH_TRUSTED_PROXIES=nginx + # Optional: # FIRECRAWL_API_KEY=your-firecrawl-api-key # VOLCENGINE_API_KEY=your-volcengine-api-key diff --git a/CHANGELOG.md b/CHANGELOG.md index 5edfa00447a..61eadf41204 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -804,6 +804,18 @@ This release closes that milestone with **439 merged pull requests**. ### Fixed +- **auth:** In the Docker stack, five wrong passwords from one client no longer + lock every user out of login for five minutes. Failed logins are counted per + client IP, and the Gateway honors `X-Real-IP` only from a peer listed in + `AUTH_TRUSTED_PROXIES`, which the compose files never set; every browser + request reaches the Gateway from the `nginx` container, so all logins shared + nginx's address and one lockout. `AUTH_TRUSTED_PROXIES` now also accepts + hostnames, resolved off the event loop and cached for 10 seconds (failures + included), and both compose files default it to the bundled `nginx` service. + nginx overwrites `X-Real-IP` with `$remote_addr` on every Gateway route, so + a client cannot choose its own address. An `AUTH_TRUSTED_PROXIES` value in + `.env` still takes precedence. Deployments behind another reverse proxy also + need nginx's `real_ip` module for that proxy; see `.env.example`. - **auth:** Login lockouts are now counted once per client IP across every Gateway replica. `POST /api/v1/auth/login/local` kept its failed-login counter in a per-process dict, so with N replicas behind one load balancer an diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 66a513855d4..59d6828a0cd 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -694,6 +694,12 @@ ### 修复 +- **认证：** 在 Docker 部署中，某个客户端输错 5 次密码不会再让所有用户 5 分钟内无法登录。登录失败按客户端 IP 计数，而 Gateway + 只在 TCP peer 属于 `AUTH_TRUSTED_PROXIES` 时信任 `X-Real-IP`，compose 文件却从未设置该变量；所有浏览器请求都经由 `nginx` + 容器到达 Gateway，因此所有登录共用 nginx 的地址和同一个锁定。现在 `AUTH_TRUSTED_PROXIES` 也接受主机名，在事件循环外解析并 + 缓存 10 秒（包括解析失败），两个 compose 文件默认将其设为内置的 `nginx` 服务。nginx 在每个转发到 Gateway 的路由上都用 + `$remote_addr` 覆盖 `X-Real-IP`，客户端无法自选地址。`.env` 中设置的 `AUTH_TRUSTED_PROXIES` 仍然优先。位于其他反向代理之后的 + 部署还需要为该代理配置 nginx 的 `real_ip` 模块，详见 `.env.example`。 - **技能：** `/技能名` 激活现在在模型调用重试时不再丢失。此前激活在调用模型之前就被标记为已完成，因此调用失败 （限流、过载、超时）或返回空响应而重试时，重试请求不再包含 `SKILL.md` 正文，而该技能的工具限制仍然生效。现在重试 会携带与首次尝试相同的激活内容，不会重新读取技能或重复记录激活，重试得到的响应也保留技能使用记录。([#6506]) diff --git a/backend/app/gateway/routers/auth.py b/backend/app/gateway/routers/auth.py index 35efe23584e..f868835a448 100644 --- a/backend/app/gateway/routers/auth.py +++ b/backend/app/gateway/routers/auth.py @@ -4,6 +4,7 @@ import logging import os import re +import socket import time import urllib.parse from ipaddress import ip_address, ip_network @@ -222,18 +223,38 @@ def _login_throttle_policy() -> tuple[int, float]: return local.max_login_attempts, local.lockout_seconds -def _trusted_proxies() -> list: - """Parse ``AUTH_TRUSTED_PROXIES`` env var into a list of ip_network objects. +# Hostname entries (e.g. the compose service ``nginx``) are re-resolved at most +# this often: a restarted proxy container comes back on a new address, and +# nginx's own resolver uses the same validity (docker/nginx/nginx.conf). +_TRUSTED_PROXY_HOST_TTL_SECONDS = 10.0 +_HOSTNAME_LABEL = re.compile(r"[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?") +# hostname -> (monotonic expiry, resolved addresses). Failed lookups are cached +# as an empty set too, so unauthenticated requests cannot drive a lookup storm. +_trusted_proxy_host_cache: dict[str, tuple[float, frozenset]] = {} - Comma-separated CIDR or single-IP entries. Empty / unset = no proxy is - trusted (direct mode). Invalid entries are skipped with a logger warning. - Read live so env-var overrides take effect immediately and tests can - ``monkeypatch.setenv`` without poking a module-level cache. + +def _is_hostname(entry: str) -> bool: + """RFC 1123 hostname whose last label is not numeric (so ``10.0.0`` is not one).""" + labels = entry.split(".") + return len(entry) <= 253 and all(_HOSTNAME_LABEL.fullmatch(label) for label in labels) and not labels[-1].isdigit() + + +def _trusted_proxies() -> tuple[list, list[str]]: + """Parse ``AUTH_TRUSTED_PROXIES`` into (ip_network objects, hostnames). + + Comma-separated CIDR, single-IP or hostname entries. Empty / unset = no + proxy is trusted (direct mode). A hostname stands for every address it + resolves to, so a proxy without a fixed address (the compose ``nginx`` + service) can still be named. Invalid entries are skipped with a logger + warning. The variable is read live so env-var overrides take effect + immediately and tests can ``monkeypatch.setenv``; only the addresses a + hostname resolves to are cached (``_trusted_proxy_host_addresses``). """ raw = os.getenv("AUTH_TRUSTED_PROXIES", "").strip() if not raw: - return [] + return [], [] nets = [] + hostnames = [] for entry in raw.split(","): entry = entry.strip() if not entry: @@ -241,11 +262,44 @@ def _trusted_proxies() -> list: try: nets.append(ip_network(entry, strict=False)) except ValueError: - logger.warning("AUTH_TRUSTED_PROXIES: ignoring invalid entry %r", entry) - return nets + if _is_hostname(entry): + hostnames.append(entry) + else: + logger.warning("AUTH_TRUSTED_PROXIES: ignoring invalid entry %r", entry) + return nets, hostnames + + +async def _trusted_proxy_host_addresses(hostname: str) -> frozenset: + """Addresses ``hostname`` resolves to, cached for ``_TRUSTED_PROXY_HOST_TTL_SECONDS``. + + The lookup runs in the loop's executor (``getaddrinfo`` blocks). A failed + lookup trusts nothing until the entry expires, which is the direct-mode + behavior, not an error for the request. + """ + cached = _trusted_proxy_host_cache.get(hostname) + if cached is not None and time.monotonic() < cached[0]: + return cached[1] + try: + infos = await asyncio.get_running_loop().getaddrinfo(hostname, None, type=socket.SOCK_STREAM) + addresses = frozenset(ip_address(info[4][0]) for info in infos) + except (OSError, ValueError) as exc: + logger.warning("AUTH_TRUSTED_PROXIES: could not resolve %r (%s); not trusting it for %.0fs", hostname, exc, _TRUSTED_PROXY_HOST_TTL_SECONDS) + addresses = frozenset() + _trusted_proxy_host_cache[hostname] = (time.monotonic() + _TRUSTED_PROXY_HOST_TTL_SECONDS, addresses) + return addresses + + +async def _is_trusted_proxy(peer_ip) -> bool: + nets, hostnames = _trusted_proxies() + if any(peer_ip in net for net in nets): + return True + for hostname in hostnames: + if peer_ip in await _trusted_proxy_host_addresses(hostname): + return True + return False -def _get_client_ip(request: Request) -> str: +async def _get_client_ip(request: Request) -> str: """Extract the real client IP for rate limiting. Trust model: @@ -255,30 +309,30 @@ def _get_client_ip(request: Request) -> str: by the client itself. - ``X-Real-IP`` is **only** honored if the TCP peer is in the ``AUTH_TRUSTED_PROXIES`` allowlist (set via env var, comma-separated - CIDR or single IPs). When set, the gateway is assumed to be behind a - reverse proxy (nginx, Cloudflare, ALB, …) that overwrites - ``X-Real-IP`` with the original client address. + CIDR, single IPs or hostnames). When set, the gateway is assumed to be + behind a reverse proxy (nginx, Cloudflare, ALB, …) that overwrites + ``X-Real-IP`` with the original client address. The compose files + name their bundled ``nginx`` service here by default. - With no ``AUTH_TRUSTED_PROXIES`` set, ``X-Real-IP`` is silently ignored — closing the bypass where any client could rotate the header to dodge per-IP rate limits in dev / direct-gateway mode. ``X-Forwarded-For`` is intentionally NOT used because it is naturally client-controlled at the *first* hop and the trust chain is harder to - audit per-request. + audit per-request. A request without ``X-Real-IP`` never resolves a + hostname entry. """ peer_host = request.client.host if request.client else None + real_ip = request.headers.get("x-real-ip", "").strip() - trusted = _trusted_proxies() - if trusted and peer_host: + if peer_host and real_ip: try: peer_ip = ip_address(peer_host) - if any(peer_ip in net for net in trusted): - real_ip = request.headers.get("x-real-ip", "").strip() - if real_ip: - return real_ip except ValueError: # peer_host wasn't a parseable IP (e.g. "unknown") — fall through - pass + peer_ip = None + if peer_ip is not None and await _is_trusted_proxy(peer_ip): + return real_ip return peer_host or "unknown" @@ -358,7 +412,7 @@ async def login_local( remember_me: bool = Form(default=True), ): """Local email/password login.""" - client_ip = _get_client_ip(request) + client_ip = await _get_client_ip(request) await _check_rate_limit(client_ip) user = await get_local_provider().authenticate({"email": form_data.username, "password": form_data.password}) @@ -672,7 +726,7 @@ async def revoke_pat(request: Request, pat_id: str): @router.get("/setup-status") async def setup_status(request: Request): """Check if an admin account exists. Returns needs_setup=True when no admin exists.""" - client_ip = _get_client_ip(request) + client_ip = await _get_client_ip(request) now = time.time() # Return cached result when within TTL — avoids 429 on multi-tab reconnection. diff --git a/backend/docs/AUTH_DESIGN.md b/backend/docs/AUTH_DESIGN.md index 9d725d2f051..4fc6991cd88 100644 --- a/backend/docs/AUTH_DESIGN.md +++ b/backend/docs/AUTH_DESIGN.md @@ -110,7 +110,7 @@ enum UserScope: - 成功后签发 JWT，放入 `access_token` HttpOnly cookie。 - 响应体只返回 `expires_in` 和 `needs_setup`，不返回 token。 -登录失败会按客户端 IP 计数。IP 解析只在 TCP peer 属于 `AUTH_TRUSTED_PROXIES` 时信任 `X-Real-IP`，不使用 `X-Forwarded-For`。阈值与锁定时长可通过 `auth.local.max_login_attempts`（默认 5）和 `auth.local.lockout_seconds`（默认 300 秒）配置，按次实时读取，改配置后下一次登录即生效，无需重启 Gateway（`max_login_attempts` 最小为 2：单次失败不得锁定 IP。时长热改按方向生效：下调可提前释放进行中的锁定、收紧阈值会保留已计数的失败；上调只延长仍在锁定期内的锁定，不会复活已服满原时长的锁定）。计数器的存放位置由 `auth.local.throttle_storage` 决定（启动时解析一次，改动需重启）：默认 `auto` 在 `database.backend` 为 `sqlite` / `postgres` 时使用应用数据库中的 `login_throttle` 表，所有共享该数据库的 Gateway 副本对同一 IP 执行同一份限制；`memory` 退回进程内计数器（N 个副本意味着攻击者有 N × `max_login_attempts` 次机会，且一个副本上的锁定对其他副本不可见，多实例部署会在启动时打 WARNING）；`db` 强制使用共享表，数据库为 `memory` 时降级为进程内计数器并打 WARNING，已配置的数据库引擎不可用时则拒绝启动（`auto` 在该情况下降级并打 WARNING）。锁定期内的再次失败不会重置 `locked_at`：锁定时长从锁定开始计算，而不是从最后一次尝试计算。 +登录失败会按客户端 IP 计数。IP 解析只在 TCP peer 属于 `AUTH_TRUSTED_PROXIES` 时信任 `X-Real-IP`，不使用 `X-Forwarded-For`。`AUTH_TRUSTED_PROXIES` 接受逗号分隔的 IP、CIDR 或主机名；主机名在事件循环外解析，结果（包括解析失败）缓存 10 秒，因此重启后换了地址的代理容器仍能被识别。两个 compose 文件默认把它设为内置的 `nginx` 服务：否则所有浏览器登录都以 nginx 的地址计数，任何人输错 5 次密码就会锁住所有用户。nginx 在每个转发到 Gateway 的 location 中都用 `$remote_addr` 覆盖 `X-Real-IP`，这是信任它的前提。阈值与锁定时长可通过 `auth.local.max_login_attempts`（默认 5）和 `auth.local.lockout_seconds`（默认 300 秒）配置，按次实时读取，改配置后下一次登录即生效，无需重启 Gateway（`max_login_attempts` 最小为 2：单次失败不得锁定 IP。时长热改按方向生效：下调可提前释放进行中的锁定、收紧阈值会保留已计数的失败；上调只延长仍在锁定期内的锁定，不会复活已服满原时长的锁定）。计数器的存放位置由 `auth.local.throttle_storage` 决定（启动时解析一次，改动需重启）：默认 `auto` 在 `database.backend` 为 `sqlite` / `postgres` 时使用应用数据库中的 `login_throttle` 表，所有共享该数据库的 Gateway 副本对同一 IP 执行同一份限制；`memory` 退回进程内计数器（N 个副本意味着攻击者有 N × `max_login_attempts` 次机会，且一个副本上的锁定对其他副本不可见，多实例部署会在启动时打 WARNING）；`db` 强制使用共享表，数据库为 `memory` 时降级为进程内计数器并打 WARNING，已配置的数据库引擎不可用时则拒绝启动（`auto` 在该情况下降级并打 WARNING）。锁定期内的再次失败不会重置 `locked_at`：锁定时长从锁定开始计算，而不是从最后一次尝试计算。 ### 注册 diff --git a/backend/tests/blocking_io/test_login_local_throttle.py b/backend/tests/blocking_io/test_login_local_throttle.py index 8629dd9c030..dee01341ac9 100644 --- a/backend/tests/blocking_io/test_login_local_throttle.py +++ b/backend/tests/blocking_io/test_login_local_throttle.py @@ -150,3 +150,31 @@ async def test_store_reset_and_probe_do_not_block_loop(store) -> None: assert await store.get(_CLIENT_IP) is None await store.reset(_CLIENT_IP) # idempotent on a clean IP + + +async def test_trusted_proxy_hostname_resolution_does_not_block_loop(store, monkeypatch) -> None: + """A hostname in AUTH_TRUSTED_PROXIES (the compose default ``nginx``) is + resolved on the login path; ``getaddrinfo`` must run off the event loop.""" + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "localhost") + auth_router._trusted_proxy_host_cache.clear() + request = Request( + { + "type": "http", + "method": "POST", + "path": "/api/v1/auth/login/local", + "headers": [(b"x-real-ip", _CLIENT_IP.encode())], + "query_string": b"", + "client": ("127.0.0.1", 44000), + "server": ("testserver", 80), + } + ) + for _ in range(_POLICY["max_attempts"]): + await store.record_failure(_CLIENT_IP, **_POLICY) # the forwarded client is locked + try: + with pytest.raises(HTTPException) as exc_info: + await auth_router.login_local(request, Response(), _form(), remember_me=True) + finally: + auth_router._trusted_proxy_host_cache.clear() + + # 429 proves the lockout was looked up under X-Real-IP, i.e. "localhost" resolved. + assert exc_info.value.status_code == 429 diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index 876ade42a0b..df638839f7c 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -1796,7 +1796,8 @@ async def authenticate(self, credentials): # ── Client IP extraction ───────────────────────────────────────────────── -def test_get_client_ip_direct_connection_no_proxy(monkeypatch): +@pytest.mark.asyncio +async def test_get_client_ip_direct_connection_no_proxy(monkeypatch): """Direct mode (no AUTH_TRUSTED_PROXIES): use TCP peer regardless of X-Real-IP.""" monkeypatch.delenv("AUTH_TRUSTED_PROXIES", raising=False) from app.gateway.routers.auth import _get_client_ip @@ -1804,10 +1805,11 @@ def test_get_client_ip_direct_connection_no_proxy(monkeypatch): req = MagicMock() req.client.host = "203.0.113.42" req.headers = {} - assert _get_client_ip(req) == "203.0.113.42" + assert await _get_client_ip(req) == "203.0.113.42" -def test_get_client_ip_x_real_ip_ignored_when_no_trusted_proxy(monkeypatch): +@pytest.mark.asyncio +async def test_get_client_ip_x_real_ip_ignored_when_no_trusted_proxy(monkeypatch): """X-Real-IP is silently ignored if AUTH_TRUSTED_PROXIES is unset. This closes the bypass where any client could rotate X-Real-IP per @@ -1819,10 +1821,11 @@ def test_get_client_ip_x_real_ip_ignored_when_no_trusted_proxy(monkeypatch): req = MagicMock() req.client.host = "127.0.0.1" req.headers = {"x-real-ip": "203.0.113.42"} - assert _get_client_ip(req) == "127.0.0.1" + assert await _get_client_ip(req) == "127.0.0.1" -def test_get_client_ip_x_real_ip_honored_from_trusted_proxy(monkeypatch): +@pytest.mark.asyncio +async def test_get_client_ip_x_real_ip_honored_from_trusted_proxy(monkeypatch): """X-Real-IP is honored when the TCP peer matches AUTH_TRUSTED_PROXIES.""" monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "10.0.0.0/8") from app.gateway.routers.auth import _get_client_ip @@ -1830,10 +1833,11 @@ def test_get_client_ip_x_real_ip_honored_from_trusted_proxy(monkeypatch): req = MagicMock() req.client.host = "10.5.6.7" # in trusted CIDR req.headers = {"x-real-ip": "203.0.113.42"} - assert _get_client_ip(req) == "203.0.113.42" + assert await _get_client_ip(req) == "203.0.113.42" -def test_get_client_ip_x_real_ip_rejected_from_untrusted_peer(monkeypatch): +@pytest.mark.asyncio +async def test_get_client_ip_x_real_ip_rejected_from_untrusted_peer(monkeypatch): """X-Real-IP is rejected when the TCP peer is NOT in the trusted list.""" monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "10.0.0.0/8") from app.gateway.routers.auth import _get_client_ip @@ -1841,10 +1845,11 @@ def test_get_client_ip_x_real_ip_rejected_from_untrusted_peer(monkeypatch): req = MagicMock() req.client.host = "8.8.8.8" # NOT in trusted CIDR req.headers = {"x-real-ip": "203.0.113.42"} # client trying to spoof - assert _get_client_ip(req) == "8.8.8.8" + assert await _get_client_ip(req) == "8.8.8.8" -def test_get_client_ip_xff_never_honored(monkeypatch): +@pytest.mark.asyncio +async def test_get_client_ip_xff_never_honored(monkeypatch): """X-Forwarded-For is never used; only X-Real-IP from a trusted peer.""" monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "10.0.0.0/8") from app.gateway.routers.auth import _get_client_ip @@ -1852,21 +1857,32 @@ def test_get_client_ip_xff_never_honored(monkeypatch): req = MagicMock() req.client.host = "10.0.0.1" req.headers = {"x-forwarded-for": "198.51.100.5"} # no x-real-ip - assert _get_client_ip(req) == "10.0.0.1" + assert await _get_client_ip(req) == "10.0.0.1" -def test_get_client_ip_invalid_trusted_proxy_entry_skipped(monkeypatch, caplog): - """Garbage entries in AUTH_TRUSTED_PROXIES are warned and skipped.""" - monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "not-an-ip,10.0.0.0/8") +@pytest.mark.asyncio +async def test_get_client_ip_invalid_trusted_proxy_entry_skipped(monkeypatch, caplog): + """Garbage entries in AUTH_TRUSTED_PROXIES are warned and skipped. + + ``10.0.0`` is neither an IP nor a hostname (numeric last label), so it is + not handed to the resolver, which would read it as ``10.0.0.0``. + """ + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "not an ip,10.0.0,10.0.0.0/8") from app.gateway.routers.auth import _get_client_ip req = MagicMock() req.client.host = "10.5.6.7" req.headers = {"x-real-ip": "203.0.113.42"} - assert _get_client_ip(req) == "203.0.113.42" # valid entry still works + with caplog.at_level("WARNING", logger="app.gateway.routers.auth"): + assert await _get_client_ip(req) == "203.0.113.42" # valid entry still works + assert [r.getMessage() for r in caplog.records] == [ + "AUTH_TRUSTED_PROXIES: ignoring invalid entry 'not an ip'", + "AUTH_TRUSTED_PROXIES: ignoring invalid entry '10.0.0'", + ] -def test_get_client_ip_no_client_returns_unknown(monkeypatch): +@pytest.mark.asyncio +async def test_get_client_ip_no_client_returns_unknown(monkeypatch): """No request.client → 'unknown' marker (no crash).""" monkeypatch.delenv("AUTH_TRUSTED_PROXIES", raising=False) from app.gateway.routers.auth import _get_client_ip @@ -1874,7 +1890,162 @@ def test_get_client_ip_no_client_returns_unknown(monkeypatch): req = MagicMock() req.client = None req.headers = {} - assert _get_client_ip(req) == "unknown" + assert await _get_client_ip(req) == "unknown" + + +@pytest.fixture +def _proxy_host_cache(): + from app.gateway.routers import auth as auth_router + + auth_router._trusted_proxy_host_cache.clear() + yield auth_router._trusted_proxy_host_cache + auth_router._trusted_proxy_host_cache.clear() + + +def _fake_getaddrinfo(monkeypatch, answers): + """Replace the resolver; ``answers`` is consumed one lookup at a time (an exception is raised).""" + import socket + + calls = [] + + def getaddrinfo(host, *args, **kwargs): + calls.append(host) + answer = answers[len(calls) - 1] + if isinstance(answer, BaseException): + raise answer + return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (address, 0)) for address in answer] + + monkeypatch.setattr(socket, "getaddrinfo", getaddrinfo) + return calls + + +def _proxied_request(peer: str, real_ip: str | None = "203.0.113.42"): + req = MagicMock() + req.client.host = peer + req.headers = {"x-real-ip": real_ip} if real_ip else {} + return req + + +@pytest.mark.asyncio +async def test_get_client_ip_hostname_entry_trusts_its_resolved_address(monkeypatch, _proxy_host_cache): + """A proxy without a fixed address (the compose ``nginx`` service) is named by hostname.""" + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "localhost") + from app.gateway.routers.auth import _get_client_ip + + assert await _get_client_ip(_proxied_request("127.0.0.1")) == "203.0.113.42" + assert await _get_client_ip(_proxied_request("10.9.9.9")) == "10.9.9.9" # not what it resolves to + + +@pytest.mark.asyncio +async def test_get_client_ip_unresolvable_hostname_trusts_nothing_and_is_cached(monkeypatch, caplog, _proxy_host_cache): + import socket + + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "nginx") + calls = _fake_getaddrinfo(monkeypatch, [socket.gaierror(socket.EAI_NONAME, "Name or service not known")]) + from app.gateway.routers.auth import _get_client_ip + + with caplog.at_level("WARNING", logger="app.gateway.routers.auth"): + assert await _get_client_ip(_proxied_request("172.18.0.5")) == "172.18.0.5" + assert await _get_client_ip(_proxied_request("172.18.0.5")) == "172.18.0.5" + # The failure is cached: an unauthenticated flood cannot drive one lookup per request. + assert calls == ["nginx"] + assert "could not resolve 'nginx'" in caplog.text + + +@pytest.mark.asyncio +async def test_get_client_ip_hostname_is_re_resolved_after_its_ttl(monkeypatch, _proxy_host_cache): + """A restarted proxy container comes back on a new address.""" + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "nginx") + calls = _fake_getaddrinfo(monkeypatch, [["172.18.0.5"], ["172.18.0.9"]]) + from app.gateway.routers.auth import _get_client_ip + + assert await _get_client_ip(_proxied_request("172.18.0.5")) == "203.0.113.42" + assert await _get_client_ip(_proxied_request("172.18.0.9")) == "172.18.0.9" # still cached + _, addresses = _proxy_host_cache["nginx"] + _proxy_host_cache["nginx"] = (0.0, addresses) # TTL elapsed + assert await _get_client_ip(_proxied_request("172.18.0.9")) == "203.0.113.42" + assert calls == ["nginx", "nginx"] + + +@pytest.mark.asyncio +async def test_get_client_ip_without_x_real_ip_never_resolves(monkeypatch, _proxy_host_cache): + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "nginx") + calls = _fake_getaddrinfo(monkeypatch, []) + from app.gateway.routers.auth import _get_client_ip + + assert await _get_client_ip(_proxied_request("172.18.0.5", real_ip=None)) == "172.18.0.5" + assert calls == [] + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("trusted_proxies", "victim_status"), [("localhost", 401), (None, 429)]) +async def test_login_lockout_is_per_client_behind_a_proxy_named_by_hostname(monkeypatch, memory_throttle_store, _proxy_host_cache, trusted_proxies, victim_status): + """Behind the bundled nginx every login arrives from the proxy's address. + + Unless the proxy is trusted, five wrong passwords from one client lock the + login out for every other client (the ``None`` case shows that shared + bucket); naming the proxy by hostname keys the lockout by ``X-Real-IP``. + """ + import httpx + + from app.gateway.routers import auth as auth_router + + class _WrongPassword: + async def authenticate(self, credentials): + return None + + monkeypatch.setattr(auth_router, "get_local_provider", lambda: _WrongPassword()) + if trusted_proxies is None: + monkeypatch.delenv("AUTH_TRUSTED_PROXIES", raising=False) + else: + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", trusted_proxies) + app = FastAPI() + app.include_router(auth_router.router) + + async def login(client_ip: str) -> int: + transport = httpx.ASGITransport(app=app, client=("127.0.0.1", 50000)) # the proxy's socket + async with httpx.AsyncClient(transport=transport, base_url="http://gateway") as client: + response = await client.post("/api/v1/auth/login/local", data={"username": "user@example.com", "password": "wrong"}, headers={"X-Real-IP": client_ip}) + return response.status_code + + assert [await login("198.51.100.7") for _ in range(5)] == [401] * 5 + assert await login("203.0.113.42") == victim_status + assert await login("198.51.100.7") == 429 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("forwarded_allow_ips", ["127.0.0.1", "172.18.0.5"], ids=["uvicorn-default", "aligned-with-nginx"]) +async def test_login_lockout_key_through_uvicorn_proxy_headers(monkeypatch, memory_throttle_store, _proxy_host_cache, forwarded_allow_ips): + """Uvicorn's ProxyHeadersMiddleware runs before the app and may rewrite the peer. + + With its default (only 127.0.0.1 trusted) the compose Gateway still sees the + nginx container as the peer and takes X-Real-IP; with FORWARDED_ALLOW_IPS + aligned to nginx, Uvicorn already replaced the peer with the client from + X-Forwarded-For. Either way the lockout is keyed by the browser's address. + """ + import httpx + from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware + + from app.gateway.routers import auth as auth_router + + class _WrongPassword: + async def authenticate(self, credentials): + return None + + monkeypatch.setattr(auth_router, "get_local_provider", lambda: _WrongPassword()) + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "nginx") + _fake_getaddrinfo(monkeypatch, [["172.18.0.5"]]) + app = FastAPI() + app.include_router(auth_router.router) + transport = httpx.ASGITransport(app=ProxyHeadersMiddleware(app, trusted_hosts=forwarded_allow_ips), client=("172.18.0.5", 50000)) + # What docker/nginx/nginx.conf sends for a browser at 198.51.100.7 that forged both headers. + headers = {"X-Real-IP": "198.51.100.7", "X-Forwarded-For": "203.0.113.99, 198.51.100.7"} + async with httpx.AsyncClient(transport=transport, base_url="http://gateway") as client: + response = await client.post("/api/v1/auth/login/local", data={"username": "user@example.com", "password": "wrong"}, headers=headers) + assert response.status_code == 401 + assert (await memory_throttle_store.get("198.51.100.7")).fail_count == 1 + assert await memory_throttle_store.get("172.18.0.5") is None # never nginx's address + assert await memory_throttle_store.get("203.0.113.99") is None # never the forged entry # ── Common-password blocklist ──────────────────────────────────────────────── diff --git a/backend/tests/test_compose_auth_trusted_proxies.py b/backend/tests/test_compose_auth_trusted_proxies.py new file mode 100644 index 00000000000..09fdb04be42 --- /dev/null +++ b/backend/tests/test_compose_auth_trusted_proxies.py @@ -0,0 +1,105 @@ +"""The compose Gateway must key the login lockout by client, not by nginx. + +``auth.py`` counts failed logins per client IP and honors ``X-Real-IP`` only +from a peer listed in ``AUTH_TRUSTED_PROXIES``. In the compose stack every +browser request reaches the Gateway from the ``nginx`` container, whose address +Docker assigns, so with the variable unset all logins shared nginx's address +and five wrong passwords from anyone locked out every user. Both compose files +name the bundled proxy by its service name; that trust is safe only while +nginx overwrites ``X-Real-IP`` on every route to the Gateway, pinned below for +the three maintained nginx configs. +""" + +from __future__ import annotations + +import json +import os +import re +import shutil +import subprocess +from pathlib import Path + +import pytest +import yaml +from support.compose import DOCKER, requires_docker_compose +from support.nginx_conf import NGINX_CONFIGS, read_config + +REPO_ROOT = Path(__file__).resolve().parents[2] +COMPOSE_PATHS = { + "prod": REPO_ROOT / "docker" / "docker-compose.yaml", + "dev": REPO_ROOT / "docker" / "docker-compose-dev.yaml", +} +EXPECTED_ENTRY = "AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-nginx}" + + +@pytest.mark.parametrize("variant", sorted(COMPOSE_PATHS)) +def test_gateway_trusts_the_bundled_nginx_service_by_default(variant: str): + compose = yaml.safe_load(COMPOSE_PATHS[variant].read_text(encoding="utf-8")) + services = compose["services"] + + assert EXPECTED_ENTRY in services["gateway"]["environment"] + # The default resolves only while the proxy is the service named ``nginx`` + # on a network the Gateway shares. + assert set(services["nginx"]["networks"]) & set(services["gateway"]["networks"]) + + +def _render(tmp_path: Path, variant: str, env_file: str | None) -> dict: + """Render the compose file the way deploy.sh / docker.sh call it (``--env-file ../.env``).""" + docker_dir = tmp_path / "docker" + shutil.copytree(REPO_ROOT / "docker", docker_dir) + (tmp_path / "frontend").mkdir(exist_ok=True) + env = {key: value for key, value in os.environ.items() if not key.startswith(("DEER_FLOW_", "COMPOSE_", "AUTH_"))} + env.update( + DEER_FLOW_ROOT=str(tmp_path), + DEER_FLOW_HOME=str(tmp_path / "home"), + DEER_FLOW_REPO_ROOT=str(tmp_path), + DEER_FLOW_CONFIG_PATH=str(tmp_path / "config.yaml"), + DEER_FLOW_EXTENSIONS_CONFIG_PATH=str(tmp_path / "extensions_config.json"), + BETTER_AUTH_SECRET="test-secret", + DEER_FLOW_INTERNAL_AUTH_TOKEN="test-token", + ) + command = [DOCKER, "compose"] + if env_file is not None: + (tmp_path / ".env").write_text(env_file, encoding="utf-8") + command += ["--env-file", str(tmp_path / ".env")] + command += ["-p", "deer-flow-trusted-proxies-test", "-f", str(docker_dir / COMPOSE_PATHS[variant].name), "config", "--format", "json"] + result = subprocess.run(command, env=env, capture_output=True, text=True, timeout=120, check=False) + assert result.returncode == 0, result.stderr + return json.loads(result.stdout)["services"]["gateway"]["environment"] + + +@requires_docker_compose +@pytest.mark.parametrize("variant", sorted(COMPOSE_PATHS)) +@pytest.mark.parametrize(("env_file", "expected"), [(None, "nginx"), ("AUTH_TRUSTED_PROXIES=10.0.0.0/8,edge-proxy\n", "10.0.0.0/8,edge-proxy")], ids=["default", "operator-override"]) +def test_real_compose_renders_the_default_and_keeps_an_operator_override(tmp_path, variant: str, env_file: str | None, expected: str): + assert _render(tmp_path, variant, env_file)["AUTH_TRUSTED_PROXIES"] == expected + + +def _gateway_locations(content: str) -> list[tuple[str, str]]: + """``(selector, block)`` for every location whose ``proxy_pass`` targets the Gateway.""" + locations = [] + for match in re.finditer(r"^\s*location\s+([^{#\n]+?)\s*\{", content, re.MULTILINE): + start = match.end() - 1 + depth = 0 + for index in range(start, len(content)): + if content[index] == "{": + depth += 1 + elif content[index] == "}": + depth -= 1 + if depth == 0: + block = content[start : index + 1] + break + proxy_pass = re.search(r"proxy_pass\s+([^;]+);", block) + if proxy_pass and "gateway" in proxy_pass.group(1): + locations.append((match.group(1), block)) + return locations + + +@pytest.mark.parametrize("path", NGINX_CONFIGS) +def test_every_gateway_location_overwrites_x_real_ip(path: str): + """A location that forwarded the client's own X-Real-IP would let anyone pick their lockout bucket.""" + locations = _gateway_locations(read_config(path)) + + assert len(locations) >= 10, f"{path}: found only {len(locations)} gateway locations; did the parser break?" + missing = [selector for selector, block in locations if not re.search(r"proxy_set_header\s+X-Real-IP\s+\$remote_addr\s*;", block)] + assert missing == [], f"{path}: gateway locations that pass the client's X-Real-IP through: {missing}" diff --git a/config.example.yaml b/config.example.yaml index 48ee1ad76ea..265c454ca7c 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -3380,7 +3380,9 @@ authorization: # auth: # local: # allow_registration: false -# # Login throttling per client IP. Raise max_login_attempts when many +# # Login throttling per client IP. The client IP is X-Real-IP from a +# # proxy listed in AUTH_TRUSTED_PROXIES (the Docker stack trusts its +# # bundled nginx), else the TCP peer. Raise max_login_attempts when many # # users share one egress IP (corporate proxy / NAT) so shared-IP # # lockouts don't block the whole office; lower it for a stricter # # posture (minimum 2 — one failed attempt must never lock an IP, or a diff --git a/docker/docker-compose-dev.yaml b/docker/docker-compose-dev.yaml index 7bf4e0590ab..7ff39254015 100644 --- a/docker/docker-compose-dev.yaml +++ b/docker/docker-compose-dev.yaml @@ -241,6 +241,11 @@ services: - DEER_FLOW_INTERNAL_AUTH_TOKEN=${DEER_FLOW_INTERNAL_AUTH_TOKEN:-} - DEER_FLOW_HOST_BASE_DIR=${DEER_FLOW_ROOT}/backend/.deer-flow - DEER_FLOW_SANDBOX_HOST=host.docker.internal + # The login lockout counts failures per client IP, but every browser + # request reaches the Gateway from nginx. Trust nginx's X-Real-IP (it + # overwrites the header on every Gateway route) so one client's wrong + # passwords cannot lock everyone out. Hostname, re-resolved every 10s. + - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-nginx} # Pass PROVISIONER_API_KEY into the gateway container so config.yaml can reference it # as sandbox.provisioner_api_key: $PROVISIONER_API_KEY - PROVISIONER_API_KEY=${PROVISIONER_API_KEY:-} diff --git a/docker/docker-compose.yaml b/docker/docker-compose.yaml index 6a0d4e723f3..9592d35ce40 100644 --- a/docker/docker-compose.yaml +++ b/docker/docker-compose.yaml @@ -168,6 +168,11 @@ services: # DooD path/network translation - DEER_FLOW_HOST_BASE_DIR=${DEER_FLOW_HOME} - DEER_FLOW_SANDBOX_HOST=host.docker.internal + # The login lockout counts failures per client IP, but every browser + # request reaches the Gateway from nginx. Trust nginx's X-Real-IP (it + # overwrites the header on every Gateway route) so one client's wrong + # passwords cannot lock everyone out. Hostname, re-resolved every 10s. + - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-nginx} # Proxy values (HTTP_PROXY/HTTPS_PROXY/ALL_PROXY) are inherited from ../.env via env_file. # Only NO_PROXY is declared here so internal service hostnames are always exempt from the proxy. - NO_PROXY=${NO_PROXY:-}${NO_PROXY:+,}localhost,127.0.0.1,::1,gateway,frontend,nginx,provisioner,openviking,host.docker.internal From e8ac7bfe313fb4bbf93ac5e30136b33e2defa855 Mon Sep 17 00:00:00 2001 From: hyeonsang010716 Date: Thu, 8 Oct 2026 23:04:39 +0900 Subject: [PATCH 2/4] docs(changelog): reference #6519 --- CHANGELOG.md | 3 ++- CHANGELOG_zh.md | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61eadf41204..498ff494d50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -815,7 +815,7 @@ This release closes that milestone with **439 merged pull requests**. nginx overwrites `X-Real-IP` with `$remote_addr` on every Gateway route, so a client cannot choose its own address. An `AUTH_TRUSTED_PROXIES` value in `.env` still takes precedence. Deployments behind another reverse proxy also - need nginx's `real_ip` module for that proxy; see `.env.example`. + need nginx's `real_ip` module for that proxy; see `.env.example`. ([#6519]) - **auth:** Login lockouts are now counted once per client IP across every Gateway replica. `POST /api/v1/auth/login/local` kept its failed-login counter in a per-process dict, so with N replicas behind one load balancer an @@ -9296,3 +9296,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#6495]: https://github.com/bytedance/deer-flow/pull/6495 [#6501]: https://github.com/bytedance/deer-flow/pull/6501 [#6506]: https://github.com/bytedance/deer-flow/pull/6506 +[#6519]: https://github.com/bytedance/deer-flow/pull/6519 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 59d6828a0cd..31e15872aac 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -699,7 +699,7 @@ 容器到达 Gateway，因此所有登录共用 nginx 的地址和同一个锁定。现在 `AUTH_TRUSTED_PROXIES` 也接受主机名，在事件循环外解析并 缓存 10 秒（包括解析失败），两个 compose 文件默认将其设为内置的 `nginx` 服务。nginx 在每个转发到 Gateway 的路由上都用 `$remote_addr` 覆盖 `X-Real-IP`，客户端无法自选地址。`.env` 中设置的 `AUTH_TRUSTED_PROXIES` 仍然优先。位于其他反向代理之后的 - 部署还需要为该代理配置 nginx 的 `real_ip` 模块，详见 `.env.example`。 + 部署还需要为该代理配置 nginx 的 `real_ip` 模块，详见 `.env.example`。([#6519]) - **技能：** `/技能名` 激活现在在模型调用重试时不再丢失。此前激活在调用模型之前就被标记为已完成，因此调用失败 （限流、过载、超时）或返回空响应而重试时，重试请求不再包含 `SKILL.md` 正文，而该技能的工具限制仍然生效。现在重试 会携带与首次尝试相同的激活内容，不会重新读取技能或重复记录激活，重试得到的响应也保留技能使用记录。([#6506]) @@ -7543,3 +7543,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#6481]: https://github.com/bytedance/deer-flow/pull/6481 [#6484]: https://github.com/bytedance/deer-flow/pull/6484 [#6506]: https://github.com/bytedance/deer-flow/pull/6506 +[#6519]: https://github.com/bytedance/deer-flow/pull/6519 From b29f4492c4c88e056efc8ac0d7dd860cd32f2b56 Mon Sep 17 00:00:00 2001 From: hyeonsang010716 Date: Thu, 8 Oct 2026 23:54:02 +0900 Subject: [PATCH 3/4] fix(auth): share one trusted-proxy lookup across concurrent cache misses A burst of setup-status / login requests on an expired hostname entry started one getaddrinfo per request while DNS was slow, occupying the default executor that password verification also uses. The cache check and an in-flight task registration now happen without a yield between them; later callers join that task through asyncio.shield, so a cancelled request cannot cancel the lookup the others joined, and the task caches its own result. The compose default keeps `:-` on purpose: every browser request arrives through nginx, so an empty AUTH_TRUSTED_PROXIES would only bring back the shared lockout. The compose comment and .env.example say so, and the rendered-compose test now covers the explicitly empty value. --- .env.example | 4 +- backend/app/gateway/routers/auth.py | 42 ++++++++++--- backend/tests/test_auth.py | 61 +++++++++++++++++++ .../test_compose_auth_trusted_proxies.py | 8 ++- docker/docker-compose-dev.yaml | 3 + docker/docker-compose.yaml | 3 + 6 files changed, 110 insertions(+), 11 deletions(-) diff --git a/.env.example b/.env.example index c83741420da..e9dd6fa4b21 100644 --- a/.env.example +++ b/.env.example @@ -41,7 +41,9 @@ UNBROWSE_API_KEY=your-unbrowse-api-key # it every browser login shares nginx's address, so five wrong passwords from # anyone lock everyone out. Behind your own front door, nginx sees that proxy's # address instead: configure nginx's real_ip module for it as well. -# Unset outside Docker (direct mode): X-Real-IP is ignored. +# Outside Docker, unset or empty is direct mode: X-Real-IP is ignored. In the +# Docker stack an empty value also falls back to `nginx`, since every browser +# request arrives through it. # AUTH_TRUSTED_PROXIES=nginx # Optional: diff --git a/backend/app/gateway/routers/auth.py b/backend/app/gateway/routers/auth.py index f868835a448..f8cca960ac4 100644 --- a/backend/app/gateway/routers/auth.py +++ b/backend/app/gateway/routers/auth.py @@ -231,6 +231,10 @@ def _login_throttle_policy() -> tuple[int, float]: # hostname -> (monotonic expiry, resolved addresses). Failed lookups are cached # as an empty set too, so unauthenticated requests cannot drive a lookup storm. _trusted_proxy_host_cache: dict[str, tuple[float, frozenset]] = {} +# hostname -> the lookup in progress. Concurrent cache misses (a burst of +# setup-status / login requests when the entry expires) join it, so a refresh +# costs one resolver call however many requests arrive while DNS is slow. +_trusted_proxy_host_inflight: dict[str, asyncio.Task[frozenset]] = {} def _is_hostname(entry: str) -> bool: @@ -269,24 +273,44 @@ def _trusted_proxies() -> tuple[list, list[str]]: return nets, hostnames +async def _resolve_trusted_proxy_host(hostname: str) -> frozenset: + """Resolve ``hostname`` in the loop's executor and cache the result (or the failure).""" + try: + infos = await asyncio.get_running_loop().getaddrinfo(hostname, None, type=socket.SOCK_STREAM) + addresses = frozenset(ip_address(info[4][0]) for info in infos) + except (OSError, ValueError) as exc: + logger.warning("AUTH_TRUSTED_PROXIES: could not resolve %r (%s); not trusting it for %.0fs", hostname, exc, _TRUSTED_PROXY_HOST_TTL_SECONDS) + addresses = frozenset() + _trusted_proxy_host_cache[hostname] = (time.monotonic() + _TRUSTED_PROXY_HOST_TTL_SECONDS, addresses) + return addresses + + async def _trusted_proxy_host_addresses(hostname: str) -> frozenset: """Addresses ``hostname`` resolves to, cached for ``_TRUSTED_PROXY_HOST_TTL_SECONDS``. The lookup runs in the loop's executor (``getaddrinfo`` blocks). A failed lookup trusts nothing until the entry expires, which is the direct-mode - behavior, not an error for the request. + behavior, not an error for the request. One lookup per hostname runs at a + time: the cache check and the in-flight registration happen without a + yield in between, and later callers join that task. Callers wait through + ``asyncio.shield``, so one cancelled request (a client disconnect) cannot + cancel the lookup the others joined; the task caches its own result, so + it is kept even if every caller went away. """ cached = _trusted_proxy_host_cache.get(hostname) if cached is not None and time.monotonic() < cached[0]: return cached[1] - try: - infos = await asyncio.get_running_loop().getaddrinfo(hostname, None, type=socket.SOCK_STREAM) - addresses = frozenset(ip_address(info[4][0]) for info in infos) - except (OSError, ValueError) as exc: - logger.warning("AUTH_TRUSTED_PROXIES: could not resolve %r (%s); not trusting it for %.0fs", hostname, exc, _TRUSTED_PROXY_HOST_TTL_SECONDS) - addresses = frozenset() - _trusted_proxy_host_cache[hostname] = (time.monotonic() + _TRUSTED_PROXY_HOST_TTL_SECONDS, addresses) - return addresses + task = _trusted_proxy_host_inflight.get(hostname) + if task is None: + task = asyncio.get_running_loop().create_task(_resolve_trusted_proxy_host(hostname)) + _trusted_proxy_host_inflight[hostname] = task + + def _release(done: asyncio.Task[frozenset]) -> None: + if _trusted_proxy_host_inflight.get(hostname) is done: + del _trusted_proxy_host_inflight[hostname] + + task.add_done_callback(_release) + return await asyncio.shield(task) async def _is_trusted_proxy(peer_ip) -> bool: diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index df638839f7c..e88cb3bf5e0 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -1898,8 +1898,10 @@ def _proxy_host_cache(): from app.gateway.routers import auth as auth_router auth_router._trusted_proxy_host_cache.clear() + auth_router._trusted_proxy_host_inflight.clear() yield auth_router._trusted_proxy_host_cache auth_router._trusted_proxy_host_cache.clear() + auth_router._trusted_proxy_host_inflight.clear() def _fake_getaddrinfo(monkeypatch, answers): @@ -1967,6 +1969,65 @@ async def test_get_client_ip_hostname_is_re_resolved_after_its_ttl(monkeypatch, assert calls == ["nginx", "nginx"] +def _gated_getaddrinfo(monkeypatch, address: str): + """A resolver that blocks in its executor thread until released, like DNS timing out.""" + import socket + import threading + + calls = [] + entered, release = threading.Event(), threading.Event() + + def getaddrinfo(host, *args, **kwargs): + calls.append(host) + entered.set() + assert release.wait(5), "the gated resolver was never released" + return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (address, 0))] + + monkeypatch.setattr(socket, "getaddrinfo", getaddrinfo) + return calls, entered, release + + +@pytest.mark.asyncio +async def test_concurrent_cache_misses_share_one_lookup(monkeypatch, _proxy_host_cache): + """A burst on an expired entry while DNS is slow costs one resolver call, not one per request.""" + import asyncio + + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "nginx") + calls, entered, release = _gated_getaddrinfo(monkeypatch, "172.18.0.5") + from app.gateway.routers.auth import _get_client_ip + + burst = [asyncio.create_task(_get_client_ip(_proxied_request("172.18.0.5"))) for _ in range(20)] + assert await asyncio.to_thread(entered.wait, 5) + release.set() + + assert await asyncio.gather(*burst) == ["203.0.113.42"] * 20 + assert calls == ["nginx"] + + +@pytest.mark.asyncio +async def test_cancelled_request_does_not_cancel_the_shared_lookup(monkeypatch, _proxy_host_cache): + """The request that started the lookup disconnecting must not fail the requests that joined it.""" + import asyncio + from ipaddress import ip_address + + monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "nginx") + calls, entered, release = _gated_getaddrinfo(monkeypatch, "172.18.0.5") + from app.gateway.routers.auth import _get_client_ip + + owner = asyncio.create_task(_get_client_ip(_proxied_request("172.18.0.5"))) + assert await asyncio.to_thread(entered.wait, 5) + joiner = asyncio.create_task(_get_client_ip(_proxied_request("172.18.0.5"))) + await asyncio.sleep(0) # the joiner is now waiting on the same lookup + owner.cancel() + with pytest.raises(asyncio.CancelledError): + await owner + release.set() + + assert await joiner == "203.0.113.42" + assert calls == ["nginx"] + assert _proxy_host_cache["nginx"][1] == frozenset({ip_address("172.18.0.5")}) + + @pytest.mark.asyncio async def test_get_client_ip_without_x_real_ip_never_resolves(monkeypatch, _proxy_host_cache): monkeypatch.setenv("AUTH_TRUSTED_PROXIES", "nginx") diff --git a/backend/tests/test_compose_auth_trusted_proxies.py b/backend/tests/test_compose_auth_trusted_proxies.py index 09fdb04be42..ba48b43d870 100644 --- a/backend/tests/test_compose_auth_trusted_proxies.py +++ b/backend/tests/test_compose_auth_trusted_proxies.py @@ -70,7 +70,13 @@ def _render(tmp_path: Path, variant: str, env_file: str | None) -> dict: @requires_docker_compose @pytest.mark.parametrize("variant", sorted(COMPOSE_PATHS)) -@pytest.mark.parametrize(("env_file", "expected"), [(None, "nginx"), ("AUTH_TRUSTED_PROXIES=10.0.0.0/8,edge-proxy\n", "10.0.0.0/8,edge-proxy")], ids=["default", "operator-override"]) +@pytest.mark.parametrize( + ("env_file", "expected"), + [(None, "nginx"), ("AUTH_TRUSTED_PROXIES=\n", "nginx"), ("AUTH_TRUSTED_PROXIES=10.0.0.0/8,edge-proxy\n", "10.0.0.0/8,edge-proxy")], + # An explicitly empty value is not direct mode here: every browser request + # comes through nginx, so it would only bring back the shared lockout. + ids=["default", "explicitly-empty", "operator-override"], +) def test_real_compose_renders_the_default_and_keeps_an_operator_override(tmp_path, variant: str, env_file: str | None, expected: str): assert _render(tmp_path, variant, env_file)["AUTH_TRUSTED_PROXIES"] == expected diff --git a/docker/docker-compose-dev.yaml b/docker/docker-compose-dev.yaml index 7ff39254015..a9e27f2db39 100644 --- a/docker/docker-compose-dev.yaml +++ b/docker/docker-compose-dev.yaml @@ -245,6 +245,9 @@ services: # request reaches the Gateway from nginx. Trust nginx's X-Real-IP (it # overwrites the header on every Gateway route) so one client's wrong # passwords cannot lock everyone out. Hostname, re-resolved every 10s. + # `:-` on purpose: an empty value also means nginx, because every browser + # request arrives through it, so direct mode would only bring back the + # shared lockout. Set another list in .env to change whom to trust. - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-nginx} # Pass PROVISIONER_API_KEY into the gateway container so config.yaml can reference it # as sandbox.provisioner_api_key: $PROVISIONER_API_KEY diff --git a/docker/docker-compose.yaml b/docker/docker-compose.yaml index 9592d35ce40..2ef39a7e439 100644 --- a/docker/docker-compose.yaml +++ b/docker/docker-compose.yaml @@ -172,6 +172,9 @@ services: # request reaches the Gateway from nginx. Trust nginx's X-Real-IP (it # overwrites the header on every Gateway route) so one client's wrong # passwords cannot lock everyone out. Hostname, re-resolved every 10s. + # `:-` on purpose: an empty value also means nginx, because every browser + # request arrives through it, so direct mode would only bring back the + # shared lockout. Set another list in .env to change whom to trust. - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-nginx} # Proxy values (HTTP_PROXY/HTTPS_PROXY/ALL_PROXY) are inherited from ../.env via env_file. # Only NO_PROXY is declared here so internal service hostnames are always exempt from the proxy. From 830302f346301ed700f306a471905acff215d822 Mon Sep 17 00:00:00 2001 From: hyeonsang010716 Date: Fri, 9 Oct 2026 09:52:33 +0900 Subject: [PATCH 4/4] fix(docker): keep a .env AUTH_TRUSTED_PROXIES under make docker-start docker.sh start runs Compose from docker/ without --env-file, so the dev compose file's ${AUTH_TRUSTED_PROXIES:-nginx} interpolated from the shell only, and the resolved environment entry outranked the operator's value loaded through env_file. Export it from .env alongside the proxy variables (a shell export still wins), and strip a CRLF before the quotes. --- CHANGELOG.md | 6 +- CHANGELOG_zh.md | 2 +- .../test_compose_auth_trusted_proxies.py | 60 ++++++++++++++++++- scripts/AGENTS.md | 7 +++ scripts/docker.sh | 10 +++- 5 files changed, 79 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6345659fb3..c851ec1af0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -814,8 +814,10 @@ This release closes that milestone with **439 merged pull requests**. included), and both compose files default it to the bundled `nginx` service. nginx overwrites `X-Real-IP` with `$remote_addr` on every Gateway route, so a client cannot choose its own address. An `AUTH_TRUSTED_PROXIES` value in - `.env` still takes precedence. Deployments behind another reverse proxy also - need nginx's `real_ip` module for that proxy; see `.env.example`. ([#6519]) + `.env` still takes precedence, including under `make docker-start`, which now + exports it for Compose interpolation like the proxy variables. Deployments + behind another reverse proxy also need nginx's `real_ip` module for that + proxy; see `.env.example`. ([#6519]) - **scheduler:** "Run once now" on a one-time task before its run time no longer cancels the scheduled run. The trial launched as the task's own run: the task was marked `running`, and the trial's outcome then finished it (`completed`, diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 334e39e6bd2..7552d70b612 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -698,7 +698,7 @@ 只在 TCP peer 属于 `AUTH_TRUSTED_PROXIES` 时信任 `X-Real-IP`，compose 文件却从未设置该变量；所有浏览器请求都经由 `nginx` 容器到达 Gateway，因此所有登录共用 nginx 的地址和同一个锁定。现在 `AUTH_TRUSTED_PROXIES` 也接受主机名，在事件循环外解析并 缓存 10 秒（包括解析失败），两个 compose 文件默认将其设为内置的 `nginx` 服务。nginx 在每个转发到 Gateway 的路由上都用 - `$remote_addr` 覆盖 `X-Real-IP`，客户端无法自选地址。`.env` 中设置的 `AUTH_TRUSTED_PROXIES` 仍然优先。位于其他反向代理之后的 + `$remote_addr` 覆盖 `X-Real-IP`，客户端无法自选地址。`.env` 中设置的 `AUTH_TRUSTED_PROXIES` 仍然优先，`make docker-start` 也是如此：它现在会像代理变量一样导出该值供 Compose 插值。位于其他反向代理之后的 部署还需要为该代理配置 nginx 的 `real_ip` 模块，详见 `.env.example`。([#6519]) - **调度器：** 在一次性任务的执行时间之前点击“立即运行一次”，不会再取消原定的执行。此前这次试运行被当作任务本身的 执行：任务被标记为 `running`，试运行结束后又按其结果把任务终结为 `completed`、`failed` 或 `cancelled`。轮询器只认领 diff --git a/backend/tests/test_compose_auth_trusted_proxies.py b/backend/tests/test_compose_auth_trusted_proxies.py index ba48b43d870..309d056ea89 100644 --- a/backend/tests/test_compose_auth_trusted_proxies.py +++ b/backend/tests/test_compose_auth_trusted_proxies.py @@ -23,6 +23,7 @@ import yaml from support.compose import DOCKER, requires_docker_compose from support.nginx_conf import NGINX_CONFIGS, read_config +from support.shell import find_script_bash REPO_ROOT = Path(__file__).resolve().parents[2] COMPOSE_PATHS = { @@ -30,6 +31,7 @@ "dev": REPO_ROOT / "docker" / "docker-compose-dev.yaml", } EXPECTED_ENTRY = "AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-nginx}" +BASH_EXECUTABLE = find_script_bash() @pytest.mark.parametrize("variant", sorted(COMPOSE_PATHS)) @@ -44,7 +46,7 @@ def test_gateway_trusts_the_bundled_nginx_service_by_default(variant: str): def _render(tmp_path: Path, variant: str, env_file: str | None) -> dict: - """Render the compose file the way deploy.sh / docker.sh call it (``--env-file ../.env``).""" + """Render the compose file the way deploy.sh calls it (``--env-file ../.env``).""" docker_dir = tmp_path / "docker" shutil.copytree(REPO_ROOT / "docker", docker_dir) (tmp_path / "frontend").mkdir(exist_ok=True) @@ -81,6 +83,62 @@ def test_real_compose_renders_the_default_and_keeps_an_operator_override(tmp_pat assert _render(tmp_path, variant, env_file)["AUTH_TRUSTED_PROXIES"] == expected +def _render_through_docker_start(tmp_path: Path, env_file: str, shell_value: str | None = None) -> dict: + """Render the dev compose file with the exact command, cwd and environment ``make docker-start`` uses. + + ``scripts/docker.sh start`` runs Compose from ``docker/`` without + ``--env-file``, so interpolation never reads the checkout ``.env`` by itself. + A ``docker`` shell function stands in for the CLI and swaps ``up ...`` for + ``config`` while keeping every argument before it. + """ + docker_dir = tmp_path / "docker" + shutil.copytree(REPO_ROOT / "docker", docker_dir) + (tmp_path / "frontend").mkdir() + (tmp_path / "frontend" / ".env").write_text("", encoding="utf-8") + (tmp_path / ".env").write_text(env_file, encoding="utf-8") + (tmp_path / "config.yaml").write_text("sandbox:\n use: deerflow.sandbox.local:LocalSandboxProvider\n", encoding="utf-8") + (tmp_path / "extensions_config.json").write_text("{}", encoding="utf-8") + rendered = tmp_path / "rendered.json" + env = {key: value for key, value in os.environ.items() if not key.startswith(("DEER_FLOW_", "COMPOSE_", "AUTH_"))} + if shell_value is not None: + env["AUTH_TRUSTED_PROXIES"] = shell_value + script = f""" +source '{REPO_ROOT / "scripts" / "docker.sh"}' +PROJECT_ROOT='{tmp_path}' +DOCKER_DIR='{docker_dir}' +require_compose_version() {{ :; }} +docker() {{ + local args=() + for arg in "$@"; do + [ "$arg" = up ] && break + args+=("$arg") + done + command '{DOCKER}' "${{args[@]}}" config --format json > '{rendered}' +}} +start +""" + subprocess.run([BASH_EXECUTABLE, "-c", script], env=env, capture_output=True, text=True, timeout=120, check=True) + return json.loads(rendered.read_text(encoding="utf-8"))["services"]["gateway"]["environment"] + + +@requires_docker_compose +@pytest.mark.skipif(BASH_EXECUTABLE is None, reason="bash is required to run scripts/docker.sh") +@pytest.mark.parametrize( + ("env_file", "shell_value", "expected"), + [ + ("", None, "nginx"), + ("AUTH_TRUSTED_PROXIES=\n", None, "nginx"), + ("AUTH_TRUSTED_PROXIES=10.0.0.0/8,edge-proxy\n", None, "10.0.0.0/8,edge-proxy"), + ("AUTH_TRUSTED_PROXIES='10.0.0.0/8'\r\n", None, "10.0.0.0/8"), + ("AUTH_TRUSTED_PROXIES=10.0.0.0/8\n", "192.0.2.1", "192.0.2.1"), + ], + ids=["default", "explicitly-empty", "dotenv-override", "quoted-crlf", "shell-export-wins"], +) +def test_make_docker_start_keeps_a_dotenv_override(tmp_path, env_file: str, shell_value: str | None, expected: str): + """The dev launcher must hand the checkout .env value to interpolation, or the default replaces it.""" + assert _render_through_docker_start(tmp_path, env_file, shell_value)["AUTH_TRUSTED_PROXIES"] == expected + + def _gateway_locations(content: str) -> list[tuple[str, str]]: """``(selector, block)`` for every location whose ``proxy_pass`` targets the Gateway.""" locations = [] diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 82d80e6560e..4c3851a28d8 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -69,6 +69,13 @@ keys the production Gateway would receive, and `make config` skips files that exist. Pinned by `backend/tests/test_compose_default_bind_host.py` and `backend/tests/test_gateway_startup.py`. +`docker.sh start` runs Compose from `docker/` without `--env-file`, so +dev-compose interpolation sees only the shell. `load_proxy_env_from_dotenv` +exports the `.env` keys interpolation needs (proxy variables and +`AUTH_TRUSTED_PROXIES`, whose `environment:` default would otherwise replace +the `env_file` value); shell exports still win. Pinned by +`backend/tests/test_compose_auth_trusted_proxies.py`. + `deploy.sh` never sources the repo-root `.env`; Compose reads it via `--env-file`, and shell exports outrank that file during interpolation (an exported-but-empty variable still wins). So `BETTER_AUTH_SECRET` and diff --git a/scripts/docker.sh b/scripts/docker.sh index ff669b8c49e..09a892265f3 100755 --- a/scripts/docker.sh +++ b/scripts/docker.sh @@ -147,6 +147,12 @@ ensure_env_files() { ensure_from_example "$PROJECT_ROOT/frontend/.env" "$PROJECT_ROOT/frontend/.env.example" "frontend/.env" } +# start runs Compose from docker/ without --env-file, so ${VAR} interpolation in +# docker-compose-dev.yaml sees the shell only, never the checkout .env. Export +# the .env values that interpolation consumes; a value already set in the shell +# (even empty) wins, as it does for Compose. AUTH_TRUSTED_PROXIES belongs here +# because the Gateway's `environment:` entry, which defaults to nginx, outranks +# the same key loaded through env_file and would replace the operator's list. load_proxy_env_from_dotenv() { local env_file="$PROJECT_ROOT/.env" local var @@ -157,16 +163,16 @@ load_proxy_env_from_dotenv() { return fi - for var in HTTP_PROXY HTTPS_PROXY ALL_PROXY NO_PROXY http_proxy https_proxy all_proxy no_proxy; do + for var in HTTP_PROXY HTTPS_PROXY ALL_PROXY NO_PROXY http_proxy https_proxy all_proxy no_proxy AUTH_TRUSTED_PROXIES; do if [ -z "${!var+x}" ]; then line="$(grep -E "^[[:space:]]*${var}=" "$env_file" | tail -n 1 || true)" if [ -n "$line" ]; then value="${line#*=}" + value="${value%$'\r'}" value="${value%\"}" value="${value#\"}" value="${value%\'}" value="${value#\'}" - value="${value%$'\r'}" export "${var}=${value}" fi fi