fix: allow active tempdir writes in file tools - #13733
Conversation
alt-glitch
added
type/bug
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>
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real macOS file-tool regression. Current main still rejects paths under /private/var/ in tools/file_tools.py:606-646, and both write_file_tool and patch_tool use that guard (tools/file_tools.py:1649, 1777).
Problems
tools/file_tools.py:134in this PR acceptscandidate_normunder the temp root even whencandidate_realresolves outside it. The early allow at PR line 152 then bypasses the sensitive-path guard for a symlinked temp subdirectory targeting a protected path. This conflicts with the resolved-and-normalized symlink defense introduced by311dac197.- The new test at
tests/tools/test_file_write_safety.py:109does not force a/private/var/folders/...temp root. On a/tmprunner, current main already allows the path, as the adjacent existing assertion demonstrates.
Suggested changes
- Base the exemption on the resolved candidate and resolved active temp root only, while retaining the later exact-path and Hermes-config checks.
- Mock the macOS temp root and add a symlink-escape refusal regression test.
Automated hermes-sweeper review.
| for root in temp_roots: | ||
| if candidate_norm == root or candidate_real == root: | ||
| return True | ||
| if candidate_norm.startswith(root + os.sep) or candidate_real.startswith(root + os.sep): |
There was a problem hiding this comment.
This lexical-path branch can allow a symlink inside the temp root whose real target is protected: candidate_norm remains under the root while candidate_real escapes it, but the or still returns True. Since _check_sensitive_path then early-returns before its denylist, require resolved-candidate membership rather than accepting the lexical path.
| from tools.file_tools import _check_sensitive_path | ||
| assert _check_sensitive_path("/tmp/safe_file.txt") is None | ||
|
|
||
| def test_active_tempdir_under_private_var_allowed(self, tmp_path: Path): |
There was a problem hiding this comment.
This does not force the macOS failure path: on runners where tmp_path is under /tmp, current main already permits it. Mock tempfile.gettempdir() to a /private/var/folders/.../T root (and add a symlink-escape refusal case) so the regression is exercised on every platform.
teknium1
added
sweeper:risk-security-boundary
3 participants
Summary
/private/varsystem-path protection in place/private/var/folders/...Why
Latest upstream introduced delegate/file-state tests that write into macOS temp directories. Those temp paths resolve under
/private/var/folders/..., and the file-tools sensitive-path guard was blocking them as if they were dangerous system paths. That made harmless temp-file writes fail.Test Plan
pytest tests/tools/test_file_write_safety.pypytest tests/tools/test_delegate.py tests/tools/test_file_state_registry.py tests/tools/test_file_write_safety.py