fix(server): allow newlines in exec command arguments (#1965)

Author: zanetworker

crates/openshell-server/src/grpc/sandbox.rs

@@ -1466,9 +1466,6 @@ fn shell_escape(value: &str) -> Result<String, String> {
     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());
     }
@@ -1555,7 +1552,11 @@ async fn stream_exec_over_relay(
     timeout_seconds: u32,
     request_tty: bool,
 ) -> Result<(), Status> {
-    let command_preview: String = command.chars().take(120).collect();
+    let command_preview: String = command
+        .chars()
+        .take(120)
+        .flat_map(char::escape_default)
+        .collect();
     info!(
         sandbox_id = %sandbox_id,
         channel_id = %channel_id,
@@ -1631,7 +1632,11 @@ async fn stream_interactive_exec_over_relay(
     cols: u32,
     rows: u32,
 ) -> Result<(), Status> {
-    let command_preview: String = command.chars().take(120).collect();
+    let command_preview: String = command
+        .chars()
+        .take(120)
+        .flat_map(char::escape_default)
+        .collect();
     info!(
         sandbox_id = %sandbox_id,
         channel_id = %channel_id,
@@ -2061,10 +2066,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 ----
@@ -2120,7 +2135,45 @@ 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]
+    fn build_remote_exec_command_multiline_with_single_quotes() {
+        use openshell_core::proto::ExecSandboxRequest;
+        let req = ExecSandboxRequest {
+            sandbox_id: "test".to_string(),
+            command: vec![
+                "python3".to_string(),
+                "-c".to_string(),
+                "print('one')\r\nprint('two')".to_string(),
+            ],
+            ..Default::default()
+        };
+        let cmd = build_remote_exec_command(&req).unwrap();
+        assert!(cmd.starts_with("python3 -c "));
+        assert!(
+            cmd.contains("'print('\"'\"'one'\"'\"')\r\nprint('\"'\"'two'\"'\"')'"),
+            "CR/LF with embedded single quotes must compose correctly: {cmd}"
+        );
     }
 
     #[test]

crates/openshell-server/src/grpc/validation.rs

@@ -32,8 +32,10 @@ pub(super) const MAX_EXEC_ARG_LEN: usize = 32 * 1024; // 32 KiB
 /// Maximum length of the workdir field (bytes).
 pub(super) const MAX_EXEC_WORKDIR_LEN: usize = 4096;
 
-/// Validate fields of an `ExecSandboxRequest` for control characters and size
-/// limits before constructing a shell command string.
+/// Validate exec request size limits and field-specific character constraints.
+///
+/// Command arguments only reject NUL (newlines are valid for inline scripts).
+/// Environment values and workdir reject both NUL and newlines.
 pub(super) fn validate_exec_request_fields(req: &ExecSandboxRequest) -> Result<(), Status> {
     if req.command.len() > MAX_EXEC_COMMAND_ARGS {
         return Err(Status::invalid_argument(format!(
@@ -46,7 +48,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 +72,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"
@@ -1800,4 +1814,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"));
+    }
 }