From cc7229a2fec9eee8546be98a55f588825d4f84e2 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Tue, 13 Oct 2026 07:38:51 +0000 Subject: [PATCH 1/7] fix(rpc): sign in a page reached through a port forward The gateway accepted a browser Origin only when it named the port the gateway bound, so any port forward that picked another local port was refused: every /auth/exchange answered 403 and every /rpc upgrade with it. The page reported an expired sign-in link over a refusal it cannot see, and re-running `raven web` minted one more link into the same refusal, 13 times on 2026-10-13. The same check already forced the container's nginx to rewrite Origin for every request it proxies. Origin now also passes when it asks the request's own origin on a loopback name. That is what a browser sends through a forward, since the forwarder relays Host unchanged, and it still refuses a page another local server shows (cookies and SameSite ignore the port) and DNS rebinding (the name is not loopback). The bound-port allowlist stays for nginx, which does not forward a port in Host. The session cookie is now named for the port the browser reached, so two gateways that both bound 18792 do not share one jar entry when one browser reaches them through different forwards. The exchange answers with the reason it refused, and the sign-in page shows that text instead of calling every failure an expired link. `_open` prints the sign-in link every time, before handing it to a browser: webbrowser.open returns True whenever it ran something, so the fallback was unreachable, and a console browser holds the terminal. --- docker/entrypoint.sh | 4 + docker/nginx/raven.conf.template | 10 +- raven/cli/serve_commands.py | 17 +++- raven/rpc/transports/ws.py | 105 +++++++++++++++++---- tests/test_cli_serve_commands.py | 35 +++++++ tests/test_docker_runtime.py | 14 ++- tests/test_rpc_transport.py | 152 +++++++++++++++++++++++++++++++ 7 files changed, 311 insertions(+), 26 deletions(-) diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index 0afc3b4a0..6d3c97084 100755 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -132,6 +132,10 @@ start_nginx() { log "the engine did not publish its browser session cookie" return 1 fi + # The name has to be the one the engine reads, and it derives that from the + # request's Host port -- which nginx forwards without one, so the engine keeps + # its own PAGE_PORT. Spelled here as that same port; test_docker_runtime.py + # holds the two sides together. RAVEN_AUTO_LOGIN_COOKIE="raven_session_${PAGE_PORT}=${cookie}; Path=/; Max-Age=2592000; HttpOnly; SameSite=Strict" fi export RAVEN_AUTO_LOGIN_COOKIE diff --git a/docker/nginx/raven.conf.template b/docker/nginx/raven.conf.template index d8b7f0c30..074bb2230 100644 --- a/docker/nginx/raven.conf.template +++ b/docker/nginx/raven.conf.template @@ -15,10 +15,12 @@ server { include /etc/nginx/proxy.conf; - # The engine answers only to an Origin naming its own loopback port - # (`_origin_ok` in raven/rpc/transports/ws.py), which no browser reaching - # this container through a published port or a hostname can send. Rewriting - # it here is what makes the proxy work at all. + # The engine accepts an Origin that names its own loopback port, or one that + # equals the request's Host on a loopback name (`_origin_ok` in + # raven/rpc/transports/ws.py). A browser reaching this container through a + # published port or a hostname sends neither -- behind nginx its Host is the + # hostname it published, and Origin names that host with the published port + # -- so rewriting Origin here is what makes the proxy work at all. # # What that gives up is a defence-in-depth layer, not the gate: the session # cookie is SameSite=Strict, so a page on another site gets no cookie diff --git a/raven/cli/serve_commands.py b/raven/cli/serve_commands.py index 9b181dc6c..7862c91e1 100644 --- a/raven/cli/serve_commands.py +++ b/raven/cli/serve_commands.py @@ -486,11 +486,9 @@ async def _serve_main(port: int, open_browser: bool) -> None: if stack.build_error is not None: typer.echo(f"warning: agent loop failed to build ({stack.build_error.message}); chat turns will error") if open_browser: - nonce = gateway.mint_nonce() - auth_url = f"{base_url}/auth#{nonce}" + auth_url = f"{base_url}/auth#{gateway.mint_nonce()}" typer.echo(f"opening browser: {base_url}") - if not webbrowser.open(auth_url): - typer.echo(f"could not open a browser automatically; visit: {auth_url}") + _open(auth_url) announcer = asyncio.create_task(_announce_updates(gateway.broadcast, stop)) @@ -1648,8 +1646,17 @@ def _run_foreground(port: int, dev: bool = False) -> None: def _open(url: str) -> None: + """Print the sign-in link, then hand it to a browser. + + Printed every time, because ``webbrowser.open`` answers True whenever it ran + something: VS Code's ``browser.sh`` exits 0 whatever happened, and a machine + with no display hands the URL to a console browser that has no JavaScript + and so never redeems it. Printed first, because a console browser holds the + terminal until it is quit. + """ + typer.echo(f"sign in with this link (it works once): {url}") if not webbrowser.open(url): - typer.echo(f"could not open a browser automatically; visit: {url}") + typer.echo("could not open a browser automatically; open the link above") def register(app: typer.Typer) -> None: diff --git a/raven/rpc/transports/ws.py b/raven/rpc/transports/ws.py index ce88189c7..005288ae7 100644 --- a/raven/rpc/transports/ws.py +++ b/raven/rpc/transports/ws.py @@ -6,7 +6,7 @@ ``/auth/exchange`` which burns the nonce and sets an HttpOnly SameSite=Strict session cookie. The ``/rpc`` WebSocket upgrade and any future ``/files/*`` endpoints require that cookie (or an ``X-Raven-Token`` header for local -tooling), plus an Origin check when a browser sends one. +Tooling authenticates with the ``X-Raven-Token`` header; a browser relies on the Origin check. Multi-connection from day one (plan D3): every authenticated socket receives all notification frames; request/response frames stay on the socket that sent @@ -16,12 +16,14 @@ from __future__ import annotations import asyncio +import ipaddress import json import secrets import time from collections.abc import Callable from pathlib import Path from typing import Any +from urllib.parse import urlsplit from aiohttp import WSCloseCode, WSMsgType, web from loguru import logger @@ -33,24 +35,40 @@ # the credential used after the one-time exchange. _NONCE_TTL_S = 30 * 60.0 +# The page shows whatever the exchange answered, so the gateway is what says +# why a sign-in was refused; the two refusals below need different remedies. _AUTH_PAGE = """ Raven

Signing in to Raven...

""" +_REFUSED_ADDRESS = ( + "Raven signs in a page opened at 127.0.0.1 or localhost, and this page is somewhere else. " + "Open the sign-in link at one of those addresses; reaching this machine through a port forward is fine." +) +_SPENT_LINK = "This sign-in link was already used or has expired. Run raven web for a new one." + _COOKIE_BASE = "raven_session" @@ -73,6 +91,34 @@ def cookie_name(port: int) -> str: return f"{_COOKIE_BASE}_{port}" +def _loopback_origin(origin: str) -> bool: + """Whether a browser origin names this machine: ``localhost`` or a loopback IP.""" + try: + hostname = urlsplit(origin).hostname + except ValueError: + return False + if hostname == "localhost": + return True + try: + return hostname is not None and ipaddress.ip_address(hostname).is_loopback + except ValueError: + return False + + +def _loopback_origin(origin: str) -> bool: + """Whether a browser origin names this machine: ``localhost`` or a loopback IP.""" + try: + hostname = urlsplit(origin).hostname + except ValueError: + return False + if hostname == "localhost": + return True + try: + return hostname is not None and ipaddress.ip_address(hostname).is_loopback + except ValueError: + return False + + _COOKIE_MAX_AGE_S = 30 * 24 * 60 * 60 """How long a browser keeps the session cookie. @@ -176,20 +222,47 @@ def _authorized(self, request: web.Request) -> bool: reachable here, but a credential check that short-circuits on the first wrong byte is not worth keeping when the fix is free. """ - cookie = request.cookies.get(cookie_name(self.port)) + cookie = request.cookies.get(self._cookie_name(request)) if cookie is not None and secrets.compare_digest(cookie, self.session_cookie): return True header = request.headers.get("X-Raven-Token") return header is not None and secrets.compare_digest(header, self.session_token) + def _cookie_name(self, request: web.Request) -> str: + """The session cookie's name, for the port the browser reached. + + That is the bound port unless a port forward picked another one, and two + machines whose gateways both bound 18792 must not share one jar entry when + one browser reaches them through different forwards. A Host with no port + -- the container's nginx forwards ``$host`` -- keeps the bound port, which + is the name that proxy's auto-login sets. + """ + try: + port = urlsplit(f"//{request.headers.get('Host', '')}").port + except ValueError: + port = None + return cookie_name(port or self.port) + def _origin_ok(self, request: web.Request) -> bool: + """Whether a browser request comes from this gateway's own page. + + Beyond the bound port's own origin (also what the container's nginx + restates), a page may sit behind a port forward, which picks another + local port whenever the remote one is taken; its origin then names a + port this process never bound. It passes when it asks its own origin + (Origin equals Host) on a loopback name. The first half refuses a page + another local server shows: cookies and SameSite ignore the port, so + that page's requests arrive carrying this session's cookie. The second + refuses DNS rebinding, where a name resolving to 127.0.0.1 asks its own + origin and the two agree. + """ origin = request.headers.get("Origin") if origin is None: return True - return origin in ( - f"http://127.0.0.1:{self.port}", - f"http://localhost:{self.port}", - ) + if origin in (f"http://127.0.0.1:{self.port}", f"http://localhost:{self.port}"): + return True + host = request.headers.get("Host", "") + return origin.lower() == f"http://{host.lower()}" and _loopback_origin(origin) # ---- handlers ------------------------------------------------------------ @@ -214,7 +287,7 @@ async def handle_auth_page(self, request: web.Request) -> web.Response: async def handle_auth_exchange(self, request: web.Request) -> web.Response: if not self._origin_ok(request): - raise web.HTTPForbidden(reason="bad origin") + raise web.HTTPForbidden(reason="bad origin", text=_REFUSED_ADDRESS) try: body = await request.json() except Exception: @@ -226,10 +299,10 @@ async def handle_auth_exchange(self, request: web.Request) -> web.Response: token_ok = isinstance(token, str) and secrets.compare_digest(token, self.session_token) nonce_ok = isinstance(nonce, str) and self.burn_nonce(nonce) if not (nonce_ok or token_ok): - raise web.HTTPForbidden(reason="invalid or used nonce") + raise web.HTTPForbidden(reason="invalid or used nonce", text=_SPENT_LINK) resp = web.json_response({"ok": True}) resp.set_cookie( - cookie_name(self.port), + self._cookie_name(request), self.session_cookie, httponly=True, samesite="Strict", diff --git a/tests/test_cli_serve_commands.py b/tests/test_cli_serve_commands.py index c92d24cda..9590c670d 100644 --- a/tests/test_cli_serve_commands.py +++ b/tests/test_cli_serve_commands.py @@ -449,6 +449,41 @@ def test_no_page_built_refuses_instead_of_opening_the_placeholder( assert started == [] and supervised == [] and opened == [] +class TestTheSignInLink: + """The one URL that signs a browser in has to reach the person, whatever the + browser launch did with it.""" + + def test_it_is_printed_when_a_browser_says_it_opened(self, monkeypatch, capsys) -> None: + """``webbrowser.open`` answers True whenever it ran something: VS Code's + ``browser.sh`` exits 0 on any outcome, and a box with no display hands the + URL to w3m, which has no JavaScript and never redeems it. A link printed + only on False was therefore never printed at all.""" + monkeypatch.setattr(serve_commands.webbrowser, "open", lambda url: True) + + serve_commands._open("http://127.0.0.1:18792/auth#n0nce") + + assert "http://127.0.0.1:18792/auth#n0nce" in capsys.readouterr().out + + def test_it_is_on_screen_before_the_browser_is_asked(self, monkeypatch, capsys) -> None: + """A console browser holds the terminal until it is quit, and a launch can + raise; printed first, the link is there in both cases.""" + on_screen: list[str] = [] + monkeypatch.setattr(serve_commands.webbrowser, "open", lambda url: on_screen.append(capsys.readouterr().out)) + + serve_commands._open("http://127.0.0.1:18792/auth#n0nce") + + assert "auth#n0nce" in on_screen[0] + + def test_a_standalone_serve_opens_through_the_same_path(self) -> None: + """`raven serve --open` with no gateway to attach to opens from inside the + coroutine that binds the port, which no test runs; pinned by source.""" + import inspect + + src = inspect.getsource(serve_commands._serve_main) + assert "_open(auth_url)" in src + assert "webbrowser.open" not in src + + class _FakeProc: """A gateway run that ends with ``code``, without running one.""" diff --git a/tests/test_docker_runtime.py b/tests/test_docker_runtime.py index c2990a81d..e05c28fde 100644 --- a/tests/test_docker_runtime.py +++ b/tests/test_docker_runtime.py @@ -101,7 +101,19 @@ def test_container_bootstraps_the_browser_after_the_engine_is_ready() -> None: assert entrypoint.index("wait_for_engine || boot=$?") < entrypoint.index("start_nginx\n announce") assert 'raven gateway --page-port "${PAGE_PORT}" &' in entrypoint - assert 'RAVEN_AUTO_LOGIN_COOKIE="raven_session_${PAGE_PORT}=' in entrypoint + # Read back the name the entrypoint builds, against the one the engine reads + # (`cookie_name`): nginx forwards Host without a port, so the engine falls back + # to its own page port and the two have to name the same jar entry. Spelled as + # a literal in both files, they agree only until the engine's rule moves. + from raven.rpc.transports.ws import cookie_name + + auto_login = next( + line + for line in entrypoint.splitlines() + if line.strip().startswith("RAVEN_AUTO_LOGIN_COOKIE=") and "cookie}" in line + ) + name = auto_login.split('"', 2)[1].split("=", 1)[0].replace("${PAGE_PORT}", str(18793)) + assert name == cookie_name(18793), f"the auto-login cookie is {name}, the engine reads {cookie_name(18793)}" assert 'add_header Set-Cookie "${RAVEN_AUTO_LOGIN_COOKIE}" always;' in nginx diff --git a/tests/test_rpc_transport.py b/tests/test_rpc_transport.py index a9a514248..e5a8b4a59 100644 --- a/tests/test_rpc_transport.py +++ b/tests/test_rpc_transport.py @@ -76,6 +76,41 @@ async def test_two_gateways_do_not_share_one_cookie_entry(tmp_path: Path) -> Non await client.close() +async def test_two_gateways_on_one_port_number_keep_their_own_cookies_behind_forwards() -> None: + """The same collision, reached through port forwards. + + Two machines both serve the page on 18792, and the browser reaches one on + its own 18792 and the other on whatever port its forwarder picked. Named + for the port each gateway bound, both cookies are ``raven_session_18792`` + on one host, so signing into either signs the other out. Named for the + port the browser reached, the jar keeps both. + """ + first, second = WsGateway(), WsGateway() + servers = [TestServer(build_app(g, None)) for g in (first, second)] + clients = [TestClient(s) for s in servers] + for client in clients: + await client.start_server() + for gateway in (first, second): + gateway.port = 18792 + try: + jar = {} + for client, gateway in zip(clients, (first, second), strict=True): + resp = await client.post("/auth/exchange", json={"nonce": gateway.mint_nonce()}) + assert resp.status == 200, await resp.text() + jar.update({name: morsel.value for name, morsel in resp.cookies.items()}) + + assert len(jar) == 2, f"two sign-ins wrote one jar entry: {sorted(jar)}" + # The jar is read back, not just written: a name no request ever sends + # would set two entries and authorize nothing. + for client in clients: + client.session.cookie_jar.clear() + client.session.cookie_jar.update_cookies(jar) + assert (await client.get("/file?path=")).status != 401, "the cookie the browser kept was not accepted" + finally: + for client in clients: + await client.close() + + # --------------------------------------------------------------------------- # who may reach the authenticated surface # --------------------------------------------------------------------------- @@ -212,6 +247,123 @@ async def test_the_gateways_own_origin_is_accepted(gateway_client) -> None: assert resp.status == 200 +@pytest.fixture +def forwarded(gateway_client): + """The gateway as a browser sees it through a port forward. + + VS Code and ``ssh -L`` relay bytes unchanged, and they pick another local + port whenever the remote one is already taken on the browser's machine, so + the page's Host and Origin name a port the gateway never bound. Moving + ``gateway.port`` off the port the client really reaches is that situation + without the forwarder. + """ + gateway, client = gateway_client + browser_port = gateway.port + gateway.port = browser_port + 1 if browser_port < 65535 else browser_port - 1 + return gateway, client, browser_port + + +async def test_a_page_reached_through_a_port_forward_signs_in(forwarded) -> None: + """Refusing the forwarded origin answered every sign-in with 403 while the + page reported an expired link, and each new link was refused the same way.""" + gateway, client, browser_port = forwarded + resp = await client.post( + "/auth/exchange", + json={"nonce": gateway.mint_nonce()}, + headers={"Origin": f"http://127.0.0.1:{browser_port}"}, + ) + assert resp.status == 200, await resp.text() + + +async def test_a_page_reached_through_a_port_forward_opens_its_socket(forwarded) -> None: + """The page upgrades /rpc from the origin it signed in on, and the handshake + passes the same check, so a sign-in alone would still leave it dead.""" + gateway, client, browser_port = forwarded + origin = {"Origin": f"http://127.0.0.1:{browser_port}"} + signed_in = await client.post("/auth/exchange", json={"nonce": gateway.mint_nonce()}, headers=origin) + assert signed_in.status == 200, await signed_in.text() + + async with client.ws_connect("/rpc", headers=origin) as ws: + assert not ws.closed + + +async def test_a_page_on_another_local_port_is_refused(gateway_client) -> None: + """Cookies and SameSite both ignore the port, so a page another local server + shows (a dev server, a package's preview) gets this session cookie attached + to whatever it sends here. Its origin is the one thing that gives it away, + which is why a forwarded origin passes only when the page asks its own.""" + gateway, client = gateway_client + elsewhere = gateway.port + 1 if gateway.port < 65535 else gateway.port - 1 + resp = await client.post( + "/auth/exchange", + json={"nonce": gateway.mint_nonce()}, + headers={"Origin": f"http://127.0.0.1:{elsewhere}"}, + ) + assert resp.status == 403 + + +async def test_a_name_that_only_resolves_to_this_machine_is_refused(gateway_client) -> None: + """DNS rebinding: a site whose name answers 127.0.0.1 asks its own origin, so + its Origin and Host agree. Requiring a loopback host is what refuses it.""" + gateway, client = gateway_client + rebound = f"rebind.example:{gateway.port}" + resp = await client.post( + "/auth/exchange", + json={"nonce": gateway.mint_nonce()}, + headers={"Origin": f"http://{rebound}", "Host": rebound}, + ) + assert resp.status == 403 + + +async def test_a_proxy_that_restates_the_gateways_origin_still_signs_in(gateway_client) -> None: + """The container's nginx rewrites Origin to the gateway's own loopback origin + and forwards ``Host: $host``, which carries no port. That pair has to keep + working, and its cookie has to stay the ``raven_session_`` the + proxy's auto-login hands out, or that login stops signing anyone in.""" + from raven.rpc.transports.ws import cookie_name + + gateway, client = gateway_client + proxied = {"Origin": f"http://127.0.0.1:{gateway.port}", "Host": "127.0.0.1"} + resp = await client.post("/auth/exchange", json={"nonce": gateway.mint_nonce()}, headers=proxied) + assert resp.status == 200, await resp.text() + assert cookie_name(gateway.port) in resp.cookies + + assert (await client.get("/rpc", headers=proxied)).status not in (401, 403) + + +async def test_a_refused_sign_in_says_which_refusal_it_was(gateway_client) -> None: + """The sign-in page shows the reader what the gateway answered, so the answer + has to tell the refusals apart. Reporting a refused address as an expired + link sent people to mint link after link, each refused the same way.""" + gateway, client = gateway_client + foreign = await client.post( + "/auth/exchange", + json={"nonce": gateway.mint_nonce()}, + headers={"Origin": "http://evil.example"}, + ) + spent = await client.post("/auth/exchange", json={"nonce": "never-minted"}) + + assert foreign.status == spent.status == 403 + foreign_says, spent_says = await foreign.text(), await spent.text() + assert "raven web" in spent_says, "a used or expired link has to name the command that mints a new one" + assert "127.0.0.1" in foreign_says, "a refused address has to say which address works" + assert "raven web" not in foreign_says, "a new link cannot fix an address that is refused" + + +@pytest.mark.parametrize("origin", ["http://[::1", "not a url", "http://127.0.0.1:99999"]) +async def test_an_origin_that_does_not_parse_is_refused_rather_than_raising(gateway_client, origin: str) -> None: + """A browser sends neither of these, so only a non-browser client reaches the + branch -- and a raise there answers a client that has no credential with a + 500 instead of a refusal.""" + gateway, client = gateway_client + resp = await client.post( + "/auth/exchange", + json={"nonce": gateway.mint_nonce()}, + headers={"Origin": origin}, + ) + assert resp.status == 403 + + # --------------------------------------------------------------------------- # /oauth/callback: unauthenticated by design, so it may not run script # --------------------------------------------------------------------------- From be597801f0b969eed3dadaffaa28bef594dc7b7e Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Tue, 13 Oct 2026 09:02:43 +0000 Subject: [PATCH 2/7] fix(rpc): drop the duplicated origin helper and cover the sign-in paths The previous commit defined _loopback_origin twice and replaced one sentence of the module docstring with an unrelated one. The first copy was shadowed at import, so behaviour was unchanged, but its ten lines could never run and took the diff-coverage gate to 60% (26/43). ruff's F811 does not report a redefined underscore name, so lint stayed clean. Both are restored, and tests now reach each branch the gate listed: - the localhost spelling through a forward. The old test used the bound port, which the previous allowlist already accepted, so the new branch never ran; - a Host whose port does not parse (out of range, not a number, an open IPv6 bracket), answered 401 rather than a 500; - an Origin equal to its Host that does not parse, answered 403; - the loopback rule asked directly, over the spellings a request-level test would each need a forward for; - `raven serve --open` run end to end, rather than pinned by source. Diff coverage at this head is 100% locally (26/26). --- raven/rpc/transports/ws.py | 25 +++------ tests/test_cli_serve_commands.py | 95 +++++++++++++++++++++++++++++++- tests/test_rpc_transport.py | 58 +++++++++++++++++-- 3 files changed, 155 insertions(+), 23 deletions(-) diff --git a/raven/rpc/transports/ws.py b/raven/rpc/transports/ws.py index 005288ae7..8a647971c 100644 --- a/raven/rpc/transports/ws.py +++ b/raven/rpc/transports/ws.py @@ -6,7 +6,7 @@ ``/auth/exchange`` which burns the nonce and sets an HttpOnly SameSite=Strict session cookie. The ``/rpc`` WebSocket upgrade and any future ``/files/*`` endpoints require that cookie (or an ``X-Raven-Token`` header for local -Tooling authenticates with the ``X-Raven-Token`` header; a browser relies on the Origin check. +tooling), plus an Origin check when a browser sends one. Multi-connection from day one (plan D3): every authenticated socket receives all notification frames; request/response frames stay on the socket that sent @@ -92,21 +92,12 @@ def cookie_name(port: int) -> str: def _loopback_origin(origin: str) -> bool: - """Whether a browser origin names this machine: ``localhost`` or a loopback IP.""" - try: - hostname = urlsplit(origin).hostname - except ValueError: - return False - if hostname == "localhost": - return True - try: - return hostname is not None and ipaddress.ip_address(hostname).is_loopback - except ValueError: - return False + """Whether a browser origin names this machine: ``localhost`` or a loopback IP. - -def _loopback_origin(origin: str) -> bool: - """Whether a browser origin names this machine: ``localhost`` or a loopback IP.""" + Both parses raise on input a client other than a browser can send: + ``urlsplit`` on an IPv6 bracket left open, ``ip_address`` on any name, which + is how the rebinding case lands here. Either one is a refusal, not an error. + """ try: hostname = urlsplit(origin).hostname except ValueError: @@ -235,7 +226,9 @@ def _cookie_name(self, request: web.Request) -> str: machines whose gateways both bound 18792 must not share one jar entry when one browser reaches them through different forwards. A Host with no port -- the container's nginx forwards ``$host`` -- keeps the bound port, which - is the name that proxy's auto-login sets. + is the name that proxy's auto-login sets. So does a Host whose port does + not parse: this is read before any credential is checked, so a raise here + would answer an unauthenticated request with a 500 instead of a refusal. """ try: port = urlsplit(f"//{request.headers.get('Host', '')}").port diff --git a/tests/test_cli_serve_commands.py b/tests/test_cli_serve_commands.py index 9590c670d..42ef16160 100644 --- a/tests/test_cli_serve_commands.py +++ b/tests/test_cli_serve_commands.py @@ -453,6 +453,54 @@ class TestTheSignInLink: """The one URL that signs a browser in has to reach the person, whatever the browser launch did with it.""" + @staticmethod + async def _port(preferred: int, **_kwargs) -> int: + return preferred + + @staticmethod + async def _rpc_stack(*_args, **_kwargs): + """A stack good enough for `_serve_main`: it reads `build_error`, hands the + loop to the app builder, and is torn down when the stop lands.""" + from types import SimpleNamespace + + async def _teardown() -> None: + pass + + return SimpleNamespace( + build_error=None, agent_loop=None, deliverables=None, dispatcher=None, teardown=_teardown + ) + + class _Application: + """The three things `build_app` touches on a real aiohttp application.""" + + def __init__(self) -> None: + self.router = self + self.on_shutdown: list = [] + + def add_get(self, *_args, **_kwargs) -> None: + pass + + def add_post(self, *_args, **_kwargs) -> None: + pass + + def add_static(self, *_args, **_kwargs) -> None: + pass + + class _Runner: + """`AppRunner`/`TCPSite`: the four awaitables `_serve_main` uses.""" + + def __init__(self, *_args) -> None: + pass + + async def setup(self) -> None: + pass + + async def cleanup(self) -> None: + pass + + async def start(self) -> None: + pass + def test_it_is_printed_when_a_browser_says_it_opened(self, monkeypatch, capsys) -> None: """``webbrowser.open`` answers True whenever it ran something: VS Code's ``browser.sh`` exits 0 on any outcome, and a box with no display hands the @@ -476,13 +524,58 @@ def test_it_is_on_screen_before_the_browser_is_asked(self, monkeypatch, capsys) def test_a_standalone_serve_opens_through_the_same_path(self) -> None: """`raven serve --open` with no gateway to attach to opens from inside the - coroutine that binds the port, which no test runs; pinned by source.""" + coroutine that binds the port; pinned by source, and driven end to end + below by `test_a_standalone_serve_prints_the_link_it_opens`.""" import inspect src = inspect.getsource(serve_commands._serve_main) assert "_open(auth_url)" in src assert "webbrowser.open" not in src + async def test_a_standalone_serve_prints_the_link_it_opens(self, monkeypatch, opened: list[str]) -> None: + """The standalone path, run rather than read: the URL it hands the browser + is one the gateway minted, and the launch that asked for a browser is the + one that gets one.""" + from contextlib import suppress + + from raven.cli import serve_commands as sc + + printed: list[str] = [] + monkeypatch.setattr(sc.typer, "echo", printed.append) + + def _arm(_port, _token, _cookie, stop) -> None: + # `_serve_main` holds until something asks it to stop; nothing here + # does, so arming it is the natural place to ask -- and it is what a + # `raven serve --stop` would do. + asyncio.get_running_loop().call_soon(stop.set) + + monkeypatch.setattr(sc.SERVE, "arm", _arm) + monkeypatch.setattr(sc.SERVE, "disarm", lambda: None) + monkeypatch.setattr("raven.rpc.transports.ws.pick_port", TestTheSignInLink._port) + monkeypatch.setattr("raven.rpc.bootstrap.build_rpc_stack", TestTheSignInLink._rpc_stack) + monkeypatch.setattr(sc, "warm_up_in_background", lambda: None) + monkeypatch.setattr("raven.updates.update_notice.maybe_refresh_async", lambda: None) + monkeypatch.setattr("raven.rpc.deck_templates.warm_covers_in_background", lambda: None) + monkeypatch.setattr("raven.rpc.deck_templates.stop_warming", lambda: None) + + async def _nothing() -> None: + return None + + from aiohttp import web + + monkeypatch.setattr(web, "AppRunner", TestTheSignInLink._Runner) + monkeypatch.setattr(web, "TCPSite", TestTheSignInLink._Runner) + monkeypatch.setattr(web, "Application", TestTheSignInLink._Application) + monkeypatch.setattr(serve_commands, "_announce_updates", lambda *a, **k: _nothing()) + monkeypatch.setattr(serve_commands, "_write_serve_state", lambda *a, **k: None) + + with suppress(KeyboardInterrupt): + await sc._serve_main(18999, True) + + assert len(opened) == 1 + assert opened[0].startswith("http://127.0.0.1:18999/auth#") + assert f"sign in with this link (it works once): {opened[0]}" in printed + class _FakeProc: """A gateway run that ends with ``code``, without running one.""" diff --git a/tests/test_rpc_transport.py b/tests/test_rpc_transport.py index e5a8b4a59..0480ac981 100644 --- a/tests/test_rpc_transport.py +++ b/tests/test_rpc_transport.py @@ -287,6 +287,20 @@ async def test_a_page_reached_through_a_port_forward_opens_its_socket(forwarded) assert not ws.closed +async def test_a_page_reached_as_localhost_through_a_port_forward_signs_in(forwarded) -> None: + """A browser may reach the gateway as `localhost` rather than `127.0.0.1`, and + the two are different origins to it. Through a forward neither is on the bound + port's allowlist, so the forwarded rule has to know the name as well as the + loopback addresses.""" + gateway, client, browser_port = forwarded + resp = await client.post( + "/auth/exchange", + json={"nonce": gateway.mint_nonce()}, + headers={"Origin": f"http://localhost:{browser_port}", "Host": f"localhost:{browser_port}"}, + ) + assert resp.status == 200, await resp.text() + + async def test_a_page_on_another_local_port_is_refused(gateway_client) -> None: """Cookies and SameSite both ignore the port, so a page another local server shows (a dev server, a package's preview) gets this session cookie attached @@ -315,6 +329,30 @@ async def test_a_name_that_only_resolves_to_this_machine_is_refused(gateway_clie assert resp.status == 403 +@pytest.mark.parametrize( + ("origin", "want"), + [ + ("http://127.0.0.1:18792", True), + ("http://localhost:18793", True), + ("http://[::1]:18792", True), + ("http://127.0.0.1", True), + ("http://rebind.example:18792", False), + ("http://127.0.0.1.evil.example", False), + ("http://10.1.2.3:18792", False), + ("http://[::1", False), + ("null", False), + ], +) +def test_the_loopback_rule_answers_each_name(origin: str, want: bool) -> None: + """The rule asked directly, over spellings a request-level test would each need + its own forward for: the IPv6 loopback, an origin with no port, a name that + only begins like a loopback address, a private address, and an origin that + does not parse.""" + from raven.rpc.transports.ws import _loopback_origin + + assert _loopback_origin(origin) is want + + async def test_a_proxy_that_restates_the_gateways_origin_still_signs_in(gateway_client) -> None: """The container's nginx rewrites Origin to the gateway's own loopback origin and forwards ``Host: $host``, which carries no port. That pair has to keep @@ -350,16 +388,24 @@ async def test_a_refused_sign_in_says_which_refusal_it_was(gateway_client) -> No assert "raven web" not in foreign_says, "a new link cannot fix an address that is refused" -@pytest.mark.parametrize("origin", ["http://[::1", "not a url", "http://127.0.0.1:99999"]) -async def test_an_origin_that_does_not_parse_is_refused_rather_than_raising(gateway_client, origin: str) -> None: - """A browser sends neither of these, so only a non-browser client reaches the - branch -- and a raise there answers a client that has no credential with a - 500 instead of a refusal.""" +@pytest.mark.parametrize("host", ["127.0.0.1:99999", "127.0.0.1:abc", "[::1"]) +async def test_a_host_that_does_not_parse_is_refused_rather_than_raising(gateway_client, host: str) -> None: + """The cookie's name is read from Host before any credential is checked, and + each of these makes that read raise. A browser sends none of them; a client + that does gets a refusal, not a 500.""" + _, client = gateway_client + resp = await client.get("/rpc", headers={"Host": host}) + assert resp.status == 401 + + +async def test_an_origin_that_does_not_parse_is_refused_rather_than_raising(gateway_client) -> None: + """An Origin equal to its Host is what sends a request on to the loopback + test, so an IPv6 bracket left open in both reaches a parse that raises.""" gateway, client = gateway_client resp = await client.post( "/auth/exchange", json={"nonce": gateway.mint_nonce()}, - headers={"Origin": origin}, + headers={"Origin": "http://[::1", "Host": "[::1"}, ) assert resp.status == 403 From 72b3a890b5b69b8c8ef2b68e9be4b52978080428 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Tue, 13 Oct 2026 17:27:12 +0800 Subject: [PATCH 3/7] test(updates): isolate native upgrade fixtures by Raven home Give each real uv upgrade case its own marker and wheel cache so parallel Windows workers cannot replace or delete cached wheels from a sibling. Preserve localized failure diagnostics when capturing subprocess output. --- tests/integration/test_cli_upgrade_real_uv.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_cli_upgrade_real_uv.py b/tests/integration/test_cli_upgrade_real_uv.py index b6be80854..c144d6c55 100644 --- a/tests/integration/test_cli_upgrade_real_uv.py +++ b/tests/integration/test_cli_upgrade_real_uv.py @@ -176,6 +176,7 @@ def test_running_uv_tool_replaces_itself_in_custom_directories(tmp_path: Path, s "PATH": str(external_tools) + os.pathsep + env["PATH"], "UV_TOOL_DIR": str(tool_dir), "UV_TOOL_BIN_DIR": str(bin_dir), + "RAVEN_HOME": str(tmp_path / "raven home"), "RAVEN_UPGRADE_SOURCE": str(Path(__file__).parents[2]), "RAVEN_UPGRADE_WHEEL": new_wheel.resolve().as_uri(), } @@ -207,6 +208,7 @@ def companion_present() -> bool: env=env, capture_output=True, text=True, + errors="replace", timeout=180, ) @@ -215,7 +217,7 @@ def companion_present() -> bool: assert "system certificate store" not in completed.stderr, completed.stderr # From uv 0.11.9 to 0.12.15 the older name draws a deprecation warning even beside the new one. assert "UV_NATIVE_TLS" not in completed.stderr, completed.stderr - assert "Raven upgraded: 1.0.0 -> 2.0.0" in completed.stdout + assert "Raven upgraded: 1.0.0 -> 2.0.0" in completed.stdout, completed.stderr version = subprocess.run( [str(executable), "--version"], check=True, From 1b2232c7b020fffb08b11a90a0515dfe11f3fd55 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Tue, 13 Oct 2026 17:27:13 +0800 Subject: [PATCH 4/7] test(rpc): report missing Windows symlink privilege Skip only the unsupported symlink setup when Windows reports privilege error 1314. Keep the path-confinement assertion active wherever symlink creation succeeds and propagate other filesystem errors. --- tests/test_rpc_transport.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test_rpc_transport.py b/tests/test_rpc_transport.py index 0480ac981..b055e0f3a 100644 --- a/tests/test_rpc_transport.py +++ b/tests/test_rpc_transport.py @@ -504,7 +504,12 @@ def test_restricted_mode_refuses_a_symlink_that_points_out(tmp_path: Path, monke outside = tmp_path.parent / "outside.txt" outside.write_text("secret") - (tmp_path / "escape.txt").symlink_to(outside) + try: + (tmp_path / "escape.txt").symlink_to(outside) + except OSError as exc: + if getattr(exc, "winerror", None) == 1314: + pytest.skip("Windows symlink creation requires an unavailable privilege") + raise monkeypatch.setattr(files_module, "load_config", lambda: _workspace_config(tmp_path, restrict=True)) with pytest.raises(PermissionError): From b95640300eb40ddcc50eafcd268b3c5354c8875d Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Tue, 13 Oct 2026 17:27:16 +0800 Subject: [PATCH 5/7] test(cli): scope owner-only state-file checks to POSIX Windows reports writable DOS mode bits rather than POSIX owner-only permissions. Keep the existing mode assertion on POSIX and identify Windows permission coverage as unsupported by this test. --- tests/test_cli_serve_commands.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_cli_serve_commands.py b/tests/test_cli_serve_commands.py index 42ef16160..a39d68e8d 100644 --- a/tests/test_cli_serve_commands.py +++ b/tests/test_cli_serve_commands.py @@ -1508,6 +1508,7 @@ def test_an_unusable_state_file_is_not_adopted(self, home: Path, body: str) -> N assert gw.session_cookie == "freshly-minted" + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX file modes are not available on Windows") def test_the_state_file_stays_owner_only(self, home: Path) -> None: """It now carries two credentials rather than one.""" path = serve_commands._write_serve_state(18792, "tok-1", "cookie-1") From 9fdc3a2a9ff08d684cbf154920ea9f4a13f265ef Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Tue, 13 Oct 2026 17:31:50 +0800 Subject: [PATCH 6/7] test(cli): restrict SIGTERM handler checks to POSIX Popen.terminate uses TerminateProcess on Windows and cannot exercise Python SIGTERM cleanup handlers. Keep these signal-delivery regressions on POSIX, alongside the existing process-group tests. --- tests/test_cli_serve_commands.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_cli_serve_commands.py b/tests/test_cli_serve_commands.py index a39d68e8d..af5f87e48 100644 --- a/tests/test_cli_serve_commands.py +++ b/tests/test_cli_serve_commands.py @@ -1787,6 +1787,7 @@ async def _broadcast(frame): assert asked[:3] == [1, 2, 3] +@pytest.mark.skipif(sys.platform == "win32", reason="Popen.terminate does not deliver SIGTERM on Windows") class TestTheSupervisorCleansUpOnTheSignalThatStopsIt: """`raven web --stop` sends SIGTERM. Python's default disposition kills the process where it stands, so the `finally` that removes `web.json` never ran From 4aed82d0cc9067e044a7a894936458672cdd3b13 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Tue, 13 Oct 2026 17:05:57 +0000 Subject: [PATCH 7/7] test(cli): exercise standalone sign-in with and without a built page Use the real aiohttp Application while replacing the runner and TCP site, so build_app can register its response hook without a partial fake. Resolve the UI from temporary candidates, cover both page states, and assert that revalidation is registered only when a built page is served. Correct the startup comment now that the coroutine is exercised in tests. --- raven/cli/serve_commands.py | 5 ++-- tests/test_cli_serve_commands.py | 41 ++++++++++++++++---------------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/raven/cli/serve_commands.py b/raven/cli/serve_commands.py index 7862c91e1..36637b60e 100644 --- a/raven/cli/serve_commands.py +++ b/raven/cli/serve_commands.py @@ -411,9 +411,8 @@ async def _serve_main(port: int, open_browser: bool) -> None: from raven.rpc.transports.ws import WsGateway, build_app, pick_port register_console_feature() - # This coroutine binds a port and serves until it is killed, so no test - # runs it; what it schedules is pinned by source in - # tests/test_cli_serve_commands.py instead. + # Startup tests replace this background warm-up to keep provider + # initialization out of the serving probe. warm_up_in_background() # pragma: no cover # Declared before anything can emit a span. The page runs on the terminal's diff --git a/tests/test_cli_serve_commands.py b/tests/test_cli_serve_commands.py index af5f87e48..870fc1c19 100644 --- a/tests/test_cli_serve_commands.py +++ b/tests/test_cli_serve_commands.py @@ -470,24 +470,8 @@ async def _teardown() -> None: build_error=None, agent_loop=None, deliverables=None, dispatcher=None, teardown=_teardown ) - class _Application: - """The three things `build_app` touches on a real aiohttp application.""" - - def __init__(self) -> None: - self.router = self - self.on_shutdown: list = [] - - def add_get(self, *_args, **_kwargs) -> None: - pass - - def add_post(self, *_args, **_kwargs) -> None: - pass - - def add_static(self, *_args, **_kwargs) -> None: - pass - class _Runner: - """`AppRunner`/`TCPSite`: the four awaitables `_serve_main` uses.""" + """`AppRunner`/`TCPSite` without binding sockets.""" def __init__(self, *_args) -> None: pass @@ -532,14 +516,21 @@ def test_a_standalone_serve_opens_through_the_same_path(self) -> None: assert "_open(auth_url)" in src assert "webbrowser.open" not in src - async def test_a_standalone_serve_prints_the_link_it_opens(self, monkeypatch, opened: list[str]) -> None: + @pytest.mark.parametrize("built_page", [False, True], ids=["without-page", "with-page"]) + async def test_a_standalone_serve_prints_the_link_it_opens( + self, two_candidates, built_page: bool, monkeypatch, opened: list[str] + ) -> None: """The standalone path, run rather than read: the URL it hands the browser is one the gateway minted, and the launch that asked for a browser is the - one that gets one.""" + one that gets one, with or without a built page.""" from contextlib import suppress from raven.cli import serve_commands as sc + packaged, _source = two_candidates + if built_page: + _build(packaged, "page") + printed: list[str] = [] monkeypatch.setattr(sc.typer, "echo", printed.append) @@ -563,9 +554,14 @@ async def _nothing() -> None: from aiohttp import web - monkeypatch.setattr(web, "AppRunner", TestTheSignInLink._Runner) + apps: list[web.Application] = [] + + def _runner(app: web.Application) -> TestTheSignInLink._Runner: + apps.append(app) + return self._Runner() + + monkeypatch.setattr(web, "AppRunner", _runner) monkeypatch.setattr(web, "TCPSite", TestTheSignInLink._Runner) - monkeypatch.setattr(web, "Application", TestTheSignInLink._Application) monkeypatch.setattr(serve_commands, "_announce_updates", lambda *a, **k: _nothing()) monkeypatch.setattr(serve_commands, "_write_serve_state", lambda *a, **k: None) @@ -575,6 +571,9 @@ async def _nothing() -> None: assert len(opened) == 1 assert opened[0].startswith("http://127.0.0.1:18999/auth#") assert f"sign in with this link (it works once): {opened[0]}" in printed + (app,) = apps + assert isinstance(app, web.Application) + assert any(hook.__name__ == "revalidate" for hook in app.on_response_prepare) is built_page class _FakeProc: