fix(agent): skip symlink escapes when listing skill supporting files

[fangliquanflq]

What does this PR do?

When building slash-skill invocation messages, _build_skill_message falls back to Path.rglob under references/ / templates/ / scripts/ / assets/. It only skipped file symlinks, so a directory symlink (or junction) could walk host paths and inject filenames + absolute paths into the agent context.

Skip redirected supporting dirs, and require each listed file's resolved path to stay inside the skill tree.

Related Issue

N/A (found via local bug hunt; related incomplete prior attempts: #6693, #6952)

Type of Change

• Security fix
• Bug fix (non-breaking change that fixes an issue)
• Tests (adding or improving test coverage)

Changes Made

• agent/skill_commands.py: skip symlink/junction supporting dirs; resolve().is_relative_to(skill_root) gate on enumerated files
• tests/agent/test_skill_commands.py: regression tests for top-level and nested directory symlink escapes

How to Test

1. scripts/run_tests.sh tests/agent/test_skill_commands.py::TestSkillDirectoryHeader -q
2. Manually: create a skill whose scripts/ is a directory symlink to a secret dir; invoke /skill-name and confirm id_rsa (etc.) is not listed under supporting files
3. Confirm a normal skill with real scripts/run.js still shows supporting-file hints

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature
• I've run targeted tests and they pass (TestSkillDirectoryHeader)
• I've added tests for my changes
• I've tested on my platform: Windows 11

Documentation & Housekeeping

• Docs — N/A
• cli-config.yaml.example — N/A
• CONTRIBUTING.md / AGENTS.md — N/A
• Cross-platform impact considered (symlink + junction check)
• Tool descriptions/schemas — N/A

[fangliquanflq]

ok

[teknium1]

Thanks for pursuing a narrow fix for a real slash-skill path-disclosure problem.

Problems

• The new fallback guard is bypassed whenever linked_files is populated: agent/skill_commands.py:278-284 copies those entries unchanged before the guarded fallback. tools/skills_tool.py:1414-1450 builds linked_files by scanning references/, templates/, assets/, and scripts/ without a resolved-path boundary check. A redirected scripts/ containing payload.py, for example, remains listed in the invocation message.
• The new id_rsa fixture at tests/agent/test_skill_commands.py:768 only exercises the fallback because the scripts linked-file scanner accepts only *.py, *.sh, *.bash, *.js, *.ts, and *.rb (tools/skills_tool.py:1447-1450).

Suggested changes

• Filter linked_files at the same resolved-root boundary, and harden its producer in tools/skills_tool.py.
• Add redirected-directory tests using recognized linked-file extensions and verify both slash invocation output and skill_view discovery stay inside the skill root.

This is an automated hermes-sweeper review.

[fangliquanflq]

Addressed the sweeper review in 220e6ad and a969ae3. Both skill_view discovery and slash invocation now enforce resolved containment within the skill root. The regression coverage uses recognized payload.py and secret.md files, checks stale linked_files at the consumer boundary, preserves safe in-root directory links, and includes Windows-only junction coverage. Targeted validation completed with 154 passing tests and one Windows-only skip on this non-Windows runner.

[egilewski]

suggesting changes

The patch closes the direct linked-file and activation-message symlink escapes and preserves safe in-root redirects, but the missing-file fallback still enumerates skill contents without containment checks. An external symlink can therefore remain visible in the available-files response when a nonexistent support file is requested.

• [P3] Missing-file inventory still exposes redirected symlink entries
  When the requested support file is absent, the fallback accepts every discovered file without verifying that its resolved target remains inside the selected skill directory or excluding symlink/junction entries. An external link can disclose filename metadata and, on junction-capable platforms, trigger traversal outside the skill tree. The guarded normal-view and activation flows do not cover this fallback.
  Remediation: apply the same containment check to every fallback candidate before adding it to the available-files response, skip symlink/junction entries, prune redirected directories, and share one guarded iterator with linked-file enumeration.

Security evidence:

• trust boundary: Skill packages and supporting references, templates, assets, and scripts are filesystem inputs; entries exposed to the model must resolve inside the selected skill directory, while configured external skill roots remain intentional.
• source/sink/invariant: The changed linked-file, normal support-directory, and direct-read flows enforce containment, but the missing-file inventory still accepts unvalidated entries, so the source-to-model containment invariant is incomplete.
• current-main reproduction: On current main, an external symlink returned its redirected filename in the available-files response when a nonexistent support file was requested.
• PR-head or patch-replay validation: The reviewed changes filter redirected entries in linked-file and activation discovery, while the missing-file fallback still accepts them.
• positive/negative cases: External links were excluded from normal linked-file and activation inventories, an in-root redirect remained available, and the missing-file response still exposed an outside link name.
• residual bypass search: Direct reads, linked-file enumeration, activation discovery, and the missing-file fallback were checked; the unguarded fallback is the remaining bypass.
• reviewer validation: Source review and focused validation support one actionable P3 finding.

Not checked:

• Windows junction execution
• Full repository test suite

Signed: GPT-5.6-luna-max in Codex

[fangliquanflq]

Addressed the remaining supporting-file fallback bypass in de14438. Missing-file inventory and slash/preload fallback discovery now share containment-aware traversal, prune external redirects and junction cycles, and preserve safe in-root redirects. Added real Windows junction-cycle coverage and avoided decoding localized mklink output.\n\nValidation:\n- security-focused symlink/junction scope: 11 passed\n- junction regression scope after the test-only cleanup: 2 passed\n- complete related-file run: 67 passed; 3 pre-existing Windows path/inline-shell assertions failed outside this change

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(agent): skip symlink escapes when listing skill supporting files

1. Performance: _iter_contained_skill_files (tools/skills_tool.py, ~line 962) calls validate_within_dir twice per level (once for current, once for each entry), and each call does two Path.resolve() filesystem walks (path + root). Compared to the old rglob, a skill tree with hundreds of files now pays O(files) extra resolve() syscalls on every skill load / skill_view. Consider resolving each directory once per level and checking containment on the resolved dir, or short-circuiting the entry check when the parent already passed.
2. _walk relies on ordering subtlety: if validate_within_dir(current, skill_dir): return runs before the resolved in resolved_ancestors cycle guard. A junction pointing back at the skill root passes the within-check (it resolves inside), and only the ancestor check stops the cycle — correct, but a one-line comment explaining why the ordering matters would prevent a future refactor from breaking cycle safety.
3. _build_skill_message (agent/skill_commands.py, ~line 335) now drops non-str and escaping linked_files entries silently. The escaping entries are skipped without logging — for an installed skill whose stale index points outside the root, a debug log would aid diagnosis (the old code surfaced them in the message).
4. Semantics verified against validate_within_dir (returns None when safe, truthy error string on escape) — the filter direction is correct, and the symlink/junction regression tests (POSIX symlinks, Windows junctions, .hub cycles) cover the attack surface well.