security: give each default screenshot its own private temp file - #297
Open
Mr-Neutr0n wants to merge 2 commits into
Open
security: give each default screenshot its own private temp file#297Mr-Neutr0n wants to merge 2 commits into
Mr-Neutr0n wants to merge 2 commits into
Conversation
…oots
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 andrewyng#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.
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 (andrewyng#289) because it edits the same
function.
Tests: extends tests/test_browser_path_confinement.py - two calls get two
distinct files, both owner-only.
Mr-Neutr0n
force-pushed
the
security/screenshot-temp-file-is-predictable
branch
from
July 28, 2026 20:52
c1759e8 to
08b0d11
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
browser_screenshot's default location is a hardcoded name in the shared temp dir:Two problems on a multi-user host. (Platform-dependent: Linux
/tmpis shared; macOS gives each uid its own temp dir, so this is a Linux/server concern.)tempfile.mkstempgives a random name created0600withO_EXCL, so neither holds. Callers passing an explicit path are unaffected; that path is root-confined by #289.On the evidence
The predictable-name half is self-evident from the line above — it's a literal constant. I want to be straight about what the test does and doesn't show: it pins the new invariant (two calls produce two distinct owner-only files), and it does fail on the current code, but for a different reason — today the file is only created inside the Playwright callback, so without Playwright installed nothing is created at all. It isn't a clean before/after demonstration of the mode, and I'd rather say so than let the red test imply more than it proves.
Tests
Extends
tests/test_browser_path_confinement.py: two successive default-location calls get two distinct files matching the expected prefix, and both are0600.Full suite unchanged from #289: 927 passed, with the same 14 pre-existing failures (
test_fake_slack.pydead-port timeouts #252,test_bedrock_provider.pymissing extra #284/#285).Reviewed and tested locally; drafted with AI assistance.