From 6fa646e7d8955688e5c65b19ddc3b4ee9097a004 Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:15:41 +0530 Subject: [PATCH] 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> --- tests/tools/test_skills_hub.py | 57 ++++++++++++++++++++++++++++++++++ tools/skills_hub.py | 8 ++++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_skills_hub.py b/tests/tools/test_skills_hub.py index f894e8adff7da..99ad2baedde84 100644 --- a/tests/tools/test_skills_hub.py +++ b/tests/tools/test_skills_hub.py @@ -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) diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 4a02998805a48..53d0e7002652b 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -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}")