Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zanetworker wants to merge 3 commits into
base: main
Choose a base branch
from
+96 −9
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac7966f
fix(server): allow newlines in shell_escape for multi-line exec scripts
zanetworker Jun 21, 2026
8a392b2
fix(server): allow newlines in exec command arguments
zanetworker Jun 21, 2026
42e884d
refactor(server): decompose reject_control_chars per reviewer feedback
zanetworker Jun 23, 2026
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
View file
Open in desktop
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() {

@elezar elezar Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this helper now allows \n/\r, the downstream command_preview fields in the exec logging paths should not log the assembled command raw. In line-oriented log sinks, user-controlled CR/LF can split records or create forged-looking continuation lines. Can we escape the preview before logging, and reuse the same helper for both non-interactive and interactive exec?

let command_preview: String = command
    .chars()
    .take(120)
    .flat_map(char::escape_default)
    .collect();

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());

@elezar elezar Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This test name still says build_remote_exec_command, but the assertion now exercises validate_exec_request_fields(). Could we rename it to validate_exec_rejects_newlines_in_workdir, or drop it since validation.rs already has this coverage?

}

#[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(),

@elezar elezar Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should include both CR/LF and a single quote in the same command argument. The risky interaction here is not just that newlines are accepted, but that multiline script text still composes correctly with POSIX single-quote escaping.

For example, something like print('one')\r\nprint('two') would give us regression coverage for the exact case this helper has to keep safe.

],
..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
64 changes: 63 additions & 1 deletion crates/openshell-server/src/grpc/validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}"))?;

@elezar elezar Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The function comment above still says this validates control characters generally, but command args now intentionally allow CR/LF and only reject NUL. Maybe reword it to something like: Validate exec request size limits and field-specific character constraints.

}
for (key, value) in &req.environment {
if value.len() > MAX_EXEC_ARG_LEN {
Expand All @@ -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"
Expand Down Expand Up @@ -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"));
}
}
Loading