diff --git a/src/octop/api/routers/workspace.py b/src/octop/api/routers/workspace.py index aa1f482adc..63af9e60f6 100644 --- a/src/octop/api/routers/workspace.py +++ b/src/octop/api/routers/workspace.py @@ -4,6 +4,7 @@ import logging import re +import urllib.parse from pathlib import Path from typing import Any, Literal @@ -155,6 +156,24 @@ def _workspace_io_path(path: str, *, from_workspace: bool = False) -> str: """ raw = path.strip() if raw.startswith("file://"): + # A file:// URL must not carry a host. `urlparse` puts the drive letter + # in netloc for the two-slash form Windows clients send + # (`file://C:/…` from `Path.as_posix()`), and that is the only netloc + # we accept. Anything else is rejected rather than resolved: + # + # - `file://server/share/x` (UNC) would otherwise silently resolve to + # the local path `/share/x`. + # - `file://C:\Users\x` (backslashes) puts the whole path in netloc + # and used to resolve to `.`, the server process's cwd. + # + # Resolving either and then testing containment depends on the host + # filesystem layout. Denying here does not. + netloc = urllib.parse.urlparse(raw).netloc + is_drive = len(netloc) == 2 and netloc[1] == ":" and netloc[0].isalpha() + if netloc and not is_drive: + raise OctopError( + ErrorCode.FORBIDDEN, f"cannot access {path!r}: file:// URL must not name a host" + ) resolved = backend_workspace_path(raw) if resolved is None: raise OctopError(ErrorCode.NOT_FOUND, f"cannot resolve {path!r}") @@ -222,7 +241,13 @@ async def read_file( """ ws = await require_agent_workspace(agent_id, user=user, as_user=as_user, server=server) io_path = _safe_io_path(ws, path, from_workspace=False) - content = await ws.aread_text(io_path) + try: + content = await ws.aread_text(io_path) + except PermissionError as exc: + # The backend's own containment check can still deny a path that + # passed _safe_io_path (e.g. ``C:/Users/...`` on Windows). Map it like + # download_file/read_doc instead of letting it surface as a 500. + raise OctopError(ErrorCode.NOT_FOUND, f"cannot read {path!r}") from exc if content is None: raise OctopError(ErrorCode.NOT_FOUND, f"cannot read {path!r}") return {"path": path, "content": coerce_read_content(content)} diff --git a/tests/integration/test_workspace_api.py b/tests/integration/test_workspace_api.py index 3bf5d147e1..0b02136d42 100644 --- a/tests/integration/test_workspace_api.py +++ b/tests/integration/test_workspace_api.py @@ -582,3 +582,54 @@ async def test_from_workspace_param_is_rejected_as_unknown(env: Any) -> None: ) # Workspace-relative 'etc/passwd' simply does not exist → 404, never 200. assert r.status_code == 404, r.text + + +@pytest.mark.asyncio +async def test_file_url_with_host_netloc_forbidden(env: Any) -> None: + """A file:// URL must not name a host - SEC-2, variant of the netloc bug. + + urlparse puts everything between // and the next / into netloc. Only a bare + drive letter (file://C:/..., which Windows clients emit from + Path.as_posix()) is meaningful here. A UNC host used to resolve to the local + path /share/x, and a backslash path used to resolve to `.`, the process cwd. + Both now fail closed with 403 instead of being resolved and then tested + against a host-layout-dependent allowlist. + """ + c, _srv, auth, aid = env + hostiles = [ + "file://server/share/etc/passwd", + "file://attacker.example/share/x", + "file://C:" + chr(92) + "Users" + chr(92) + "a" + chr(92) + "x.txt", + ] + for hostile in hostiles: + r = await c.get( + f"/api/agents/{aid}/workspace/file", + params={"path": hostile}, + headers=auth, + ) + assert r.status_code == 403, (hostile, r.text) + + +@pytest.mark.asyncio +async def test_file_url_bare_drive_netloc_is_not_rejected_as_host(env: Any) -> None: + """The two-slash Windows form must keep working - SEC-2 regression guard. + + file://C:/... is the shape Windows CI builds. It must resolve to a real + absolute path: not rejected as a host, and not collapsed to `.`. + """ + c, _srv, auth, aid = env + # Built literally: Path.as_posix() only produces a drive letter on Windows, + # so on Linux CI this would silently degrade to the three-slash form and + # stop testing the shape we care about. + drive_url = "file://C:/Users/nobody/sec2-drive-shape-must-not-exist.txt" + r = await c.get( + f"/api/agents/{aid}/workspace/file", + params={"path": drive_url}, + headers=auth, + ) + # A bare drive letter is the one netloc we accept, so the host rejection + # must not fire on the shape Windows actually sends. Assert on the reason, + # not the status: on Windows the path is host-absolute and the containment + # layer still denies it (404); on Linux it is workspace-relative (404). + assert "must not name a host" not in r.text, (drive_url, r.text) + assert r.status_code in (403, 404), (drive_url, r.status_code, r.text)