fix(skills): block path traversal during quarantine install

[Gutslabs]

What does this PR do?

Fixes a path traversal issue in Skills Hub quarantine/install handling.

Previously, quarantine_bundle() trusted bundle-controlled file paths and wrote them to disk before scanning. That meant a malicious bundle could use absolute paths or .. segments to write outside the quarantine directory before the security scan ran.

This change validates bundle names, bundle file paths, and install targets before writing or moving anything on disk.

Type of Change

• Bug fix
• Security fix
• Tests

Changes Made

• Reject absolute paths and traversal segments in skill bundle file paths
• Reject invalid bundle names and install target paths
• Block invalid bundle installs cleanly in the CLI with audit logging
• Added regression tests covering quarantine and install path validation

How to Test

1. Run source .venv/bin/activate
2. Run pytest -o addopts='' tests/tools/test_skills_hub.py -q
3. Confirm bundles with paths like /tmp/file.txt or ../../../outside.txt are rejected
4. Confirm normal nested skill files like assets/... still install correctly

[nidhishgajjar]

Orb Code Review (powered by GLM 5.1 on Orb Cloud)

Summary

Security fix that prevents path traversal attacks during skill quarantine and installation. A malicious skill bundle could previously use ../ sequences or absolute paths in file names/paths to write files outside the intended directory (e.g., ../../../etc/cron.d/backdoor). The PR adds _sanitize_bundle_subpath() which validates and normalizes all bundle-controlled paths.

Architecture

The fix is well-placed:

• _sanitize_bundle_subpath() in tools/skills_hub.py is a standalone validation function with clear semantics
• quarantine_bundle() now validates bundle.name (single segment) and all file paths (nested allowed) before writing
• install_from_quarantine() validates skill_name and category before constructing the install directory
• CLI layer (hermes_cli/skills_hub.py) catches ValueError from both functions, cleans up, and logs to the audit trail

Issues

Warning — Redundant file_dest safety after sanitization in quarantine_bundle:

After _sanitize_bundle_subpath has already validated that rel_path is relative with no .. components, the code does:

file_dest = dest / rel_path

This is safe because PurePosixPath normalization + the .. check ensures file_dest is always under dest. However, for defense-in-depth, consider adding an assertion:

file_dest = dest / safe_rel_path
assert file_dest.resolve().is_relative_to(dest.resolve()), f"path escaped quarantine: {safe_rel_path}"

This is optional since the current validation is correct, but a belt-and-suspenders resolve().is_relative_to() check would catch any future regressions in _sanitize_bundle_subpath.

Suggestion — The re.match(r"^[A-Za-z]:/", normalized) check for Windows absolute paths is good but doesn't cover UNC paths (\\server\share). On Linux hosts this isn't exploitable, but if the agent ever runs on Windows:

if normalized.startswith("//") or normalized.startswith(r"\\"):
    raise ValueError(...)

Minor since the backslash→forward-slash normalization already converts \\ to //, and PurePosixPath("//server/share").parts == ('//server', 'share') which would pass the .. check but still be relative to dest. Low risk.

Cross-file impact

• hermes_cli/skills_hub.py correctly imports the new append_audit_log and wraps both quarantine_bundle() and install_from_quarantine() in try/except ValueError. The cleanup with shutil.rmtree(q_path, ignore_errors=True) on install failure is correct.
• Tests in tests/tools/test_skills_hub.py cover both the quarantine and install paths with good edge cases.

Assessment

approve ✅ — Solid security fix that addresses a real attack vector (path traversal via crafted skill bundles). The centralized _sanitize_bundle_subpath() function is the right approach. Good test coverage of the attack scenarios.

[nidhishgajjar]

Orb Code Review (powered by GLM 5.1 on Orb Cloud)

Summary

Security fix that prevents path traversal attacks during skill quarantine and installation. A malicious skill bundle could previously use ../ sequences or absolute paths in file names/paths to write files outside the intended directory (e.g., ../../../etc/cron.d/backdoor). The PR adds _sanitize_bundle_subpath() which validates and normalizes all bundle-controlled paths.

Architecture

The fix is well-placed:

• _sanitize_bundle_subpath() in tools/skills_hub.py is a standalone validation function with clear semantics
• quarantine_bundle() now validates bundle.name (single segment) and all file paths (nested allowed) before writing
• install_from_quarantine() validates skill_name and category before constructing the install directory
• CLI layer (hermes_cli/skills_hub.py) catches ValueError from both functions, cleans up, and logs to the audit trail

Issues

Warning — Redundant file_dest safety after sanitization in quarantine_bundle:

After _sanitize_bundle_subpath has already validated that rel_path is relative with no .. components, the code does:

file_dest = dest / rel_path

This is safe because PurePosixPath normalization + the .. check ensures file_dest is always under dest. However, for defense-in-depth, consider adding an assertion:

file_dest = dest / safe_rel_path
assert file_dest.resolve().is_relative_to(dest.resolve()), f"path escaped quarantine: {safe_rel_path}"

This is optional since the current validation is correct, but a belt-and-suspenders resolve().is_relative_to() check would catch any future regressions in _sanitize_bundle_subpath.

Suggestion — The re.match(r"^[A-Za-z]:/", normalized) check for Windows absolute paths is good but doesn't cover UNC paths (\\server\share). On Linux hosts this isn't exploitable, but if the agent ever runs on Windows:

if normalized.startswith("//") or normalized.startswith(r"\\"):
    raise ValueError(...)

Minor since the backslash→forward-slash normalization already converts \\ to //, and PurePosixPath("//server/share").parts == ('//server', 'share') which would pass the .. check but still be relative to dest. Low risk.

Cross-file impact

• hermes_cli/skills_hub.py correctly imports the new append_audit_log and wraps both quarantine_bundle() and install_from_quarantine() in try/except ValueError. The cleanup with shutil.rmtree(q_path, ignore_errors=True) on install failure is correct.
• Tests in tests/tools/test_skills_hub.py cover both the quarantine and install paths with good edge cases.

Assessment

approve ✅ — Solid security fix that addresses a real attack vector (path traversal via crafted skill bundles). The centralized _sanitize_bundle_subpath() function is the right approach. Good test coverage of the attack scenarios.

[teknium1]

Thanks @Gutslabs — same vulnerability class, valid catch. Closing as redundant: this was independently fixed in #3986 (merged 2026-03-30), which landed _normalize_bundle_path() plus _validate_skill_name / _validate_category_name / _validate_bundle_rel_path helpers and wired them into quarantine_bundle(), install_from_quarantine(), and the zip-extraction path. Same mitigation as your PR; #3986 also covers the zip path your PR didn't touch.

Appreciate the report and the regression tests — please keep them coming.