diff --git a/contributors/emails/menhguin@users.noreply.github.com b/contributors/emails/menhguin@users.noreply.github.com new file mode 100644 index 000000000000..e85d851e9c4f --- /dev/null +++ b/contributors/emails/menhguin@users.noreply.github.com @@ -0,0 +1 @@ +menhguin diff --git a/hermes_cli/skills_hub.py b/hermes_cli/skills_hub.py index 5c8653b3bac8..e34ade41c0a2 100644 --- a/hermes_cli/skills_hub.py +++ b/hermes_cli/skills_hub.py @@ -502,7 +502,8 @@ def do_browse(page: int = 1, page_size: int = 20, source: str = "all", def do_install(identifier: str, category: str = "", force: bool = False, console: Optional[Console] = None, skip_confirm: bool = False, invalidate_cache: bool = True, - name_override: str = "") -> None: + name_override: str = "", + source_id: Optional[str] = None) -> None: """Fetch, quarantine, scan, confirm, and install a skill. ``name_override`` lets non-interactive callers (slash commands, gateway, @@ -511,10 +512,18 @@ def do_install(identifier: str, category: str = "", force: bool = False, triggers a prompt instead; ``skip_confirm=True`` means "non-interactive" (so pair it with ``name_override`` when installing from a URL that has no frontmatter). + + ``source_id`` pins resolution to a single source adapter (e.g. ``clawhub``). + Callers that already know a skill's provenance -- notably ``do_update``, + which reads it from the lockfile -- should pass it so a bare, slash-less + identifier cannot be fuzzy-resolved to a same-named skill in a different + registry. Skill names are not namespaced across registries, so an + unconstrained resolve can silently change a skill's provenance. """ from tools.skills_hub import ( GitHubAuth, create_source_router, ensure_hub_dirs, quarantine_bundle, install_from_quarantine, HubLockFile, + _source_matches, ) from tools.skills_guard import scan_skill_cached, should_allow_install, format_scan_report @@ -525,6 +534,18 @@ def do_install(identifier: str, category: str = "", force: bool = False, auth = GitHubAuth() sources = create_source_router(auth) + if source_id: + pinned = [src for src in sources if _source_matches(src, source_id)] + if pinned: + sources = pinned + else: + c.print( + f"[bold red]Error:[/] no source adapter for '{source_id}'. " + f"Refusing to resolve '{identifier}' against other registries " + f"(that would change the skill's provenance).\n" + ) + return + # If identifier looks like a short name (no slashes), resolve it via search if "/" not in identifier: identifier = _resolve_short_name(identifier, sources, c) @@ -1058,7 +1079,20 @@ def do_update(name: Optional[str] = None, console: Optional[Console] = None) -> installed = lock.get_installed(entry["name"]) category = _derive_category_from_install_path(installed.get("install_path", "")) if installed else "" c.print(f"[bold]Updating:[/] {entry['name']}") - do_install(entry["identifier"], category=category, force=True, console=c) + # Pin the update to the source registry recorded in the lockfile. + # Without this, a bare (slash-less) identifier such as "reddit" falls + # through to _resolve_short_name()'s fuzzy catalog search inside + # do_install, which can match a same-named skill in a DIFFERENT + # registry and install that instead -- overwriting the user's files + # and rewriting the lock's `source`. An update must never change a + # skill's provenance. + do_install( + entry["identifier"], + category=category, + force=True, + console=c, + source_id=entry.get("source", "") or None, + ) c.print(f"[bold green]Updated {len(updates)} skill(s).[/]\n") diff --git a/tests/hermes_cli/test_skills_hub.py b/tests/hermes_cli/test_skills_hub.py index 9b2c775ccf94..579b3e97a2f6 100644 --- a/tests/hermes_cli/test_skills_hub.py +++ b/tests/hermes_cli/test_skills_hub.py @@ -92,7 +92,7 @@ def _capture_update(monkeypatch, results) -> tuple[str, list[tuple[str, str, boo monkeypatch.setattr(hub, "HubLockFile", lambda: type("L", (), { "get_installed": lambda self, name: {"install_path": "category/" + name} })()) - monkeypatch.setattr(cli_hub, "do_install", lambda identifier, category="", force=False, console=None: installs.append((identifier, category, force))) + monkeypatch.setattr(cli_hub, "do_install", lambda identifier, category="", force=False, console=None, source_id=None: installs.append((identifier, category, force))) do_update(console=console) return sink.getvalue(), installs @@ -249,6 +249,123 @@ def test_do_update_reinstalls_outdated_skills(monkeypatch): assert "Updated 1 skill" in output +# --------------------------------------------------------------------------- +# Cross-registry hijack regression tests +# +# An update must never change a skill's source registry. Skill names are not +# namespaced across registries, so an unconstrained name resolve can install a +# different author's same-named skill over the user's files. +# --------------------------------------------------------------------------- + + +def test_do_update_pins_install_to_locked_source(monkeypatch): + """do_update must forward the lockfile's `source` to do_install. + + Without the pin, a bare identifier like "reddit" reaches + _resolve_short_name()'s fuzzy catalog search inside do_install and can + resolve to a same-named skill in another registry. + """ + import tools.skills_hub as hub + import hermes_cli.skills_hub as cli_hub + + sink = StringIO() + console = Console(file=sink, force_terminal=False, color_system=None) + calls = [] + + monkeypatch.setattr(hub, "check_for_skill_updates", lambda **_kwargs: [ + {"name": "reddit", "identifier": "reddit", "source": "clawhub", + "status": "update_available"}, + ]) + monkeypatch.setattr(hub, "HubLockFile", lambda: type("L", (), { + "get_installed": lambda self, name: {"install_path": "category/" + name} + })()) + monkeypatch.setattr( + cli_hub, "do_install", + lambda identifier, category="", force=False, console=None, source_id=None: + calls.append({"identifier": identifier, "source_id": source_id}), + ) + + do_update(console=console) + + assert calls == [{"identifier": "reddit", "source_id": "clawhub"}] + + +def test_do_install_refuses_unknown_pinned_source(monkeypatch, hub_env): + """A pinned source with no matching adapter must abort, not fall back. + + Falling back to the full source router is what allowed a foreign registry + to satisfy the install. + """ + import tools.skills_hub as hub + + sink = StringIO() + console = Console(file=sink, force_terminal=False, color_system=None) + resolved = [] + + monkeypatch.setattr(hub, "create_source_router", lambda auth=None: []) + monkeypatch.setattr(hub, "GitHubAuth", lambda: object()) + monkeypatch.setattr( + "hermes_cli.skills_hub._resolve_short_name", + lambda name, sources, c: resolved.append(name) or "", + ) + + do_install("reddit", source_id="clawhub", console=console) + + assert resolved == [], "must not attempt a fuzzy resolve when the pin is unsatisfiable" + assert "provenance" in sink.getvalue() + + +def test_check_for_skill_updates_does_not_fall_back_across_registries(): + """An entry whose source has no adapter reports `unavailable`. + + Previously `candidate_sources ... or sources` fell back to every source, so + a same-named skill in another registry could satisfy the fetch and be + reported as this entry's update -- the step that preceded the overwrite. + The foreign source here returns a *valid* bundle with a different hash, so + the old code reports `update_available` (sourced from the wrong registry) + while the fixed code reports `unavailable`. + """ + from tools.skills_hub import check_for_skill_updates + + class _ForeignBundle: + name = "reddit" + files = {"SKILL.md": "# a different author's reddit skill"} + source = "skills.sh" + identifier = "skills-sh/someone-else/reddit" + trust_level = "community" + metadata: dict = {} + + class _ForeignSource: + """skills-sh adapter; must NOT be consulted for a clawhub-locked entry.""" + + def source_id(self): + return "skills-sh" + + def fetch(self, identifier): + return _ForeignBundle() + + def inspect(self, identifier): + return _ForeignBundle() + + lock = _DummyLockFile([ + {"name": "reddit", "identifier": "reddit", "source": "clawhub", + "content_hash": "hash-of-the-clawhub-copy"}, + ]) + + results = check_for_skill_updates( + lock=lock, # type: ignore[arg-type] # duck-typed double, matches _DummyLockFile usage above + sources=[_ForeignSource()], # type: ignore[list-item] + ) + + assert len(results) == 1 + assert results[0]["source"] == "clawhub", "provenance must be preserved" + assert results[0]["status"] == "unavailable", ( + "a clawhub-locked skill must not be matched against a skills-sh bundle; " + "reporting update_available here is the cross-registry hijack" + ) + assert "bundle" not in results[0], "must not carry a foreign registry's bundle" + + def test_handle_skills_slash_search_accepts_chatconsole_without_status_errors(): results = [type("R", (), { "name": "kubernetes", diff --git a/tools/skills_hub.py b/tools/skills_hub.py index bb44e5e686ec..ab186b403d0f 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -3742,7 +3742,22 @@ def check_for_skill_updates( for entry in installed: identifier = entry.get("identifier", "") source_name = entry.get("source", "") - candidate_sources = [src for src in sources if _source_matches(src, source_name)] or sources + candidate_sources = [src for src in sources if _source_matches(src, source_name)] + if not candidate_sources: + # No adapter for the recorded source (e.g. a tap was removed, or the + # source was renamed upstream). Previously this fell back to *all* + # sources, which meant a same-named skill in a DIFFERENT registry + # could satisfy the fetch and be reported as an update for this + # entry -- silently reassigning provenance. Skill names are not + # namespaced across registries, so that fallback is unsafe by + # construction. Report unavailable instead and let the user decide. + results.append({ + "name": entry.get("name", ""), + "identifier": identifier, + "source": source_name, + "status": "unavailable", + }) + continue bundle = None for src in candidate_sources: