Skip to content

security(approval): catch killall sweeps and find -execdir rm - #27460

Closed
ht1072 wants to merge 1 commit into
NousResearch:mainfrom
ht1072:pr/dangerous-killall-execdir-v2
Closed

security(approval): catch killall sweeps and find -execdir rm#27460
ht1072 wants to merge 1 commit into
NousResearch:mainfrom
ht1072:pr/dangerous-killall-execdir-v2

Conversation

@ht1072

@ht1072 ht1072 commented May 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • require approval for killall -9, killall -KILL, killall -s KILL, and killall -r sweeps
  • extend the existing find -exec rm guard to also catch find -execdir rm
  • add focused regression coverage for the new approval patterns

Why

The current dangerous-command heuristics already gate pkill -9 and find -exec rm, but they still miss equivalent destructive forms:

  • killall -9 / -KILL / -s KILL / -r <regex> can terminate broad sets of processes
  • find -execdir rm has the same destructive effect as find -exec rm while bypassing the current literal -exec pattern

This keeps the change small and heuristic-scoped without changing the broader approval model.

Test Plan

  • uv run --no-sync python3 -m pytest tests/tools/test_approval.py -q

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels May 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #26829 (merged). The killall -9/-KILL/-s KILL/-r patterns and find -execdir rm detection are already on main via commit 6ba35ec. This PR also removes unrelated code (TestDetectSudoStdin, _get_session_platform, _is_gateway_approval_context) which is destructive.

@liuhao1024

Copy link
Copy Markdown
Contributor

I found an issue with this PR that looks worth fixing before merge.

Title/body description does not match actual diff.

The PR title and body claim to:

  • "require approval for killall -9, killall -KILL, killall -s KILL, and killall -r sweeps"
  • "extend the existing find -exec rm guard to also catch find -execdir rm"
  • "add focused regression coverage for the new approval patterns"

However, the actual diff shows:

  • 17 additions, 128 deletions (net -111 lines)
  • Deleted test class TestDetectSudoStdin (27 test methods removed)
  • Deleted test class TestMacOSPrivateSystemPaths (11 test methods removed)
  • Deleted _SUDO_STDIN_RE regex and _check_sudo_stdin_guard() function
  • Deleted _is_gateway_approval_context() function
  • No new test methods added for the claimed killall / find -execdir patterns

The PR removes critical security regression tests without adding replacements for the newly-claimed coverage. This is a regression in test coverage for sudo stdin attacks and macOS private system path attacks.

Why it matters: If the intent is to remove these test classes, the PR description should explain why. If the intent is to add killall / find -execdir protection, that code is missing from the diff.

Suggested fix: Either (1) update the PR description to match the actual changes (removal of sudo/stdin/macOS-private-path guards and their tests), or (2) add the missing killall / find -execdir protection and regression tests as claimed in the body.

@ht1072

ht1072 commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

Closing this PR based on the current diff relative to main. The intended killall / find -execdir rm hardening is already present upstream via #26829 / commit 6ba35ec, and this branch is based on an older state, so the PR currently presents as a rollback of later approval/security coverage (including unrelated test/guard removals). Rather than try to repair this thread in place, I'm closing it and will only re-open the topic from a fresh branch off current main if there is still a small independent delta worth upstreaming.

@ht1072 ht1072 closed this May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants