fix(skills): reject colon in bundle path components (NTFS ADS bypass)

_normalize_bundle_path rejected absolute paths, .. traversal, and a bare
drive-letter prefix, but permitted a colon inside a later path component.
On NTFS a bundle member named scripts/helper.py:payload writes a hidden
Alternate Data Stream into the visible file scripts/helper.py. The skill
scanner walks with rglob('*'), which does not enumerate streams, so both
operator review and the guard scanner miss the executable bytes.

Reject a colon in any component (the whole class, not just the trailing
one). This subsumes the previous bare drive-letter check, which is folded
into the single colon guard. '/' is the only legal separator once
normalized, so no portable bundle path needs a colon.

Adds an OS-independent quarantine_bundle regression plus a direct
normalizer unit test covering leading/mid/trailing-component colons,
bare/qualified drive letters, and the empty stream name.

Reported-by: JoaoMarcos44 <87440198+JoaoMarcos44@users.noreply.github.com>
This commit is contained in:
kshitij 2026-08-10 10:15:41 +05:30
parent 2afa4be932
commit 6fa646e7d8
2 changed files with 64 additions and 1 deletions

View File

@ -880,6 +880,63 @@ class TestQuarantineBundleBinaryAssets:
assert not absolute_target.exists()
def test_quarantine_bundle_rejects_ads_colon_file_paths(self, tmp_path):
"""F-02: a bundle member with a colon in a component (``file.py:payload``)
is an NTFS Alternate Data Stream marker the visible file passes
``rglob``-based review while hidden, scanner-invisible bytes are written
into it. Reject it before it reaches disk, on any OS."""
import tools.skills_hub as hub
hub_dir = tmp_path / "skills" / ".hub"
with patch.object(hub, "SKILLS_DIR", tmp_path / "skills"), \
patch.object(hub, "HUB_DIR", hub_dir), \
patch.object(hub, "LOCK_FILE", hub_dir / "lock.json"), \
patch.object(hub, "QUARANTINE_DIR", hub_dir / "quarantine"), \
patch.object(hub, "AUDIT_LOG", hub_dir / "audit.log"), \
patch.object(hub, "TAPS_FILE", hub_dir / "taps.json"), \
patch.object(hub, "INDEX_CACHE_DIR", hub_dir / "index-cache"):
bundle = SkillBundle(
name="demo",
files={
"SKILL.md": "---\nname: demo\n---\n",
"scripts/helper.py:payload": "print(24680)",
},
source="well-known",
identifier="well-known:https://example.com/.well-known/skills/demo",
trust_level="community",
)
with pytest.raises(ValueError, match="Unsafe bundle file path"):
quarantine_bundle(bundle)
assert not (tmp_path / "skills" / "scripts").exists()
def test_normalize_bundle_path_rejects_colon_anywhere(self):
"""The colon guard covers the whole class, not just ``helper.py:payload``:
a colon in any component (leading drive letter, mid-path, or bare) is
rejected, while ordinary portable paths still normalize."""
from tools.skills_hub import _normalize_bundle_path
rejected = (
"scripts/helper.py:payload", # trailing-component ADS marker
"scripts/a:b.py", # mid-component colon
"a:b/scripts/helper.py", # leading-component colon
"scripts/helper.py:", # empty stream name
"C:", # bare Windows drive letter
"C:/Windows/System32", # drive-qualified absolute-ish path
)
for bad in rejected:
with pytest.raises(ValueError, match="Unsafe bundle file path"):
_normalize_bundle_path(bad, field_name="bundle file path", allow_nested=True)
# Legitimate portable paths are unaffected.
assert _normalize_bundle_path(
"scripts/helper.py", field_name="bundle file path", allow_nested=True
) == "scripts/helper.py"
assert _normalize_bundle_path(
"assets/data/sample.wav", field_name="bundle file path", allow_nested=True
) == "assets/data/sample.wav"
# ---------------------------------------------------------------------------
# GitHubSource._download_directory — tree API + fallback (#2940)

View File

@ -209,7 +209,13 @@ def _normalize_bundle_path(path_value: str, *, field_name: str, allow_nested: bo
raise ValueError(f"Unsafe {field_name}: {path_value}")
if not parts or any(part == ".." for part in parts):
raise ValueError(f"Unsafe {field_name}: {path_value}")
if re.fullmatch(r"[A-Za-z]:", parts[0]):
# Reject a colon in any component. On Windows a colon marks either a drive
# (``C:`` / ``C:foo``) or an NTFS Alternate Data Stream: a bundle member
# named ``file.py:payload`` writes hidden, scanner-invisible bytes into the
# visible ``file.py`` (rglob-based review never enumerates the stream).
# ``/`` is the only legal separator once normalized, so no portable bundle
# path needs a colon in a component.
if any(":" in part for part in parts):
raise ValueError(f"Unsafe {field_name}: {path_value}")
if not allow_nested and len(parts) != 1:
raise ValueError(f"Unsafe {field_name}: {path_value}")