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 1/2] 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")) From 08b0d11f036593a0f89584d8cdf6a735d9c4e7ce Mon Sep 17 00:00:00 2001 From: Mr-Neutr0n <64578610+Mr-Neutr0n@users.noreply.github.com> Date: Tue, 28 Jul 2026 20:25:23 +0530 Subject: [PATCH 2/2] security: give each default screenshot its own private temp file browser_screenshot's default location was a fixed name in the shared temp dir: Path(tempfile.gettempdir()) / "coworker-browser-screenshot.png" Two problems on a multi-user host (Linux /tmp is shared; macOS gives each uid its own, so this is platform-dependent): - the path is predictable, so another local user can pre-create it as a symlink and the screenshot write follows it - the image is created at the umask default. A full-page screenshot of whatever the agent is driving may show a logged-in inbox, a Slack channel or a bank page, left group/world readable tempfile.mkstemp gives a random name created 0600 with O_EXCL, so neither holds. Callers that pass an explicit path are unaffected (that path is root-confined by the preceding commit). Stacked on security/browser-path-confinement (#289) because it edits the same function. Tests: extends tests/test_browser_path_confinement.py - two calls get two distinct files, both owner-only. --- coworker/connectors/browser_automation.py | 15 ++++++++--- tests/test_browser_path_confinement.py | 31 +++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/coworker/connectors/browser_automation.py b/coworker/connectors/browser_automation.py index f3477180..744984d7 100644 --- a/coworker/connectors/browser_automation.py +++ b/coworker/connectors/browser_automation.py @@ -6,6 +6,7 @@ from __future__ import annotations +import os import re import tempfile import threading @@ -575,9 +576,17 @@ def browser_screenshot(path: str = "") -> dict[str, Any]: if err: return err else: - out = ( - Path(tempfile.gettempdir()) / "coworker-browser-screenshot.png" - ).resolve() + # A fixed name in the shared temp dir is two problems on a + # multi-user host (Linux /tmp; macOS gives each uid its own). + # Another user can pre-create the path as a symlink and the + # screenshot follows it, and the image itself — which may show a + # logged-in inbox or bank page — lands world-readable. mkstemp + # gives a random name created 0600 with O_EXCL. + fd, name = tempfile.mkstemp( + prefix="coworker-browser-screenshot.", suffix=".png" + ) + os.close(fd) + out = Path(name).resolve() def run(page): out.parent.mkdir(parents=True, exist_ok=True) diff --git a/tests/test_browser_path_confinement.py b/tests/test_browser_path_confinement.py index 5fbc1e97..7e81acbf 100644 --- a/tests/test_browser_path_confinement.py +++ b/tests/test_browser_path_confinement.py @@ -12,6 +12,7 @@ """ from pathlib import Path +import tempfile as tempfile_mod import pytest @@ -132,3 +133,33 @@ 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")) + + +# -- the default screenshot location ------------------------------------------ + +def test_default_screenshot_path_is_unpredictable_and_private(tmp_path, monkeypatch): + """A fixed name in a shared temp dir is pre-creatable by another local user. + + On Linux /tmp is shared, so `coworker-browser-screenshot.png` could be planted as a + symlink (the write then follows it) and the image — potentially a logged-in inbox — was + left at the umask default. mkstemp gives a random name created 0600 with O_EXCL. + """ + import re + import stat + + shared = tmp_path / "shared_tmp" + shared.mkdir() + monkeypatch.setattr(tempfile_mod, "gettempdir", lambda: str(shared)) + + shot = _tools([RootDir(path=tmp_path / "scratch", writable=True)])["browser_screenshot"] + shot() # no path -> default location + shot() + + created = sorted(shared.iterdir()) + assert len(created) == 2, "each call must get its own file, not a shared fixed name" + assert created[0].name != created[1].name + + for f in created: + assert re.match(r"coworker-browser-screenshot\..+\.png$", f.name), f.name + mode = stat.S_IMODE(f.stat().st_mode) + assert mode & (stat.S_IRGRP | stat.S_IROTH) == 0, f"{f.name} was {oct(mode)}"