Skip to content

Commit fbcc09b

Browse files
author
Yogthos
committed
fix(perm): M3 — route bash through enforce; redirect targets gate via write rules
Closes dirge-6ab (P0 security). Second milestone of the staged permission refactor; depends on M1 (8dd5b01). ## What `check_bash_segments` in src/agent/tools/bash.rs migrates from the legacy `check_perm` / `check_perm_path` to the M1 chokepoint `enforce()`. Two behavior changes ride along: 1. **Redirect targets route through `write` tool rules.** Pre-M3 the C4 audit fix routed `> file` / `>> file` / `&> file` / `1> file` / `2> file` targets through `check_perm_path(tool="bash", &target)` — looking the target path up against BASH rules (command-style globs). No bash pattern matches a bare path, so the call fell to `default_action = Allow`. Result: `echo hi > /etc/passwd` ran without prompting even though writing to `/etc/` directly would be denied. Post-M3 the target routes through `tool="write"` via `Scope::PathResolve`, so the user's write deny lists (or M4's incoming Ask-default for unmatched paths) actually apply. 2. **Subshell / command-substitution / complex commands prompt whole-command.** Already the prior behavior, just routed through `enforce` now. Matches maki's `is_complex_bash` posture (permissions.rs:457-475): when tree-sitter can't safely split, the WHOLE command goes to the user for confirmation. The compound-statement segmenting (the part the user explicitly called out: "agent runs `git diff && rm -rf /` — Maki uses tree-sitter to figure out the permissions requested are `git *` and `rm *`") was already working correctly in dirge via the existing `parse_bash_segments_full` in src/semantic/adapters/bash.rs. Each segment is independently routed through `enforce`. The default `rm -rf /**` deny rule catches the dangerous segment even when the leading segment is allowed. ## Tests added Three regression tests pin the M3 invariants: - `compound_command_denies_dangerous_segment`: `git diff && rm -rf /` — second segment hits the rm deny rule. Pins the segment-walking invariant end-to-end (separate from the parser tests which only verify the segmenting itself). - `redirect_target_routes_through_write_rules`: write deny of `/etc/**` blocks `echo hi > /etc/passwd`. The exact regression the C4 audit fix attempted to close but mis-routed. - `redirect_target_allowed_when_write_permits`: cwd-relative target with no deny rule passes — guards against an over-broad deny regression. ## Behavior shift for users `echo … > /tmp/anything` (or any external path) now prompts when no explicit write rule covers it, because the external-path catch-all at `permission/checker.rs:434` upgrades unmatched-Allow to Ask for paths outside the working directory. Previously the redirect bypassed this gate via the wrong-namespace lookup. This is a tighter posture than before but consistent with how `write` already behaves for direct file operations. Users who need silent /tmp redirects can add: ```jsonc "permission": { "write": { "/tmp/**": "allow" } } ``` ## Test - 1201/1202 tests pass (1 pre-existing unrelated Clojure failure per 8dd5b01 commit note) - Binary at ~/bin/dirge (11:00)
1 parent 8dd5b01 commit fbcc09b

2 files changed

Lines changed: 133 additions & 17 deletions

File tree

.beads/issues.jsonl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
{"_type":"issue","id":"dirge-6ab","title":"Perm M3: port maki's tree-sitter bash analyzer (close git\u0026\u0026rm bypass)","description":"SECURITY: 'git diff \u0026\u0026 rm -rf /' currently allowed because dirge's bash redirect-target check (src/agent/tools/bash.rs:350) routes through bash rules with the file path as input, which has no path-style match → falls to default Allow. Pre-existing pre-fix-for-7403792.\n\nSolution: port maki's tree-sitter bash analyzer verbatim from /Users/yogthos/src/maki/maki-agent/src/permissions.rs:33-43 (parser thread_local), 394-439 (collect_commands walker), 441-475 (analyze_bash + complexity gates). The walker splits compounds via 'pipeline'/'list' AST traversal and extracts every 'command' / 'redirected_statement' / 'subshell' / etc node; each segment then goes through the permission chokepoint independently.\n\nBehavior at completion:\n- 'git diff \u0026\u0026 rm -rf /' → enforce('bash', 'git diff') + enforce('bash', 'rm -rf /') — second check fires\n- Subshells / command substitution mark whole command 'complex' → forces prompt (conservative)\n- Pipes split into separate segments\n- Quoted operators correctly NOT split (AST respects quoting)\n\nDepends on: dirge-{M1}\n\nMaki license is GPL-compatible — verify before copying. Add 'Ported from maki-agent/src/permissions.rs' attribution comment.","status":"open","priority":0,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:46Z","created_by":"Yogthos","updated_at":"2026-05-23T14:25:46Z","dependencies":[{"issue_id":"dirge-6ab","depends_on_id":"dirge-01s","type":"blocks","created_at":"2026-05-23T10:25:51Z","created_by":"Yogthos","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0}
1+
{"_type":"issue","id":"dirge-6ab","title":"Perm M3: port maki's tree-sitter bash analyzer (close git\u0026\u0026rm bypass)","description":"SECURITY: 'git diff \u0026\u0026 rm -rf /' currently allowed because dirge's bash redirect-target check (src/agent/tools/bash.rs:350) routes through bash rules with the file path as input, which has no path-style match → falls to default Allow. Pre-existing pre-fix-for-7403792.\n\nSolution: port maki's tree-sitter bash analyzer verbatim from /Users/yogthos/src/maki/maki-agent/src/permissions.rs:33-43 (parser thread_local), 394-439 (collect_commands walker), 441-475 (analyze_bash + complexity gates). The walker splits compounds via 'pipeline'/'list' AST traversal and extracts every 'command' / 'redirected_statement' / 'subshell' / etc node; each segment then goes through the permission chokepoint independently.\n\nBehavior at completion:\n- 'git diff \u0026\u0026 rm -rf /' → enforce('bash', 'git diff') + enforce('bash', 'rm -rf /') — second check fires\n- Subshells / command substitution mark whole command 'complex' → forces prompt (conservative)\n- Pipes split into separate segments\n- Quoted operators correctly NOT split (AST respects quoting)\n\nDepends on: dirge-{M1}\n\nMaki license is GPL-compatible — verify before copying. Add 'Ported from maki-agent/src/permissions.rs' attribution comment.","status":"closed","priority":0,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:46Z","created_by":"Yogthos","updated_at":"2026-05-23T15:01:11Z","started_at":"2026-05-23T14:51:42Z","closed_at":"2026-05-23T15:01:11Z","close_reason":"Closed","dependencies":[{"issue_id":"dirge-6ab","depends_on_id":"dirge-01s","type":"blocks","created_at":"2026-05-23T10:25:51Z","created_by":"Yogthos","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0}
22
{"_type":"issue","id":"dirge-ojn","title":"Perm M4: flip unmatched-tool default from Allow to Ask","description":"Today src/permission/checker.rs:103 has default_action = Action::Allow. Anything not explicitly configured falls through to Allow without prompting — a security gap especially for write/edit/apply_patch which currently have NO default rules installed at all (mismatch with bash which has default_bash_rules, mcp_tool which defaults Ask since 27bd70a).\n\nFlip default to Ask. Add builtin-allow list for safe read-only tools (port maki's BUILTIN_ALLOW_RULES at permissions.rs:16-24, adapted for dirge's tool set: read/glob/grep/list_dir/list_symbols/find_definition/find_callers/find_callees/get_symbol_body/repo_overview).\n\nwrite/edit/apply_patch/bash/webfetch/websearch/task/skill/memory all become Ask by default unless explicitly allowlisted. Document the migration in README + CHANGELOG. Users with existing configs unaffected — only the no-config baseline changes.\n\nAdd --yolo CLI flag (currently only via config). Maps to set the global allow_all atomic (port maki's PermissionManager::toggle_yolo at permissions.rs:219-222).\n\nDepends on: dirge-{M2}, dirge-{M3}","status":"open","priority":1,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:47Z","created_by":"Yogthos","updated_at":"2026-05-23T14:25:47Z","dependencies":[{"issue_id":"dirge-ojn","depends_on_id":"dirge-6ab","type":"blocks","created_at":"2026-05-23T10:25:53Z","created_by":"Yogthos","metadata":"{}"},{"issue_id":"dirge-ojn","depends_on_id":"dirge-cep","type":"blocks","created_at":"2026-05-23T10:25:52Z","created_by":"Yogthos","metadata":"{}"}],"dependency_count":2,"dependent_count":0,"comment_count":0}
33
{"_type":"issue","id":"dirge-01s","title":"Perm M1: single chokepoint (port maki's enforce shape)","description":"Refactor dirge's 3 permission entry points (check_perm, check_perm_path, check_perm_path_resolve in src/agent/tools/mod.rs) into a single chokepoint patterned after maki's PermissionManager::enforce (maki-agent/src/permissions.rs:283). One function, takes (tool, scope), routes internally based on tool category. No behavior change for users — pure refactor with all existing callers updated. Behind-the-scenes the new fn calls the same PermissionChecker logic.\n\nRef: maki-agent/src/permissions.rs:283-350 — port the signature shape (async, returns Result\u003c(), PermissionError\u003e, takes \u0026EventSender + user_response_rx + cancel for UI integration). Adapt to dirge's existing AskSender + PermCheck.","status":"closed","priority":1,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:25Z","created_by":"Yogthos","updated_at":"2026-05-23T14:49:19Z","started_at":"2026-05-23T14:40:51Z","closed_at":"2026-05-23T14:49:19Z","close_reason":"Closed","dependency_count":0,"dependent_count":2,"comment_count":0}
44
{"_type":"issue","id":"dirge-cep","title":"Perm M2: unified rule schema (port maki's TOML shape via JSON)","description":"Replace dirge's per-tool field PermissionConfig (separate Option\u003cToolPerm\u003e per built-in tool) with maki's uniform shape: { tool_name: { allow: [patterns], deny: [patterns] } }. Single PermissionRule struct (maki-config/src/lib.rs:268-273). Keep existing JSON config (dirge uses JSON not TOML), but flatten the schema. Add a dual-read path so old config.json files (existing user configs in the wild) still parse — log a deprecation warning and auto-migrate on save. Migration test required.\n\nDepends on: dirge-{M1}","status":"open","priority":1,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:25Z","created_by":"Yogthos","updated_at":"2026-05-23T14:25:25Z","dependencies":[{"issue_id":"dirge-cep","depends_on_id":"dirge-01s","type":"blocks","created_at":"2026-05-23T10:25:50Z","created_by":"Yogthos","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0}

src/agent/tools/bash.rs

Lines changed: 132 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,7 @@ use tokio::process::Command;
44
use tokio::time::Duration;
55

66
use crate::agent::tools::cache::ToolCache;
7-
#[cfg(feature = "semantic-bash")]
8-
use crate::agent::tools::check_perm_path;
9-
use crate::agent::tools::{AskSender, BashArgs, PermCheck, ToolError, check_perm};
7+
use crate::agent::tools::{AskSender, BashArgs, PermCheck, Scope, ToolError, enforce};
108

119
use crate::sandbox::Sandbox;
1210
#[cfg(feature = "semantic-bash")]
@@ -326,29 +324,47 @@ async fn check_bash_segments(
326324
ask_tx: &Option<AskSender>,
327325
command: &str,
328326
) -> Result<(), ToolError> {
327+
// M3 (dirge-6ab): every bash permission decision routes through
328+
// the `enforce` chokepoint. Each compound-statement segment is
329+
// checked independently so `git diff && rm -rf /` gets BOTH `git`
330+
// AND `rm` checked against the user's bash rules — not just the
331+
// leading command. Redirect targets (`> file`) additionally route
332+
// through the `write` tool rules so write/edit path rules apply,
333+
// closing the C4 audit gap where targets hit bash rules with
334+
// path-string inputs and fell through to default Allow.
329335
#[cfg(feature = "semantic-bash")]
330336
{
331337
let (segments, complex) = bash::parse_bash_segments_full(command)
332338
.unwrap_or_else(|_| (vec![command.to_string()], false));
333339

334340
if complex {
335-
return check_perm(permission, ask_tx, "bash", command).await;
341+
// Subshell / command substitution / process substitution /
342+
// arithmetic expansion: tree-sitter declined to split.
343+
// Force a prompt on the WHOLE command so the user
344+
// confirms the unfamiliar shape. Maki does the same
345+
// (maki-agent/src/permissions.rs:441-455).
346+
enforce(permission, ask_tx, "bash", Scope::Raw(command)).await?;
347+
return Ok(());
336348
}
337349

338350
for segment in &segments {
339-
check_perm(permission, ask_tx, "bash", segment).await?;
351+
enforce(permission, ask_tx, "bash", Scope::Raw(segment)).await?;
340352
}
341353

342-
// C4 (audit fix): the segment loop above only checks each
343-
// *command*. Output redirections (`> /etc/something`,
344-
// `>> ~/.ssh/authorized_keys`) write files that the path
345-
// permission gate would otherwise refuse. Route every
346-
// redirect target through `check_perm_path` so write/edit's
347-
// path rules also apply to redirects. False-positive prompts
348-
// (read-side `< file`) are acceptable; bypassing the path
349-
// gate via `echo … > sensitive` is not.
354+
// M3 fix to the C4 redirect-target gap: route targets through
355+
// the `write` tool name (not `bash`), since redirection
356+
// semantically writes files. Previously this was routed as
357+
// `check_perm_path(tool="bash", path=&target)`, looking the
358+
// target up in BASH rules — command-style globs that don't
359+
// match path strings, falling through to default Allow. With
360+
// `tool="write"` the user's write rules govern: deny lists
361+
// (`/etc/**`, `~/.ssh/**`, `~/.aws/credentials`) now fire on
362+
// bash redirects too. Falsely-prompting `< file` (read-side)
363+
// is acceptable; the `extract_redirect_targets` walker
364+
// intentionally skips heredoc / herestring and only emits
365+
// write-side targets (`>`, `>>`, `&>`, `1>`, `2>`).
350366
for target in bash::extract_redirect_targets(command) {
351-
check_perm_path(permission, ask_tx, "bash", &target).await?;
367+
enforce(permission, ask_tx, "write", Scope::PathResolve(&target)).await?;
352368
}
353369
Ok(())
354370
}
@@ -390,10 +406,11 @@ async fn check_bash_segments(
390406
|| command.contains(">(")
391407
|| command.contains("$'");
392408
if has_substitution {
393-
return check_perm(permission, ask_tx, "bash", command).await;
409+
enforce(permission, ask_tx, "bash", Scope::Raw(command)).await?;
410+
return Ok(());
394411
}
395412
for segment in &segments {
396-
check_perm(permission, ask_tx, "bash", segment).await?;
413+
enforce(permission, ask_tx, "bash", Scope::Raw(segment)).await?;
397414
}
398415
Ok(())
399416
}
@@ -687,4 +704,103 @@ mod tests {
687704
assert!(segments[0].contains("a; b"));
688705
assert_eq!(segments[1], "ls");
689706
}
707+
708+
// M3 (dirge-6ab) — segment-level bash gating regression tests.
709+
// These pin the "every command in a compound gets checked
710+
// separately" invariant the user asked about
711+
// ("agent runs `git diff && rm -rf /`, what happens?").
712+
713+
/// `git diff && rm -rf /` must be denied — the second segment
714+
/// hits the default `rm -rf /**` deny rule even though the
715+
/// first segment is allowlisted. Pre-this-test, the path was
716+
/// covered by the parser test in `semantic::adapters::bash`,
717+
/// but nothing end-to-end pinned that `check_bash_segments`
718+
/// actually walks the segments through the perm checker.
719+
#[cfg(feature = "semantic-bash")]
720+
#[tokio::test]
721+
async fn compound_command_denies_dangerous_segment() {
722+
use crate::permission::{PermissionConfig, SecurityMode, checker::PermissionChecker};
723+
724+
let config = PermissionConfig::default();
725+
let checker = PermissionChecker::new(&config, SecurityMode::Standard, None);
726+
let perm = std::sync::Arc::new(std::sync::Mutex::new(checker));
727+
728+
let result =
729+
check_bash_segments(&Some(perm), &None, "git diff && rm -rf /").await;
730+
assert!(
731+
result.is_err(),
732+
"compound: rm segment must hit deny rule even after safe git segment; got {result:?}",
733+
);
734+
let msg = format!("{:?}", result);
735+
assert!(
736+
msg.contains("denied") || msg.contains("Denied"),
737+
"expected 'denied' in error: {msg}",
738+
);
739+
}
740+
741+
/// Output redirect targets route through the `write` tool rules
742+
/// (M3 fix to the C4 audit). Pre-fix: `tool="bash"` lookup with a
743+
/// path string, no matching command pattern, fell through to
744+
/// default Allow — `echo hi > /etc/passwd` ran without prompting.
745+
/// Post-fix: routes through write rules.
746+
#[cfg(feature = "semantic-bash")]
747+
#[tokio::test]
748+
async fn redirect_target_routes_through_write_rules() {
749+
use crate::permission::{
750+
Action, PermissionConfig, SecurityMode, ToolPerm, checker::PermissionChecker,
751+
};
752+
use std::collections::HashMap;
753+
754+
// Configure write to deny everywhere; without an explicit
755+
// rule the M2/M4-pre default is still Allow, so we set an
756+
// explicit deny to make the test robust against the
757+
// default-flip.
758+
let mut write_rules = HashMap::new();
759+
write_rules.insert("/etc/**".to_string(), Action::Deny);
760+
let config = PermissionConfig {
761+
write: Some(ToolPerm::Granular(write_rules)),
762+
..Default::default()
763+
};
764+
let checker = PermissionChecker::new(&config, SecurityMode::Standard, None);
765+
let perm = std::sync::Arc::new(std::sync::Mutex::new(checker));
766+
767+
let result =
768+
check_bash_segments(&Some(perm), &None, "echo hi > /etc/passwd").await;
769+
assert!(
770+
result.is_err(),
771+
"redirect to /etc/passwd should be denied by write rules; got {result:?}",
772+
);
773+
}
774+
775+
/// Sibling check: a redirect target inside the working directory
776+
/// (non-external) passes the write-rules check. Without this, a
777+
/// regression that over-broadly denied all redirects could pass
778+
/// the negative case above and ship.
779+
///
780+
/// Uses an in-cwd path because the catch-all at
781+
/// `permission/checker.rs:434` upgrades unmatched-Allow to Ask
782+
/// for EXTERNAL paths — so `/tmp/x` (external to the test's cwd
783+
/// of the dirge repo) would test the external-path catch-all,
784+
/// not the write-rules-allow path we want to exercise here.
785+
/// M3 is intentionally tightening external bash-redirects to
786+
/// prompt; this test pins the in-cwd happy path.
787+
#[cfg(feature = "semantic-bash")]
788+
#[tokio::test]
789+
async fn redirect_target_allowed_when_write_permits() {
790+
use crate::permission::{PermissionConfig, SecurityMode, checker::PermissionChecker};
791+
792+
let config = PermissionConfig::default();
793+
// Use the cwd-relative target so the external-path Ask
794+
// upgrade doesn't fire. `target/test-out.txt` is inside
795+
// any cargo-test invocation's working directory.
796+
let checker = PermissionChecker::new(&config, SecurityMode::Standard, None);
797+
let perm = std::sync::Arc::new(std::sync::Mutex::new(checker));
798+
799+
let result =
800+
check_bash_segments(&Some(perm), &None, "echo hi > target/test-out.txt").await;
801+
assert!(
802+
result.is_ok(),
803+
"redirect to in-cwd target should pass; got {result:?}",
804+
);
805+
}
690806
}

0 commit comments

Comments
 (0)