Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 33 additions & 8 deletions crates/openshell-server/src/grpc/sandbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1457,9 +1457,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() {
Comment thread
zanetworker marked this conversation as resolved.
return Ok("''".to_string());
}
Expand Down Expand Up @@ -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 ----
Expand Down Expand Up @@ -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());
Comment thread
zanetworker marked this conversation as resolved.
}

#[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(),
Comment thread
zanetworker marked this conversation as resolved.
],
..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]
Expand Down
70 changes: 67 additions & 3 deletions crates/openshell-server/src/grpc/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand All @@ -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}"))?;
Comment thread
zanetworker marked this conversation as resolved.
}
for (key, value) in &req.environment {
if value.len() > MAX_EXEC_ARG_LEN {
Expand All @@ -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"
Expand Down Expand Up @@ -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"));
}
}
Loading