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,25 @@ 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. + + 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: + 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 +213,49 @@ 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. 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 + 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 +280,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 +292,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/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, diff --git a/tests/test_cli_serve_commands.py b/tests/test_cli_serve_commands.py index c92d24cda..870fc1c19 100644 --- a/tests/test_cli_serve_commands.py +++ b/tests/test_cli_serve_commands.py @@ -449,6 +449,133 @@ 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.""" + + @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 _Runner: + """`AppRunner`/`TCPSite` without binding sockets.""" + + 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 + 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; 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 + + @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, 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) + + 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 + + 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(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 + (app,) = apps + assert isinstance(app, web.Application) + assert any(hook.__name__ == "revalidate" for hook in app.on_response_prepare) is built_page + + class _FakeProc: """A gateway run that ends with ``code``, without running one.""" @@ -1380,6 +1507,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") @@ -1658,6 +1786,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 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..b055e0f3a 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,169 @@ 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_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 + 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 + + +@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 + working, and its cookie has to stay the ``raven_session_