security: confine the browser connector's file tools to the session roots

[Mr-Neutr0n]

Fixes #189, and fixes a second instance of the same root cause that hadn't been reported.

The gap

PermissionEngine.evaluate path-scopes only the built-in WRITE_TOOLS, and only by reading arguments["path"]:

if is_write:
    path = arguments.get("path")
    if path is not None and not self._under_writable_root(path):
        return Decision(False, ...)

classify() resolves connector tools to EXTERNAL or READ, never WRITE_LOCAL, so a connector tool that touches local files is invisible to the roots mechanism. Two browser tools do, in opposite directions:

tool	what it does today
browser_upload_file(target, path)	Path(path).expanduser().resolve(), no containment, then hands the bytes to whatever page is loaded
browser_screenshot(path)	same unconfined resolve, then out.parent.mkdir(parents=True) and a write

The first is an arbitrary-read → exfiltration primitive (~/.ssh/id_rsa, ~/.aws/credentials, or secrets.json itself). The second is an arbitrary file overwrite plus directory creation anywhere the process can reach.

Scope, stated honestly: 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 what the roots list exists to prevent. Given the agent's input is untrusted by design (it reads web pages, email, Slack), an approval dialog naming a plausible-looking path is thin protection.

The fix

make_integration_tools already receives roots, with this comment:

Email needs the session roots: attachment downloads land in the primary scratch and outgoing attachments must resolve inside a granted directory.

The browser tools simply weren't passed them. This threads roots through and reuses the confinement email_tools already applies to outgoing attachments (email_tools.py:640): resolve first, then require the result inside a granted root. Reads accept any root; the screenshot write requires a writable one.

Containment is now decided before the browser layer is touched, which fixes two ordering problems:

• a refused screenshot no longer creates its parent directories on the way to failing
• a refused upload no longer stats the path, so the error can't be used to test whether an ungranted file exists

Paths inside the session behave exactly as before, and the default screenshot location (the temp file) is untouched.

Tests

tests/test_browser_path_confinement.py — 11 cases: .. traversal, symlink escaping a root, write refused into a read-only root, no-roots denies everything, refusal ordering, and constructibility without roots for CLI callers.

Verified against the unpatched tree: the only thing preventing the upload there was Playwright being absent, not any security check.

Full suite: 927 passed. The 14 failures on this branch reproduce identically on a clean checkout — test_fake_slack.py (dead-port timeouts, being fixed in #252) and test_bedrock_provider.py (the missing bedrock extra, #284/#285).

---

Reviewed and tested locally; drafted with AI assistance.

[Mr-Neutr0n]

For whoever picks these up — I opened five security PRs today and want to be upfront about how they relate, since three of them touch overlapping files.

PR	Files	Independent?
#289 (this)	browser_automation.py, integration_tools.py	yes
#290	integration_tools.py, web/fetch.py, web/guard.py (new)	yes
#291	secrets.py	yes
#295	tools/shell.py	yes
#297	browser_automation.py	stacked on #289

• security: confine the browser connector's file tools to the session roots #289 and security: block loopback/private/metadata addresses in model-supplied URL fetches #290 both edit integration_tools.py but in different places (this one changes the make_browser_automation_tools(...) call; security: block loopback/private/metadata addresses in model-supplied URL fetches #290 changes _request and browser_read_url). I test-merged them: clean, no conflicts, either order.
• security: give each default screenshot its own private temp file #297 is stacked on this branch because it edits the same function. Its GitHub diff therefore includes this PR's changes; only its last commit is new. Merge this one first and security: give each default screenshot its own private temp file #297 collapses to a few lines.
• security: create secret files private, never chmod them after (#143) #291 and security: keep OpenWorker's own sidecar token out of the shell environment #295 touch nothing the others do — safe to take in isolation.

Two of the five have companion issues with the analysis written up (#293 for the screenshot half of this one, #292 for #290), in case you'd rather triage from there. I also filed #296, which is a design question rather than a bug and needs your call, not a patch.

Happy to rebase, split, or squash any of these into whatever shape suits your review flow.

[Mr-Neutr0n]

Rebased all five of these onto current main (they were 9 commits behind), no conflicts and no diff changes.

The reason, in case the force-push looked arbitrary: 8674e30 landed today pinning mcp<2 because "CI installs latest mcp; today's 2.0.0 release breaks every MCP-client import." These branches predated it, so whenever someone approves the workflow runs, CI would have failed on that import break and it would have looked like one of these PRs caused it. Now they pick up the pin.

New heads: #289 cd85b1e · #290 ff86735 · #291 ee94da2 · #295 1d7860f · #297 08b0d11

#297 is still exactly one commit on top of #289, so the stacking note above still holds. Each branch's own tests re-run green after the rebase (11 / 24 / 7 / 5 / 12).