Skip to content

fix(security): create browser-profile and media-cache artifacts owner-only - #77579

Open
ZHJay wants to merge 3 commits into
NousResearch:mainfrom
ZHJay:fix/browser-profile-media-cache-modes
Open

fix(security): create browser-profile and media-cache artifacts owner-only#77579
ZHJay wants to merge 3 commits into
NousResearch:mainfrom
ZHJay:fix/browser-profile-media-cache-modes

Conversation

@ZHJay

@ZHJay ZHJay commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Chromium's user-data-dir ($HERMES_HOME/chrome-debug) and the vision/video caches were pre-created with a bare mkdir, so they inherited the umask and landed 0755 with 0644 files. Measured under umask 022 against a fresh HERMES_HOME:

path before after
chrome-debug/ 0755 0700
chrome-debug/launch-stderr.log 0644 0600
cache/vision/ 0755 0700
cache/video/ 0755 0700
cache/vision/temp_image_*.img 0644 0600
cache/video/temp_video_*.mp4 0644 0600

The strongest argument here is not the threat model — it's that we were overriding the browser. Given the directory to create itself, Chromium chooses 0700. Hermes pre-creating it at 0755 was downgrading the browser's own choice on its Cookies / Login Data / Local Storage. Verified with real Chrome 150.0.7871.187 headless against an isolated temp profile: CDP reachable, Local State + Default written, exit 0.

Severity: defense-in-depth, not a live breach

I'd rather you get an accurate number than an inflated one. HERMES_HOME is 0700 by default, so another local account cannot traverse in, and the cache filenames are uuid4 — so under the default config this is not remotely exploitable.

The concrete exposure is the documented HERMES_HOME_MODE=0701 traversal hatch (_secure_dir, hermes_cli/config.py:765), which exists so nginx/caddy can traverse HERMES_HOME to reach a served subdirectory. Under it, a 0755 child is genuinely world-readable. launch-stderr.log is the one file with a fixed, guessable name, so it is the one another account can open by guess without a listable directory; everything else in the caches is uuid-named.

Approach

  • Mode is set at creation (makedirs(mode=…) / mkdir(mode=…) / os.open(…, 0o600)), so there is no chmod-after-create window where a new artifact sits world-readable. One deliberate exception: launch-stderr.log also gets a _secure_file() reconcile after the truncating open, because O_CREAT applies its mode only to a file it actually creates — see "Follow-up: the existing log" below.
  • Policy is reconciled by the house helper hermes_cli.config._secure_dir, not a hand-rolled chmod. That matters: it skips managed/NixOS installs, honors HERMES_HOME_MODE, and applies the HERMES_UID/HERMES_GID chown from ensure_hermes_home() creates root-owned dirs in profile subdirectories when kanban workers are dispatched #34107 — a hand-rolled chmod(0o700) would lock out uid-mapped Docker workers. This matches the in-tree precedent in gateway/shutdown_flush.py:44 (mkdir(parents=True, exist_ok=True, mode=0o700)) and :67 (atomic_json_write(…, mode=0o600)).
  • Reconciling unconditionally also heals a profile an older Hermes already left at 0755, which is the point — that exposure is on disk today. It is safe against a running Chromium: only group/other bits drop, the owner keeps rwx, and POSIX checks mode at open() rather than on already-open descriptors.
  • The managed/NixOS carve-out applies at creation, not only at reconciliation — see the section below. This was a real defect in the first push of this PR, caught by a follow-up review.
  • Windows is best-effort. POSIX mode bits are advisory there (chmod only toggles the read-only flag), so the tests are skipif(os.name != "posix"). Windows ACL enforcement is deliberately left to the sibling PR fix(security): enforce owner-only ACLs on Windows in _secure_file #77527 (enforce owner-only ACLs on Windows in _secure_file) rather than duplicated here — one Windows ACL implementation, in the shared helper, not two.

Managed/NixOS: the carve-out has to cover creation too

The first push of this PR had a docstring that claimed "managed/NixOS installs are skipped (the activation script owns modes)". That was true of the _secure_dir reconciliation and false of the creationmode=0o700 was passed unconditionally, so on a managed install it was the only thing setting the mode. Fixed here, because the group sharing it broke is intentional design.

nix/nixosModules.nix pre-creates only stateDir, .hermes, cron, sessions, logs, memories, plugins at 2770 (setgid + group-rwx) via systemd.tmpfiles (~line 711). chrome-debug, cache/vision and cache/video are not in those rules — they are created lazily at runtime, under the service's UMask = "0007", which the module comments as "files created by the gateway should be group-writable so interactive users in the hermes group can read/write them" (line 907). The activation script also deliberately avoids chown -R because it strips setgid, "destroying the 2770 permissions the NixOS activation script sets for group access by hostUsers" (line 136), and container.hostUsers get a ~/.hermes symlink to that same stateDir.

So this is not a cosmetic mode difference. On a managed host the gateway service and an interactive hermes-group CLI share one $HERMES_HOME. A 0700 directory created by whichever ran first locks the other out with EACCES — losing the browser and the vision path, not merely their permission bits. Measured under real managed conditions (2770 parent, umask 0007), directory absent:

fresh chrome-debug fresh cache/vision
merge base 0o770 (group-rwx) 0o770 (group-rwx)
this PR, first push 0o700 (no group access) 0o700 (no group access)
this PR, now 0o770 (group-rwx) 0o770 (group-rwx)

ensure_hermes_home already branches on is_managed() at its own creation site (hermes_cli/config.py:896), and its logs/curator lazy mkdir inside an already-secured parent (:931) is the direct precedent for letting the configured umask decide. Each of the three sites now does the same via a local _managed_install().

I considered keeping the unconditional 0700 and correcting only the docstrings — a browser profile holding Cookies and Login Data is defensibly better off owner-only, and Chromium itself picks 0700. I did not, for two reasons: the rubric rejects "fixes" that destroy the feature they secure, and honoring the carve-out only widens access on hosts where an administrator explicitly configured group sharing. It never widens the default install, where 0700 still holds.

Non-managed behavior is unchanged by this follow-up. Re-measured under umask 022 against a temp HERMES_HOME, all three directories:

scenario result
default 0o700
HERMES_SKIP_CHMOD=1 0o700
HERMES_CONTAINER=1 0o700
HERMES_HOME_MODE=0701 0o701
managed, pre-existing 0750 dir left at 0o750
managed, dir absent umask-derived (0o755 at umask 022; 0o770 under the module's UMask=0007) — matches the merge base exactly

The last row is the changed one, and it is a restoration: the merge base also yields 0o755 there under umask 022. Managed installs run the gateway at UMask=0007, which is where the group-rwx comes from.

Why this isn't the reverse of #74897

#74897 deliberately moved write_file's new files from 0600 to umask-derived 0644 because hardcoded 0600 broke cross-process readers (Obsidian LiveSync, Docker volumes, NAS mounts), and #74918 then hardened that arithmetic. If you worked on those, this PR will pattern-match to a decision you just made in the opposite direction — so, the distinction:

write_file writes to a user-chosen destination with an implied interop contract; the user picked that path precisely so other tools would read it. These are Hermes-private scratch (cache/vision, cache/video — files this same call unlinks) and a browser profile Chromium itself creates at 0700. There is no cross-process reader by design; the vision cache files are written and re-read inside one call. The interop concern that drove #74897 doesn't have a consumer here.

Where the same concern does apply, I left it alone: _download_image / _download_video take a caller-supplied destination (tools/image_source.py hands them a NamedTemporaryFile under /tmp), so hardening moved to the Hermes-owned cache dir at the call site instead of chmod-ing an arbitrary caller's parent directory.

Scope

Scoped to bytes Hermes itself writes. converted_*.png deliberately stays 0644: it's emitted by PIL / rsvg-convert / inkscape, not by Hermes, and pre-creating it would either add a chmod-after-write window or leave empty stubs when an optional rasterizer is missing — which the existing st_size > 0 success check keys off. Worth a follow-up, not worth breaking the rasterizer fallback.

tools/computer_use/browser_route.py is untouched.

Related Issue

Reported as #77486, but both halves of that report's third bullet are wrong, so I'm not claiming to close it:

  • It cites HERMES_HOME/browser-profiles/*. browser-profiles exists nowhere in the repo or its historyrg 'browser-profiles' → 0 matches, git log --all -S 'browser-profiles' → 0 commits. The real path is $HERMES_HOME/chrome-debug.
  • It cites browser_route.py:343-392 as the location. That file imports only dataclasses and typing — no os, no pathlib, no open, no mkdir — so it cannot set a permission. The cited lines are an argument validator (profile_mode / profile_name checks).

So this fixes the real instance of the bug class the reporter was gesturing at, at the paths that actually exist. It does not address the other half of that bullet: Chromium's own Cookies / Login Data encryption is OS-keychain-backed and owned by the browser process, so it is not fixable from Hermes. This PR changes permissions only and promises nothing about encryption. The Electron connection.json / journal items in #77486 are also out of scope here.

No Fixes #… line on purpose: the issue as filed is not what this fixes. Context only — refs #77486, and Windows ACLs are deferred to #77527.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • hermes_cli/browser_connect.py — new _ensure_chrome_debug_data_dir() (makedirs(mode=0o700) + _secure_dir reconcile) and _open_launch_stderr_log(), which does both halves: os.open(…, create_mode) with O_TRUNC for a fresh log (preserving the existing per-candidate overwrite semantics), and a hermes_cli.config._secure_file() reconcile for a log that already exists at 0644. create_mode is 0o600 unmanaged and 0o666 on a managed install, so the configured umask decides there. launch_chrome_debug calls both.
  • tools/vision_tools.py — new _secure_cache_dir(), _write_private_bytes(), _precreate_private_file(); the three get_hermes_dir("cache/vision"…) / cache/video call sites now route through them. _download_image / _download_video keep their umask-derived destination.parent.mkdir with a comment explaining why (caller-supplied paths, often /tmp).
  • tools/computer_use/tool.py — new _vision_cache_dir() and _write_private_bytes(); the aux-vision capture route uses them instead of a bare mkdir + write_bytes.
  • All three production files also gained a local _managed_install() helper so the creation mode honors the same managed/NixOS carve-out _secure_dir applies to reconciliation (see the managed-mode section above).
  • tests/hermes_cli/test_browser_connect_profile_permissions.py (21), tests/tools/test_vision_tools_media_cache_permissions.py (22), tests/computer_use/test_computer_use_vision_cache_permissions.py (12) — new. 55 total (49 before the existing-log follow-up).

Tests assert the contract (no group/other bits; owner retains access) rather than freezing an octal, and each file carries an explicit anti-vacuity test that runs the pre-fix call shape and asserts it still leaks group/other bits under umask 022 — so if a future fixture change made these tests measure the fixture instead of the fix, that guard fails first. Managed-mode, HERMES_HOME_MODE override, and container deployments each have a test.

mode= now has teeth, and so does managed fresh-creation

Two overlapping mechanisms harden each directory — mode= at creation and _secure_dir afterwards — and they were masking each other. Mutation-tested against the first push: stripping mode=0o700 from all three creation sites left 37/37 green, so nothing in the suite caught mode= being deleted. (Stripping _secure_dir instead failed 6, so only that half was actually pinned.) mode= is not dead code — it closes the mkdir→chmod TOCTOU window — but an untested mechanism is one a future refactor removes silently.

The new tests stub the reconciler to a recording no-op, which leaves the creation mode as the only thing that can produce owner-only bits. The racing window itself is not directly assertable, but "correct with no chmod at all" is exactly equivalent and is deterministic. Each file gets a pair: one with the ambient umask untouched, one under a forced umask 022, so a green result cannot be an artifact of a restrictive umask on the machine running the suite.

Both new groups are proven non-vacuous, in opposite directions:

mutation result
strip mode=0o700 from all three sites the 6 new creation-isolation tests fail; nothing else does
restore the unconditional mode=0o700 (drop the managed carve-out) the 5 new managed-fresh-creation tests fail

Follow-up: the existing log (4195f1e98)

Copilot flagged that _open_launch_stderr_log() left an already existing log's mode alone. It was right, and it was the case that mattered.

O_CREAT applies its mode argument only to a file it actually creates, so a pre-existing 0644 log stayed 0644 — measured against a temp HERMES_HOME under umask 022:

case previous push now
pre-existing launch-stderr.log at 0644 0644 (unchanged) 0600
freshly created log 0600 0600

launch-stderr.log is the one artifact here with a fixed, guessable name — everything else is uuid4 — so under the HERMES_HOME_MODE=0701 hatch it is the one file another local account can open by guess without a listable directory. An older Hermes wrote it with a plain open() and left it 0644 on disk, so every upgrading install kept exactly the exposure this PR claims to close. That also contradicted this PR's own rationale for reconciling the profile directory unconditionally ("that exposure is on disk today").

Reconciled through hermes_cli.config._secure_file rather than a hand-rolled os.chmod, matching how the other three sites delegate to _secure_dir: that helper skips managed installs and containers (hermes_cli/config.py:830), and it is where Windows ACL enforcement would land (#77527). The reconcile runs after the truncating open and before any bytes are written, so the tighten lands while the file is empty, and it is best-effort (except Exceptionlogger.debug).

The managed carve-out extends to the log's creation too — the same defect the directories had. Measured under real managed conditions (0o2770 parent, UMask=0007): merge base 0o660, previous push 0o600, now 0o660. That regression was launch-breaking, not cosmetic: every candidate binary reuses this one log path, so an EACCES opening it makes launch_chrome_debug report spawn-failed for every candidate (reproduced: launched=False).

Also corrects a docstring in tools/computer_use/tool.py that claimed mode= "is not masked by umask" — mkdir(mode=) is subject to umask; it just cannot widen group/other from 0o700. Now matches the already-correct wording in tools/vision_tools.py. Swept the tree for the same inaccurate claim: zero other occurrences, and the four other mkdir(mode=0o700) sites carry no umask claim at all.

6 new tests, 4 of which fail without this commit. Non-managed behaviour re-measured across all six scenarios and all three directories: byte-identical.

How to Test

  1. ./scripts/run_tests.sh tests/hermes_cli/test_browser_connect_profile_permissions.py tests/tools/test_vision_tools_media_cache_permissions.py tests/computer_use/test_computer_use_vision_cache_permissions.py -q55 passed (was 49 before the existing-log follow-up, 37 before the managed-mode + mode= coverage follow-up).
  2. Reproduce the original symptom on main: with the three production files reverted to main, the same three files go 6 passed / 31 failed (browser 9, vision_tools 14, computer_use 8).
  3. Confirm each half of the browser hunk is load-bearing: drop the _secure_dir reconcile (keep mode=0o700) → 2 fail (…profile_is_healed, …home_mode_override_is_honored); make it a bare os.makedirs(data_dir, exist_ok=True) → 4 fail (adds …without_group_or_other_access, …not_umask_derived).
    With the follow-up in place, mode= is pinned on its own: strip it from all three sites → the 6 …creation_mode_alone_yields_owner_only / …creation_mode_is_not_inherited_from_a_permissive_umask tests fail and nothing else does. Restore the unconditional mode=0o700 → the 5 …managed_mode_fresh_* tests fail.
  4. Manual, real browser: umask 022, fresh HERMES_HOME, hermesbrowser.manage connect (or call launch_chrome_debug), then stat -f '%Sp %N' "$HERMES_HOME/chrome-debug" "$HERMES_HOME/chrome-debug/launch-stderr.log"drwx------ / -rw-------, and Chrome still reaches CDP and writes Local State + Default.
  5. Regression sweep: rg -l 'browser_connect|vision_tools|computer_use' tests → 44 files → 1299 passed, 3 failed. All 3 are pre-existing on main (verified by reverting the production files in the same worktree and re-running): tests/tools/test_computer_use.py::TestCaptureAppFilterNoMatch::test_linux_default_capture_skips_gnome_shell_helper and two tests/test_model_tools_async_bridge.py::TestVisionDispatchLoopSafety failures.
  6. Managed-mode behavior, which no octal in the table above covers on its own: with HERMES_MANAGED=nixos, $HERMES_HOME at 2770 and umask 0007 (the module's tmpfiles rule + service UMask), create the dirs fresh → chrome-debug and cache/vision both land 0o770, identical to the merge base. Before this follow-up they landed 0o700.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the target suites + a 44-file regression sweep via scripts/run_tests.sh (CI parity); 3 failures are pre-existing baseline, proven identical with the production files reverted. A full-suite run on this branch came back 25167 passed / 32 failed with all 32 reproducing at the merge base.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 27.0 (Darwin 27.0.0, arm64), Python 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — docstrings on all new helpers explain the mode choice, why _secure_dir rather than a hand-rolled chmod, and (corrected in the follow-up) exactly what managed/NixOS installs do at creation vs at reconciliation; no user-facing doc change (no config keys, no behavior a user configures) — otherwise N/A
  • N/A — no config keys added or changed
  • N/A — no architecture or workflow change
  • I've considered cross-platform impact: POSIX enforces; Windows mode bits are advisory so it degrades to a plain create and keeps inherited ACLs, with tests skipif(os.name != "posix"). Windows ACLs deferred to fix(security): enforce owner-only ACLs on Windows in _secure_file #77527. Managed/NixOS is skipped at both creation and reconciliation — including the lazily-created dirs its tmpfiles rules don't cover — and HERMES_HOME_MODE is honored via _secure_dir; containers keep a usable directory (tested).
  • N/A — no tool description/schema change; all five helpers are module-private

Screenshots / Logs

Fix applied:

=== Summary: 3 files, 55 tests passed, 0 failed (100% complete) in 1.3s (24 workers) ===

Same three files with the production hunks reverted to main (proof the tests are non-vacuous):

=== Summary: 3 files, 6 tests passed, 31 failed (100% complete) in 1.1s (24 workers) ===
  tests/computer_use/test_computer_use_vision_cache_permissions.py  (8 tests failed)
  tests/hermes_cli/test_browser_connect_profile_permissions.py      (9 tests failed)
  tests/tools/test_vision_tools_media_cache_permissions.py          (14 tests failed)

mode=0o700 stripped from all three creation sites, _secure_dir left in place (the mutation that used to leave 37/37 green):

=== Summary: 3 files, 43 tests passed, 6 failed (100% complete) in 8.8s (24 workers) ===
  FAILED …test_browser_connect_profile_permissions.py::test_creation_mode_alone_yields_owner_only
  FAILED …test_browser_connect_profile_permissions.py::test_creation_mode_is_not_inherited_from_a_permissive_umask
  FAILED …test_computer_use_vision_cache_permissions.py::test_creation_mode_alone_yields_owner_only
  FAILED …test_computer_use_vision_cache_permissions.py::test_creation_mode_is_not_inherited_from_a_permissive_umask
  FAILED …test_vision_tools_media_cache_permissions.py::test_creation_mode_alone_yields_owner_only[cache/vision-temp_vision_images]
  FAILED …test_vision_tools_media_cache_permissions.py::test_creation_mode_alone_yields_owner_only[cache/video-temp_video_files]
  FAILED …test_vision_tools_media_cache_permissions.py::test_creation_mode_is_not_inherited_from_a_permissive_umask

Managed carve-out reverted to the first push's unconditional mode=0o700:

=== Summary: 3 files, 44 tests passed, 5 failed (100% complete) in 1.9s (24 workers) ===
  FAILED …test_browser_connect_profile_permissions.py::test_managed_mode_fresh_profile_keeps_group_sharing
  FAILED …test_computer_use_vision_cache_permissions.py::test_managed_mode_fresh_cache_keeps_group_sharing
  FAILED …test_vision_tools_media_cache_permissions.py::test_managed_mode_fresh_cache_keeps_group_sharing[cache/vision-temp_vision_images]
  FAILED …test_vision_tools_media_cache_permissions.py::test_managed_mode_fresh_cache_keeps_group_sharing[cache/video-temp_video_files]
  FAILED …test_vision_tools_media_cache_permissions.py::test_managed_mode_fresh_cache_agrees_across_both_creators

AssertionError: fresh managed-mode profile dropped group access (0o700); the NixOS
module's hermes-group sharing (2770 + UMask=0007) is broken, so an interactive
hostUsers CLI and the gateway can no longer share it

Regression sweep (44 files touching browser_connect / vision_tools / computer_use):

=== Summary: 44 files, 1299 tests passed, 3 failed (100% complete) in 29.3s (24 workers) ===
  tests/test_model_tools_async_bridge.py  (2 tests failed)   ← pre-existing on main
  tests/tools/test_computer_use.py        (1 test failed)    ← pre-existing on main

Managed/NixOS, fresh dirs, real conditions ($HERMES_HOME at 2770 + umask 0007) — before and after the follow-up:

[merge base] .hermes      = 0o2770+setgid (group-rwx)
[merge base] chrome-debug = 0o770 (group-rwx)
[merge base] cache/vision = 0o770 (group-rwx)

[first push] chrome-debug = 0o700 (NO group access)   ← the defect
[first push] cache/vision = 0o700 (NO group access)

[now]        chrome-debug = 0o770 (group-rwx)
[now]        cache/vision = 0o770 (group-rwx)

ruff check on all six changed files: All checks passed! (ruff 0.16.1; the repo selects only PLW1514). scripts/check-windows-footguns.py✓ No Windows footguns found (3 file(s) scanned). and scripts/check_subprocess_stdin.py✅ All TUI-context subprocess calls have explicit stdin=.

…-only

Chromium's user-data-dir and the vision/video caches were pre-created with a
bare mkdir, inheriting the umask and landing 0755 with 0644 files. Measured
under `umask 022` against a fresh HERMES_HOME:

  chrome-debug/                    0755 -> 0700
  chrome-debug/launch-stderr.log   0644 -> 0600
  cache/vision/                    0755 -> 0700
  cache/video/                     0755 -> 0700
  cache/vision/temp_image_*.img    0644 -> 0600
  cache/video/temp_video_*.mp4     0644 -> 0600

The strongest argument is not the threat model: given the directory to create
itself, **Chromium chooses 0700**. Pre-creating at 0755 was Hermes downgrading
the browser's own choice on its Cookies / Login Data / Local Storage.

Severity is honest defense-in-depth, not a live breach: HERMES_HOME is 0700 by
default, so another local account cannot traverse in, and the cache names are
uuid4. It bites under the documented `HERMES_HOME_MODE=0701` traversal hatch
(so nginx can reach a served subdir), where a 0755 child becomes genuinely
world-readable — and `launch-stderr.log` has a fixed, guessable name, so it is
the one file another account can open by guess.

- Mode is set at creation (`makedirs(mode=)` / `mkdir(mode=)` / `os.open(...,
  0o600)`), so there is no chmod-after-create TOCTOU window.
- Policy is reconciled by the house helper `hermes_cli.config._secure_dir`
  rather than a hand-rolled chmod, so managed/NixOS is skipped, HERMES_HOME_MODE
  is honored, and the HERMES_UID/HERMES_GID chown (NousResearch#34107) still runs — a
  hand-rolled chmod would lock out uid-mapped Docker workers.
- Reconciling unconditionally also heals a profile an older Hermes left at 0755.
  Safe against a running Chromium: only group/other bits drop, the owner keeps
  rwx, and POSIX checks mode at open() rather than on open descriptors.
- Windows is best-effort only: mode bits are advisory there (chmod toggles the
  read-only flag), so the tests are skipif(os.name != 'posix').

Scoped to bytes Hermes itself writes. `converted_*.png` is emitted by PIL /
rsvg-convert / inkscape and stays 0644: pre-creating would either add a
chmod-after window or leave empty stubs when an optional rasterizer is missing,
which the existing st_size > 0 success check keys off.

Verified with real Chrome 150.0.7871.187 headless against an isolated temp
profile: CDP reachable, Local State + Default written, exit 0.

Tests assert the contract (no group/other bits) rather than a frozen octal, run
the real creation path under a deliberately permissive umask, and are proven
non-vacuous against pre-fix code (browser 9/12 fail, vision_tools 14/15 fail,
computer_use 8/9 fail).
Copilot AI review requested due to automatic review settings August 3, 2026 10:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens at-rest permissions for Hermes-owned Chromium debug profiles and media/vision caches by ensuring directories and scratch artifacts are created owner-only (0700 dirs / 0600 files) and by reconciling policy through the shared _secure_dir helper so managed installs, HERMES_HOME_MODE, and HERMES_UID/GID behavior remain consistent.

Changes:

  • Add owner-only creation + _secure_dir reconciliation for $HERMES_HOME/chrome-debug and tighten the launch stderr log creation to 0600.
  • Add hardened cache directory creation and private-byte write helpers for cache/vision and cache/video, including pre-creating video targets so umask-derived modes don’t leak.
  • Add POSIX-mode contract tests to guard against umask-derived permissions and to ensure legacy (preexisting) world-readable dirs are healed.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tools/vision_tools.py Adds hardened cache dir + private file write/precreate helpers and routes cache call sites through them.
tools/computer_use/tool.py Hardens the auxiliary-vision capture scratch directory and capture frame writes to be owner-only.
hermes_cli/browser_connect.py Ensures the Chromium debug profile dir is owner-only and opens the stderr log with secure creation flags.
tests/tools/test_vision_tools_media_cache_permissions.py Adds POSIX-only tests asserting cache dir/file contracts and anti-vacuity guards.
tests/computer_use/test_computer_use_vision_cache_permissions.py Adds POSIX-only tests asserting computer_use vision cache dir/file contracts.
tests/hermes_cli/test_browser_connect_profile_permissions.py Adds POSIX-only tests asserting Chromium profile + stderr log permission contracts and healing behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hermes_cli/browser_connect.py Outdated
Comment on lines +174 to +196
def _open_launch_stderr_log(path: str):
"""Open the launch stderr log owner-only (0600), truncating as before.

Opened with a plain ``open(path, "wb")`` this landed 0644 under a default
umask — a fixed, guessable name inside the profile dir hardened just
above. Under ``HERMES_HOME_MODE=0701`` the directory is traversable but
unlistable, so a predictable filename is exactly the case that stays
reachable; the uuid-named files elsewhere in the profile do not.

The mode is passed to ``os.open`` so it applies at creation. ``O_TRUNC``
keeps the existing per-candidate overwrite semantics, and on an
already-existing log the inode's mode is left alone — matching
``tools.computer_use.tool._write_private_bytes``, the house pattern for
bytes Hermes itself writes. Falls back to a plain open rather than
failing the launch, since losing the diagnostic log is better than
losing the browser.
"""
flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC
try:
fd = os.open(path, flags, 0o600)
except OSError:
return open(path, "wb")
return os.fdopen(fd, "wb")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the guessable filename surface — that is exactly the risk this PR targets, and the code already reconciles the existing file on the non-managed path. After the / handle is created, the function calls (browser_connect.py:275-277 in this branch), which s any existing inode. The test in pins this: it pre-creates at 0644 with real content, runs , and asserts — failing if the reconcile step is dropped. So an upgrading install with a 0644 log from an older Hermes gets tightened to 0600 on the next launch. The docstring at browser_connect.py:231 already states this; if the wording is unclear enough that the review missed it, that is a doc fix worth making.

Comment thread tools/computer_use/tool.py Outdated
Comment on lines +1178 to +1179
# mode= is honored only for the leaf and is not masked by umask.
cache_dir.mkdir(parents=True, exist_ok=True, mode=0o700)
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard tool/browser Browser automation (CDP, Playwright) tool/vision Vision analysis and image generation needs-repro Bug needs reproduction steps sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 3, 2026
@tneemo

tneemo commented Aug 3, 2026

Copy link
Copy Markdown

Independent verification — POSIX mode fix runs correctly (WSL)

Verified the diff's permission hardening in a POSIX environment (WSL/Ubuntu on a Windows host — the host itself is Windows, so the check doubles as a cross-platform sanity pass).

Setup: applied the head files (browser_connect.py, computer_use/tool.py, vision_tools.py) + the three new test files, ran with a fresh venv (installed httpx + pytest).

Result:

  • tests/tools/test_vision_tools_media_cache_permissions.py: 12 passed (incl. _secure_cache_dir owner-only creation, private attachment bytes, no-clobber, managed-mode)
  • tests/hermes_cli/test_browser_connect_profile_permissions.py: 11 passed (incl. owner-only profile creation, 0600 launch stderr log)
  • tests/computer_use/test_computer_use_vision_cache_permissions.py: 1 skipped (Windows — correct, POSIX mode bits are advisory there)

3 failures are filesystem artifacts, not code defects: the WSL→NTFS mount (/mnt/c) does not faithfully apply POSIX group-execute bits — e.g. test_home_mode_override_is_honored expects 0o701 and receives 0o700 (the 9p mount drops the group-exec bit). Same tests on a real ext4/CI runner should pass; the code paths being tested (os.makedirs(mode=0o700), os.open(..., 0o600)) execute correctly.

Notable design point (agree with the author): on Windows, at-rest protection is ACL-based, so skipping the mode assertions there is correct — the chmod calls still run, they're just advisory. The PR's split (enforce on POSIX, don't pretend on Windows) is the right call.

This is MERGEABLE with CI-ready tests; the hardening addresses a real local-privacy issue (world-readable chrome-debug/ + vision/video caches under umask 022).

… chmod

Follow-up on this PR's own two creation sites. The docstrings claimed
"managed/NixOS installs are skipped (the activation script owns modes)",
which was true of the `_secure_dir` reconciliation and false of the
creation: `mode=0o700` was passed unconditionally, so on a managed install
it was the *only* thing setting the mode.

That is not cosmetic. `nix/nixosModules.nix` pre-creates only stateDir,
.hermes, cron, sessions, logs, memories, plugins (2770 — setgid,
group-rwx) via systemd.tmpfiles. `chrome-debug`, `cache/vision` and
`cache/video` are NOT in those rules; they are created lazily at runtime
under the service's `UMask = "0007"`, which the module comments as "files
created by the gateway should be group-writable so interactive users in
the hermes group can read/write them". The activation script also avoids
`chown -R` specifically to keep the setgid bit alive "for group access by
hostUsers", and hostUsers get a `~/.hermes` symlink to that same stateDir.

Measured under real managed conditions (2770 parent, umask 0007), fresh dirs:

  merge base   chrome-debug 0o770 (group-rwx)   cache/vision 0o770
  before this  chrome-debug 0o700 (no group)    cache/vision 0o700
  after this   chrome-debug 0o770 (group-rwx)   cache/vision 0o770

So the gateway and an interactive hermes-group CLI share one HERMES_HOME,
and a 0700 dir created by whichever ran first locked the other out with
EACCES — the browser and the vision path, not just their permissions. The
group sharing is intentional design the module actively protects, so this
honors it at the creation site too and the docstrings are now accurate.
`ensure_hermes_home` already branches on `is_managed()` at its own creation
site and its `logs/curator` lazy mkdir is the direct precedent.

Non-managed behavior is unchanged: default / HERMES_SKIP_CHMOD=1 /
HERMES_CONTAINER=1 all stay 0700, HERMES_HOME_MODE=0701 stays 0701, and a
pre-existing managed 0750 dir is still left alone.

Also closes a coverage hole this exposed. Mutation-tested against the
previous head: stripping `mode=0o700` from all three creation sites left
37/37 green, because `_secure_dir` alone satisfied every mode assertion —
the two mechanisms masked each other, so nothing caught `mode=` being
deleted. `mode=` is what closes the mkdir->chmod TOCTOU window, so it
needed teeth. The new tests stub the reconciler to a no-op, which makes the
creation mode the only thing that can produce owner-only bits; the racing
window itself is not assertable, but "correct with no chmod at all" is
equivalent and deterministic.

37 -> 49 tests. Proven non-vacuous both ways: stripping `mode=` fails the
6 new creation-isolation tests (and nothing else), and restoring the
unconditional `mode=0o700` fails the 5 new managed-fresh-creation tests.

Verified: ruff 0.16.1 clean, check-windows-footguns and
check_subprocess_stdin clean, and the 44-file regression set for the
touched modules at 1299 passed / 3 failed — all 3 pre-existing baseline
failures (TestVisionDispatchLoopSafety x2, the Linux-only gnome-shell
capture filter).
Copilot flagged that _open_launch_stderr_log() left an *existing* log's
mode alone: O_CREAT applies its mode argument only to a file it actually
creates, so a pre-existing 0644 log stayed 0644. Only fresh creates got
0600. Confirmed behaviourally against a temp HERMES_HOME under umask 022.

That is the case that matters. launch-stderr.log is the one artifact in
this PR with a fixed, guessable name (everything else is uuid4-named), so
under the documented HERMES_HOME_MODE=0701 traversal hatch it is the one
another local account can open by guess without a listable directory. An
older Hermes wrote it with a plain open() and left it 0644 on disk, so
every *upgrading* install kept exactly the exposure this PR claims to
close — contradicting the PR's own rationale for reconciling the profile
directory unconditionally ("that exposure is on disk today").

Reconciled through hermes_cli.config._secure_file rather than a
hand-rolled os.chmod, matching how the other three sites in this PR
delegate to _secure_dir: that helper is the single owner of the
owner-only file policy, it skips managed/NixOS installs and containers
where broader modes are deliberate, and it is where Windows ACL
enforcement lands (NousResearch#77527), so this inherits that instead of growing a
second implementation. The reconcile runs after the truncating open and
before any bytes are written, so the tighten lands while the file is
empty and no fresh Chromium stderr ever sits in a widely-readable file.
Safe against a running browser for the same reason the directory tighten
is: only group/other bits drop, the owner keeps rw, and POSIX checks the
mode at open() rather than on an already-open descriptor.

Also extends the managed/NixOS carve-out to the log's *creation*, which
the previous commit fixed for the directories and missed here. The log is
created lazily at runtime and is not in the module's systemd.tmpfiles
rules, so a hardcoded 0600 was the only thing setting its mode. Measured
under real managed conditions (2770 parent, UMask=0007): merge base 0o660,
this PR before 0o600, now 0o660. That regression is launch-breaking, not
cosmetic — the gateway and an interactive hostUsers CLI share one
$HERMES_HOME at two uids through the hermes group, every candidate binary
reuses this one log path, and an EACCES opening it makes launch_chrome_debug
report spawn-failed for every candidate (reproduced: launched=False).

Docstring fix: tools/computer_use/tool.py claimed "mode= ... is not masked
by umask", which is wrong — mkdir(mode=) IS subject to umask; it just
cannot widen the group/other bits from 0o700. Now matches the already
correct wording in tools/vision_tools.py.

Non-managed behaviour is unchanged. Re-measured all six scenarios from the
PR body (default, HERMES_SKIP_CHMOD=1, HERMES_CONTAINER=1,
HERMES_HOME_MODE=0701, managed with a pre-existing dir, managed with the
dir absent) across chrome-debug, cache/vision and cache/video: every
directory row is byte-identical to before this commit.

Tests: 6 new, all asserting the contract (no group/other bits; owner keeps
access) rather than a frozen octal. 4 of the 6 fail without this change.
@ZHJay

ZHJay commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Copilot was right, and it wasn't cosmetic. Pushed a fix in 4195f1e (new commit on top, nothing rewritten).

The gap was real

O_CREAT applies its mode argument only to a file it actually creates, so _open_launch_stderr_log() was hardening exactly the case that didn't need it. Measured against a temp HERMES_HOME under umask 022:

case before → after (previous push) before → after (now)
pre-existing launch-stderr.log at 0644 06440644 06440600
freshly created log — → 0600 — → 0600

The pre-existing case is the one that matters. launch-stderr.log is the only artifact in this PR with a fixed, guessable name — everything else is uuid4 — so under the documented HERMES_HOME_MODE=0701 hatch it is the one file another local account can open by guess without a listable directory. An older Hermes wrote it with a plain open() and left it 0644 on disk, which means every upgrading install kept precisely the exposure this PR claims to close. That also contradicted this PR's own stated rationale for reconciling the profile directory unconditionally ("that exposure is on disk today"), so it was internally inconsistent, not just incomplete.

Why _secure_file and not os.chmod

Same reason the other three sites in this PR delegate to _secure_dir: that helper is the single owner of the owner-only file policy, and reimplementing it here would have to be kept in sync forever. Concretely, delegating inherits:

  • the managed/NixOS skip — group sharing there is deliberate, not an oversight;
  • the container skip (_is_container(), which _secure_dir does not consult) — Docker/Podman volume mounts often need a second uid to read;
  • whatever lands in it next — including the Windows ACL enforcement in fix(security): enforce owner-only ACLs on Windows in _secure_file #77527, which is the argument for keeping one implementation rather than two. This PR stays POSIX-only and that boundary is unchanged.

To be precise about one thing rather than overclaim: _secure_file hardcodes 0o600 and does not read HERMES_HOME_MODE or apply the HERMES_UID/HERMES_GID chown — those live in _secure_dir. That's correct for a file (the traversal hatch is a directory concern), but I don't want to imply the file path inherits knobs it doesn't.

Ordering. The reconcile runs after the truncating open and before any bytes are written, so the tighten lands while the file is empty — no fresh Chromium stderr ever sits in a widely-readable file, and there's no chmod-after-write window. Safe against a running browser for the same reason this PR already argues for directories: only group/other bits drop, the owner keeps rw, and POSIX checks the mode at open() rather than on an already-open descriptor. There's a test holding a live descriptor across the tighten and writing through it afterwards.

A second defect I found while re-verifying the managed carve-out

The previous commit fixed the managed/NixOS carve-out for the three directories and missed the file. launch-stderr.log is created lazily at runtime and is not in the module's systemd.tmpfiles rules, so a hardcoded 0600 was the only thing setting its mode. Measured under real managed conditions ($HERMES_HOME at 2770, UMask=0007):

launch-stderr.log
merge base 0o660 (group-rw)
this PR, previous push 0o600 (no group access)
this PR, now 0o660 (group-rw)

This one is launch-breaking rather than cosmetic. On a managed host the gateway and an interactive hostUsers CLI share one $HERMES_HOME at two uids through the hermes group, and every candidate binary reuses this one log path — so a 0600 log created by whichever ran first makes the other's truncating open fail with EACCES. Reproduced end to end: _open_launch_stderr_log raises PermissionError, and launch_chrome_debug then returns launched=False with every candidate spawn-failed. That loses the browser, not just the diagnostic — the exact "fix that destroys the feature it secures" shape the rubric rejects. Flagging it explicitly since it's beyond what Copilot caught.

Non-managed behaviour is unchanged

Re-measured all six scenarios from the PR body across chrome-debug, cache/vision and cache/video, then diffed against the previous push. Every directory row is byte-identical; the only line that moved in the whole matrix is the managed-fresh launch-stderr.log row above, and that is a restoration of the merge base (0o644 at umask 022, 0o660 at UMask=0007), not a new value.

scenario chrome-debug cache/vision cache/video
default 0o700 0o700 0o700
HERMES_SKIP_CHMOD=1 0o700 0o700 0o700
HERMES_CONTAINER=1 0o700 0o700 0o700
HERMES_HOME_MODE=0701 0o701 0o701 0o701
managed, pre-existing 0750 0o750 0o750 0o750
managed, dir absent 0o755 0o755 0o755

Clearing needs-repro (@alt-glitch)

Real run on this machine just now, default umask, temp HERMES_HOME, merge-base call shapes (os.makedirs(d, exist_ok=True) + open(path, "wb"), straight out of launch_chrome_debug):

$ umask
022
$ ls -ld $HERMES_HOME/chrome-debug
drwxr-xr-x@ 3 zhanghjay  wheel  96 Aug  4 19:57 /tmp/hermes-repro-GCph/.hermes/chrome-debug
$ ls -l $HERMES_HOME/chrome-debug
total 8
-rw-r--r--@ 1 zhanghjay  wheel  18 Aug  4 19:57 launch-stderr.log

0755 directory, 0644 log. And it is a real Chromium user-data-dir — --user-data-dir points straight at it (_chrome_debug_args). Launched real Chrome 150.0.7871.187 headless against that same path:

$ curl -s http://127.0.0.1:59223/json/version
{ "Browser": "Chrome/150.0.7871.187", "Protocol-Version": "1.3", ... }

$ find $HERMES_HOME/chrome-debug -name 'Cookies' -o -name 'Login Data' -o -name 'Local Storage' -o -name 'Local State'
-rw-------@  .../chrome-debug/Default/Login Data
drwx------@  .../chrome-debug/Default/Local Storage
-rw-------@  .../chrome-debug/Default/Cookies
-rw-------@  .../chrome-debug/Local State

So CDP came up and the credential stores landed in that 0755 directory. Worth being exact about the boundary, because it sharpens rather than weakens the case: Chromium protects its own files (Default/ is 0700, the stores 0600). What the umask-derived 0755 actually exposed is the top-level listing plus the 0644 launch-stderr.log — which is why that file, being the fixed-name one, is the concrete exposure here and why leaving an existing one at 0644 mattered. The broader framing stands: given the directory to create itself, Chromium picks 0700, and Hermes pre-creating it at 0755 was overriding the browser's own choice on the directory holding its Cookies and Login Data.

Tests

6 new, all asserting the contract (no group/other bits survive; owner keeps access) rather than freezing an octal:

  • pre-existing group/other-readable log ends up owner-only
  • not exposed while it holds bytes (pins the reconcile-before-write ordering)
  • a live descriptor keeps working across the tighten
  • managed mode leaves an existing log alone
  • managed fresh log keeps group sharing
  • container carve-out inherited, log still writable

Teeth check: 4 of the 6 fail without this change (…is_healed, …while_it_holds_bytes, …running_browsers_handle_working, …managed_mode_fresh_…keeps_group_sharing). The other two are the managed/container carve-out guards, which pass either way by construction — they exist to catch over-reach, not to prove the fix.

One portability note: the healing test pins _is_container() to False, because _secure_file sniffs for /.dockerenv and /proc/1/cgroup — not just the env vars the fixture clears — so a suite running inside a container would otherwise skip the tighten and fail for an ambient reason unrelated to this code. The container carve-out gets its own test in the other direction.

./scripts/run_tests.sh tests/hermes_cli/test_browser_connect_profile_permissions.py \
  tests/tools/test_vision_tools_media_cache_permissions.py \
  tests/computer_use/test_computer_use_vision_cache_permissions.py
=== Summary: 3 files, 55 tests passed, 0 failed ===   (was 49)

Regression sweep, 45 files touching browser_connect / vision_tools / computer_use: 1305 passed, 3 failed. All 3 reproduce on clean upstream/main (f5be9236e) — test_computer_use.py::TestCaptureAppFilterNoMatch::test_linux_default_capture_skips_gnome_shell_helper and two test_model_tools_async_bridge.py::TestVisionDispatchLoopSafety — so not mine. scripts/check-windows-footguns.py clean; ruff isn't installed in this env, so that one is unverified rather than passing.

No workflow has ever run on this PR (fork check-suites sit at action_required pending maintainer approval), so the above is the only evidence that exists for it.

@andrexibiza

Copy link
Copy Markdown
Contributor

Good catch on the guessable-filename surface — that is exactly the risk this PR targets, and the current code already reconciles the existing file on the non-managed path. After the os.open/os.fdopen handle is created, the function calls hermes_cli.config._secure_file(path) (browser_connect.py:275-277 in this branch), which os.chmod(path, 0o600)s any existing inode. The test test_preexisting_group_readable_stderr_log_is_healed in tests/hermes_cli/test_browser_connect_profile_permissions.py pins this: it pre-creates launch-stderr.log at 0644 with real content, runs _open_launch_stderr_log, and asserts not (mode & GROUP_OTHER_BITS) — failing if the reconcile step is dropped. So an upgrading install with a 0644 log from an older Hermes gets tightened to 0600 on the next launch.

It looks like the review comment was written against an earlier push where the _secure_file reconcile call was not yet present. The docstring at browser_connect.py:231 already states this behavior; if the wording is unclear enough that the review missed it, a doc clarification is worth making.

@andrexibiza

Copy link
Copy Markdown
Contributor

Correct — Path.mkdir(mode=0o700) is subject to the process umask, and the comment phrasing is misleading. With umask 022 the result is 0o700 & ~0o022 = 0o700, so the group/other bits can't be widened, but the comment implies the umask doesn't apply at all. This matters precisely because it could give a false sense of security if someone reuses the pattern with a broader mode (e.g. 0o750).

The same comment appears in tools/vision_tools.py:144 as well — the review body references vision_tools.py as if it had different/correct wording, but it has the identical inaccurate line. Both should be corrected to something like: "mode= is honored only for the leaf; umask can clear owner bits but cannot widen group/other bits past 0o700 — _secure_dir reconciles anything unusual."

@alt-glitch alt-glitch removed needs-repro Bug needs reproduction steps sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have tool/browser Browser automation (CDP, Playwright) tool/vision Vision analysis and image generation type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants