From 86dda3cd36e134e19af41e1d8979772b79245ca2 Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Thu, 27 Aug 2026 20:11:26 +0800 Subject: [PATCH] fix(skills): fetch explicitly linked same-directory siblings on install (#96310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _referenced_support_paths only kept links whose first path segment was one of the five support directories (references/templates/scripts/ assets/examples), so a SKILL.md linking same-directory siblings — mattpocock/skills' domain-modeling links ./CONTEXT-FORMAT.md and ADR-FORMAT.md — installed 'successfully' with those files silently omitted: the bundle came out semantically incomplete with unresolved links. A second pass now collects same-directory markdown-link targets (](./FILE.ext) or ](FILE.ext)) that name an extension-bearing file, carry no internal slash, and are not SKILL.md itself; a leading '..' is rejected fail-closed exactly like the support-dir traversal branch, external URLs/anchors/mailto/site-absolute targets are left to their own resolution, and every accepted name still runs the bundle path validator. Unlinked siblings remain excluded — the fetch-minimization contract is unchanged; only files the document explicitly links ship. --- tests/tools/test_skill_bundle_provenance.py | 38 +++++++++++++++++++++ tools/skills_hub.py | 33 ++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/tests/tools/test_skill_bundle_provenance.py b/tests/tools/test_skill_bundle_provenance.py index aaf4803b18..79040e7ffe 100644 --- a/tests/tools/test_skill_bundle_provenance.py +++ b/tests/tools/test_skill_bundle_provenance.py @@ -112,6 +112,44 @@ def test_url_source_rejects_traversal_reference(monkeypatch): assert source.fetch("https://example.com/bad/SKILL.md") is None +def test_same_dir_linked_siblings_are_fetched(served_repo, monkeypatch): + """#96310: explicitly linked same-skill-directory files must ship in the + bundle — dropping them made installs "succeed" with unresolved links.""" + repo, url = served_repo + (repo / "CONTEXT-FORMAT.md").write_text("format\n") + (repo / "DEEPENING.md").write_text("deepening\n") + (repo / "SKILL.md").write_text(SKILL_MD + "See [the format](./CONTEXT-FORMAT.md) and [deepening](DEEPENING.md).\n") + monkeypatch.setattr("tools.skills_hub.is_safe_url", lambda _url: True) + monkeypatch.setattr("tools.skills_hub.check_website_access", lambda _url: None) + + bundle = UrlSource().fetch(url) + + assert bundle is not None + assert bundle.files["CONTEXT-FORMAT.md"] == b"format\n" + assert bundle.files["DEEPENING.md"] == b"deepening\n" + # Unlinked siblings stay excluded — same fetch-minimization contract. + assert "README.md" not in bundle.files + + +def test_same_dir_traversal_link_is_rejected(monkeypatch): + source = UrlSource() + skill = ( + "---\nname: bad\ndescription: bad\n---\n" + "[bad](./../outside-secret.md)\n" + ) + monkeypatch.setattr(source, "_fetch_text", lambda _url: skill) + + assert source.fetch("https://example.com/bad/SKILL.md") is None + + +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" + assert _referenced_support_paths(skill) == set() + + 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") diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 221a39365c..eb28d55dca 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -187,6 +187,17 @@ def _query_is_concrete(query: str) -> bool: for part in parts ) +# Same-directory links (``](./FILE.ext)`` / ``](FILE.ext)``) — siblings of +# SKILL.md that the document explicitly links. Skills legitimately ship +# supporting docs next to SKILL.md instead of under a support directory +# (e.g. mattpocock/skills' domain-modeling links ./CONTEXT-FORMAT.md); +# dropping them made the install "succeed" while the bundle came out with +# unresolved links (#96310). The trailing extension requirement keeps prose +# words out; the code-side checks keep this strictly to the skill's own +# directory (support-dir links stay on _LOCAL_LINK_RE). +_SAMEDIR_LINK_RE = re.compile(r"\]\(([^)\s\"'<>]+)") +_SAMEDIR_NAME_RE = re.compile(r"^(?:\./)?[A-Za-z0-9][A-Za-z0-9._-]*$") + def _referenced_support_paths(skill_md: str) -> Optional[set[str]]: """Extract safe referenced paths; return None on a traversal attempt.""" @@ -216,6 +227,28 @@ def _referenced_support_paths(skill_md: str) -> Optional[set[str]]: if re.search(r"[*?<>]", safe) or "." not in safe.rsplit("/", 1)[-1]: continue paths.add(safe) + for match in _SAMEDIR_LINK_RE.finditer(normalized): + raw = match.group(1).rstrip(".,;:") + # 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:", "#", "/")): + 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("."): + continue + if not _SAMEDIR_NAME_RE.match(raw): + continue + try: + safe = _validate_bundle_rel_path(name) + except ValueError: + return None + paths.add(safe) return paths