fix(skills): fetch explicitly linked same-directory siblings on install (#96310)
_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.
This commit is contained in:
@@ -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")
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user