From ac7966fccd3b9b28b138f63133c96bbf61d26dfc Mon Sep 17 00:00:00 2001 From: Adel Zaalouk Date: Sun, 21 Jun 2026 21:23:55 +0200 Subject: [PATCH 1/3] fix(server): allow newlines in shell_escape for multi-line exec scripts Single-quoted POSIX strings treat newlines as literal characters, not command separators. The newline rejection prevented multi-line scripts passed via sandbox exec (e.g. python3 -c with a multi-line body). Null byte rejection is preserved since null bytes truncate C-level shell parsing. Closes #1963 Signed-off-by: Adel Zaalouk --- crates/openshell-server/src/grpc/sandbox.rs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index e60ce3995..642961ca1 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -1457,9 +1457,6 @@ fn shell_escape(value: &str) -> Result { if value.bytes().any(|b| b == 0) { return Err("value contains null bytes".to_string()); } - if value.bytes().any(|b| b == b'\n' || b == b'\r') { - return Err("value contains newline or carriage return".to_string()); - } if value.is_empty() { return Ok("''".to_string()); } @@ -1998,10 +1995,20 @@ mod tests { } #[test] - fn shell_escape_rejects_newlines() { - assert!(shell_escape("line1\nline2").is_err()); - assert!(shell_escape("line1\rline2").is_err()); - assert!(shell_escape("line1\r\nline2").is_err()); + fn shell_escape_allows_newlines() { + assert!(shell_escape("line1\nline2").is_ok()); + assert!(shell_escape("line1\rline2").is_ok()); + assert!(shell_escape("line1\r\nline2").is_ok()); + } + + #[test] + fn shell_escape_preserves_newlines_in_single_quotes() { + assert_eq!(shell_escape("line1\nline2").unwrap(), "'line1\nline2'"); + assert_eq!( + shell_escape("def f():\n return 1").unwrap(), + "'def f():\n return 1'" + ); + assert_eq!(shell_escape("line1\r\nline2").unwrap(), "'line1\r\nline2'"); } // ---- build_remote_exec_command ---- From 8a392b24620c80336153aae686f53f74afb2f777 Mon Sep 17 00:00:00 2001 From: Adel Zaalouk Date: Sun, 21 Jun 2026 21:31:00 +0200 Subject: [PATCH 2/3] fix(server): allow newlines in exec command arguments Add reject_null_bytes() for exec command argument validation, replacing reject_control_chars() which also rejected newlines. Multi-line scripts passed to bash -c or python3 -c are legitimate and safe because shell_escape() wraps them in POSIX single quotes where newlines are literal. Newline rejection is preserved for workdir (no legitimate path contains newlines) and environment values. Closes #1963 Signed-off-by: Adel Zaalouk --- crates/openshell-server/src/grpc/sandbox.rs | 20 +++++- .../openshell-server/src/grpc/validation.rs | 66 ++++++++++++++++++- 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 642961ca1..c2a81694d 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -2064,7 +2064,25 @@ mod tests { workdir: "/tmp\nmalicious".to_string(), ..Default::default() }; - assert!(build_remote_exec_command(&req).is_err()); + // Validation layer rejects newlines in workdir + assert!(validate_exec_request_fields(&req).is_err()); + } + + #[test] + fn build_remote_exec_command_accepts_multiline_script() { + use openshell_core::proto::ExecSandboxRequest; + let req = ExecSandboxRequest { + sandbox_id: "test".to_string(), + command: vec![ + "python3".to_string(), + "-c".to_string(), + "def f():\n return 1\nprint(f())".to_string(), + ], + ..Default::default() + }; + let cmd = build_remote_exec_command(&req).unwrap(); + assert!(cmd.starts_with("python3 -c ")); + assert!(cmd.contains("'def f():\n return 1\nprint(f())'")); } #[test] diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 03a69d6e9..792902862 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -46,7 +46,7 @@ pub(super) fn validate_exec_request_fields(req: &ExecSandboxRequest) -> Result<( "command argument {i} exceeds {MAX_EXEC_ARG_LEN} byte limit" ))); } - reject_control_chars(arg, &format!("command argument {i}"))?; + reject_null_bytes(arg, &format!("command argument {i}"))?; } for (key, value) in &req.environment { if value.len() > MAX_EXEC_ARG_LEN { @@ -83,6 +83,20 @@ pub(super) fn reject_control_chars(value: &str, field_name: &str) -> Result<(), Ok(()) } +/// Reject only null bytes in a user-supplied value. +/// +/// Use this for exec command arguments where newlines are legitimate +/// (e.g. multi-line scripts passed to `bash -c` or `python3 -c`). +/// The shell-escape layer handles newline safety via single-quoting. +pub(super) fn reject_null_bytes(value: &str, field_name: &str) -> Result<(), Status> { + if value.bytes().any(|b| b == 0) { + return Err(Status::invalid_argument(format!( + "{field_name} contains null bytes" + ))); + } + Ok(()) +} + // --------------------------------------------------------------------------- // Sandbox spec validation // --------------------------------------------------------------------------- @@ -1718,4 +1732,54 @@ mod tests { assert!(reject_control_chars("line1\nline2", "test").is_err()); assert!(reject_control_chars("line1\rline2", "test").is_err()); } + + #[test] + fn validate_exec_allows_newlines_in_command_args() { + let req = ExecSandboxRequest { + sandbox_id: "test".to_string(), + command: vec![ + "python3".to_string(), + "-c".to_string(), + "def f():\n return 1\nprint(f())".to_string(), + ], + ..Default::default() + }; + assert!(validate_exec_request_fields(&req).is_ok()); + } + + #[test] + fn validate_exec_still_rejects_null_bytes_in_command_args() { + let req = ExecSandboxRequest { + sandbox_id: "test".to_string(), + command: vec!["echo".to_string(), "hello\x00world".to_string()], + ..Default::default() + }; + let err = validate_exec_request_fields(&req).unwrap_err(); + assert!(err.message().contains("null")); + } + + #[test] + fn validate_exec_still_rejects_newlines_in_workdir() { + let req = ExecSandboxRequest { + sandbox_id: "test".to_string(), + command: vec!["ls".to_string()], + workdir: "/tmp\nmalicious".to_string(), + ..Default::default() + }; + let err = validate_exec_request_fields(&req).unwrap_err(); + assert!(err.message().contains("newline")); + } + + #[test] + fn validate_exec_still_rejects_newlines_in_env_values() { + let req = ExecSandboxRequest { + sandbox_id: "test".to_string(), + command: vec!["ls".to_string()], + environment: std::iter::once(("VAR".to_string(), "val\nmalicious".to_string())) + .collect(), + ..Default::default() + }; + let err = validate_exec_request_fields(&req).unwrap_err(); + assert!(err.message().contains("newline")); + } } From 42e884d3c92c5a755e7e69fb4356e6f4cc66a217 Mon Sep 17 00:00:00 2001 From: Adel Zaalouk Date: Tue, 23 Jun 2026 11:12:28 +0200 Subject: [PATCH 3/3] refactor(server): decompose reject_control_chars per reviewer feedback Split reject_control_chars into reject_null_char and reject_newline_chars so callers can compose the checks they need. reject_control_chars now delegates to both. Exec command arguments call reject_null_char directly. Addresses review feedback from @elezar on PR #1965. Signed-off-by: Adel Zaalouk --- .../openshell-server/src/grpc/validation.rs | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 792902862..6785a3e1a 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -46,7 +46,7 @@ pub(super) fn validate_exec_request_fields(req: &ExecSandboxRequest) -> Result<( "command argument {i} exceeds {MAX_EXEC_ARG_LEN} byte limit" ))); } - reject_null_bytes(arg, &format!("command argument {i}"))?; + reject_null_char(arg, &format!("command argument {i}"))?; } for (key, value) in &req.environment { if value.len() > MAX_EXEC_ARG_LEN { @@ -70,28 +70,26 @@ pub(super) fn validate_exec_request_fields(req: &ExecSandboxRequest) -> Result<( /// Reject null bytes and newlines in a user-supplied value. pub(super) fn reject_control_chars(value: &str, field_name: &str) -> Result<(), Status> { + reject_null_char(value, field_name)?; + reject_newline_chars(value, field_name)?; + Ok(()) +} + +/// Reject null bytes in a user-supplied value. +pub(super) fn reject_null_char(value: &str, field_name: &str) -> Result<(), Status> { if value.bytes().any(|b| b == 0) { return Err(Status::invalid_argument(format!( "{field_name} contains null bytes" ))); } - if value.bytes().any(|b| b == b'\n' || b == b'\r') { - return Err(Status::invalid_argument(format!( - "{field_name} contains newline or carriage return characters" - ))); - } Ok(()) } -/// Reject only null bytes in a user-supplied value. -/// -/// Use this for exec command arguments where newlines are legitimate -/// (e.g. multi-line scripts passed to `bash -c` or `python3 -c`). -/// The shell-escape layer handles newline safety via single-quoting. -pub(super) fn reject_null_bytes(value: &str, field_name: &str) -> Result<(), Status> { - if value.bytes().any(|b| b == 0) { +/// Reject newline and carriage return characters in a user-supplied value. +pub(super) fn reject_newline_chars(value: &str, field_name: &str) -> Result<(), Status> { + if value.bytes().any(|b| b == b'\n' || b == b'\r') { return Err(Status::invalid_argument(format!( - "{field_name} contains null bytes" + "{field_name} contains newline or carriage return characters" ))); } Ok(())