Skip to content

fix(tools): allow file writes under the active temp dir on macOS (salvage #13733) - #41285

Open
HeLLGURD wants to merge 3 commits into
NousResearch:mainfrom
HeLLGURD:salvage/13733-tempdir-file-writes
Open

fix(tools): allow file writes under the active temp dir on macOS (salvage #13733)#41285
HeLLGURD wants to merge 3 commits into
NousResearch:mainfrom
HeLLGURD:salvage/13733-tempdir-file-writes

Conversation

@HeLLGURD

@HeLLGURD HeLLGURD commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Salvage of #13733 by @matt-dean-git. Re-verified the bug on current main and
re-applied the fix to the current _check_sensitive_path (its signature changed
since the original PR, leaving #13733 unmergeable). Regression test ported.

The bug (still on main) - a cross-platform defect

tools/file_tools.py blocks writes to sensitive system paths via
_SENSITIVE_PATH_PREFIXES, which (correctly) includes /private/var/:

_SENSITIVE_PATH_PREFIXES = (
    "/etc/", "/boot/", "/usr/lib/systemd/",
    "/private/etc/", "/private/var/",
)

On macOS, the OS temp directory (tempfile.gettempdir()) resolves under
/private/var/folders/.... So any harmless write into the process temp dir -
including the temp files that delegate / file-state tests and ordinary agent
work create - trips the /private/var/ guard and is refused. The same code
works fine on Linux (temp dir is /tmp), so this is a macOS-specific
regression.

The fix

Allow writes that resolve inside the active process temp directory before
the sensitive-prefix check:

if _is_within_active_tempdir(resolved) or _is_within_active_tempdir(normalized):
    return None
for prefix in _SENSITIVE_PATH_PREFIXES:
    ...

_is_within_active_tempdir compares the candidate (both normalized and
realpath-resolved) against tempfile.gettempdir() (also normalized and
realpath-resolved), matching the dir itself or anything beneath it.

This does not weaken the guard for real system paths: /etc, /boot,
/usr/lib/systemd, /private/etc, the docker sockets, and the Hermes config
file are all outside the OS temp dir, so they remain blocked. Only the OS temp
directory - a per-user scratch location - is allowed.

Test (ported from the original PR)

tests/tools/test_file_write_safety.py::test_active_tempdir_under_private_var_allowed

  • pytest''s tmp_path lives under the OS temp dir (under /private/var/folders
    on macOS), and the test asserts _check_sensitive_path returns None for it.

Verification

  • Confirmed /private/var/ is in _SENSITIVE_PATH_PREFIXES on current main.
  • Confirmed _is_within_active_tempdir does not exist and tempfile was not
    imported on main (both added).
  • Adapted to main''s current _check_sensitive_path(filepath, task_id="default")
    signature (the original PR predated the task_id parameter).
  • All anchors matched current main exactly before patching.

Credit to @matt-dean-git for the original fix, test, and analysis (#13733).

HeLLGURD and others added 2 commits June 7, 2026 16:08
Salvaged from NousResearch#13733 (matt-dean-git). macOS temp paths resolve under
/private/var/folders/..., which trips the /private/var sensitive-path guard
and blocks harmless temp-file writes. Re-applied to current main (the guard
function signature changed). 

Co-authored-by: matt-dean-git <matt-dean-git@users.noreply.github.com>
Salvaged from NousResearch#13733 (matt-dean-git).

Co-authored-by: matt-dean-git <matt-dean-git@users.noreply.github.com>
@daimon-nous daimon-nous Bot added type/bug Something isn't working tool/file File tools (read, write, patch, search) P2 Medium — degraded but workaround exists labels Jun 7, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verified — temp dir path check bypass is narrowly scoped

Reviewed tools/file_tools.py and tests/tools/test_file_write_safety.py for the _is_within_active_tempdir guard.

  • Scope: The bypass only matches tempfile.gettempdir() (the current process's temp root), not arbitrary /private/var paths. Both os.path.normpath and os.path.realpath are checked, so symlinked temp dirs (macOS /tmp/private/tmp) are handled correctly.
  • Ordering: The temp-dir check runs before the _SENSITIVE_PATH_PREFIXES loop, so it's an early-exit allow — confirmed it doesn't weaken the deny for non-temp paths under /private/var.
  • Test coverage: test_active_tempdir_under_private_var_allowed uses pytest's tmp_path which resolves to /private/var/folders/... on macOS — directly exercises the reported failure case.

The fix is correct and well-scoped. No issues found.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for preserving a real macOS regression: current main still applies the /private/var/ deny prefix in tools/file_tools.py:635-638,673-675, while the repository documents macOS tempdirs under /private/var/folders/.../T (hermes_cli/codex_runtime_plugin_migration.py:531-554).

Problems

  • The new active-tempdir early return is before the Hermes config protection at tools/file_tools.py:678-688. A config located beneath the active tempdir would become writable, defeating the guard added in 8f2931e3ee518ddbb78789fc013fbc69aa868851; existing coverage deliberately uses a tmp_path config fixture (tests/tools/test_file_tools.py:596-628).
  • The added tmp_path test only exercises the new branch on macOS. On Linux it remains under already-allowed /tmp (tests/tools/test_file_write_safety.py:194-196).

Suggested changes

  • Check exact sensitive paths and the Hermes config before allowing the active temp directory, then retain the tempdir exception ahead of the prefix loop.
  • Mock tempfile.gettempdir() to a /private/var/folders/.../T root and assert both that its child is allowed and that a sibling /private/var/db/... path remains denied.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
…ig checks

Address the hermes-sweeper review on NousResearch#41285. The active-tempdir allow-list ran
before the exact-path and Hermes-config checks, so a docker.sock or a relocated
config.yaml living under the OS temp dir would become writable, defeating the
config guard. Reorder _check_sensitive_path so the exact-path and Hermes-config
checks run first, keep the tempdir exception ahead of the /private/var prefix
loop. Add mocked tests (gettempdir -> /private/var/folders/.../T): child of the
temp root allowed, sibling /private/var/db denied, config under the temp root
still refused.
@HeLLGURD

Copy link
Copy Markdown
Contributor Author

Thanks for the review - addressed in the latest commit:

  • Reordered _check_sensitive_path so the exact-path (docker.sock) and Hermes-config checks run before the active-tempdir exception, and kept the tempdir exception ahead of the /private/var prefix loop. A relocated config.yaml or a sensitive socket under the OS temp dir is now still refused.
  • Added mocked tests (tempfile.gettempdir -> /private/var/folders/.../T): a child of the temp root is allowed, a sibling /private/var/db/... stays denied, and a Hermes config under the temp root is still blocked - so the exception is narrowly scoped and can't be used to disable exec approval.

@alt-glitch alt-glitch removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants