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}")