diff --git a/coworker/connectors/browser_automation.py b/coworker/connectors/browser_automation.py index e1e22e0c..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 @@ -17,6 +18,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 +361,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 +515,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 +571,24 @@ def run(page): ) def browser_screenshot(path: str = "") -> dict[str, Any]: - def run(page): - out = ( - Path(path).expanduser() - if path - else Path(tempfile.gettempdir()) / "coworker-browser-screenshot.png" + if path: + out, err = _resolve_in_roots(path, roots, need_write=True) + if err: + return err + else: + # 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" ) - out = out.resolve() + os.close(fd) + out = Path(name).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..7e81acbf --- /dev/null +++ b/tests/test_browser_path_confinement.py @@ -0,0 +1,165 @@ +"""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 tempfile as tempfile_mod + +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")) + + +# -- 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)}"