From 77dd2086bdfba27300202518b780b2b0eed2f91a Mon Sep 17 00:00:00 2001 From: ahmad-ajmal Date: Mon, 21 Sep 2026 13:07:05 +0100 Subject: [PATCH 1/2] WS auth issue --- app/ui_layer/adapters/browser_adapter.py | 52 ++++- app/ui_layer/adapters/ws_auth.py | 110 ++++++++++ .../frontend/src/store/socket/SocketClient.ts | 20 +- .../src/store/socket/socketInstance.ts | 4 +- .../frontend/src/store/socket/types.ts | 6 + .../browser/frontend/src/utils/connection.ts | 15 ++ tests/test_ws_auth.py | 193 ++++++++++++++++++ 7 files changed, 395 insertions(+), 5 deletions(-) create mode 100644 app/ui_layer/adapters/ws_auth.py create mode 100644 tests/test_ws_auth.py diff --git a/app/ui_layer/adapters/browser_adapter.py b/app/ui_layer/adapters/browser_adapter.py index 2de74c0af..35f7cba9c 100644 --- a/app/ui_layer/adapters/browser_adapter.py +++ b/app/ui_layer/adapters/browser_adapter.py @@ -129,6 +129,7 @@ from app.ui_layer.diagnostics import LoopStallMonitor from app.ui_layer.adapters.ws_channel import ClientChannel from app.ui_layer.adapters.ws_lanes import message_lane +from app.ui_layer.adapters.ws_auth import WS_PROTOCOL, WsAuth from app.ui_layer.adapters.session_buffer import ( SESSION_BUFFER_LIMIT, SESSION_BUFFER_SLACK, @@ -838,6 +839,8 @@ def __init__( super().__init__(controller, "browser") self._host = host self._port = int(os.environ.get("BROWSER_PORT", port)) + # Origin allowlist + per-process session token for /ws (ws_auth.py). + self._ws_auth = WsAuth(self._port) self._theme_adapter = BrowserThemeAdapter(BaseTheme()) self._chat = BrowserChatComponent(self) self._action_panel = BrowserActionPanelComponent(self) @@ -1041,6 +1044,7 @@ async def _on_start(self) -> None: # API and WebSocket routes (must be registered first) self._app.router.add_get("/ws", self._websocket_handler) + self._app.router.add_get("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/api/session-token", self._session_token_handler) self._app.router.add_get("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/api/state", self._state_handler) self._app.router.add_get("/api/debug/loop", self._debug_loop_handler) if os.getenv("CRAFTBOT_DEBUG_ENDPOINTS") == "1": @@ -1204,9 +1208,20 @@ async def _websocket_handler( from aiohttp import web, WSMsgType import asyncio + # Reject foreign pages before upgrading (browsers apply no CORS here). + rejected = self._ws_auth.check_ws_handshake(request.headers) + if rejected: + logger.warning( + f"[BROWSER ADAPTER] Rejected /ws handshake ({rejected}): " + f"origin={request.headers.get('Origin')!r} " + f"host={request.headers.get('Host')!r}" + ) + raise web.HTTPForbidden(reason="WebSocket handshake not allowed") + ws = web.WebSocketResponse( max_msg_size=100 * 1024 * 1024, heartbeat=30.0, # Send ping every 30s to keep connection alive + protocols=(WS_PROTOCOL,), ) try: @@ -9739,6 +9754,31 @@ async def _index_handler(self, request: "web.Request") -> "web.Response": html = self._get_index_html() return web.Response(text=html, content_type="text/html") + async def _session_token_handler(self, request: "web.Request") -> "web.Response": + """Hand the /ws session token to CraftBot's own UI. + + No CORS headers are ever set here, so a cross-origin page can't read + the response; ws_auth also refuses rebinding Hosts and cross-site + fetches outright. + """ + from aiohttp import web + + rejected = self._ws_auth.check_token_request(request.headers) + if rejected: + logger.warning( + f"[BROWSER ADAPTER] Refused session token ({rejected}): " + f"origin={request.headers.get('Origin')!r} " + f"host={request.headers.get('Host')!r}" + ) + raise web.HTTPForbidden() + return web.json_response( + {"token": self._ws_auth.token}, + headers={ + "Cache-Control": "no-store", + "X-Content-Type-Options": "nosniff", + }, + ) + async def _state_handler(self, request: "web.Request") -> "web.Response": """API endpoint for current state.""" from aiohttp import web @@ -10057,9 +10097,17 @@ def _get_index_html(self) -> str: let ws; let state = { messages: [], actions: [], status: 'Connecting...' }; - function connect() { + async function connect() { const protocol = location.protocol === 'https:' ? 'wss:' : 'ws:'; - ws = new WebSocket(`${protocol}//${location.host}/ws`); + let token; + try { + const resp = await fetch('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/api/session-token', { cache: 'no-store' }); + token = (await resp.json()).token; + } catch (err) { + setTimeout(connect, 2000); + return; + } + ws = new WebSocket(`${protocol}//${location.host}/ws`, ['craftbot', `craftbot-auth.${token}`]); ws.onopen = () => { console.log('Connected to CraftBot'); diff --git a/app/ui_layer/adapters/ws_auth.py b/app/ui_layer/adapters/ws_auth.py new file mode 100644 index 000000000..e9e93d097 --- /dev/null +++ b/app/ui_layer/adapters/ws_auth.py @@ -0,0 +1,110 @@ +"""Handshake guard for the browser UI's /ws WebSocket. + +Browsers apply no CORS to WebSocket handshakes, so without a check any page +the user opens could drive the agent through /ws. Two independent checks: + +1. Origin (and Host) — the handshake must come from CraftBot's own UI origin, + addressed by a loopback name. The Host check defeats DNS rebinding. +2. Session token — a random secret generated at startup. The UI reads it from + GET /api/session-token (same-origin only: no CORS headers, so a foreign + page can't read the response) and sends it in Sec-WebSocket-Protocol, which + keeps it out of URLs and access logs. + +Legitimate UI origins: the frontend port (Vite dev server, or run.py's static +server) and the backend port (aiohttp serving the built UI itself), each as +localhost / 127.0.0.1 / [::1]. Tunnels and LAN URLs only ever expose Agent App +ports, never this UI, so they are not allowed here. +""" + +from __future__ import annotations + +import hmac +import os +import secrets +from typing import Mapping, Optional, Set + +# Subprotocol the server echoes back; the token rides alongside it as +# "craftbot-auth." in the client's protocol list. +WS_PROTOCOL = "craftbot" +WS_TOKEN_PROTOCOL_PREFIX = "craftbot-auth." + +_LOOPBACK_HOSTS = ("localhost", "127.0.0.1", "[::1]") +_DEFAULT_FRONTEND_PORT = 7925 + + +def _ui_ports(backend_port: int) -> Set[int]: + ports = {int(backend_port)} + try: + ports.add(int(os.environ.get("VITE_PORT", _DEFAULT_FRONTEND_PORT))) + except ValueError: + ports.add(_DEFAULT_FRONTEND_PORT) + return ports + + +class WsAuth: + """Origin/Host allowlist plus the per-process session token.""" + + def __init__(self, backend_port: int, token: Optional[str] = None) -> None: + ports = _ui_ports(backend_port) + self.allowed_hosts: Set[str] = { + f"{h}:{p}" for h in _LOOPBACK_HOSTS for p in ports + } + self.allowed_origins: Set[str] = { + f"http://{hp}" for hp in self.allowed_hosts + } + self.token: str = token or secrets.token_hex(32) + + # -- individual checks ------------------------------------------------- + + def host_ok(self, host: Optional[str]) -> bool: + return bool(host) and host.lower() in self.allowed_hosts + + def origin_ok(self, origin: Optional[str]) -> bool: + return bool(origin) and origin.lower().rstrip("/") in self.allowed_origins + + def token_ok(self, presented: Optional[str]) -> bool: + if not presented: + return False + return hmac.compare_digest(presented.encode(), self.token.encode()) + + @staticmethod + def token_from_protocols(header: Optional[str]) -> Optional[str]: + """Pull the token out of a Sec-WebSocket-Protocol header value.""" + for proto in (header or "").split(","): + proto = proto.strip() + if proto.startswith(WS_TOKEN_PROTOCOL_PREFIX): + return proto[len(WS_TOKEN_PROTOCOL_PREFIX):] + return None + + # -- request-level decisions ------------------------------------------ + + def check_ws_handshake(self, headers: Mapping[str, str]) -> Optional[str]: + """Return None if the handshake may proceed, else the rejection reason.""" + if not self.host_ok(headers.get("Host")): + return "host" + if not self.origin_ok(headers.get("Origin")): + return "origin" + if not self.token_ok( + self.token_from_protocols(headers.get("Sec-WebSocket-Protocol")) + ): + return "token" + return None + + def check_token_request(self, headers: Mapping[str, str]) -> Optional[str]: + """Return None if GET /api/session-token may be served, else the reason. + + The main protection is that the response carries no CORS headers, so a + cross-origin page can't read it. These checks add defense in depth: + a rebinding Host, a foreign Origin, or a browser-labelled cross-site + fetch are refused outright. + """ + if not self.host_ok(headers.get("Host")): + return "host" + origin = headers.get("Origin") + if origin is not None and not self.origin_ok(origin): + return "origin" + site = headers.get("Sec-Fetch-Site") + if site is not None and site not in ("same-origin", "none"): + return "fetch-site" + return None + diff --git a/app/ui_layer/browser/frontend/src/store/socket/SocketClient.ts b/app/ui_layer/browser/frontend/src/store/socket/SocketClient.ts index 33865a3b9..19247a567 100644 --- a/app/ui_layer/browser/frontend/src/store/socket/SocketClient.ts +++ b/app/ui_layer/browser/frontend/src/store/socket/SocketClient.ts @@ -48,6 +48,7 @@ export class SocketClient { private readonly outboxTtlMs: number private readonly livenessIntervalMs: number private readonly livenessTimeoutMs: number + private readonly getProtocols?: () => Promise private ws: WebSocket | null = null private connecting = false @@ -78,6 +79,7 @@ export class SocketClient { this.outboxTtlMs = opts.outboxTtlMs ?? 60000 this.livenessIntervalMs = opts.livenessIntervalMs ?? 5000 this.livenessTimeoutMs = opts.livenessTimeoutMs ?? 3000 + this.getProtocols = opts.getProtocols if (typeof window !== 'undefined') { window.addEventListener('online', () => this.reconnectNow()) @@ -108,12 +110,28 @@ export class SocketClient { this.ws = null } + if (!this.getProtocols) { + this.open([]) + return + } + this.getProtocols().then( + protocols => this.open(protocols), + err => { + // Backend down or restarting: retry with the usual backoff. + console.warn('[SocketClient] could not get session token:', err) + this.connecting = false + this.scheduleReconnect() + }, + ) + } + + private open(protocols: string[]): void { const attemptId = newClientId() const url = `${this.url}${this.url.includes('?') ? '&' : '?'}attempt=${attemptId}` let ws: WebSocket try { - ws = new WebSocket(url) + ws = new WebSocket(url, protocols) } catch (err) { console.error('[SocketClient] failed to construct WebSocket:', err) this.connecting = false diff --git a/app/ui_layer/browser/frontend/src/store/socket/socketInstance.ts b/app/ui_layer/browser/frontend/src/store/socket/socketInstance.ts index e0aebb614..4518ab424 100644 --- a/app/ui_layer/browser/frontend/src/store/socket/socketInstance.ts +++ b/app/ui_layer/browser/frontend/src/store/socket/socketInstance.ts @@ -1,5 +1,5 @@ import { SocketClient } from './SocketClient' -import { getWsUrl } from '../../utils/connection' +import { getWsUrl, getWsProtocols } from '../../utils/connection' // Module-level singleton. Created lazily so tests / SSR-ish environments // don't pay for a WebSocket at import time. The middleware calls connect() @@ -9,7 +9,7 @@ let instance: SocketClient | null = null export function getSocketClient(): SocketClient { if (!instance) { - instance = new SocketClient({ url: getWsUrl() }) + instance = new SocketClient({ url: getWsUrl(), getProtocols: getWsProtocols }) } return instance } diff --git a/app/ui_layer/browser/frontend/src/store/socket/types.ts b/app/ui_layer/browser/frontend/src/store/socket/types.ts index 644aed615..7020bbcbe 100644 --- a/app/ui_layer/browser/frontend/src/store/socket/types.ts +++ b/app/ui_layer/browser/frontend/src/store/socket/types.ts @@ -27,4 +27,10 @@ export interface SocketClientOptions { livenessIntervalMs?: number /** How long a ping may go unanswered before the backend counts as busy. */ livenessTimeoutMs?: number + /** + * Subprotocols for each connection attempt, resolved fresh every time (the + * backend's session token rotates on restart). A rejection counts as a + * failed attempt and is retried with backoff. + */ + getProtocols?: () => Promise } diff --git a/app/ui_layer/browser/frontend/src/utils/connection.ts b/app/ui_layer/browser/frontend/src/utils/connection.ts index 4e3c0fb45..62a041dde 100644 --- a/app/ui_layer/browser/frontend/src/utils/connection.ts +++ b/app/ui_layer/browser/frontend/src/utils/connection.ts @@ -19,6 +19,21 @@ export function getWsUrl(): string { return `${protocol}//${window.location.host}/ws` } +/** + * WebSocket subprotocols carrying the backend's per-process session token. + * + * The token comes from a same-origin fetch (proxied to the backend in dev and + * static-server modes); the backend never sends CORS headers for it, so other + * sites can't read it. Fetched on every connect: a restarted backend has a + * new token. + */ +export async function getWsProtocols(): Promise { + const resp = await fetch('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/api/session-token', { cache: 'no-store' }) + if (!resp.ok) throw new Error(`session token request failed: ${resp.status}`) + const { token } = (await resp.json()) as { token: string } + return ['craftbot', `craftbot-auth.${token}`] +} + /** * Resolve the base URL for API requests to the backend. */ diff --git a/tests/test_ws_auth.py b/tests/test_ws_auth.py new file mode 100644 index 000000000..aea353c52 --- /dev/null +++ b/tests/test_ws_auth.py @@ -0,0 +1,193 @@ +"""/ws handshake guard: Origin/Host allowlist + per-session token. + +The integration tests run the real BrowserAdapter handlers (built with +``__new__`` and minimal stubs) behind an aiohttp TestServer. +""" + +import asyncio +import json +from types import SimpleNamespace + +import aiohttp +import pytest +from aiohttp import web +from aiohttp.test_utils import TestServer + +from app.ui_layer.adapters.browser_adapter import BrowserAdapter +from app.ui_layer.adapters.ws_auth import WsAuth + +TOKEN = "a" * 64 + + +@pytest.fixture(autouse=True) +def _frontend_port(monkeypatch): + monkeypatch.setenv("VITE_PORT", "7925") + + +# ---------------------------------------------------------------- unit + + +def test_allowlist_covers_frontend_and_backend_ports(): + auth = WsAuth(7926, token=TOKEN) + for host in ("localhost", "127.0.0.1", "[::1]"): + for port in (7925, 7926): + assert auth.origin_ok(f"http://{host}:{port}") + assert auth.host_ok(f"{host}:{port}") + assert not auth.origin_ok("https://evil.example") + assert not auth.origin_ok("http://localhost:3000") # e.g. an Agent App + assert not auth.origin_ok("http://192.168.1.50:7925") + assert not auth.origin_ok("null") + assert not auth.origin_ok(None) + assert not auth.host_ok("evil.example:7926") # DNS rebinding + + +def test_token_compare(): + auth = WsAuth(7926, token=TOKEN) + assert auth.token_ok(TOKEN) + assert not auth.token_ok("b" * 64) + assert not auth.token_ok("") + assert not auth.token_ok(None) + assert WsAuth(7926).token != WsAuth(7926).token # random per process + + +def test_token_from_protocols(): + assert WsAuth.token_from_protocols(f"craftbot, craftbot-auth.{TOKEN}") == TOKEN + assert WsAuth.token_from_protocols("craftbot") is None + assert WsAuth.token_from_protocols(None) is None + + +def test_token_request_checks(): + auth = WsAuth(7926, token=TOKEN) + ok = {"Host": "localhost:7925"} + assert auth.check_token_request(ok) is None + assert auth.check_token_request({**ok, "Sec-Fetch-Site": "same-origin"}) is None + assert auth.check_token_request({**ok, "Sec-Fetch-Site": "cross-site"}) == "fetch-site" + assert auth.check_token_request({**ok, "Sec-Fetch-Site": "same-site"}) == "fetch-site" + assert auth.check_token_request({**ok, "Origin": "https://evil.example"}) == "origin" + assert auth.check_token_request({"Host": "evil.example:7926"}) == "host" + + +# --------------------------------------------------------- integration + + +def _stub_adapter() -> BrowserAdapter: + adapter = BrowserAdapter.__new__(BrowserAdapter) + # Not the first client, so the soft-onboarding hook (needs a workspace) is skipped. + adapter._ws_clients = {object()} + adapter._channels = {} + adapter._metrics_subscribers = set() + adapter._lane_locks = {} + adapter._lane_tasks = set() + adapter._started_at = 0.0 + adapter._ws_prepare_failures = 0 + adapter._get_initial_state = lambda: {"stub": True} + adapter._get_skill_meta = lambda: {} + adapter._agent_app_manager = SimpleNamespace(list_projects=lambda: []) + return adapter + + +async def _with_server(fn): + adapter = _stub_adapter() + app = web.Application() + app.router.add_get("/ws", adapter._websocket_handler) + app.router.add_get("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/api/session-token", adapter._session_token_handler) + server = TestServer(app, host="127.0.0.1") + await server.start_server() + adapter._ws_auth = WsAuth(server.port, token=TOKEN) + try: + async with aiohttp.ClientSession() as session: + return await fn(session, server.port) + finally: + await server.close() + + +async def _handshake(session, port, origin, protocols=()): + headers = {"Origin": origin} if origin else {} + try: + async with session.ws_connect( + f"http://127.0.0.1:{port}/ws", headers=headers, protocols=protocols + ) as ws: + first = json.loads((await asyncio.wait_for(ws.receive(), 5)).data) + return ("open", first["type"], ws.protocol) + except aiohttp.WSServerHandshakeError as e: + return ("rejected", e.status, None) + + +def _run(fn): + return asyncio.run(_with_server(fn)) + + +def test_foreign_origin_rejected_even_with_token(): + async def go(s, port): + return await _handshake( + s, port, "https://evil.example", ("craftbot", f"craftbot-auth.{TOKEN}") + ) + + assert _run(go) == ("rejected", 403, None) + + +def test_missing_origin_rejected(): + async def go(s, port): + return await _handshake(s, port, None, ("craftbot", f"craftbot-auth.{TOKEN}")) + + assert _run(go) == ("rejected", 403, None) + + +def test_missing_token_rejected(): + async def go(s, port): + return await _handshake(s, port, f"http://127.0.0.1:{port}", ("craftbot",)) + + assert _run(go) == ("rejected", 403, None) + + +def test_wrong_token_rejected(): + async def go(s, port): + return await _handshake( + s, port, f"http://127.0.0.1:{port}", ("craftbot", "craftbot-auth." + "b" * 64) + ) + + assert _run(go) == ("rejected", 403, None) + + +def test_correct_origin_and_token_accepted(): + async def go(s, port): + # Frontend-port origin (Vite / static server) and backend-port origin. + return [ + await _handshake( + s, port, origin, ("craftbot", f"craftbot-auth.{TOKEN}") + ) + for origin in ("http://localhost:7925", f"http://127.0.0.1:{port}") + ] + + assert _run(go) == [("open", "init", "craftbot")] * 2 + + +def test_token_endpoint_same_origin_only(): + async def go(s, port): + url = f"http://127.0.0.1:{port}/api/session-token" + async with s.get(url, headers={"Sec-Fetch-Site": "same-origin"}) as r: + ok = (r.status, await r.json(), r.headers.get("Access-Control-Allow-Origin")) + async with s.get(url, headers={"Origin": "https://evil.example"}) as r: + foreign = r.status + async with s.get(url, headers={"Sec-Fetch-Site": "cross-site"}) as r: + cross = r.status + async with s.get(url, headers={"Host": "evil.example"}) as r: + rebind = r.status + return ok, foreign, cross, rebind + + ok, foreign, cross, rebind = _run(go) + assert ok == (200, {"token": TOKEN}, None) + assert (foreign, cross, rebind) == (403, 403, 403) + + +def test_fetched_token_opens_socket(): + """The flow the UI runs: fetch token, then connect with it.""" + + async def go(s, port): + async with s.get(f"http://127.0.0.1:{port}/api/session-token") as r: + token = (await r.json())["token"] + return await _handshake( + s, port, "http://localhost:7925", ("craftbot", f"craftbot-auth.{token}") + ) + + assert _run(go) == ("open", "init", "craftbot") From 309105ce698b4fa4acc6668c012d5fe8dbff40ce Mon Sep 17 00:00:00 2001 From: ahmad-ajmal Date: Tue, 22 Sep 2026 08:11:23 +0100 Subject: [PATCH 2/2] http auth issue --- app/ui_layer/adapters/browser_adapter.py | 41 +++++- app/ui_layer/adapters/ws_auth.py | 29 ++++- run.py | 15 ++- tests/test_ws_auth.py | 152 +++++++++++++++++++++++ 4 files changed, 228 insertions(+), 9 deletions(-) diff --git a/app/ui_layer/adapters/browser_adapter.py b/app/ui_layer/adapters/browser_adapter.py index 35f7cba9c..c872fec77 100644 --- a/app/ui_layer/adapters/browser_adapter.py +++ b/app/ui_layer/adapters/browser_adapter.py @@ -1040,7 +1040,7 @@ async def _on_start(self) -> None: ) self._chat._messages.insert(0, welcome_message) - self._app = web.Application() + self._app = web.Application(middlewares=[self._api_guard()]) # API and WebSocket routes (must be registered first) self._app.router.add_get("/ws", self._websocket_handler) @@ -3702,9 +3702,14 @@ async def _chat_attachment_upload_handler( from aiohttp import web try: - name = ( - request.rel_url.query.get("name", "attachment").strip() or "attachment" - ) + # A display name, never a path: keep only the final component, or + # "../../x" walks out of download/ (Windows collapses ".." without + # the intermediate directory existing). + name = Path( + request.rel_url.query.get("name", "").strip().replace("\\", "/") + ).name + if name in ("", ".", ".."): + name = "attachment" file_type = ( request.rel_url.query.get("type", "application/octet-stream").strip() or "application/octet-stream" @@ -8576,8 +8581,10 @@ def _validate_path(self, file_path: str) -> Path: target = workspace / file_path target = target.resolve() - # Security check - ensure path is within workspace - if not str(target).startswith(str(workspace)): + # Security check - ensure path is within workspace. Containment by + # path components, never by string prefix: "../workspace_x/f" resolves + # to a SIBLING whose string starts with the workspace's. + if not target.is_relative_to(workspace): raise ValueError(f"Path '{file_path}' is outside workspace") return target @@ -9754,6 +9761,28 @@ async def _index_handler(self, request: "web.Request") -> "web.Response": html = self._get_index_html() return web.Response(text=html, content_type="text/html") + def _api_guard(self): + """Middleware: /api/* refuses rebinding Hosts and foreign-origin writes + (ws_auth.check_api_request). /ws guards its own handshake.""" + from aiohttp import web + + @web.middleware + async def api_guard(request, handler): + if request.path == "/api" or request.path.startswith("/api/"): + rejected = self._ws_auth.check_api_request( + request.method, request.headers + ) + if rejected: + logger.warning( + f"[BROWSER ADAPTER] Refused {request.method} {request.path} " + f"({rejected}): origin={request.headers.get('Origin')!r} " + f"host={request.headers.get('Host')!r}" + ) + raise web.HTTPForbidden(reason="Request not allowed") + return await handler(request) + + return api_guard + async def _session_token_handler(self, request: "web.Request") -> "web.Response": """Hand the /ws session token to CraftBot's own UI. diff --git a/app/ui_layer/adapters/ws_auth.py b/app/ui_layer/adapters/ws_auth.py index e9e93d097..bb400445b 100644 --- a/app/ui_layer/adapters/ws_auth.py +++ b/app/ui_layer/adapters/ws_auth.py @@ -1,4 +1,4 @@ -"""Handshake guard for the browser UI's /ws WebSocket. +"""Request guards for the browser UI backend: the /ws handshake and /api/*. Browsers apply no CORS to WebSocket handshakes, so without a check any page the user opens could drive the agent through /ws. Two independent checks: @@ -10,6 +10,10 @@ page can't read the response) and sends it in Sec-WebSocket-Protocol, which keeps it out of URLs and access logs. +/api/* gets the Host check plus an Origin check on writes (check_api_request): +the same cross-site threat over plain HTTP, where multipart/text-plain POSTs +need no preflight. + Legitimate UI origins: the frontend port (Vite dev server, or run.py's static server) and the backend port (aiohttp serving the built UI itself), each as localhost / 127.0.0.1 / [::1]. Tunnels and LAN URLs only ever expose Agent App @@ -30,6 +34,7 @@ _LOOPBACK_HOSTS = ("localhost", "127.0.0.1", "[::1]") _DEFAULT_FRONTEND_PORT = 7925 +_MUTATING = frozenset({"POST", "PUT", "PATCH", "DELETE"}) def _ui_ports(backend_port: int) -> Set[int]: @@ -90,6 +95,28 @@ def check_ws_handshake(self, headers: Mapping[str, str]) -> Optional[str]: return "token" return None + def check_api_request( + self, method: str, headers: Mapping[str, str] + ) -> Optional[str]: + """Return None if an /api/* request may proceed, else the reason. + + The same threat as /ws, over plain HTTP: a multipart or text/plain + POST is a CORS "simple" request, so any page can SEND one with no + preflight — the side effect happens even though the response is + unreadable. So: every request must be addressed by a loopback Host + (defeats DNS rebinding, which would also make responses readable), + and a write carrying a foreign Origin is refused. Browsers always + send Origin on a cross-origin write; no Origin means a non-browser + caller (the agent-app bridge, scripts), which CSRF cannot forge. + """ + if not self.host_ok(headers.get("Host")): + return "host" + if method.upper() in _MUTATING: + origin = headers.get("Origin") + if origin is not None and not self.origin_ok(origin): + return "origin" + return None + def check_token_request(self, headers: Mapping[str, str]) -> Optional[str]: """Return None if GET /api/session-token may be served, else the reason. diff --git a/run.py b/run.py index 9e72cbaf8..6e5596275 100644 --- a/run.py +++ b/run.py @@ -474,8 +474,19 @@ def _proxy_request(self): # Build proxy request req = urllib.request.Request(target_url, data=body, method=self.command) - # Forward relevant headers - for header in ("Content-Type", "Authorization", "Accept"): + # Forward relevant headers. Host, Origin and Sec-Fetch-Site are + # what the backend's /api guard judges (ws_auth.py): dropping + # them would turn this proxy into a way around it — a + # cross-site upload would arrive Origin-less, and a + # DNS-rebinding Host would arrive as localhost. + for header in ( + "Content-Type", + "Authorization", + "Accept", + "Host", + "Origin", + "Sec-Fetch-Site", + ): if self.headers.get(header): req.add_header(header, self.headers[header]) diff --git a/tests/test_ws_auth.py b/tests/test_ws_auth.py index aea353c52..fda2cde3b 100644 --- a/tests/test_ws_auth.py +++ b/tests/test_ws_auth.py @@ -191,3 +191,155 @@ async def go(s, port): ) assert _run(go) == ("open", "init", "craftbot") + + +# ------------------------------------------------ /api/* over plain HTTP +# +# The same cross-site threat as /ws: a multipart POST is a CORS "simple" +# request, so any page can send one without a preflight and the write +# happens even though the response is unreadable. + + +def test_api_request_checks(): + auth = WsAuth(7926, token=TOKEN) + ui = {"Host": "localhost:7925", "Origin": "http://localhost:7925"} + assert auth.check_api_request("POST", ui) is None + assert auth.check_api_request("POST", {"Host": "localhost:7926"}) is None, ( + "no Origin = non-browser caller (the agent-app bridge)" + ) + for method in ("POST", "PUT", "PATCH", "DELETE"): + assert auth.check_api_request( + method, {**ui, "Origin": "https://evil.example"} + ) == "origin", method + assert auth.check_api_request("POST", {**ui, "Origin": "null"}) == "origin" + assert auth.check_api_request("POST", {**ui, "Origin": "http://127.0.0.1:3100"}) == ( + "origin" + ), "an Agent App page is not the CraftBot UI" + # reads: Origin is the browser's business (no CORS grant), Host is ours + assert auth.check_api_request("GET", {**ui, "Origin": "https://evil.example"}) is None + assert auth.check_api_request("GET", {"Host": "evil.example:7926"}) == "host" + assert auth.check_api_request("POST", {"Host": "evil.example:7925"}) == "host" + + +async def _with_upload_server(fn, workspace): + import app.ui_layer.adapters.browser_adapter as ba + + adapter = _stub_adapter() + broadcasts = [] + + async def _broadcast(msg): + broadcasts.append(msg) + + adapter._broadcast = _broadcast + app = web.Application(middlewares=[adapter._api_guard()]) + app.router.add_post("/api/workspace/upload", adapter._workspace_upload_handler) + app.router.add_post( + "/api/chat-attachments/upload", adapter._chat_attachment_upload_handler + ) + app.router.add_get("/api/workspace/{path:.*}", adapter._workspace_file_handler) + server = TestServer(app, host="127.0.0.1") + await server.start_server() + adapter._ws_auth = WsAuth(server.port, token=TOKEN) + old_root = ba.AGENT_WORKSPACE_ROOT + ba.AGENT_WORKSPACE_ROOT = str(workspace) + try: + async with aiohttp.ClientSession() as session: + return await fn(session, server.port) + finally: + ba.AGENT_WORKSPACE_ROOT = old_root + await server.close() + + +def _upload_form(payload=b"pwned"): + form = aiohttp.FormData() + form.add_field("file", payload, filename="f.txt") + return form + + +def test_upload_foreign_origin_refused(tmp_path): + ws = tmp_path / "workspace" + ws.mkdir() + + async def go(s, port): + results = {} + for label, headers in ( + ("evil", {"Origin": "https://evil.example"}), + ("rebind", {"Host": "evil.example:7926"}), + ("ui", {"Origin": f"http://localhost:{port}"}), + ): + async with s.post( + f"http://127.0.0.1:{port}/api/workspace/upload?path={label}.txt", + data=_upload_form(), + headers=headers, + ) as r: + results[label] = r.status + return results + + assert asyncio.run(_with_upload_server(go, ws)) == { + "evil": 403, + "rebind": 403, + "ui": 200, + } + assert sorted(p.name for p in ws.iterdir()) == ["ui.txt"], "a refused upload wrote" + + +def test_upload_cannot_escape_workspace(tmp_path): + """`startswith` passed "../workspace_x/..." (a sibling sharing the prefix), + and mkdir(parents=True) then created the escape directory itself.""" + ws = tmp_path / "workspace" + ws.mkdir() + (ws / "keep.txt").write_text("original") + + async def go(s, port): + statuses = [] + for path in ( + "../workspace_x/payload", + "../workspace/../outside.txt", + "../../outside.txt", + "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/../outside.txt", + ): + async with s.post( + f"http://127.0.0.1:{port}/api/workspace/upload", + params={"path": path}, + data=_upload_form(), + headers={"Origin": f"http://localhost:{port}"}, + ) as r: + statuses.append(r.status) + async with s.post( + f"http://127.0.0.1:{port}/api/workspace/upload", + params={"path": "sub/dir/ok.txt"}, + data=_upload_form(b"fine"), + headers={"Origin": f"http://localhost:{port}"}, + ) as r: + statuses.append(r.status) + return statuses + + assert asyncio.run(_with_upload_server(go, ws)) == [400, 400, 400, 400, 200] + assert not (tmp_path / "workspace_x").exists(), "escape dir was created" + assert not (tmp_path / "outside.txt").exists() + assert (ws / "sub" / "dir" / "ok.txt").read_bytes() == b"fine" + assert (ws / "keep.txt").read_text() == "original" + + +def test_attachment_name_is_not_a_path(tmp_path): + ws = tmp_path / "workspace" + ws.mkdir() + + async def go(s, port): + names = [] + for name in ("../../../escaped.txt", r"..\..\escaped2.txt", "..", "ok.txt"): + async with s.post( + f"http://127.0.0.1:{port}/api/chat-attachments/upload", + params={"name": name}, + data=_upload_form(), + headers={"Origin": f"http://localhost:{port}"}, + ) as r: + assert r.status == 200, (name, r.status) + names.append((await r.json())["name"]) + return names + + names = asyncio.run(_with_upload_server(go, ws)) + assert names == ["escaped.txt", "escaped2.txt", "attachment", "ok.txt"] + written = sorted(p.name.split("_", 1)[1] for p in (ws / "download").iterdir()) + assert written == ["attachment", "escaped.txt", "escaped2.txt", "ok.txt"] + assert not list(tmp_path.glob("escaped*")), "attachment escaped the workspace"