codewhale-hq/Codewhale
1
0 Fork 0
mirror of https://github.com/codewhale-hq/Codewhale.git synced 2026-11-08 08:04:25 -10:00
Code Issues Packages Projects Releases 1 Wiki Activity

fix(permissions): keep the destructive floor around workspace cleanup clearance

Browse Source 
Third review of GH6787 found regressions versus v0.10.0 for interpreter-embedded deletes, earlier path rebinding and wrappers that reinterpret absolute paths. Only a direct literal rm may use pre-execution workspace clearance. Preserve ordinary relative/absolute workspace cleanup. Move policy tests into their UI owner instead of raising the core-to-UI boundary budget; keep actual child execution coverage. Fix a Unix-only test fixture declaration for Windows.

Evidence:26-case v0.10.0/wave/fixed source comparison; two isolated tests pass and fixes-off control fails as expected. Repository command_safety tests78 passed/0 failed. Combined focused TUI tests143 passed/0 failed, including workspace cleanup, detached holds and Full Access child execution. Config fixtures6/0; tool guidance2/0. npm test674/0; check:web passed. Boundary scan passes with baseline lowered, rustfmt/diff checks pass. Linux native acceptance and final hosted CI remain open; no release claim.
Signed-off-by: Hunter B <[email protected]>
This commit is contained in:
Hunter B committed 2026-09-29 20:04:34 -07:00
1 parent d6cfd0d56b
commit 124331ef9b
5 files changed
+200 -60
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

crates/execpolicy/src/command_safety.rs
+56 -2
View File
@@ -1814,8 +1814,10 @@ pub fn analyze_command(command: &str) -> SafetyAnalysis {
// Check for dangerous patterns first. The token-aware pass above handles
// spacing and quoting variants; these literal patterns remain as a compact
// fallback for legacy shapes. A pattern must not run on into a longer
// path: `rm -rf /` is the root, `rm -rf /home/me/project/build` is not.
// fallback for legacy shapes. Only a single literal rm invocation may
// treat `rm -rf /some/path` as a path instead of this legacy root match.
// Opaque code such as Python's system("rm -rf /etc") was held by this
// fallback before workspace cleanup was exempted; keep that protection.
for (pattern, reason) in DANGEROUS_PATTERNS {
let pattern = pattern.to_lowercase();
if command_lower.match_indices(&pattern).any(|(at, _)| {
@@ -1825,6 +1827,7 @@ pub fn analyze_command(command: &str) -> SafetyAnalysis {
.is_none_or(|next| {
!(next.is_alphanumeric() || matches!(next, '_' | '-' | '.' | '/'))
})
|| (pattern == "rm -rf /" && !is_literal_rm_invocation(command))
}) {
return SafetyAnalysis::dangerous(
command,
@@ -2046,6 +2049,21 @@ fn shell_words(segment: &str) -> Vec<String> {
})
}
/// Whether this is one direct, literal rm invocation. Only that shape may
/// clear an existing workspace path: commands run earlier can replace an
/// ancestor, and wrappers such as sudo --chroot can reinterpret absolute paths.
/// Reuse the shell expander to reject composition, dynamic words and redirects.
pub fn is_literal_rm_invocation(command: &str) -> bool {
let expansion = crate::shell_expand::expand_command(command);
if expansion.control || expansion.dynamic || expansion.arguments_dynamic || expansion.redirects
{
return false;
}
shlex::split(command)
.and_then(|tokens| tokens.first().map(|token| command_word(token) == "rm"))
.unwrap_or(false)
}
/// Every argv that could run as a command somewhere in `command`: each stage
/// (`;`, `&&`, `||`, `|`), started at *every* word of it — so no wrapper's
/// options (`sudo -u me`, `timeout -s KILL 60`, `xargs -0`) can hide the
@@ -2855,6 +2873,42 @@ mod tests {
);
}
#[test]
fn opaque_absolute_deletes_keep_the_legacy_literal_hold() {
for command in [
r#"python3 -c '__import__("os").system("rm -rf /etc")'"#,
r#"perl -e 'system("rm -rf /etc")'"#,
] {
assert_eq!(
analyze_command(command).level,
SafetyLevel::Dangerous,
"{command}"
);
}
}
#[test]
fn literal_rm_clearance_excludes_prior_effects_and_dynamic_targets() {
for command in [
"rm -rf target build node_modules",
"/bin/rm -r -f /workspace/build",
] {
assert!(is_literal_rm_invocation(command), "{command}");
}
for command in [
"mv build saved && rm -rf /workspace/build",
"sudo --chroot=/other-root rm -rf /workspace/build",
"bash -c 'rm -rf /workspace/build'",
"bash -c 'mv build saved; rm -rf /workspace/build'",
"rm -rf /workspace/$(make_path)",
"rm -rf /workspace/$TARGET",
"rm -rf /workspace/build > output",
r#"python3 -c '__import__("os").system("rm -rf /workspace/build")'"#,
] {
assert!(!is_literal_rm_invocation(command), "{command}");
}
}
#[test]
fn command_invocations_reach_past_wrapper_options_and_into_shell_payloads() {
let has_rm = |command: &str| {
crates/tui/src/core/engine/tests.rs
-29
View File
@@ -11245,35 +11245,6 @@ fn auto_review_routes_shell_commands_requiring_approval_to_reviewer() {
}
}
#[test]
fn full_access_blocks_detached_catastrophic_tools_without_prompting() {
for run_origin in [
crate::tui::auto_review::RunOrigin::Background,
crate::tui::auto_review::RunOrigin::Headless,
] {
let (decision, audit) = auto_review_plan_decision(
&crate::tui::auto_review::AutoReviewPolicy::default(),
"exec_shell",
&json!({"command": "rm -rf ~/", "background": true}),
run_origin,
ApprovalMode::Bypass,
true,
None,
);
assert_eq!(
decision,
AutoReviewPlanDecision::Block(
"Built-in safety gate requires approval: destructive background/headless action requires durable review"
.to_string()
)
);
assert_eq!(audit["approval_mode"], "BYPASS");
assert_eq!(audit["run_origin"], run_origin.as_str());
assert_eq!(audit["decision"], "hold_for_review");
}
}
#[test]
fn auto_review_plan_decision_uses_configured_policy() {
let policy = crate::tui::auto_review::AutoReviewPolicy {
crates/tui/src/tools/subagent/tests.rs
+2 -20
View File
@@ -24689,26 +24689,8 @@ mod child_permission_gate {
let build = workspace.join("build");
std::fs::create_dir_all(build.join("out")).unwrap();
let command = format!("rm -rf {}", build.display());
// The parent turn's own verdict for this exact call under Full Access.
let parent_context = crate::tui::auto_review::AutoReviewContext::from_tool_call(
"bash",
&json!({"command": command}),
crate::core::engine::auto_review_run_origin_for_plan(false),
ApprovalMode::Bypass,
true,
Some(&workspace),
);
let (parent_decision, _) = crate::core::engine::auto_review_plan_decision_for_context(
&crate::tui::auto_review::AutoReviewPolicy::default(),
&parent_context,
);
assert!(
!matches!(
parent_decision,
crate::core::engine::AutoReviewPlanDecision::Block(_)
),
"precondition: the parent runs it: {parent_decision:?}"
);
// Parent classification is covered in tui::auto_review; this runtime
// test proves the child actually executes and preserves the gate receipt.
registry
.execute("agent_gate", "bash", json!({"command": command}))
.await
crates/tui/src/tui/auto_review.rs
+138 -5
View File
@@ -1025,7 +1025,9 @@ fn shell_params_are_publish_like(params: &Value) -> bool {
/// durable-review floor armed now that the floor no longer treats every
/// non-read-only command as destructive (#3883).
fn shell_params_are_destructive_like(params: &Value, workspace: Option<&std::path::Path>) -> bool {
use codewhale_execpolicy::command_safety::{SafetyLevel, analyze_command, command_invocations};
use codewhale_execpolicy::command_safety::{
SafetyLevel, analyze_command, command_invocations, is_literal_rm_invocation,
};
let Some(command) = params
.get("command")
.or_else(|| params.get("cmd"))
@@ -1037,11 +1039,11 @@ fn shell_params_are_destructive_like(params: &Value, workspace: Option<&std::pat
split_shell_segments_for_review(command)
.iter()
.any(|segment| analyze_command(segment).level == SafetyLevel::Dangerous)
// The same reader command_safety uses; unreadable nesting holds. A
// command that makes links can change what a path means before `rm`
// runs, so it gets no in-workspace clearance.
// Only one literal rm may use paths resolved before execution. Any
// earlier command could replace an ancestor (mv and Python can do
// that just as ln can), invalidating the workspace clearance.
|| command_invocations(command).is_none_or(|argvs| {
let workspace = workspace.filter(|_| !argvs.iter().any(|argv| argv[0] == "ln"));
let workspace = workspace.filter(|_| is_literal_rm_invocation(command));
argvs.iter().any(|argv| argv_is_destroyer(argv, workspace))
})
}
@@ -1703,16 +1705,147 @@ mod tests {
"\\rm -rf /home/me",
"/bin/rm -rf /home/me",
"FOO=1 env -i rm -rf /home/me",
"command rm -rf /etc",
"exec rm -rf /etc",
"eval 'rm -rf /etc'",
"r''m -rf /etc",
"rm -r -f /etc",
"rm --recursive --force /etc",
"rm -rf -- /etc",
"find / -delete",
"busybox rm -rf /etc",
"nohup rm -rf /etc",
"xargs -0 rm -rf /etc",
] {
assert!(destroyer_held(workspace.path(), command), "{command}");
}
}
#[test]
fn opaque_program_deletes_keep_the_legacy_destroyer_floor() {
let workspace = tempfile::tempdir().expect("tempdir");
for command in [
r#"python3 -c '__import__("os").system("rm -rf /etc")'"#,
r#"perl -e 'system("rm -rf /etc")'"#,
] {
assert!(destroyer_held(workspace.path(), command), "{command}");
}
}
#[test]
fn wrappers_cannot_borrow_workspace_path_clearance() {
let workspace = tempfile::tempdir().expect("workspace");
std::fs::create_dir(workspace.path().join("build")).unwrap();
let build = workspace.path().join("build").display().to_string();
for command in [
format!("sudo --chroot=/other-root rm -rf {build}"),
format!("sudo --chroot=/other-root rm -r -f {build}"),
] {
assert!(destroyer_held(workspace.path(), &command), "{command}");
}
}
#[cfg(unix)]
#[test]
fn composed_deletes_cannot_clear_paths_that_an_earlier_command_rebinds() {
let workspace = tempfile::tempdir().expect("workspace");
let outside = tempfile::tempdir().expect("outside");
let root = workspace.path();
std::fs::create_dir_all(root.join("build/data")).unwrap();
std::fs::create_dir_all(outside.path().join("data")).unwrap();
let sentinel = outside.path().join("data/keep");
std::fs::write(&sentinel, b"keep").unwrap();
std::os::unix::fs::symlink(outside.path(), root.join("link")).unwrap();
let ws = root.display();
for flags in ["-rf", "-r -f", "--recursive --force"] {
let command = format!(
"mv {ws}/build {ws}/saved && mv {ws}/link {ws}/build && rm {flags} {ws}/build/data"
);
assert!(destroyer_held(root, &command), "{command}");
}
// Exercise only the harmless rebinding, never the destructive command:
// the path checked inside the workspace would now delete outside it.
assert!(
root.join("build/data")
.canonicalize()
.unwrap()
.starts_with(root.canonicalize().unwrap())
);
std::fs::rename(root.join("build"), root.join("saved")).unwrap();
std::fs::rename(root.join("link"), root.join("build")).unwrap();
assert_eq!(
root.join("build/data").canonicalize().unwrap(),
outside.path().join("data").canonicalize().unwrap()
);
assert_eq!(std::fs::read(sentinel).unwrap(), b"keep");
}
#[test]
fn full_access_workspace_cleanup_stays_clear() {
use crate::core::engine::{AutoReviewPlanDecision, auto_review_plan_decision_for_context};
let workspace = tempfile::tempdir().expect("workspace");
let root = workspace.path();
for directory in ["target", "build", "node_modules"] {
std::fs::create_dir(root.join(directory)).unwrap();
}
let ws = root.display();
for command in [
"rm -rf target build node_modules".to_string(),
format!("rm -rf {ws}/target {ws}/build {ws}/node_modules"),
] {
assert!(!destroyer_held(root, &command), "{command}");
let context = AutoReviewContext::from_tool_call(
"bash",
&json!({"command": command}),
RunOrigin::Interactive,
ApprovalMode::Bypass,
true,
Some(root),
);
let (decision, _) =
auto_review_plan_decision_for_context(&AutoReviewPolicy::default(), &context);
assert!(
!matches!(decision, AutoReviewPlanDecision::Block(_)),
"Full Access parent cleanup must run: {decision:?}"
);
}
}
#[test]
fn full_access_blocks_detached_catastrophic_tools_without_prompting() {
use crate::core::engine::{AutoReviewPlanDecision, auto_review_plan_decision_for_context};
for run_origin in [RunOrigin::Background, RunOrigin::Headless] {
let context = AutoReviewContext::from_tool_call(
"exec_shell",
&json!({"command": "rm -rf ~/", "background": true}),
run_origin,
ApprovalMode::Bypass,
true,
None,
);
let (decision, audit) =
auto_review_plan_decision_for_context(&AutoReviewPolicy::default(), &context);
assert_eq!(
decision,
AutoReviewPlanDecision::Block(
"Built-in safety gate requires approval: destructive background/headless action requires durable review"
.to_string()
)
);
assert_eq!(audit["approval_mode"], "BYPASS");
assert_eq!(audit["run_origin"], run_origin.as_str());
assert_eq!(audit["decision"], "hold_for_review");
}
}
/// Second review of 05264125e: targets whose meaning is only known when
/// the shell runs never count as inside the workspace.
#[test]
fn unknowable_or_unsafe_targets_are_never_inside_the_workspace() {
let workspace = tempfile::tempdir().expect("tempdir");
#[cfg(unix)]
let outside = tempfile::tempdir().expect("tempdir");
let root = workspace.path();
std::fs::create_dir_all(root.join("build")).unwrap();
scripts/runtime-boundary-baseline.json
+4 -4
View File
@@ -1,8 +1,8 @@
{
"_comment": "Runtime -> UI boundary ratchet (docs/design/TUI_DECONSTRUCTION.md, runtime split). Counts may only go down. Regenerate with python3 scripts/split/module_graph.py --update after removing references; never raise a count by hand.",
"totals": {
"prod": 75,
"test": 89,
"prod": 74,
"test": 88,
"late": 1,
"uilib": 1,
"doc": 1
@@ -17,12 +17,12 @@
"runtime_api|tui": 1,
"runtime_threads|lib.rs": 2,
"tools|commands": 2,
"tools|tui": 8
"tools|tui": 7
},
"test": {
"client|tui": 3,
"config|tui": 9,
"core|tui": 40,
"core|tui": 39,
"doctor|lib.rs": 5,
"fleet|lib.rs": 4,
"prompts|tui": 1,