Skip to content

fix(approval): add glob matching for command_allowlist entries - #9163

Open
richardiitse wants to merge 1 commit into
NousResearch:mainfrom
richardiitse:fix/command-allowlist-glob-matching
Open

fix(approval): add glob matching for command_allowlist entries#9163
richardiitse wants to merge 1 commit into
NousResearch:mainfrom
richardiitse:fix/command-allowlist-glob-matching

Conversation

@richardiitse

Copy link
Copy Markdown

Problem

command_allowlist entries (e.g. python3 << *) are glob patterns, but is_approved() was doing only set membership checks against pattern keys like script execution via heredoc. This means glob entries in command_allowlist never matched and commands always required approval.

Fix

Add fnmatch-based glob checking before the pattern-key lookup in check_dangerous_command(). Now glob patterns like python3 << * correctly bypass approval prompts.

# In check_dangerous_command(), after detect_dangerous_command():
with _lock:
    if any(fnmatch.fnmatch(command, p) for p in _permanent_approved):
        return {"approved": True, "message": None}

Testing

check_dangerous_command("python3 << EOF\nprint(1)\nEOF", "local")
# => {"approved": True, "message": None}  # was: approval_required

check_dangerous_command("pdftotext /root/test.pdf", "local")
# => {"approved": True, "message": None}  # already worked

Files changed

  • tools/approval.py: +1 import (fnmatch), +6 lines (glob pre-check)

command_allowlist entries (e.g. 'python3 << *') are glob patterns, but
is_approved() was doing only set membership checks against pattern keys
like 'script execution via heredoc', so allowlist entries never matched.

Add fnmatch-based glob check before pattern-key lookup so glob entries
in command_allowlist work as intended.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management labels Apr 27, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the command-text versus danger-key mismatch.

Problems

  • tools/approval.py:612 matches the entire command with an unrestricted glob. That permits a pattern such as python3 << * to span embedded newlines and shell syntax. Current main intentionally rejects compound commands before allowlist matching (tools/approval.py:1588-1607) and has regression coverage for chaining, pipes, redirects, newlines, backticks, and substitutions (tests/tools/test_command_guards.py:276-304).
  • The patch only covers check_dangerous_command(). Current main uses its shared matcher from both check_dangerous_command() and check_all_command_guards() (tools/approval.py:2276, tools/approval.py:2591).

Suggested changes

  • A safe heredoc-specific policy would need a constrained parser-based design and tests rather than unrestricted full-command globbing.

Automated hermes-sweeper review.

Comment thread tools/approval.py
return {"approved": True, "message": None}

# Check glob-based allowlist BEFORE pattern-key lookup.
# command_allowlist entries like "python3 << *" are glob patterns, not pattern keys.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fnmatch here runs against the whole shell string, so * can span newlines and shell operators. This would let a glob such as python3 << * approve appended/chained content; use a constrained matcher that rejects compound shell syntax before granting the shortcut.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
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 P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants