fix(security): close skill-scanning and command-approval gaps for non…

hermes_cli/skills_hub.py

@@ -503,9 +503,17 @@ 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 = "",
-               source_id: Optional[str] = None) -> None:
+               source_id: Optional[str] = None) -> bool:
     """Fetch, quarantine, scan, confirm, and install a skill.
 
+    Returns True if the skill was actually installed, False for any
+    failure/block/cancellation. Callers that report install status back to
+    a UI (e.g. the TUI/Desktop skill browser's JSON-RPC install action)
+    must check this return value rather than assuming success — a
+    non-interactive caller (skip_confirm=True) can hit an "ask" verdict
+    that requires a human decision no prompt is available to collect, and
+    must fail closed instead of silently installing.
+
     ``name_override`` lets non-interactive callers (slash commands, gateway,
     scripts) supply a skill name when the upstream SKILL.md lacks a valid
     ``name:`` frontmatter field. On interactive TTY surfaces, a missing name
@@ -544,13 +552,13 @@ def do_install(identifier: str, category: str = "", force: bool = False,
                 f"Refusing to resolve '{identifier}' against other registries "
                 f"(that would change the skill's provenance).\n"
             )
-            return
+            return False
 
     # If identifier looks like a short name (no slashes), resolve it via search
     if "/" not in identifier:
         identifier = _resolve_short_name(identifier, sources, c)
         if not identifier:
-            return
+            return False
 
     c.print(f"\n[bold]Fetching:[/] {identifier}")
 
@@ -574,7 +582,7 @@ def do_install(identifier: str, category: str = "", force: bool = False,
             )
         else:
             c.print()
-        return
+        return False
 
     # URL-sourced skills may arrive with an empty name when SKILL.md has no
     # ``name:`` in frontmatter AND the URL path doesn't yield a valid
@@ -591,7 +599,7 @@ def do_install(identifier: str, category: str = "", force: bool = False,
                 "Must be a lowercase identifier (letters, digits, hyphens, "
                 "underscores; starts with a letter).\n"
             )
-            return
+            return False
         elif skip_confirm:
             # Non-interactive surface (slash command / TUI / gateway). Can't
             # prompt — emit an actionable error.
@@ -606,14 +614,14 @@ def do_install(identifier: str, category: str = "", force: bool = False,
                 "[dim]Or ask the SKILL.md's author to add a `name:` field to "
                 "its YAML frontmatter.[/]\n"
             )
-            return
+            return False
         else:
             # Interactive TTY — prompt.
             url = bundle_meta.get("url") or identifier
             chosen = _prompt_for_skill_name(c, url)
             if not chosen:
                 c.print("[dim]Installation cancelled.[/]\n")
-                return
+                return False
             bundle.name = chosen
             bundle_meta["awaiting_name"] = False
         # Keep SkillMeta in sync so downstream "already installed" checks,
@@ -643,7 +651,7 @@ def do_install(identifier: str, category: str = "", force: bool = False,
         c.print(f"[yellow]Warning:[/] '{bundle.name}' is already installed at {existing['install_path']}")
         if not force:
             c.print("Use --force to reinstall.\n")
-            return
+            return False
 
     extra_metadata = dict(getattr(meta, "extra", {}) or {})
     extra_metadata.update(getattr(bundle, "metadata", {}) or {})
@@ -656,7 +664,7 @@ def do_install(identifier: str, category: str = "", force: bool = False,
         from tools.skills_hub import append_audit_log
         append_audit_log("BLOCKED", bundle.name, bundle.source,
                          bundle.trust_level, "invalid_path", str(exc))
-        return
+        return False
     c.print(f"[dim]Quarantined to {q_path.relative_to(q_path.parent.parent.parent)}[/]")
 
     # Scan
@@ -690,15 +698,40 @@ def do_install(identifier: str, category: str = "", force: bool = False,
 
     # Check install policy
     allowed, reason = should_allow_install(result, force=force)
-    if not allowed:
+    if allowed is False:
         c.print(f"\n[bold red]Installation blocked:[/] {reason}")
         # Clean up quarantine
         shutil.rmtree(q_path, ignore_errors=True)
         from tools.skills_hub import append_audit_log
         append_audit_log("BLOCKED", bundle.name, bundle.source,
                          bundle.trust_level, result.verdict,
                          f"{len(result.findings)}_findings")
-        return
+        return False
+
+    if allowed is None:
+        if skip_confirm:
+            # "ask" means this skill needs a human decision, but
+            # skip_confirm=True means the confirmation prompt below will
+            # never run — there is no interactive session available to
+            # actually make that call (e.g. the TUI/Desktop skill browser's
+            # JSON-RPC install action, which discards all console output
+            # and never shows a y/N prompt). An ask-verdict skill must not
+            # install silently just because no prompt could run — fail
+            # closed here, matching the allowed is False path above.
+            c.print(
+                f"\n[bold red]Installation blocked:[/] {reason} "
+                f"(requires interactive confirmation, unavailable in this context)"
+            )
+            shutil.rmtree(q_path, ignore_errors=True)
+            from tools.skills_hub import append_audit_log
+            append_audit_log("BLOCKED", bundle.name, bundle.source,
+                             bundle.trust_level, result.verdict,
+                             f"{len(result.findings)}_findings_no_interactive_confirmation")
+            return False
+        # "ask" verdict — findings were already printed above via
+        # format_scan_report(); fall through to the confirmation prompt
+        # below instead of treating this the same as a hard block.
+        c.print(f"\n[bold yellow]Review required:[/] {reason}")
 
     if extra_metadata:
         metadata_lines = _format_extra_metadata_lines(extra_metadata)
@@ -736,7 +769,7 @@ def do_install(identifier: str, category: str = "", force: bool = False,
         if answer not in {"y", "yes"}:
             c.print("[dim]Installation cancelled.[/]\n")
             shutil.rmtree(q_path, ignore_errors=True)
-            return
+            return False
 
     # Install
     try:
@@ -747,7 +780,7 @@ def do_install(identifier: str, category: str = "", force: bool = False,
         from tools.skills_hub import append_audit_log
         append_audit_log("BLOCKED", bundle.name, bundle.source,
                          bundle.trust_level, "invalid_path", str(exc))
-        return
+        return False
     from tools.skills_hub import SKILLS_DIR
     c.print(f"[bold green]Installed:[/] {install_dir.relative_to(SKILLS_DIR)}")
     c.print(f"[dim]Files: {', '.join(bundle.files.keys())}[/]\n")
@@ -804,6 +837,8 @@ def do_install(identifier: str, category: str = "", force: bool = False,
         c.print("[dim]Skill will be available in your next session.[/]")
         c.print("[dim]Use /reset to start a new session now, or --now to activate immediately (invalidates prompt cache).[/]\n")
 
+    return True
+
 
 def do_inspect(identifier: str, console: Optional[Console] = None) -> None:
     """Preview a skill's SKILL.md content without installing."""

tests/hermes_cli/test_skills_hub.py

@@ -313,3 +313,128 @@ def test_do_search_json_flag_emits_full_identifiers(capsys):
     # Table render must be suppressed — sink should be empty (no "Searching for:" header).
     assert "Searching for:" not in sink.getvalue()
 
+
+def _ask_verdict_mocks(monkeypatch, tmp_path):
+    """Shared setup for the ask-verdict tests below: a trusted-source skill
+    with a caution-level finding, so should_allow_install() returns
+    (None, ...), and install_from_quarantine is mocked to record whether it
+    was actually called (not just whether "Installed:" appears in output)."""
+    import tools.skills_guard as guard
+    import tools.skills_hub as hub
+
+    canonical_identifier = "skills-sh/anthropics/skills/frontend-design"
+
+    class _ResolvedSource:
+        def inspect(self, identifier):
+            return type("Meta", (), {
+                "extra": {},
+                "identifier": canonical_identifier,
+            })()
+
+        def fetch(self, identifier):
+            return type("Bundle", (), {
+                "name": "frontend-design",
+                "files": {"SKILL.md": "# Frontend Design"},
+                "source": "skills.sh",
+                "identifier": canonical_identifier,
+                "trust_level": "trusted",
+                "metadata": {},
+            })()
+
+    q_path = tmp_path / "skills" / ".hub" / "quarantine" / "frontend-design"
+    q_path.mkdir(parents=True)
+    (q_path / "SKILL.md").write_text("# Frontend Design")
+
+    monkeypatch.setattr(hub, "ensure_hub_dirs", lambda: None)
+    monkeypatch.setattr(hub, "create_source_router", lambda auth: [_ResolvedSource()])
+    monkeypatch.setattr(hub, "quarantine_bundle", lambda bundle: q_path)
+    monkeypatch.setattr(hub, "HubLockFile", lambda: type("Lock", (), {"get_installed": lambda self, name: None})())
+    monkeypatch.setattr(guard, "scan_skill", lambda skill_path, source="community": guard.ScanResult(
+        skill_name="frontend-design", source=source, trust_level="trusted", verdict="caution",
+    ))
+    monkeypatch.setattr(guard, "format_scan_report", lambda result: "scan report with findings")
+    monkeypatch.setattr(
+        guard, "should_allow_install",
+        lambda result, force=False: (None, "Requires confirmation (trusted source + caution verdict, 1 findings)"),
+    )
+
+    install_calls = []
+
+    def _fake_install(q_path, name, category, bundle, result):
+        install_calls.append(name)
+        return tmp_path / "skills" / "frontend-design"
+
+    monkeypatch.setattr(hub, "install_from_quarantine", _fake_install)
+
+    return canonical_identifier, install_calls
+
+
+def test_do_install_ask_verdict_with_skip_confirm_fails_closed(
+    monkeypatch, tmp_path, hub_env
+):
+    """Regression test for a downgrade found in review: should_allow_install()
+    returning None ("ask") means a human needs to review findings before
+    install. If skip_confirm=True, the confirmation prompt never runs at
+    all — there is no interactive session to make that call (e.g. the
+    TUI/Desktop skill browser's JSON-RPC install action, which discards all
+    console output and never shows a y/N prompt). Silently falling through
+    to install in that case defeats the entire point of "ask" — this must
+    fail closed instead, exactly like the allowed is False path."""
+    canonical_identifier, install_calls = _ask_verdict_mocks(monkeypatch, tmp_path)
+
+    sink = StringIO()
+    console = Console(file=sink, force_terminal=False, color_system=None)
+
+    result = do_install(canonical_identifier, console=console, skip_confirm=True)
+
+    output = sink.getvalue()
+    assert "Installation blocked" in output
+    assert "Installed:" not in output
+    assert install_calls == []
+    assert result is False
+
+
+def test_do_install_ask_verdict_interactive_still_falls_through_to_confirmation(
+    monkeypatch, tmp_path, hub_env
+):
+    """The fix for the skip_confirm=True downgrade must not regress the
+    original bug this whole flow exists to fix: a genuinely interactive
+    caller (skip_confirm=False) with an "ask" verdict must still reach the
+    real y/N prompt, not be hard-blocked outright."""
+    canonical_identifier, install_calls = _ask_verdict_mocks(monkeypatch, tmp_path)
+    monkeypatch.setattr("builtins.input", lambda prompt="": "y")
+
+    sink = StringIO()
+    console = Console(file=sink, force_terminal=False, color_system=None)
+
+    result = do_install(canonical_identifier, console=console, skip_confirm=False)
+
+    output = sink.getvalue()
+    assert "Review required" in output
+    assert "Installed:" in output
+    assert install_calls == ["frontend-design"]
+    assert result is True
+
+
+def test_do_install_ask_verdict_interactive_reject_cancels(
+    monkeypatch, tmp_path, hub_env
+):
+    """An interactive caller answering "n" to the ask-verdict confirmation
+    must cancel, not install — the confirmation must be a real, respected
+    decision point, not a rubber stamp."""
+    canonical_identifier, install_calls = _ask_verdict_mocks(monkeypatch, tmp_path)
+    monkeypatch.setattr("builtins.input", lambda prompt="": "n")
+
+    sink = StringIO()
+    console = Console(file=sink, force_terminal=False, color_system=None)
+
+    result = do_install(canonical_identifier, console=console, skip_confirm=False)
+
+    output = sink.getvalue()
+    assert "cancelled" in output.lower()
+    assert "Installed:" not in output
+    assert install_calls == []
+    assert result is False
+
+
+

tests/test_tui_gateway_server.py

@@ -15208,3 +15208,39 @@ def start(self):
         assert captured.get("persist_user_message") == "hi"
     finally:
         server._sessions.pop("sid", None)
+
+
+def test_skills_manage_install_reports_actual_result_not_always_true(monkeypatch):
+    """Regression test: the "skills.manage" install action used to report
+    installed: True unconditionally, regardless of what do_install() (which
+    always returned None) actually did. Combined with the ask-verdict fix
+    in hermes_cli/skills_hub.py (which now correctly blocks an "ask"
+    verdict when no interactive confirmation is available, as is always
+    the case for this JSON-RPC action's skip_confirm=True call), a blocked
+    install must be reported as installed: False, not silently reported as
+    a success."""
+    import hermes_cli.skills_hub as skills_hub
+
+    monkeypatch.setattr(skills_hub, "do_install", lambda *a, **k: False)
+
+    resp = server.handle_request({
+        "id": "1",
+        "method": "skills.manage",
+        "params": {"action": "install", "query": "some-risky-skill"},
+    })
+
+    assert resp["result"]["installed"] is False
+
+
+def test_skills_manage_install_reports_true_on_actual_success(monkeypatch):
+    import hermes_cli.skills_hub as skills_hub
+
+    monkeypatch.setattr(skills_hub, "do_install", lambda *a, **k: True)
+
+    resp = server.handle_request({
+        "id": "1",
+        "method": "skills.manage",
+        "params": {"action": "install", "query": "a-clean-skill"},
+    })
+
+    assert resp["result"]["installed"] is True

tests/tools/test_approval.py

@@ -172,6 +172,35 @@ def test_powershell_benign_path_containing_del_not_matched_as_delete(self):
         assert dangerous is True
         assert key != "Windows PowerShell destructive delete"
 
+    def test_powershell_inline_command_payload_f_is_not_the_file_flag(self):
+        # An inline payload can legitimately carry its own -f (Write-Host's
+        # -ForegroundColor alias). Shell tokenization keeps that payload as a
+        # single invocation argument, so the inner -f is never read as the
+        # outer -File flag. The command is still gated, by -Command.
+        assert approval_module._interpreter_exec_flag(
+            "powershell", ["-Command", "Write-Host -f Green ok"]
+        ) == "-command"
+        assert approval_module._interpreter_exec_flag(
+            "powershell", ["-NoProfile", "-Command", "Write-Host -File x"]
+        ) == "-command"
+
+        dangerous, key, desc = detect_dangerous_command(
+            'powershell -Command "Write-Host -f Green ok"'
+        )
+        assert dangerous is True
+
+    def test_powershell_file_flag_after_leading_options_still_matches(self):
+        # The same scan must still reach -File when ordinary PowerShell
+        # options precede it.
+        assert approval_module._interpreter_exec_flag(
+            "powershell", ["-ExecutionPolicy", "Bypass", "-File", "helper.ps1"]
+        ) == "-file"
+
+        dangerous, key, desc = detect_dangerous_command(
+            "powershell -ExecutionPolicy Bypass -File helper.ps1"
+        )
+        assert dangerous is True
+
     def test_plain_text_does_not_trigger_windows_delete(self):
         dangerous, key, desc = detect_dangerous_command(
             "echo remember to del old notes"