diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index e60ce3995..c2a81694d 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 ---- @@ -2057,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..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_control_chars(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,11 +70,23 @@ 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" ))); } + Ok(()) +} + +/// 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 newline or carriage return characters" @@ -1718,4 +1730,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")); + } }