From f1d4fcb3028a7c2cb452855785ee28b8c44458f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Tvrd=C3=ADk?= Date: Fri, 27 Mar 2026 16:46:15 +0100 Subject: [PATCH] fix: validate shell_wrapper template against injection Add validate_shell_wrapper() that checks template parts outside {shell} for dangerous shell metacharacters (;, &&, ||, |, backticks, $( , >, <). If validation fails, the wrapper is skipped and the original shell is returned with a warning log. Closes #71 Co-Authored-By: Claude Code --- crates/okena-workspace/src/hooks.rs | 106 +++++++++++++++++++++++++--- 1 file changed, 97 insertions(+), 9 deletions(-) diff --git a/crates/okena-workspace/src/hooks.rs b/crates/okena-workspace/src/hooks.rs index 9717e4cf..a57b8202 100644 --- a/crates/okena-workspace/src/hooks.rs +++ b/crates/okena-workspace/src/hooks.rs @@ -938,16 +938,43 @@ pub fn resolve_shell_wrapper( resolve_hook_with_parent(project_hooks, parent_hooks, global_hooks, |h| &h.terminal.shell_wrapper) } +/// Dangerous shell metacharacters that indicate injection attempts in wrapper templates. +const DANGEROUS_PATTERNS: &[&str] = &[";", "&&", "||", "|", "`", "$(", ">", "<"]; + +/// Validate a `shell_wrapper` template against shell injection. +/// +/// Splits the wrapper on `{shell}` and checks that the surrounding parts do not contain +/// dangerous shell metacharacters (`;`, `&&`, `||`, `|`, `` ` ``, `$(`, `>`, `<`). +/// Returns `Ok(())` if the template is safe, or `Err(message)` describing the problem. +pub fn validate_shell_wrapper(wrapper: &str) -> Result<(), String> { + let parts: Vec<&str> = wrapper.split("{shell}").collect(); + for (i, part) in parts.iter().enumerate() { + for pattern in DANGEROUS_PATTERNS { + if part.contains(pattern) { + let position = if i == 0 { "before" } else { "after" }; + return Err(format!( + "shell_wrapper contains dangerous pattern '{}' {} {{shell}}: {:?}", + pattern, position, wrapper, + )); + } + } + } + Ok(()) +} + /// Apply shell_wrapper to a ShellType, producing a new ShellType. /// The wrapper template uses `{shell}` as a placeholder for the resolved shell command. /// Environment variables are exported so they persist in the shell session. /// -/// If the result contains shell metacharacters (`&&`, `||`, `;`, `|`), it is wrapped -/// in `sh -c` for proper execution. Otherwise, it is split into executable + args directly, -/// avoiding an extra `sh` process layer (important for session backends like dtach/tmux). +/// If the wrapper fails validation (contains dangerous shell metacharacters outside +/// `{shell}`), a warning is logged and the original shell is returned unwrapped. /// /// The shell is expected to be already resolved (not `ShellType::Default`). pub fn apply_shell_wrapper(shell: &ShellType, wrapper: &str, env_vars: &HashMap) -> ShellType { + if let Err(msg) = validate_shell_wrapper(wrapper) { + log::warn!("Ignoring unsafe shell_wrapper: {}", msg); + return shell.clone(); + } let shell_cmd = shell.to_command_string(); // Replace {shell} with `exec ` so the shell replaces the wrapper process. // This is critical for session backends (dtach/tmux) that monitor the top-level process. @@ -1136,21 +1163,21 @@ mod tests { } #[test] - fn apply_shell_wrapper_with_metacharacters() { + fn apply_shell_wrapper_rejects_metacharacters() { use super::apply_shell_wrapper; let shell = ShellType::Custom { path: "/bin/zsh".to_string(), args: vec![], }; + // Wrapper with && before {shell} is rejected; original shell returned let wrapper = "echo hello && {shell}"; let wrapped = apply_shell_wrapper(&shell, wrapper, &HashMap::new()); match &wrapped { - ShellType::Custom { path: _, args } => { - // for_command uses $SHELL -ic on Unix - assert!(args[0] == "-c" || args[0] == "-ic", "got: {}", args[0]); - assert!(args[1].contains("echo hello && exec /bin/zsh"), "got: {}", args[1]); + ShellType::Custom { path, args } => { + assert_eq!(path, "/bin/zsh"); + assert_eq!(args, &Vec::::new()); } - other => panic!("Expected ShellType::Custom, got: {:?}", other), + other => panic!("Expected original ShellType::Custom, got: {:?}", other), } } @@ -1243,4 +1270,65 @@ mod tests { other => panic!("Expected ShellType::Custom, got: {:?}", other), } } + + // --- validate_shell_wrapper tests --- + + #[test] + fn validate_shell_wrapper_valid_direnv() { + assert!(validate_shell_wrapper("direnv exec . {shell}").is_ok()); + } + + #[test] + fn validate_shell_wrapper_valid_nix() { + assert!(validate_shell_wrapper("nix develop --command {shell}").is_ok()); + } + + #[test] + fn validate_shell_wrapper_injection_semicolon_before() { + let result = validate_shell_wrapper("malicious; {shell}"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains(";")); + } + + #[test] + fn validate_shell_wrapper_injection_subshell_before() { + let result = validate_shell_wrapper("$(evil) {shell}"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("$(")); + } + + #[test] + fn validate_shell_wrapper_injection_semicolon_after() { + let result = validate_shell_wrapper("{shell}; evil"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains(";")); + } + + #[test] + fn validate_shell_wrapper_injection_and_after() { + let result = validate_shell_wrapper("{shell} && steal_data"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("&&")); + } + + #[test] + fn validate_shell_wrapper_injection_backtick() { + let result = validate_shell_wrapper("`whoami` {shell}"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("`")); + } + + #[test] + fn validate_shell_wrapper_injection_pipe() { + let result = validate_shell_wrapper("{shell} | tee /tmp/log"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("|")); + } + + #[test] + fn validate_shell_wrapper_injection_redirect() { + let result = validate_shell_wrapper("{shell} > /tmp/out"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains(">")); + } }