fix(agent): wrap read_file/terminal results from externally-fetched paths as untrusted - #57712
Conversation
…aths as untrusted read_file and terminal were never in _UNTRUSTED_TOOL_NAMES, so content read back from a git-cloned or curl/wget-downloaded path (e.g. reviewing a PR, checking a dependency) got none of the <untrusted_tool_result> framing that web_extract/browser_*/mcp_* results get, even though it's exactly as attacker-controllable. Adding read_file to the tool-name set alone wouldn't have closed it either, since terminal (cat/type) reads the same content. Track filesystem provenance instead: recognize git clone/gh repo clone/ curl/wget in terminal commands, remember the destination as an untrusted root for the session, and wrap read_file/terminal results that target (or, for terminal, reference) one of those roots. Reading the user's own project files is unaffected. Fixes NousResearch#57710 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real provenance gap: current main still leaves read_file and terminal outside _is_untrusted_tool() (agent/tool_dispatch_helpers.py:482-505).
Problems
- The proposed
_fs_cwduses onlyenv.cwd, butterminalsupports a per-callworkdirand resolves execution through it (tools/terminal_tool.py:2035,2390-2395). A clone underworkdir="/tmp"would be recorded relative to the session cwd, missing subsequent reads of/tmp/<repo>. - Current main's output-risk metadata checks only
_is_untrusted_tool(name)(agent/tool_dispatch_helpers.py:508-516). The newpath_untrustedwrapper would not classify cloned-file content or emit the existing risk callback (agent/tool_executor.py:984-1000,1668-1684).
Suggested changes
- Use terminal's effective per-call cwd for provenance and cover that flow in an executor-level test.
- Carry provenance into
_tool_output_risk_metadataand preserve currenteffect_disposition/ risk-callback handling when salvaging onto main.
Automated hermes-sweeper review.
Addresses teknium1's review on NousResearch#57712: terminal accepts a per-call workdir that overrides the session cwd for that command (tools/terminal_tool.py::_resolve_command_cwd — "workdir= must still override everything"), but the fs-provenance cwd computed at both tool_executor.py dispatch sites only looked at env.cwd, ignoring it. A git clone/curl/wget issued with an explicit workdir got its untrusted root recorded relative to the wrong directory, so a later read_file/terminal read of that same clone silently skipped the <untrusted_tool_result> wrapper. Reuse _resolve_command_cwd itself at both call sites so provenance tracking resolves cwd exactly the way terminal actually executes the command. Add a regression test that reproduces the exact failure mode (clone under an explicit workdir, then read_file on its real absolute path) and confirms it fails without the fix and passes with it. teknium1's second point (route path_untrusted into an existing "_tool_output_risk_metadata" / risk-callback system) doesn't apply: no such risk-metadata or risk-callback plumbing exists anywhere in this codebase tied to _is_untrusted_tool — grepped for effect_disposition/risk_callback/_tool_output_risk_metadata repo-wide, no hits. Left alone rather than fabricating scaffolding the codebase doesn't have. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nal-untrusted-provenance # Conflicts: # agent/tool_dispatch_helpers.py # agent/tool_executor.py
Completes teknium1's second review point on NousResearch#57712, which turned out to be real rather than hypothetical — it just didn't exist yet on the stale branch this PR was opened from (2122 commits behind upstream/main). After merging upstream/main in, agent/tool_dispatch_helpers.py:: make_tool_result_message already runs _tool_output_risk_metadata(name, content) gated solely by _is_untrusted_tool(name), and tool_executor.py already fires a "tool.output_risk" progress callback when that metadata scores risk != "low". A read_file/terminal result forced untrusted by provenance (path_untrusted=True) went through the content wrapper but never through this scan or callback — a cloned repo's poisoned README got the <untrusted_tool_result> framing but no risk classification. _tool_output_risk_metadata now takes the same force_untrusted parameter _maybe_wrap_untrusted already had, and make_tool_result_message passes path_untrusted through to it. No call-site changes needed beyond that: tool_executor.py's two risk-callback blocks already read whatever make_tool_result_message put in _tool_output_risk, so provenance-driven findings flow through the existing callback for free. Also fixes _resolve_command_cwd usage broken by the merge: upstream/main replaced its env-based cwd lookup with a session_key + durable get_session_cwd() record (tools/terminal_tool.py), so tool_executor.py's two fs-provenance call sites now resolve session_key the same way terminal_tool() itself does (get_current_session_key() falling back to task_id) instead of passing the no-longer-accepted env= kwarg. Both fixes verified with a manual mutation check: temporarily reverting each one locally reproduces the exact failure the corresponding test (tests/agent/test_tool_dispatch_helpers.py::TestFsProvenanceEndToEnd:: test_path_untrusted_read_file_gets_risk_metadata_like_web_extract and tests/run_agent/test_tool_executor_fs_untrusted_workdir.py) is meant to catch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Reviewed the diff end-to-end. The provenance-based approach is the right call over blanket-marking tool names, and the workdir resolution + risk-metadata propagation cover the gaps the earlier review flagged. CI is green and the tests are solid. One real bypass remains that this PR doesn't close, worth either fixing here or explicitly scoping into a follow-up issue:
|
|
Related provenance source found on current main: gateway-uploaded attachments are cached by Hermes and several adapters inline small UTF-8 attachment bodies directly into I opened #103689 to keep that source distinct rather than expanding this PR silently. Architecturally it looks compatible with this PR's direction: gateway attachment paths could seed the same session-level untrusted filesystem provenance, while inline attachment text needs data-only framing before it is concatenated with the user caption. No need for a parallel scanner/registry. This is not intended as a blocker for #57712; it is a concrete follow-up/source that the provenance model should eventually accept. |
Fixes #57710
Problem
`UNTRUSTED_TOOL_NAMES` (agent/tool_dispatch_helpers.py) wraps `web_extract`/`web_search`/`browser`/`mcp_` results in `<untrusted_tool_result>` delimiters — the promptware defense against indirect prompt injection from #496. `read_file`/`terminal` are deliberately excluded (there's an existing test documenting this: reading the user's own project files shouldn't get wrapper noise).
That's correct for the common case, but the boundary is tool-name-based, not provenance-based. If the agent does `git clone`/`curl`/`wget` against an attacker-influenceable URL (reviewing a PR, checking a dependency, following a link from an issue), the fetched content is read back via `read_file`/`terminal` completely unwrapped — a malicious README/AGENTS.md is just as capable of indirect prompt injection as a poisoned web page, but never gets the "treat as DATA" framing.
A one-line "add read_file to the frozenset" fix wouldn't actually close this: `terminal` (`cat`/`type` on the same file) is an equally viable bypass, and unconditionally wrapping every `read_file` call would spam the wrapper on the overwhelming common case of reading the user's own repo.
Fix
Track filesystem provenance instead of blanket tool-name marking:
Heuristic in the same spirit as the existing `_is_destructive_command` — best-effort pattern matching, not full shell-semantics parsing. Exotic invocations (`sh -c` wrapping, archive extraction, `gh pr checkout`) are intentionally out of scope for this pass; noted as a limitation.
Test plan