From 6e487db569377191a8dc166f43397c57c07150c4 Mon Sep 17 00:00:00 2001 From: Hasan Ozdemir Date: Sun, 26 Jul 2026 03:14:06 -0700 Subject: [PATCH] Confine browser_upload_file sources to granted session roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit browser_upload_file resolved a caller-supplied path and uploaded it with no session-root check. The tool is kind=write → EXTERNAL risk, and Mode.AUTO returns full access with no path scoping — so a model-controlled path could exfiltrate ~/.config/coworker/secrets.json (or an SSH key) through a page file input. Thread the session roots into make_browser_automation_tools (same seam the email tools and the open browser_screenshot fix use) and require the upload source to resolve under a granted root before Playwright opens. Any granted root is fine — this is a read/exfil boundary, not a write. Relative paths resolve against the primary root. Hermetic tests assert the refusal paths without Playwright: absolute escape, ../ traversal, no-roots, and that a path inside a read-only granted root still passes confinement. --- coworker/connectors/browser_automation.py | 36 ++++++++++-- coworker/connectors/integration_tools.py | 4 +- tests/test_browser_upload_confine.py | 72 +++++++++++++++++++++++ 3 files changed, 107 insertions(+), 5 deletions(-) create mode 100644 tests/test_browser_upload_confine.py diff --git a/coworker/connectors/browser_automation.py b/coworker/connectors/browser_automation.py index e1e22e0c..750a7643 100644 --- a/coworker/connectors/browser_automation.py +++ b/coworker/connectors/browser_automation.py @@ -324,9 +324,31 @@ def _snapshot(page, max_chars: int) -> dict[str, Any]: } -def make_browser_automation_tools() -> list[Callable[..., Any]]: +def make_browser_automation_tools( + roots: Optional[list[Any]] = None, +) -> list[Callable[..., Any]]: tools: list[Callable[..., Any]] = [] + def _confine_upload_path(raw: str) -> tuple[Optional[Path], Optional[dict]]: + """Resolve a caller-supplied upload source inside a granted session root. + + browser_upload_file is kind=write → EXTERNAL, and Mode.AUTO returns full access + with no path check — so without this, a model-controlled `path` can exfiltrate + anything readable (secrets store, SSH keys) through a page file input. Any + granted root is fine (this is a read/exfil boundary, not a write). Mirrors + email outgoing-attachment confinement and the sibling browser_screenshot fix. + """ + allowed = [Path(r.path) for r in (roots or [])] + if not allowed: + return None, {"error": "no granted session directory for the upload"} + p = Path(raw).expanduser() + cand = (p if p.is_absolute() else allowed[0] / p).resolve() + if not any(cand.is_relative_to(root.resolve()) for root in allowed): + return None, { + "error": f"{cand} is outside the session's granted directories" + } + return cand, None + def browser_open_url( url: str, wait_until: str = "domcontentloaded" ) -> dict[str, Any]: @@ -476,8 +498,13 @@ def browser_select(target: str, value: str) -> dict[str, Any]: ) def browser_upload_file(target: str, path: str) -> dict[str, Any]: - file_path = Path(path).expanduser().resolve() - if not file_path.exists(): + # Confine BEFORE opening the browser (fail fast; hermetic tests assert without + # Playwright). Relative paths resolve against the primary granted root. + file_path, err = _confine_upload_path(path) + if err: + return err + assert file_path is not None + if not file_path.is_file(): return {"error": f"file not found: {file_path}"} return _BROWSER.call( "upload_file", @@ -495,7 +522,8 @@ def browser_upload_file(target: str, path: str) -> dict[str, Any]: browser_upload_file, _schema( "browser_upload_file", - "Upload a local file through a file input. Requires approval.", + "Upload a local file through a file input. The path must be inside a " + "granted session directory. Requires approval.", {"target": {"type": "string"}, "path": {"type": "string"}}, ["target", "path"], ), diff --git a/coworker/connectors/integration_tools.py b/coworker/connectors/integration_tools.py index 7588136d..273fe00e 100644 --- a/coworker/connectors/integration_tools.py +++ b/coworker/connectors/integration_tools.py @@ -525,7 +525,9 @@ def make_integration_tools( enabled_tools: Optional[set[str]] = None, roots: Optional[list[Any]] = None, ) -> list[Callable[..., Any]]: - tools: list[Callable[..., Any]] = make_browser_automation_tools() + # Browser tools need the session roots: an upload's source path must resolve inside + # a granted directory (AUTO mode has no path check for EXTERNAL tools). + tools: list[Callable[..., Any]] = make_browser_automation_tools(roots=roots) # Email needs the session roots: attachment downloads land in the primary scratch # and outgoing attachments must resolve inside a granted directory. tools.extend(make_email_tools(secrets, roots=roots)) diff --git a/tests/test_browser_upload_confine.py b/tests/test_browser_upload_confine.py new file mode 100644 index 00000000..41b68307 --- /dev/null +++ b/tests/test_browser_upload_confine.py @@ -0,0 +1,72 @@ +"""browser_upload_file must confine the source path to a granted session root. + +The tool is kind=write → EXTERNAL risk, and Mode.AUTO returns "full access" with no +path check — so a model-controlled path could upload ~/.config/coworker/secrets.json +(or an SSH key) through a page file input. Confinement runs before the browser opens, +so these cases assert without Playwright. +""" + +from __future__ import annotations + +from coworker.connectors.browser_automation import make_browser_automation_tools +from coworker.roots import RootDir + + +def _upload_tool(roots): + tools = make_browser_automation_tools(roots=roots) + return next(t for t in tools if t.__name__ == "browser_upload_file") + + +def test_upload_rejects_path_outside_granted_roots(tmp_path): + workspace = tmp_path / "scratch" + workspace.mkdir() + secrets = tmp_path / "secrets.json" + secrets.write_text('{"api_key": "sk-secret"}') + + tool = _upload_tool([RootDir(path=workspace, writable=True)]) + res = tool(target="#file", path=str(secrets)) + assert "error" in res and "outside" in res["error"] + assert secrets.read_text() == '{"api_key": "sk-secret"}' # never read for upload + + +def test_upload_rejects_traversal_escape(tmp_path): + workspace = tmp_path / "scratch" + workspace.mkdir() + outside = tmp_path / "secret.txt" + outside.write_text("nope") + tool = _upload_tool([RootDir(path=workspace, writable=True)]) + res = tool(target="#file", path="../secret.txt") + assert "error" in res and "outside" in res["error"] + + +def test_upload_rejects_when_no_roots(tmp_path): + f = tmp_path / "doc.txt" + f.write_text("x") + tool = _upload_tool([]) + res = tool(target="#file", path=str(f)) + assert "error" in res and "no granted" in res["error"] + + +def test_upload_allows_readable_root_not_only_writable(tmp_path): + """Upload is a read/exfil boundary — a read-only granted root is still fine.""" + ro = tmp_path / "ro" + ro.mkdir() + doc = ro / "doc.txt" + doc.write_text("ok") + tool = _upload_tool([RootDir(path=ro, writable=False)]) + res = tool(target="#file", path=str(doc)) + # Confinement passed; Playwright isn't installed here, so we get a browser setup + # error — not an "outside" / "no granted" rejection. + assert "outside" not in str(res.get("error", "")) + assert "no granted" not in str(res.get("error", "")) + + +def test_upload_accepts_path_inside_writable_root_then_reaches_browser(tmp_path): + workspace = tmp_path / "scratch" + workspace.mkdir() + doc = workspace / "doc.txt" + doc.write_text("ok") + tool = _upload_tool([RootDir(path=workspace, writable=True)]) + res = tool(target="#file", path=str(doc)) + assert "outside" not in str(res.get("error", "")) + assert "no granted" not in str(res.get("error", ""))