From cd85b1eaf80cb026f057236e3f99f0779321b433 Mon Sep 17 00:00:00 2001 From: Mr-Neutr0n <64578610+Mr-Neutr0n@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:07:45 +0530 Subject: [PATCH] security: confine the browser connector's file tools to the session roots The permission engine path-scopes only the built-in WRITE_TOOLS, and only by reading arguments["path"] (PermissionEngine.evaluate). A connector tool that touches the local filesystem is therefore invisible to it. Two of the browser tools do, in both directions: browser_upload_file(target, path) Path(path).expanduser().resolve() with no containment, then hands the bytes to whatever page is loaded. Any readable file on the machine can be posted to a remote site: ~/.ssh/id_rsa, ~/.aws/credentials, or the OpenWorker secret store itself. This is issue #189. browser_screenshot(path) the same unconfined resolve, then out.parent.mkdir(parents=True) and a write. That is an arbitrary file overwrite plus directory creation anywhere the process can reach. Not previously reported. Both are approval-gated in Interactive mode, so this is not a silent compromise there. It is still the wrong boundary: in Auto mode there is no prompt at all, and even with a prompt the tools reach files the user never granted the session, which is exactly what the roots list exists to prevent. The fix follows the confinement email_tools already applies to outgoing attachments (email_tools.py:640): resolve, then require the result to sit inside a granted root. Reads accept any root; the screenshot write requires a writable one. make_integration_tools already receives roots for precisely this reason - browser tools simply were not passed them. Containment is now decided before the browser layer is touched, so a refused screenshot no longer creates its parent directories on the way to failing, and a refused upload does not stat the path (the error cannot be used to test for the existence of ungranted files). Behaviour is unchanged for paths inside the session, and the default screenshot location (the temp file) is untouched. Tests: tests/test_browser_path_confinement.py - traversal, symlink escape, read-only root rejected for writes, no-roots denies, and refusal ordering. --- coworker/connectors/browser_automation.py | 61 ++++++++-- coworker/connectors/integration_tools.py | 2 +- tests/test_browser_path_confinement.py | 134 ++++++++++++++++++++++ 3 files changed, 187 insertions(+), 10 deletions(-) create mode 100644 tests/test_browser_path_confinement.py diff --git a/coworker/connectors/browser_automation.py b/coworker/connectors/browser_automation.py index e1e22e0c..f3477180 100644 --- a/coworker/connectors/browser_automation.py +++ b/coworker/connectors/browser_automation.py @@ -17,6 +17,42 @@ import aisuite as ai +from ..roots import RootDir + + +def _resolve_in_roots( + path: str, roots: Optional[list[RootDir]], *, need_write: bool +) -> tuple[Optional[Path], Optional[dict[str, Any]]]: + """Resolve a local path and require it to sit inside a granted session root. + + The permission engine only path-scopes the built-in ``WRITE_TOOLS`` by inspecting + ``arguments["path"]``, so a connector tool that touches the filesystem is invisible to + it. These two do, in both directions: ``browser_upload_file`` reads a file and hands it + to whatever page is loaded, and ``browser_screenshot`` writes one. Without this check + they reach anything the OS lets the process reach — ``~/.ssh/id_rsa``, the secret store + itself — regardless of which folders the user actually granted the session. + + Mirrors the confinement ``email_tools`` already applies to outgoing attachments. + Returns ``(resolved_path, None)`` on success or ``(None, error_dict)`` for the tool to + return verbatim. + """ + candidates = [r for r in (roots or []) if r.writable or not need_write] + if not candidates: + return None, { + "error": ( + "no writable session directory is available" + if need_write + else "this session has no granted directories" + ) + } + resolved = Path(str(path)).expanduser().resolve() + if not any(resolved.is_relative_to(r.path) for r in candidates): + verb = "writable " if need_write else "" + return None, { + "error": f"{path} is outside the session's {verb}directories" + } + return resolved, None + def _meta( name: str, *, approval: bool = False, capabilities: Optional[list[str]] = None @@ -324,7 +360,9 @@ 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[RootDir]] = None, +) -> list[Callable[..., Any]]: tools: list[Callable[..., Any]] = [] def browser_open_url( @@ -476,9 +514,11 @@ 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() + file_path, err = _resolve_in_roots(path, roots, need_write=False) + if err: + return err if not file_path.exists(): - return {"error": f"file not found: {file_path}"} + return {"error": f"file not found: {path}"} return _BROWSER.call( "upload_file", lambda page: ( @@ -530,13 +570,16 @@ def run(page): ) def browser_screenshot(path: str = "") -> dict[str, Any]: - def run(page): + if path: + out, err = _resolve_in_roots(path, roots, need_write=True) + if err: + return err + else: out = ( - Path(path).expanduser() - if path - else Path(tempfile.gettempdir()) / "coworker-browser-screenshot.png" - ) - out = out.resolve() + Path(tempfile.gettempdir()) / "coworker-browser-screenshot.png" + ).resolve() + + def run(page): out.parent.mkdir(parents=True, exist_ok=True) page.screenshot(path=str(out), full_page=True) return {"ok": True, "path": str(out), "url": page.url} diff --git a/coworker/connectors/integration_tools.py b/coworker/connectors/integration_tools.py index 7588136d..f0ff8556 100644 --- a/coworker/connectors/integration_tools.py +++ b/coworker/connectors/integration_tools.py @@ -525,7 +525,7 @@ 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() + 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_path_confinement.py b/tests/test_browser_path_confinement.py new file mode 100644 index 00000000..5fbc1e97 --- /dev/null +++ b/tests/test_browser_path_confinement.py @@ -0,0 +1,134 @@ +"""The browser connector's two filesystem tools must stay inside the session's roots. + +The permission engine path-scopes only the built-in ``WRITE_TOOLS``, and only by reading +``arguments["path"]`` (``permissions.PermissionEngine.evaluate``). A connector tool that +touches local files is therefore invisible to it, in both directions: + + browser_upload_file reads any readable path and hands it to the loaded page + browser_screenshot writes a PNG to any writable path, creating parent dirs + +``email_tools`` already confines outgoing attachments to the granted roots; these tests pin +the same rule for the browser tools. +""" + +from pathlib import Path + +import pytest + +from coworker.connectors.browser_automation import ( + _resolve_in_roots, + make_browser_automation_tools, +) +from coworker.roots import RootDir + + +def _tools(roots): + return {t.__name__: t for t in make_browser_automation_tools(roots=roots)} + + +@pytest.fixture +def session(tmp_path): + scratch = tmp_path / "scratch" + readonly = tmp_path / "readonly" + outside = tmp_path / "outside" + for d in (scratch, readonly, outside): + d.mkdir() + (readonly / "notes.txt").write_text("granted, read-only") + (outside / "id_rsa").write_text("PRIVATE KEY") + return { + "roots": [RootDir(path=scratch, writable=True), + RootDir(path=readonly, writable=False)], + "scratch": scratch, + "readonly": readonly, + "outside": outside, + } + + +# -- the helper --------------------------------------------------------------- + +def test_read_is_allowed_from_any_granted_root(session): + resolved, err = _resolve_in_roots( + str(session["readonly"] / "notes.txt"), session["roots"], need_write=False) + assert err is None + assert resolved == (session["readonly"] / "notes.txt").resolve() + + +def test_write_is_refused_into_a_read_only_root(session): + resolved, err = _resolve_in_roots( + str(session["readonly"] / "shot.png"), session["roots"], need_write=True) + assert resolved is None + assert "outside the session's writable directories" in err["error"] + + +def test_ungranted_path_is_refused(session): + _, err = _resolve_in_roots( + str(session["outside"] / "id_rsa"), session["roots"], need_write=False) + assert err and "outside the session's directories" in err["error"] + + +def test_traversal_out_of_a_root_is_refused(session): + escape = str(session["scratch"] / ".." / "outside" / "id_rsa") + _, err = _resolve_in_roots(escape, session["roots"], need_write=False) + assert err, "`..` must be resolved before the containment check, not after" + + +def test_symlink_pointing_out_of_a_root_is_refused(session): + link = session["scratch"] / "innocent.txt" + try: + link.symlink_to(session["outside"] / "id_rsa") + except (OSError, NotImplementedError): # Windows without developer mode + pytest.skip("symlinks unavailable") + _, err = _resolve_in_roots(str(link), session["roots"], need_write=False) + assert err, "resolve() must follow the symlink before containment is decided" + + +def test_no_roots_means_no_filesystem_access(session): + _, err = _resolve_in_roots(str(session["outside"] / "id_rsa"), [], need_write=False) + assert err and "no granted directories" in err["error"] + + +# -- the tools ---------------------------------------------------------------- + +def test_upload_refuses_a_path_outside_the_session(session): + upload = _tools(session["roots"])["browser_upload_file"] + out = upload("input[type=file]", str(session["outside"] / "id_rsa")) + assert "error" in out and "outside the session" in out["error"] + + +def test_upload_refuses_before_reporting_whether_the_file_exists(session): + """The refusal must not double as an existence oracle for ungranted paths.""" + upload = _tools(session["roots"])["browser_upload_file"] + missing = upload("input", str(session["outside"] / "nope.txt")) + present = upload("input", str(session["outside"] / "id_rsa")) + # Same refusal for both: containment is decided before the file is stat'd, so the + # error never distinguishes "exists but ungranted" from "does not exist". + assert "outside the session" in missing["error"] + assert "outside the session" in present["error"] + assert "not found" not in missing["error"] + + +def test_screenshot_refuses_a_path_outside_the_session(session): + shot = _tools(session["roots"])["browser_screenshot"] + out = shot(str(session["outside"] / "evil.png")) + assert "error" in out and "outside the session" in out["error"] + + +def test_screenshot_is_refused_before_the_browser_is_touched(session): + """Containment must be decided by the tool, not incidentally by the browser layer. + + Previously the check did not exist and `out.parent.mkdir(parents=True)` sat inside the + Playwright callback, so on a machine with Playwright installed a refused path still had + its directory tree created. The refusal now happens first, which is observable here + because the error is the containment error rather than Playwright's. + """ + shot = _tools(session["roots"])["browser_screenshot"] + out = shot(str(session["outside"] / "a" / "b" / "c.png")) + assert "outside the session" in out["error"] + assert "laywright" not in out["error"] + assert not (session["outside"] / "a").exists() + + +def test_tools_are_constructible_without_roots(session): + """CLI/direct callers pass no roots; construction must still work (and deny).""" + upload = _tools(None)["browser_upload_file"] + assert "error" in upload("input", str(session["outside"] / "id_rsa"))