From 7f436bf64c995f2c5092f130145ce41acb906b6c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Jul 2026 22:53:11 -0400 Subject: [PATCH] fix(agent): avoid nested Bash for sandbox MCP scripts --- lib/components/fabro-agent/src/session.rs | 39 +++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/lib/components/fabro-agent/src/session.rs b/lib/components/fabro-agent/src/session.rs index ffc334ce7..a37a199b3 100644 --- a/lib/components/fabro-agent/src/session.rs +++ b/lib/components/fabro-agent/src/session.rs @@ -2104,8 +2104,20 @@ const fn is_auth_error(err: &LlmError) -> bool { /// the current `$BASH` because the sandbox evaluates this string as non-login /// Bash and may resolve that executable outside `/bin` (for example on NixOS). fn sandbox_mcp_launch_script(command: &[String]) -> String { - let cmd_str = shell::shell_join(command); - let inner = format!("{cmd_str} > /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log"); + let command_source = match command { + // Sandbox MCP `script` entries resolve to this exact argv shape. The + // surrounding launcher is already the provider-selected Bash, so + // evaluate the source in that process instead of PATH-resolving a + // second interpreter. Grouping keeps the log redirections scoped to + // the whole script, including multi-command and trailing-comment + // forms. + [interpreter, flag, source] if interpreter == "bash" && flag == "-c" => { + format!("{{\n{source}\n}}") + } + _ => shell::shell_join(command), + }; + let inner = + format!("{command_source} > /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log"); format!( "setsid \"$BASH\" -c {quoted} /dev/null 2>&1 &\necho $!", quoted = shell::shell_quote(&inner) @@ -2180,6 +2192,29 @@ mod tests { ); } + #[test] + fn sandbox_mcp_launch_wrapper_evaluates_scripts_in_the_selected_bash() { + let source = + "PATH=/mcp-only\nprintf 'starting server\\n'\nexec my-server --port 3100 # ready"; + let script = + sandbox_mcp_launch_script(&["bash".to_string(), "-c".to_string(), source.to_string()]); + + let wrapper_argument = script + .strip_prefix("setsid \"$BASH\" -c ") + .and_then(|rest| rest.strip_suffix(" /dev/null 2>&1 &\necho $!")) + .expect("launch wrapper should have the canonical shape"); + let unwrapped = shlex::split(wrapper_argument).expect("wrapper argument should parse"); + + assert_eq!(unwrapped, vec![format!( + "{{\n{source}\n}} > /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log" + )]); + assert!( + !unwrapped[0].contains("bash -c"), + "script entries must not PATH-resolve a nested Bash: {}", + unwrapped[0] + ); + } + #[test] fn sandbox_mcp_launch_wrapper_quotes_arbitrary_argv() { // A quote or metacharacter in any argv element must not break out of