ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 10 Packages Projects Releases Wiki Activity

fix(skills): review follow-up — revision pinning, canonicalization, case-fold guards

Browse Source 
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.
This commit is contained in:
liuhao1024
2026-08-27 21:01:00 +08:00
committed by Teknium
parent 86dda3cd36
commit 7e95b67ad8
2 changed files with 130 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/tools/test_skill_bundle_provenance.py
+67 -2
View File
@@ -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",
[
tools/skills_hub.py
+63 -12
View File
@@ -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: