fix(security): operator opt-in for plugin tool_override (sink-enforced) + enable-time consent - #55599
Merged
Conversation
…event silent built-in tool replacement The tool_override flag landed in v0.14.0 (#26759) so plugins can replace a built-in tool with their own implementation. It works as advertised but there is no trust gate, so any enabled third-party plugin can silently override any built-in like shell_exec, write_file, or web_fetch and exfiltrate everything the agent invokes through it. The only trace is a DEBUG-level log line. Compare with ctx.llm (#23194) which does gate the equivalent privilege escalation: overriding the provider requires plugins.entries.<id>.llm.allow_provider_override: true in config.yaml. The policy shape exists, it just was not extended to tool overrides. Fix: * Add PluginToolOverrideError(PermissionError) for the gate failure. * register_tool() now checks _tool_override_allowed(name) when override=True. Bundled plugins (manifest.source == 'bundled') are trusted by default. Every other source requires plugins.entries.<plugin_id>.allow_tool_override: true in config.yaml. * fail-closed: if config.yaml cannot be loaded for any reason, _tool_override_allowed returns False. Same posture as MSGraphWebhookAdapter.connect() in #22353. Backwards compatibility: * Bundled plugins: no change (source == 'bundled' short-circuits the gate). * Third-party plugins not using override: no change (gate is only consulted when override=True). * Third-party plugins using override: registration fails until the operator opts in. The error message includes the exact config path to add, so the fix is one config edit away for legitimate use cases. Same migration path users went through for allow_provider_override after #23194 landed. Regression tests: * tests/hermes_cli/test_plugins.py::test_register_tool_override_replaces_existing and ::test_register_tool_override_on_new_name_is_noop_path were written before the gate existed. Updated their test configs to include allow_tool_override: true under plugins.entries.<plugin_id>, mirroring how a legitimate operator would now grant the privilege. * New regression test ::test_register_tool_override_blocked_without_operator_opt_in exercises both the PluginManager-catches-error path (built-in tool is preserved, attacker plugin is skipped) and the direct-call path (PluginToolOverrideError is raised with a message that names the config key to set). Verified the test fails without this fix and passes with it. * All 73 tests in test_plugins.py continue to pass.
… direct-import bypass The opt-in gate lived only in PluginContext.register_tool, so a plugin could bypass it by importing tools.registry and calling registry.register(..., override=True) directly. Enforce the same gate at the sink: during plugin load, the registry rejects an override from a plugin without operator opt-in regardless of the path taken. Built-in and MCP registrations (no active plugin scope) are unaffected. Adds a regression test covering the direct-registry bypass.
… plugin module egilewski found the prior sink gate was transient: it only applied while PluginManager executed register(ctx). A plugin could defer a direct registry.register(..., override=True) to a post-load callback/thread, after the scope was cleared, and still replace a built-in. Make authorization durable by binding it to where the handler is DEFINED (handler.__globals__['__name__']) rather than to call timing. At load, each plugin's module namespace is mapped to its allow_tool_override opt-in in a table that is never cleared. The sink resolves the handler's owning plugin module and rejects an override from any plugin namespace without opt-in, regardless of when or on which thread the call happens. Plugin namespaces with no recorded policy are treated as not-opted-in (fail-closed). Built-in and MCP handlers live outside the plugin namespace and are unaffected. Adds a regression test for the delayed/post-load direct-registry override.
Builds on memosr's sink-level opt-in gate (#29249). Enabling a non-bundled plugin now surfaces the privileged allow_tool_override decision at `hermes plugins enable` time instead of leaving the operator to discover the config key after a runtime rejection. - `hermes plugins enable <name>` prompts for non-bundled plugins: 'Allow this plugin to replace built-in tools?' Default is deny (blank Enter / non-interactive stdin / EOF all fail closed). - --allow-tool-override / --no-allow-tool-override flags for non-interactive and scripted use (and a future desktop checkbox). - Bundled plugins are trusted: never prompted, no entry written. - Writes plugins.entries.<key>.allow_tool_override, the same key the sink gate reads (manifest.key == discovery key), so consent and enforcement compose end to end.
8 tasks
alt-glitch
added
type/security
Collaborator
srojk34
added a commit
to srojk34/hermes-agent
that referenced
this pull request
Jun 30, 2026
…to package A plugin could bypass the register(override=True) authorization chain by calling registry.deregister(name) to clear the existing entry, then calling plain registry.register() over the now-empty slot — register() only runs its override check when an existing entry is present, so removing the entry first skips the check entirely (NousResearch#55599 sibling gap). Ownership check binds to the plugin package root (hermes_plugins.{name}), not the exact leaf module string: hermes_plugins.pkg (root cleanup code) is allowed to deregister a tool whose handler was defined in hermes_plugins.pkg.handlers — they share the same package. Exact module equality would have blocked same-plugin self-cleanup (egilewski review). MCP toolsets (mcp-*) are exempt — dynamic tool refresh legitimately nukes and repaves its own tools on every server refresh.
This was referenced Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A plugin can no longer silently replace a built-in tool: enabling a non-bundled plugin now requires an explicit operator decision about the privileged
tool_overridecapability, and the registry enforces that grant at the point of registration.Salvage of #29249 (@memosr) — the sink-level opt-in gate, rebased onto current
main— plus an enable-time consent layer on top so the operator makes the call when they enable the plugin instead of discovering a config key after a runtime rejection.Root cause:
tool_override(#26759) let any enabled plugin callregister(override=True)to replace a built-in (shell_exec,write_file,web_fetch, …) with only a DEBUG log line as a trace. No trust gate, despite the codebase already gating the equivalentctx.llmprovider override behindallow_provider_override(#23194).Changes
tools/registry.py: enforce the override opt-in at the registration sink. Authorization is bound to the handler's defining plugin module (handler.__globals__["__name__"]), captured at load and never cleared — so directregistry.register(override=True), threaded, and delayed-callback paths are all gated identically. Built-in/MCP handlers live outside the plugin namespace and are unaffected. (@memosr)hermes_cli/plugins.py:PluginContext.register_tool(override=True)checksplugins.entries.<id>.allow_tool_override; bundled plugins exempt; fail-closed on config load failure. (@memosr)hermes_cli/plugins_cmd.py+hermes_cli/subcommands/plugins.py: enable-time consent.hermes plugins enable <non-bundled>prompts "Allow this plugin to replace built-in tools?" with a deny default (blank Enter / non-interactive stdin / EOF all fail closed).--allow-tool-override/--no-allow-tool-overrideflags for scripted and headless use. Bundled plugins are never prompted and never get an entry written. The choice is persisted under the sameplugins.entries.<key>.allow_tool_overridekey the sink reads (manifest.key== discovery key), so consent and enforcement compose end to end.Validation
--allow-tool-overridefull argparse pathHERMES_HOMEwith real plugin load and real config I/O (all rows above).tests/hermes_cli/test_plugins.pyandtests/hermes_cli/test_plugins_cmd_enable_disable_nested.py— @memosr's 3 sink-gate regression tests (direct-import + delayed-callback bypass) plus 6 new consent tests. ruff clean.Infographic
Salvaged from #29249 — @memosr's commits cherry-picked with authorship preserved; consent layer added on top.