Skip to content

fix(security): gate deregister() with the same plugin opt-in policy as register() - #56261

Merged
kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:salvage/55840-deregister-gate
Jul 1, 2026
Merged

fix(security): gate deregister() with the same plugin opt-in policy as register()#56261
kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:salvage/55840-deregister-gate

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Summary

Closes a symmetric authorization hole: ToolRegistry.deregister() was ungated, letting a plugin bypass the merged register(override=True) opt-in gate (#55599) entirely — deregister a built-in it doesn't own, then plain-register() a malicious handler over the now-empty slot (the override check only fires when an existing entry is present, so clearing it first skips the check).

Salvaged from #55840 by @srojk34 onto current main (253 commits behind); authorship preserved. The trivial popget/del conflict and a dropped _caller_module/import sys hunk were resolved so the salvaged files are byte-identical to the contributor's intended change.

Changes

  • tools/registry.py: deregister() now inspects the calling module (_caller_module(), frame-based, mirroring _plugin_owner_of's bind-to-definition philosophy). A hermes_plugins.* caller that does not own the entry's handler and lacks operator opt-in (plugins.entries.<id>.allow_tool_override) gets a PermissionError and the entry is left intact. Ownership binds to the plugin package root (hermes_plugins.<slug>) so root-module cleanup code can remove tools registered by a submodule of the same plugin. mcp-* toolsets are exempt (dynamic tool refresh legitimately nuke-and-repaves its own tools). Core (non-plugin) callers are never gated.
  • tests/tools/test_registry.py: TestDeregisterAuthorization — blocked-without-opt-in, allowed-with-opt-in, self-cleanup, package-root submodule ownership, opted-in submodule caller, MCP exemption, core-caller-always-allowed, and the full deregister→register bypass rejected end-to-end.

Validation

Scenario Before After
plugin (no opt-in) deregisters unowned built-in silently removed → hijackable PermissionError, entry intact
plugin with allow_tool_override deregisters (no gate) allowed
plugin deregisters its own tool (no gate) allowed (owner match)
MCP dynamic refresh deregisters mcp-* tool worked worked (exempt)
core Hermes code deregisters worked worked (not gated)
deregister → plain re-register bypass full bypass blocked at both ends

pytest tests/tools/test_registry.py → 41 passed. pytest tests/tools/test_mcp_tool.py → 202 passed (dynamic refresh unaffected). E2E against a real ToolRegistry with a real hermes_plugins.* caller frame confirmed the bypass is closed for naive/accidental plugin code.

Scope note

This gate — like the existing register(override=True) gate it mirrors — is an operator-consent + auditability guard against a plugin silently shadowing a built-in, at the project's established plugin-trust level. It is not a sandbox against actively-hostile plugin code (frame __globals__ can be forged; the same is true of the merged register() gate). Upgrading both gates from frame inspection to a capability-token / entry-stored-owner model is tracked as a follow-up.

Closes #55840.

…ookup

_plugin_override_policy is keyed by the plugin package root
(e.g. hermes_plugins.allowed), but the lookup used caller_mod
(the exact leaf module string). A call from hermes_plugins.allowed.cleanup
would evaluate _plugin_override_policy.get("hermes_plugins.allowed.cleanup")
→ False and raise PermissionError even when the plugin registered opt-in
under its package root.

Switch the policy lookup to caller_root (.join of the first two segments)
so submodule callers inherit the package-level allow_tool_override grant.

Adds a focused regression test for the opted-in submodule case.
@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) July 1, 2026 10:04
@kshitijk4poor
kshitijk4poor merged commit db0fd8f into NousResearch:main Jul 1, 2026
29 checks passed
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/tools Tool registry, model_tools, toolsets sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P1 High — major feature broken, no workaround labels Jul 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Salvage of #55840 (rebased onto current main, authorship preserved). Closes the symmetric deregister() half of the register(override=True) opt-in gate (#55599). related_to #55840. Not a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P1 High — major feature broken, no workaround sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants