From 7e95b67ad8587e298ef6ca665e00bdcd1997e1d8 Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Thu, 27 Aug 2026 21:01:00 +0800 Subject: [PATCH] =?UTF-8?q?fix(skills):=20review=20follow-up=20=E2=80=94?= =?UTF-8?q?=20revision=20pinning,=20canonicalization,=20case-fold=20guards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #96336: - Every GitHub byte fetch in an install (SKILL.md included) now carries the resolved tree's SHA as ?ref=, closing the pre-existing TOCTOU where /contents floated to the default-branch HEAD and bytes could come from a newer revision than the tree the paths were validated against. The tree is resolved first (idempotent + cached) so the pin covers the whole install. - Same-dir link targets are canonicalized before validation: query/ fragment stripped via urlsplit, percent-decoded, leading ./ removed — the same normalization the support-dir branch applies. - A case-variant link to skill.md never ships as a bundle entry, and a case-folded collision among accepted siblings (A.md + a.md) drops the pair — both would overwrite/collide on case-insensitive filesystems. --- tests/tools/test_skill_bundle_provenance.py | 69 ++++++++++++++++++- tools/skills_hub.py | 75 +++++++++++++++++---- 2 files changed, 130 insertions(+), 14 deletions(-) diff --git a/tests/tools/test_skill_bundle_provenance.py b/tests/tools/test_skill_bundle_provenance.py index 79040e7ffe..2bf1e88ce6 100644 --- a/tests/tools/test_skill_bundle_provenance.py +++ b/tests/tools/test_skill_bundle_provenance.py @@ -146,13 +146,78 @@ def test_same_dir_link_without_extension_is_ignored(monkeypatch): """Prose targets that aren't file links (no extension) never fetch.""" from tools.skills_hub import _referenced_support_paths - skill = "---\nname: x\ndescription: x\n---\nsee [notes](NOTES) and `README`\n" + skill = "---\nname: x\ndescription: x\---\nsee [notes](NOTES) and `README`\n" assert _referenced_support_paths(skill) == set() +def test_same_dir_link_query_and_fragment_are_stripped(): + """?query and #fragment never leak into the fetched bundle path.""" + from tools.skills_hub import _referenced_support_paths + + skill = ( + "---\nname: x\ndescription: x\---\n" + "[a](CONTEXT-FORMAT.md?raw=1) [b](DEEPENING.md#usage)\n" + ) + assert _referenced_support_paths(skill) == {"CONTEXT-FORMAT.md", "DEEPENING.md"} + + +def test_case_variant_of_skill_md_is_never_a_sibling_entry(): + """skill.md must not ship as a bundle file (case-insensitive FS collision).""" + from tools.skills_hub import _referenced_support_paths + + skill = "---\nname: x\ndescription: x\---\n[home](skill.md)\n" + assert _referenced_support_paths(skill) == set() + + +def test_case_folded_sibling_collision_drops_the_pair(): + """A.md + a.md would collide on install — neither ships.""" + from tools.skills_hub import _referenced_support_paths + + skill = "---\nname: x\ndescription: x\---\n[a](A.md) [a2](a.md)\n" + assert _referenced_support_paths(skill) == set() + + +def test_github_fetches_pin_to_the_tree_revision(monkeypatch): + """#96310 review: every byte fetch carries the tree's SHA as ?ref=.""" + source = GitHubSource(GitHubAuth()) + fetched: list[tuple[str, dict | None]] = [] + + def _fake_content(repo, path, ref=None): + fetched.append((path, {"ref": ref} if ref else None)) + return SKILL_MD if path.endswith("SKILL.md") else "x" + + monkeypatch.setattr(source, "_fetch_file_content", _fake_content) + monkeypatch.setattr( + source, + "_fetch_file_bytes", + lambda repo, path, ref=None: fetched.append((path, {"ref": ref} if ref else None)) or b"x", + ) + source._tree_cache["owner/repo"] = ( + "main", + [ + {"path": "skill/SKILL.md", "type": "blob", "mode": "100644"}, + {"path": "skill/CONTEXT-FORMAT.md", "type": "blob", "mode": "100644"}, + ], + ) + source._tree_revisions["owner/repo"] = "treesha123" + + minimal_skill = "---\nname: x\ndescription: x\n---\nSee [the format](CONTEXT-FORMAT.md).\n" + monkeypatch.setattr( + source, "_fetch_file_content", + lambda repo, path, ref=None: fetched.append((path, {"ref": ref} if ref else None)) or minimal_skill, + ) + + bundle = source.fetch("owner/repo/skill") + + assert bundle is not None + assert fetched, "expected byte fetches" + for path, params in fetched: + assert params == {"ref": "treesha123"}, path + + def test_github_source_rejects_symlink_in_referenced_directory(monkeypatch): source = GitHubSource(GitHubAuth()) - monkeypatch.setattr(source, "_fetch_file_content", lambda _repo, path: SKILL_MD if path.endswith("SKILL.md") else "x") + monkeypatch.setattr(source, "_fetch_file_content", lambda _repo, path, ref=None: SKILL_MD if path.endswith("SKILL.md") else "x") source._tree_cache["owner/repo"] = ( "main", [ diff --git a/tools/skills_hub.py b/tools/skills_hub.py index eb28d55dca..17e948ecac 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -229,26 +229,51 @@ def _referenced_support_paths(skill_md: str) -> Optional[set[str]]: paths.add(safe) for match in _SAMEDIR_LINK_RE.finditer(normalized): raw = match.group(1).rstrip(".,;:") + # Canonicalize first: drop query/fragment components (``?raw=1``, + # ``#section``) and percent-decode — the same normalization the + # support-dir branch applies via urlsplit+unquote — then strip a + # leading ``./``. The set below deduplicates case-SENSITIVE repeats; + # case-VARIANT collisions are rejected rather than merged (below). + name = unquote(urlsplit(raw).path) + name = name[2:] if name.startswith("./") else name # External URLs, anchors, mailto and site-absolute targets are not # same-directory file links — leave them to their own resolution. - if "://" in raw or raw.startswith(("mailto:", "#", "/")): + if not name or "://" in raw or raw.startswith(("mailto:", "#", "/")): continue - name = raw[2:] if raw.startswith("./") else raw # A ``..`` prefix is a traversal attempt — same fail-closed contract # as the support-dir branch above, before any shape-based skipping. if name.startswith(".."): return None # Only unambiguous file links: an extension, no internal slash, and - # never SKILL.md itself (that IS the bundle root). - if "/" in name or name == "SKILL.md" or "." not in name.lstrip("."): + # never SKILL.md itself (that IS the bundle root). The casefold + # check keeps a ``skill.md`` link from shipping as a bundle entry + # that collides with SKILL.md on case-insensitive filesystems + # (macOS/Windows) — skipped, not merged, so the bundle root is + # never overwritten (#96310 review). + if ( + "/" in name + or name.casefold() == "skill.md" + or "." not in name.lstrip(".") + ): continue - if not _SAMEDIR_NAME_RE.match(raw): + if not _SAMEDIR_NAME_RE.match(name): continue try: safe = _validate_bundle_rel_path(name) except ValueError: return None paths.add(safe) + # Case-folded collision among the accepted same-dir names themselves + # (``A.md`` + ``a.md``) would also collide on install — drop the pair + # rather than guess which variant the author meant. + folded: dict[str, str] = {} + for p in sorted(paths): + key = p.casefold() + if key in folded: + paths.discard(folded[key]) + paths.discard(p) + else: + folded[key] = p return paths @@ -733,7 +758,19 @@ class GitHubSource(SkillSource): repo = f"{parts[0]}/{parts[1]}" skill_path = parts[2] - skill_md = self._fetch_file_content(repo, f"{skill_path.rstrip('/')}/SKILL.md") + # Resolve the tree FIRST so every byte fetch in this install — + # SKILL.md included — can be pinned to the same revision. Without the + # pin the /contents endpoint floats to the default-branch HEAD and + # the downloaded bytes can come from a NEWER revision than the tree + # the paths were validated against (TOCTOU between "the tree says + # this is a regular blob" and "what actually gets downloaded"). + # Idempotent + cached, so callers that already primed the tree pay + # nothing extra. + tree = self._get_repo_tree(repo) + pinned_ref = self._tree_revisions.get(repo) + skill_md = self._fetch_file_content( + repo, f"{skill_path.rstrip('/')}/SKILL.md", ref=pinned_ref + ) if skill_md is None: return None referenced = _referenced_support_paths(skill_md) @@ -741,7 +778,6 @@ class GitHubSource(SkillSource): return None files: Dict[str, Union[str, bytes]] = {"SKILL.md": skill_md} - tree = self._get_repo_tree(repo) if tree is not None: # Download the FULL skill directory, not just SKILL.md-linked # paths. Link-driven fetching silently dropped every support file @@ -770,7 +806,7 @@ class GitHubSource(SkillSource): except ValueError: logger.warning("Rejected unsafe file path in skill bundle: %s", item_path) return None - content = self._fetch_file_bytes(repo, item_path) + content = self._fetch_file_bytes(repo, item_path, ref=pinned_ref) if content is None: logger.warning("Failed to fetch referenced skill support " "file; continuing without it: %s", item_path) @@ -1169,9 +1205,11 @@ class GitHubSource(SkillSource): return None - def _fetch_file_content(self, repo: str, path: str) -> Optional[str]: + def _fetch_file_content( + self, repo: str, path: str, ref: Optional[str] = None + ) -> Optional[str]: """Fetch a single text file from GitHub.""" - content = self._fetch_file_bytes(repo, path) + content = self._fetch_file_bytes(repo, path, ref=ref) if content is None: return None try: @@ -1179,12 +1217,25 @@ class GitHubSource(SkillSource): except UnicodeDecodeError: return None - def _fetch_file_bytes(self, repo: str, path: str) -> Optional[bytes]: - """Fetch exact file bytes from GitHub without text decoding.""" + def _fetch_file_bytes( + self, repo: str, path: str, ref: Optional[str] = None + ) -> Optional[bytes]: + """Fetch exact file bytes from GitHub without text decoding. + + ``ref`` pins the fetch to a specific commit/tree SHA. Without it the + contents endpoint floats to the default-branch HEAD, so the bytes can + come from a NEWER revision than the tree the paths were validated + against — a TOCTOU between "what the tree says is a regular blob" + and "what actually gets downloaded". Callers that resolved paths from + a tree pass that tree's SHA; ``None`` keeps the legacy unpinned + behavior for call sites with no revision in hand. + """ encoded_path = quote(path, safe="/") url = f"https://api.github.com/repos/{repo}/contents/{encoded_path}" + params = {"ref": ref} if ref else None resp = self._github_get( url, + params=params, headers={**self.auth.get_headers(), "Accept": "application/vnd.github.v3.raw"}, ) if resp is not None and resp.status_code == 200: