fix(approval): bound dangerous-command regex input (#7485)

[rodboev]

Summary

The earlier branch implemented the limit against an older approval implementation. Replaying it on current main would have replaced newer parser safety, malformed-input handling, and verification cleanup behavior.

This update extends the current approval parser with one shared raw, parser, and normalized command classification. Over-limit input fails closed before parser expansion, hardline, sudo-stdin, deny, and dangerous matching, and before approval bypasses. The shared terminal flow, TUI shell.exec, and approval-history scan reuse that result while their existing one-argument detector seams remain compatible.

Changes

• tools/approval.py: add the shared classification and classified detector workers, preserve parser-limit and cleanup semantics, and route approval entrypoints through the fail-closed result
• tui_gateway/methods_tools.py: classify once before direct detector calls and process execution
• hermes_cli/approvals_suggest.py: classify once before direct detection and history ranking
• Focused approval, TUI, and history tests: cover over-limit rejection, parser and normalization boundaries, bypass preservation, direct-consumer routing, and detector compatibility

Validation

Scenario	Before	After
10,001-character separator-rich command	could reach approval parsing and matcher work	returns the canonical length block before downstream parser, matcher, or bypass work
Command above the raw ceiling	could enter parser work	returns the canonical length block before parsing or normalization
Current parser-limit and malformed input	current parser result	unchanged
Valid verification-artifact cleanup within the limit	exempt from dangerous detection	unchanged
TUI execution and approval-history scanning	each called direct detectors independently	reuse one classification and exclude over-limit input before execution or ranking

Test plan

• python -m pytest tests/tools/test_approval.py::TestApprovalDetectionLengthClassifier -v --timeout=0 — 14 passed
• python -m pytest tests/tools/test_hardline_blocklist.py -v --timeout=0 — 168 passed
• python -m pytest tests/tools/test_approval_deny_rules.py tests/tools/test_command_guards.py tests/tools/test_cron_approval_mode.py -v --timeout=0 — 68 passed
• python -m pytest tests/tui_gateway/test_subprocess_encoding.py tests/hermes_cli/test_approvals_suggest.py -v --timeout=0 — 20 passed
• python -m pytest tests/tools/test_approval.py -v --timeout=0 — 102 passed; two unchanged Windows cleanup-fixture cases reproduce on current main

Not in scope

This does not rewrite existing denylist regexes, add a regex-timeout dependency, change approval keys or modes, or alter explicit force=True terminal behavior.

Upstream

Closes #7485.
Reported by @DataAdvisory.

[liuhao1024]

Verification: Looks correct and safe.

Reviewed the diff — the 10,000-char length guard on detect_dangerous_command is well-designed:

1. Fail-closed: commands exceeding the limit return is_dangerous=True, forcing approval rather than silently skipping detection. This is the right default for a security gate.
2. Normalization-before-check: the guard runs after _normalize_command_for_detection() but before .lower() and the regex loop, so it bounds the input to the compiled patterns correctly.
3. No semantic change for normal commands: the 10K threshold is generous — typical shell commands are <1K chars. Only adversarial/repetitive inputs would hit this.
4. Test coverage: all three boundary cases are tested (over limit → dangerous, at limit → normal detection, existing pattern → original description preserved).

One minor observation: _MAX_DANGEROUS_COMMAND_LENGTH is defined at module level next to DANGEROUS_PATTERNS_COMPILED, which is the right placement for a constant that gates the compiled regex loop.

No issues found.

[liuhao1024]

Verification review — reviewed the diff as part of a scheduled code review pass.

Clean, minimal fix with correct fail-closed semantics.

1. Fail-closed design: Commands exceeding 10,000 characters are treated as dangerous and routed to the approval flow. This is the safe default — a false positive here just means an extra approval prompt, while a false negative could mean an undetected dangerous command.

2. Original dangerous patterns preserved: When the command is within the length limit and matches an existing dangerous pattern, the original description is returned (not the generic "command length limit"). The test_existing_dangerous_pattern_keeps_original_description test validates this — bash -lc 'echo pwned' still returns "shell" in the description, not "length limit".

3. Regex DoS prevention: The primary motivation is bounding the total regex work on DANGEROUS_PATTERNS_COMPILED — without this cap, a crafted multi-megabyte command string could cause catastrophic backtracking across all compiled patterns. The len() check is O(1) and runs before any regex.

No issues found. LGTM.

[teknium1]

Thanks for the focused fail-closed proposal. The unbounded dangerous-pattern loop remains on current main at tools/approval.py:1417-1420, so the underlying concern is still present.

Problems

• The new guard is inside detect_dangerous_command(), but the terminal path enters check_all_command_guards() and first runs detect_hardline_command() (tools/approval.py:2582). That helper independently runs generated variants through HARDLINE_PATTERNS_COMPILED (tools/approval.py:459-462) before this guard can run.
• The same pre-dangerous flow also searches _SUDO_STDIN_RE on the full normalized command (tools/approval.py:447-449, called at tools/approval.py:2592). Thus the PR does not bound all regex work in the approval path it describes.

Suggested changes

• Put a shared over-limit classification before the hardline, sudo, and dangerous regex paths, then use the existing fail-closed approval mechanism.
• Add a check_all_command_guards() regression test proving an over-limit command does not reach any of those matcher loops.

Current main also added the cleanup exemption at tools/approval.py:1414-1415 in 0c8bcd339, and GitHub marks this PR DIRTY, so salvage needs conflict resolution.

Automated hermes-sweeper review.