fix(security): TTS output_path traversal + update ZIP symlink reject (salvage #6693 + #15881) - #32056
Merged
Conversation
text_to_speech_tool accepts an explicit output_path. Without a traversal guard, a path containing '..' components (whether prompt-injection- controlled, from a confused skill, or just a buggy caller) could escape its declared base and write the audio to a system location — e.g. `output_path='audio/../../etc/cron.d/x'` lands the file outside the intended audio cache. Reject '..' components in the user-supplied path. Explicit absolute paths are unchanged (the agent legitimately writes audio wherever the user/caller asks); only traversal-style escapes are blocked. The terminal tool can still write anywhere with approval — this just keeps the unattended TTS surface from materializing files via traversal. Regression tests cover: '..' in the middle (audio/../../etc/...), bare '..' prefix, and the negative cases (absolute paths + relative paths without '..' both pass through unchanged). Salvaged from PR #6693 by @aaronlab. The original PR confined output to DEFAULT_OUTPUT_DIR-or-cwd, which broke 9 existing tests that legitimately write to tmp_path locations. The traversal-only check covers the actual threat (path-escape via '..' from prompt injection) without restricting where users can choose to write their audio. The remaining pieces of #6693 (skill_commands rglob symlink rejection, delegate_tool batch prefix display) are dropped: - skill_commands rglob: breaks the documented design supporting ~/.hermes/skills/<name> as a symlink to a checked-out skill elsewhere (see comment at agent/skill_commands.py:73-75) - delegate_tool batch prefix: pure UX, doesn't belong in a security PR Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>
_update_via_zip downloads a source ZIP from GitHub and calls zipfile.ZipFile.extractall. The existing zip-slip path guard validates each member's path stays under tmp_dir, but does not check member type — so a ZIP containing a symlink member would still be materialized by extractall, and a symlink target could point outside the extracted tree (or to a sensitive system path). This isn't a high-likelihood threat for hermes-agent's actual GitHub source ZIPs (we don't ship symlinks), but the extractall path runs as the user's account and a compromised mirror could plant arbitrary files via the symlink → target → write chain. Reject any member whose Unix mode bits (upper 16 bits of external_attr) are S_IFLNK before extractall. Hermes source ZIPs contain only regular files and directories; a symlink member is unambiguously suspicious. Regression tests cover: symlink member rejection (raises ValueError, caught by the outer try/except as a clean SystemExit, no extraction), and the happy-path verification that a normal ZIP doesn't trigger the symlink reject message. Salvaged from PR #15881 by @codeblackhole1024. The remaining pieces of that PR were already on main or contradicted explicit design decisions: - config.yaml write-deny: already in agent/file_safety.py's control_file_names denylist (the modern guard); the proposed addition to build_write_denied_paths was the legacy path. - Quick commands danger detection: contradicts the explicit cli.py:8491-8492 comment 'shell=True is intentional: quick_commands are user-defined shell snippets from config.yaml — not agent/LLM controlled.' - Memory plugin shlex.split for dep checks: already on main (hermes_cli/memory_setup.py:133). Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
1 |
First entries
tests/hermes_cli/test_update_zip_symlink_reject.py:17: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
✅ Fixed issues: none
Unchanged: 4930 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
alt-glitch
added
type/security
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
4 participants
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.
Two narrow salvages from S04 symlink/path-traversal cluster.
Commits (rebased, contributor authorship preserved)
@aaronlab — fix(tts): reject '..' traversal in output_path (fix: skill symlink guard, delegate batch prefix, TTS path traversal and cleanup #6693)
text_to_speech_tool's
output_pathrejected by traversal-component check(
audio/../../etc/cron.d/xetc.). Explicit absolute paths unchanged —the agent legitimately writes audio anywhere the user/caller asks.
@codeblackhole1024 — fix(update): reject symlink members in update ZIP (hermes-agent: Harden quick commands and import checks #15881)
_update_via_zippreviously calledzf.extractall(tmp_dir)after onlyzip-slip path validation. A ZIP containing a symlink member would still
be materialized. Reject any member whose Unix mode bits are S_IFLNK.
Salvage scope notes
#6693 originally bundled three concerns. Salvaged TTS only. Dropped:
agent/skill_commands.pyrglob symlink rejection: would have broken thedocumented design supporting
~/.hermes/skills/<name>as a symlink to achecked-out skill elsewhere (comment at skill_commands.py:73-75).
tools/delegate_tool.pybatch prefix display: pure UX, doesn't belongin a security PR.
#6693's original TTS guard confined output to DEFAULT_OUTPUT_DIR-or-cwd,
which broke 9 existing tests writing to tmp_path locations. Narrowed to
traversal-component-only.
#15881 originally bundled four concerns. Salvaged ZIP symlink only. Dropped:
config.yamlwrite-deny: already on main viais_write_denied.cli.pycommentthat quick_commands are user-defined shell snippets from config.yaml,
not agent/LLM controlled.
shlex.splitfor dep checks: already on main.Test plan
Co-authored-by: aaronlab 1115117931@qq.com
Co-authored-by: codeblackhole1024 lichriszhang@gmail.com
Closes #6693
Closes #15881