diff --git a/src/entrypoints/serve_cli.py b/src/entrypoints/serve_cli.py index 4c1e57301..e125c33bb 100644 --- a/src/entrypoints/serve_cli.py +++ b/src/entrypoints/serve_cli.py @@ -31,6 +31,7 @@ import argparse import asyncio +import ipaddress import json import logging import os @@ -68,6 +69,22 @@ ReadyHook = Callable[[str, int, str], None] +_LOOPBACK_NAMES = frozenset({"localhost", "127.0.0.1", "::1", ""}) + + +def is_loopback(host: str) -> bool: + """True when ``host`` binds to this machine only.""" + normalized = (host or "").strip().lower() + if normalized in _LOOPBACK_NAMES: + return True + try: + return ipaddress.ip_address(normalized).is_loopback + except ValueError: + # A hostname we cannot classify (a LAN name, a container alias) is not + # provably local, so it is treated as remote. + return False + + def _build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser( prog="clawcodex serve", @@ -75,6 +92,10 @@ def _build_parser() -> argparse.ArgumentParser: ) parser.add_argument("--host", default="127.0.0.1", help="Bind address (default: 127.0.0.1 — loopback only).") + parser.add_argument("--allow-remote", action="store_true", dest="allow_remote", + help="Permit a non-loopback --host. GET / hands out this " + "server's session token unauthenticated, so only do " + "this behind your own auth.") parser.add_argument("--port", type=int, default=0, help="Port (default: 0 — OS-assigned, announced on stdout).") parser.add_argument("--token", default=None, @@ -176,11 +197,30 @@ def run_serve_subcommand(argv: list[str], *, on_ready: ReadyHook | None = None) bypass_requested=dangerously or allow_dangerously, ) + # `GET /` is unauthenticated by construction — it is the page that *hands + # out* the session token, so that the desktop shell and the browser client + # can both adopt a running backend (see `server/web_assets.py`). That is + # safe exactly as long as this port is reachable from this machine alone, + # which the default bind gives and an arbitrary `--host` does not. So a + # non-loopback bind is refused unless the caller says they have put their + # own authentication in front of it — the same gate `clawcodex web` has + # always had, on the command that actually opens the socket. + if not is_loopback(args.host) and not args.allow_remote: + print( + f"serve: refusing to bind {args.host}: GET / hands out this server's " + "session token without authentication, which is safe only on a " + "loopback bind. Pass --allow-remote if you have your own auth in " + "front of it.", + file=sys.stderr, + ) + return 2 + workspace = str(Path(args.workspace).resolve()) if args.workspace else str(Path.cwd()) # The desktop is an INTERACTIVE surface with a real user at the window, and - # this server is its own loopback, token-gated child — the same trust model - # as the TUI launcher spawning its agent-server. So it resolves permissions + # this server is its own token-gated child, loopback unless the operator + # asked otherwise above — the same trust model as the TUI launcher spawning + # its agent-server. So it resolves permissions # through the shared interactive resolver (src/cli.py + tui_launcher use it # too), which means Full Access by default, exactly like `clawcodex`. # diff --git a/src/entrypoints/web_cli.py b/src/entrypoints/web_cli.py index b5221c933..7835da450 100644 --- a/src/entrypoints/web_cli.py +++ b/src/entrypoints/web_cli.py @@ -25,7 +25,6 @@ from __future__ import annotations import argparse -import ipaddress import os import socket import subprocess @@ -41,7 +40,6 @@ DEFAULT_WEB_PORT = 8081 # Hosts that only accept connections originating on this machine. -_LOOPBACK_NAMES = frozenset({"localhost", "127.0.0.1", "::1", ""}) def repo_root() -> Path: @@ -53,17 +51,10 @@ def web_app_dir(root: Path | None = None) -> Path: return (root or repo_root()) / "ui-web" -def is_loopback(host: str) -> bool: - """True when ``host`` binds to this machine only.""" - normalized = (host or "").strip().lower() - if normalized in _LOOPBACK_NAMES: - return True - try: - return ipaddress.ip_address(normalized).is_loopback - except ValueError: - # A hostname we cannot classify (a LAN name, a container alias) is not - # provably local, so it is treated as remote. - return False +# Re-exported: `clawcodex serve` owns the predicate now, because it owns the +# bind. Kept importable here for the callers and tests that already read it +# from this module. +from src.entrypoints.serve_cli import is_loopback # noqa: E402 def browser_url(host: str, port: int, token: str) -> str: @@ -232,6 +223,11 @@ def _serve_argv(args: argparse.Namespace) -> list[str]: argv.append("--nano") if args.dangerously_skip_permissions: argv.append("--dangerously-skip-permissions") + # Forwarded, not re-derived: `serve` refuses a non-loopback bind on its own + # now, so a `web --allow-remote` that did not pass this along would be + # stopped by the child after clearing the parent's identical gate. + if args.allow_remote: + argv.append("--allow-remote") return argv diff --git a/src/server/desktop_serve.py b/src/server/desktop_serve.py index 492d3c7d1..702eeef2a 100644 --- a/src/server/desktop_serve.py +++ b/src/server/desktop_serve.py @@ -17,9 +17,15 @@ :mod:`src.server.desktop_gateway`. Auth: REST accepts the ``X-ClawCodex-Session-Token`` header or a Bearer -token; the WebSocket accepts ``?token=``. One constant-time comparison, -loopback binding, no cookies — this is the local token mode of the desktop's -connection config. +token; the WebSocket accepts ``?token=``. One constant-time comparison, no +cookies — the local token mode of the desktop's connection config. + +``GET /`` is the exception, and deliberately so: it is the page that *hands +out* the token, so it cannot require one. The trust model therefore rests on +the bind being reachable from this machine alone, which both entry points now +enforce — ``clawcodex serve`` and ``clawcodex web`` each refuse a non-loopback +``--host`` without ``--allow-remote``, by which the caller takes on putting +their own authentication in front of it. """ from __future__ import annotations diff --git a/src/server/web_assets.py b/src/server/web_assets.py index b140772f5..e1b73e049 100644 --- a/src/server/web_assets.py +++ b/src/server/web_assets.py @@ -20,8 +20,11 @@ serves both readers. That page is unauthenticated by construction (it is what *hands out* the -token), which is safe exactly as long as the server is bound to loopback. The -``clawcodex web`` entry enforces that; see ``src/entrypoints/web_cli.py``. +token), which is safe exactly as long as the server is bound to loopback. +Both entry points enforce that — ``clawcodex serve`` and ``clawcodex web`` +refuse a non-loopback ``--host`` unless ``--allow-remote`` says the caller has +put their own authentication in front of it. See ``entrypoints/serve_cli.py`` +and ``entrypoints/web_cli.py``. """ from __future__ import annotations diff --git a/tests/entrypoints/test_serve_cli_bind.py b/tests/entrypoints/test_serve_cli_bind.py new file mode 100644 index 000000000..bf8a632ac --- /dev/null +++ b/tests/entrypoints/test_serve_cli_bind.py @@ -0,0 +1,69 @@ +"""`clawcodex serve` refuses to hand its session token to the network. + +`GET /` is unauthenticated by construction — it is the page that *hands out* +the token so the desktop shell and the browser client can adopt a running +backend — so the trust model holds only while the port is reachable from this +machine alone. `clawcodex web` has always gated that; the command that actually +opens the socket had not. +""" + +from __future__ import annotations + +import pytest + +from src.entrypoints import serve_cli + + +class _Bound(Exception): + """Raised from the far side of the gate, to prove where a run reached.""" + + +def test_a_loopback_bind_is_allowed_by_default() -> None: + for host in ("127.0.0.1", "localhost", "::1", ""): + assert serve_cli.is_loopback(host) is True + + +def test_a_name_we_cannot_classify_is_treated_as_remote() -> None: + # Not provably local is not local: a LAN name or container alias could + # resolve anywhere. + for host in ("0.0.0.0", "192.168.1.10", "build-box.lan"): + assert serve_cli.is_loopback(host) is False + + +def test_serve_refuses_a_non_loopback_bind(capsys, monkeypatch) -> None: + # `build_app` is stubbed to raise so that a missing guard FAILS here rather + # than hanging: without it this call reaches uvicorn and blocks forever, + # which in CI is a timed-out job instead of a red test. + import src.server.desktop_serve as desktop_serve + + def _stop(_state: object) -> None: + raise _Bound + + monkeypatch.setattr(desktop_serve, "build_app", _stop) + + code = serve_cli.run_serve_subcommand(["--host", "0.0.0.0", "--port", "0"]) + + assert code == 2 + message = capsys.readouterr().err + assert "refusing to bind 0.0.0.0" in message + # Says why, and what to do about it. + assert "session token" in message + assert "--allow-remote" in message + + +def test_allow_remote_gets_past_the_gate(monkeypatch) -> None: + """The opt-out must actually opt out. + + Asserting the flag parses would pass even if the guard still refused it, so + this stops the run at `build_app` — the far side of the gate — and asserts + it was reached rather than that a return code was avoided. + """ + import src.server.desktop_serve as desktop_serve + + def _stop(_state: object) -> None: + raise _Bound + + monkeypatch.setattr(desktop_serve, "build_app", _stop) + + with pytest.raises(_Bound): + serve_cli.run_serve_subcommand(["--host", "0.0.0.0", "--allow-remote", "--port", "0"]) diff --git a/tests/entrypoints/test_web_cli.py b/tests/entrypoints/test_web_cli.py index 374e35fff..37a2dd664 100644 --- a/tests/entrypoints/test_web_cli.py +++ b/tests/entrypoints/test_web_cli.py @@ -73,6 +73,7 @@ def _args(**overrides: object) -> argparse.Namespace: base = dict( host="127.0.0.1", port=0, token=None, workspace=None, provider=None, model=None, effort=None, permission_mode=None, nano=False, dangerously_skip_permissions=False, + allow_remote=False, ) base.update(overrides) return argparse.Namespace(**base) @@ -84,6 +85,14 @@ def test_serve_argv_is_minimal_by_default() -> None: assert web_cli._serve_argv(_args(port=8081)) == ["--host", "127.0.0.1", "--port", "8081"] +def test_serve_argv_forwards_allow_remote() -> None: + """`serve` refuses a non-loopback bind on its own now, so a `web` that + cleared its own identical gate must pass the permission along — otherwise + the child stops what the parent just allowed.""" + assert "--allow-remote" in web_cli._serve_argv(_args(allow_remote=True)) + assert "--allow-remote" not in web_cli._serve_argv(_args()) + + def test_serve_argv_forwards_every_agent_flag() -> None: argv = web_cli._serve_argv( _args( diff --git a/tests/nano/test_nano_web.py b/tests/nano/test_nano_web.py index 8c20a5061..3b00e5933 100644 --- a/tests/nano/test_nano_web.py +++ b/tests/nano/test_nano_web.py @@ -51,7 +51,7 @@ def _args(**overrides): base = dict( host="127.0.0.1", port=8081, token=None, workspace=None, provider=None, model=None, effort=None, permission_mode=None, - nano=False, dangerously_skip_permissions=False, + nano=False, dangerously_skip_permissions=False, allow_remote=False, ) base.update(overrides) return argparse.Namespace(**base)