From 32e3ffdc5a5168b35f978703f7ab910ce4f74f7c Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Wed, 3 Jun 2026 18:04:27 -0400 Subject: [PATCH 1/5] feat(cli): add --env flag to sandbox create and exec Add `--env KEY=VALUE` (repeatable) to `openshell sandbox create` and `openshell sandbox exec`, wiring user-specified environment variables into the existing `SandboxSpec.environment` and `ExecSandboxRequest.environment` protobuf fields which were previously only accessible via the SDK/gRPC API. Fix a bug where user-specified environment variables set via `SandboxSpec.environment` were silently dropped inside sandbox SSH sessions. The SSH handler's `apply_child_env()` calls `env_clear()` to strip supervisor-internal secrets before spawning user shells, but this also wiped container-level env vars including user-specified ones. This was introduced in 5fd4885a (2026-02-26, "feat(sandbox): VS Code Remote-SSH support with platform detection fix") which added `env_clear()` to fix a VS Code platform detection timeout caused by leaked supervisor env vars. The fix introduces an `OPENSHELL_USER_ENVIRONMENT` sidecar env var: compute drivers (Docker, Podman, Kubernetes, VM) serialize the user-specified environment as JSON into this variable at container creation time, the sandbox supervisor deserializes it at startup, and the SSH handler re-injects the values into child processes after `env_clear()`. User-specified vars are applied at lowest priority so proxy, TLS, and provider credentials still take precedence. Signed-off-by: Russell Bryant --- Cargo.lock | 1 + crates/openshell-cli/src/main.rs | 16 ++++ crates/openshell-cli/src/run.rs | 22 ++++-- .../sandbox_create_lifecycle_integration.rs | 77 +++++++++++++++++++ crates/openshell-core/src/sandbox_env.rs | 7 ++ crates/openshell-driver-docker/Cargo.toml | 1 + crates/openshell-driver-docker/src/lib.rs | 14 +++- .../openshell-driver-kubernetes/src/driver.rs | 7 ++ .../openshell-driver-podman/src/container.rs | 14 +++- crates/openshell-driver-vm/src/driver.rs | 11 ++- crates/openshell-sandbox/src/lib.rs | 9 +++ crates/openshell-sandbox/src/ssh.rs | 22 ++++++ 12 files changed, 191 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4bc657be3..d0cd77f85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3482,6 +3482,7 @@ dependencies = [ "futures", "openshell-core", "serde", + "serde_json", "tar", "temp-env", "tempfile", diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index d4a19c4bf..d592bc262 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -1275,6 +1275,10 @@ enum SandboxCommands { #[arg(long = "label")] labels: Vec, + /// Environment variables to inject into the sandbox (KEY=VALUE format, repeatable). + #[arg(long = "env", value_name = "KEY=VALUE")] + envs: Vec, + /// Approval mode for agent-authored policy proposals. /// /// `manual` (default): every proposal lands in the draft inbox for @@ -1381,6 +1385,10 @@ enum SandboxCommands { #[arg(long, overrides_with = "tty")] no_tty: bool, + /// Environment variables to set for the command (KEY=VALUE format, repeatable). + #[arg(long = "env", value_name = "KEY=VALUE")] + envs: Vec, + /// Command and arguments to execute. #[arg(required = true, trailing_var_arg = true, allow_hyphen_values = true)] command: Vec, @@ -2558,6 +2566,7 @@ async fn main() -> Result<()> { auto_providers, no_auto_providers, labels, + envs, approval_mode, command, } => { @@ -2592,6 +2601,9 @@ async fn main() -> Result<()> { labels_map.insert(parts[0].to_string(), parts[1].to_string()); } + // Parse --env flags into a HashMap. + let env_map = run::parse_env_pairs(&envs)?; + // Parse --upload specs into [(local_path, sandbox_path, git_ignore)]. let upload_specs: Vec<(String, Option, bool)> = upload .iter() @@ -2639,6 +2651,7 @@ async fn main() -> Result<()> { tty_override, auto_providers_override, &labels_map, + &env_map, &approval_mode, &tls, )) @@ -2751,6 +2764,7 @@ async fn main() -> Result<()> { timeout, tty, no_tty, + envs, command, } => { let name = resolve_sandbox_name(name, &ctx.name)?; @@ -2762,6 +2776,7 @@ async fn main() -> Result<()> { } else { None // auto-detect }; + let env_map = run::parse_key_value_pairs(&envs, "--env")?; let exit_code = run::sandbox_exec_grpc( endpoint, &name, @@ -2769,6 +2784,7 @@ async fn main() -> Result<()> { workdir.as_deref(), timeout, tty_override, + &env_map, &tls, ) .await?; diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 64145d883..7361c1acc 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -1756,6 +1756,7 @@ pub async fn sandbox_create( tty_override: Option, auto_providers_override: Option, labels: &HashMap, + environment: &HashMap, approval_mode: &str, tls: &TlsOptions, ) -> Result<()> { @@ -1836,6 +1837,7 @@ pub async fn sandbox_create( spec: Some(SandboxSpec { gpu: requested_gpu, gpu_device: gpu_device.unwrap_or_default().to_string(), + environment: environment.clone(), policy, providers: configured_providers, template, @@ -2638,6 +2640,7 @@ const MAX_STDIN_PAYLOAD: usize = 4 * 1024 * 1024; /// Execute a command in a running sandbox via gRPC, streaming output to the terminal. /// /// Returns the remote command's exit code. +#[allow(clippy::too_many_arguments, clippy::implicit_hasher)] pub async fn sandbox_exec_grpc( server: &str, name: &str, @@ -2645,6 +2648,7 @@ pub async fn sandbox_exec_grpc( workdir: Option<&str>, timeout_seconds: u32, tty_override: Option, + environment: &HashMap, tls: &TlsOptions, ) -> Result { let mut client = grpc_client(server, tls).await?; @@ -2699,8 +2703,15 @@ pub async fn sandbox_exec_grpc( .unwrap_or_else(|| std::io::stdin().is_terminal() && std::io::stdout().is_terminal()); if tty_override == Some(true) && std::io::stdin().is_terminal() { - return sandbox_exec_interactive_grpc(client, &sandbox, command, workdir, timeout_seconds) - .await; + return sandbox_exec_interactive_grpc( + client, + &sandbox, + command, + workdir, + timeout_seconds, + environment, + ) + .await; } // Make the streaming gRPC call. @@ -2709,7 +2720,7 @@ pub async fn sandbox_exec_grpc( sandbox_id: sandbox.object_id().to_string(), command: command.to_vec(), workdir: workdir.unwrap_or_default().to_string(), - environment: HashMap::new(), + environment: environment.clone(), timeout_seconds, stdin: stdin_payload, tty, @@ -3055,6 +3066,7 @@ async fn sandbox_exec_interactive_grpc( command: &[String], workdir: Option<&str>, timeout_seconds: u32, + environment: &HashMap, ) -> Result { use openshell_core::proto::{ExecSandboxInput, ExecSandboxWindowResize, exec_sandbox_input}; use tokio_stream::wrappers::ReceiverStream; @@ -3070,7 +3082,7 @@ async fn sandbox_exec_interactive_grpc( sandbox_id: sandbox.object_id().to_string(), command: command.to_vec(), workdir: workdir.unwrap_or_default().to_string(), - environment: HashMap::new(), + environment: environment.clone(), timeout_seconds, stdin: Vec::new(), tty: true, @@ -3888,7 +3900,7 @@ async fn auto_create_provider( Ok(()) } -fn parse_key_value_pairs(items: &[String], flag: &str) -> Result> { +pub fn parse_key_value_pairs(items: &[String], flag: &str) -> Result> { let mut map = HashMap::new(); for item in items { diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 96821655d..c2d52f291 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -795,6 +795,7 @@ async fn sandbox_create_keeps_command_sessions_by_default() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -838,6 +839,7 @@ async fn sandbox_create_sends_cpu_and_memory_limits_only() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -989,6 +991,7 @@ async fn sandbox_create_does_not_infer_command_providers_when_v2_enabled() { Some(true), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1047,6 +1050,7 @@ async fn sandbox_create_returns_vm_error_without_waiting_for_timeout() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1101,6 +1105,7 @@ async fn sandbox_create_keeps_waiting_while_vm_progress_arrives() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1147,6 +1152,7 @@ async fn sandbox_create_times_out_when_only_logs_arrive() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1189,6 +1195,7 @@ async fn sandbox_create_deletes_command_sessions_with_no_keep() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1235,6 +1242,7 @@ async fn sandbox_create_deletes_shell_sessions_with_no_keep() { Some(true), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1281,6 +1289,7 @@ async fn sandbox_create_keeps_sandbox_with_hidden_keep_flag() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1327,6 +1336,7 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1335,3 +1345,70 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { assert!(deleted_names(&server).await.is_empty()); } + +#[tokio::test] +async fn sandbox_create_sends_environment_variables() { + let server = run_server().await; + let fake_ssh_dir = tempfile::tempdir().unwrap(); + let xdg_dir = tempfile::tempdir().unwrap(); + let _env = test_env(&fake_ssh_dir, &xdg_dir); + let tls = test_tls(&server); + install_fake_ssh(&fake_ssh_dir); + + let mut env_map = HashMap::new(); + env_map.insert("FOO".to_string(), "bar".to_string()); + env_map.insert("BAZ".to_string(), "qux=with=equals".to_string()); + + run::sandbox_create( + &server.endpoint, + Some("env-test"), + None, + "openshell", + None, + true, + false, + None, + None, + None, + None, + &[], + None, + None, + &["echo".to_string(), "OK".to_string()], + Some(false), + Some(false), + &HashMap::new(), + &env_map, + "manual", + &tls, + ) + .await + .expect("sandbox create should succeed"); + + let requests = create_requests(&server).await; + let environment = &requests[0] + .spec + .as_ref() + .expect("spec should be present") + .environment; + assert_eq!(environment.get("FOO").map(String::as_str), Some("bar")); + assert_eq!( + environment.get("BAZ").map(String::as_str), + Some("qux=with=equals") + ); + assert_eq!(environment.len(), 2); +} + +#[tokio::test] +async fn sandbox_create_env_rejects_invalid_format() { + let err = run::parse_key_value_pairs( + &["VALID=ok".to_string(), "NOEQUALSSIGN".to_string()], + "--env", + ) + .unwrap_err(); + let msg = format!("{err}"); + assert!( + msg.contains("--env") && msg.contains("NOEQUALSSIGN"), + "error should mention the flag and bad value, got: {msg}" + ); +} diff --git a/crates/openshell-core/src/sandbox_env.rs b/crates/openshell-core/src/sandbox_env.rs index 9ffbf79f0..1059c0d08 100644 --- a/crates/openshell-core/src/sandbox_env.rs +++ b/crates/openshell-core/src/sandbox_env.rs @@ -50,6 +50,13 @@ pub const SANDBOX_TOKEN: &str = "OPENSHELL_SANDBOX_TOKEN"; /// the token is held in process memory thereafter. pub const SANDBOX_TOKEN_FILE: &str = "OPENSHELL_SANDBOX_TOKEN_FILE"; +/// JSON-serialized map of user-specified environment variables. +/// +/// Set by compute drivers from `SandboxSpec.environment`. The sandbox +/// supervisor deserializes this at startup and injects the variables into +/// SSH child processes (which use `env_clear()` for security isolation). +pub const USER_ENVIRONMENT: &str = "OPENSHELL_USER_ENVIRONMENT"; + /// Path to the projected `ServiceAccount` JWT (Kubernetes driver). /// /// Used to bootstrap a gateway-minted JWT via `IssueSandboxToken`. Kubelet diff --git a/crates/openshell-driver-docker/Cargo.toml b/crates/openshell-driver-docker/Cargo.toml index 0cdc205ed..fb2a643ea 100644 --- a/crates/openshell-driver-docker/Cargo.toml +++ b/crates/openshell-driver-docker/Cargo.toml @@ -20,6 +20,7 @@ tokio-stream = { workspace = true } tracing = { workspace = true } bytes = { workspace = true } serde = { workspace = true } +serde_json = { workspace = true } bollard = { version = "0.20" } tar = "0.4" tempfile = "3" diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 864d91f22..2d69a1d88 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -1643,10 +1643,20 @@ fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig ]); if let Some(spec) = sandbox.spec.as_ref() { + let mut user_env = HashMap::new(); if let Some(template) = spec.template.as_ref() { - environment.extend(template.environment.clone()); + user_env.extend(template.environment.clone()); } - environment.extend(spec.environment.clone()); + user_env.extend(spec.environment.clone()); + if !user_env.is_empty() + && let Ok(json) = serde_json::to_string(&user_env) + { + environment.insert( + openshell_core::sandbox_env::USER_ENVIRONMENT.to_string(), + json, + ); + } + environment.extend(user_env); } environment.insert( diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index 449cee58d..8fb079f5c 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -1625,6 +1625,13 @@ fn build_env_list( let mut env = existing_env.cloned().unwrap_or_default(); apply_env_map(&mut env, template_environment); apply_env_map(&mut env, spec_environment); + let mut user_env = template_environment.clone(); + user_env.extend(spec_environment.clone()); + if !user_env.is_empty() + && let Ok(json) = serde_json::to_string(&user_env) + { + upsert_env(&mut env, openshell_core::sandbox_env::USER_ENVIRONMENT, &json); + } apply_required_env( &mut env, sandbox_id, diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 13f053e93..44f53a9ee 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -244,6 +244,7 @@ fn build_env( let mut env: BTreeMap = BTreeMap::new(); // 1. User-supplied environment (lowest priority). + let mut user_env: BTreeMap = BTreeMap::new(); if let Some(s) = spec { if !s.log_level.is_empty() { env.insert( @@ -252,14 +253,23 @@ fn build_env( ); } for (k, v) in &s.environment { - env.insert(k.clone(), v.clone()); + user_env.insert(k.clone(), v.clone()); } } if let Some(t) = template { for (k, v) in &t.environment { - env.insert(k.clone(), v.clone()); + user_env.insert(k.clone(), v.clone()); } } + if !user_env.is_empty() + && let Ok(json) = serde_json::to_string(&user_env) + { + env.insert( + openshell_core::sandbox_env::USER_ENVIRONMENT.into(), + json, + ); + } + env.extend(user_env); // 2. Required driver vars (highest priority -- always overwrite). env.insert( diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index 445905a1e..bd124abfb 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -3498,7 +3498,16 @@ fn build_guest_environment( ), ])); } - environment.extend(merged_environment(sandbox)); + let user_env = merged_environment(sandbox); + if !user_env.is_empty() + && let Ok(json) = serde_json::to_string(&user_env) + { + environment.insert( + openshell_core::sandbox_env::USER_ENVIRONMENT.to_string(), + json, + ); + } + environment.extend(user_env); environment.insert( openshell_core::sandbox_env::TELEMETRY_ENABLED.to_string(), openshell_core::telemetry::enabled_env_value().to_string(), diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index e9d8921b6..90ba4fdb7 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -424,6 +424,13 @@ pub async fn run_sandbox( ); let provider_env = provider_credentials.snapshot().child_env.clone(); + let user_environment: std::collections::HashMap = std::env::var( + openshell_core::sandbox_env::USER_ENVIRONMENT, + ) + .ok() + .and_then(|json| serde_json::from_str(&json).ok()) + .unwrap_or_default(); + // Create identity cache for SHA256 TOFU when OPA is active let identity_cache = opa_engine .as_ref() @@ -806,6 +813,7 @@ pub async fn run_sandbox( let netns_fd = ssh_netns_fd; let ca_paths = ca_file_paths.clone(); let provider_credentials_clone = provider_credentials.clone(); + let user_env_clone = user_environment.clone(); let (ssh_ready_tx, ssh_ready_rx) = tokio::sync::oneshot::channel(); @@ -819,6 +827,7 @@ pub async fn run_sandbox( proxy_url, ca_paths, provider_credentials_clone, + user_env_clone, ) .await { diff --git a/crates/openshell-sandbox/src/ssh.rs b/crates/openshell-sandbox/src/ssh.rs index 67fbc7e57..3eddc4b6d 100644 --- a/crates/openshell-sandbox/src/ssh.rs +++ b/crates/openshell-sandbox/src/ssh.rs @@ -107,6 +107,7 @@ pub async fn run_ssh_server( proxy_url: Option, ca_file_paths: Option<(PathBuf, PathBuf)>, provider_credentials: ProviderCredentialState, + user_environment: HashMap, ) -> Result<()> { let (listener, config, ca_paths) = match ssh_server_init(&listen_path, &ca_file_paths) { Ok(v) => { @@ -131,6 +132,7 @@ pub async fn run_ssh_server( let proxy_url = proxy_url.clone(); let ca_paths = ca_paths.clone(); let provider_credentials = provider_credentials.clone(); + let user_environment = user_environment.clone(); tokio::spawn(async move { if let Err(err) = handle_connection( @@ -142,6 +144,7 @@ pub async fn run_ssh_server( proxy_url, ca_paths, provider_credentials, + user_environment, ) .await { @@ -168,6 +171,7 @@ async fn handle_connection( proxy_url: Option, ca_file_paths: Option>, provider_credentials: ProviderCredentialState, + user_environment: HashMap, ) -> Result<()> { // Access is gated by the Unix-socket filesystem permissions (root-only), // not by an application-level preface. The supervisor bridges the @@ -190,6 +194,7 @@ async fn handle_connection( proxy_url, ca_file_paths, provider_credentials, + user_environment, ); russh::server::run_stream(config, stream, handler) .await @@ -217,10 +222,12 @@ struct SshHandler { proxy_url: Option, ca_file_paths: Option>, provider_credentials: ProviderCredentialState, + user_environment: HashMap, channels: HashMap, } impl SshHandler { + #[allow(clippy::too_many_arguments)] fn new( policy: SandboxPolicy, workdir: Option, @@ -228,6 +235,7 @@ impl SshHandler { proxy_url: Option, ca_file_paths: Option>, provider_credentials: ProviderCredentialState, + user_environment: HashMap, ) -> Self { Self { policy, @@ -236,6 +244,7 @@ impl SshHandler { proxy_url, ca_file_paths, provider_credentials, + user_environment, channels: HashMap::new(), } } @@ -458,6 +467,7 @@ impl russh::server::Handler for SshHandler { self.proxy_url.clone(), self.ca_file_paths.clone(), &self.provider_credentials.snapshot().child_env, + &self.user_environment, )?; let state = self.channels.get_mut(&channel).ok_or_else(|| { anyhow::anyhow!("subsystem_request on unknown channel {channel:?}") @@ -553,6 +563,7 @@ impl SshHandler { self.proxy_url.clone(), self.ca_file_paths.clone(), &provider_snapshot.child_env, + &self.user_environment, )?; state.pty_master = Some(pty_master); state.input_sender = Some(input_sender); @@ -570,6 +581,7 @@ impl SshHandler { self.proxy_url.clone(), self.ca_file_paths.clone(), &provider_snapshot.child_env, + &self.user_environment, )?; state.input_sender = Some(input_sender); } @@ -668,6 +680,7 @@ fn session_user_and_home(policy: &SandboxPolicy) -> (String, String) { } } +#[allow(clippy::too_many_arguments)] fn apply_child_env( cmd: &mut Command, session_home: &str, @@ -676,6 +689,7 @@ fn apply_child_env( proxy_url: Option<&str>, ca_file_paths: Option<&(PathBuf, PathBuf)>, provider_env: &HashMap, + user_environment: &HashMap, ) { let path = std::env::var("PATH").unwrap_or_else(|_| "/usr/local/bin:/usr/bin:/bin".into()); @@ -687,6 +701,10 @@ fn apply_child_env( .env("PATH", &path) .env("TERM", term); + for (key, value) in user_environment { + cmd.env(key, value); + } + if let Some(url) = proxy_url { for (key, value) in child_env::proxy_env_vars(url) { cmd.env(key, value); @@ -716,6 +734,7 @@ fn spawn_pty_shell( proxy_url: Option, ca_file_paths: Option>, provider_env: &HashMap, + user_environment: &HashMap, ) -> anyhow::Result<(std::fs::File, mpsc::Sender>)> { let winsize = Winsize { ws_row: to_u16(pty.row_height.max(1)), @@ -764,6 +783,7 @@ fn spawn_pty_shell( proxy_url.as_deref(), ca_file_paths.as_deref(), provider_env, + user_environment, ); cmd.stdin(stdin).stdout(stdout).stderr(stderr); @@ -879,6 +899,7 @@ fn spawn_pipe_exec( proxy_url: Option, ca_file_paths: Option>, provider_env: &HashMap, + user_environment: &HashMap, ) -> anyhow::Result>> { let mut cmd = command.map_or_else( || { @@ -909,6 +930,7 @@ fn spawn_pipe_exec( proxy_url.as_deref(), ca_file_paths.as_deref(), provider_env, + user_environment, ); cmd.stdin(Stdio::piped()) .stdout(Stdio::piped()) From 3b9db93b74b642887f6374d32da3e3b402ca04ab Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Thu, 4 Jun 2026 14:46:42 -0400 Subject: [PATCH 2/5] fix(sandbox): address PR review feedback for env var injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - VM driver: restructure build_guest_environment to apply user env before required driver vars, preventing user-specified values from overriding OPENSHELL_ENDPOINT, PATH, and other critical vars. Matches the pattern already used by Docker/Podman/Kubernetes. - All drivers: insert OPENSHELL_USER_ENVIRONMENT sidecar after environment.extend(user_env) so a user-supplied key of the same name cannot clobber the JSON payload the supervisor parses. - Podman driver: fix template/spec env precedence — template env is now applied first with spec overwriting, matching the other drivers (spec is user-specified and should win over image defaults). - CLI: add parse_env_pairs() that rejects OPENSHELL_* prefixed keys with a clear error message. Both sandbox create and exec call sites updated to use it. - SSH handler: filter out OPENSHELL_* keys from user_environment before injecting into child processes. - Docs: add "Set Environment Variables" section to manage-sandboxes.mdx documenting --env for both create and exec, per-command override semantics, and the OPENSHELL_* reservation. - Tests: add sandbox_create_env_rejects_reserved_prefix test. - Fix rustfmt formatting in lib.rs, container.rs, driver.rs. --- crates/openshell-cli/src/main.rs | 2 +- crates/openshell-cli/src/run.rs | 12 +++ .../sandbox_create_lifecycle_integration.rs | 11 +++ crates/openshell-driver-docker/src/lib.rs | 2 +- .../openshell-driver-kubernetes/src/driver.rs | 6 +- .../openshell-driver-podman/src/container.rs | 18 ++-- crates/openshell-driver-vm/src/driver.rs | 98 +++++++++---------- crates/openshell-sandbox/src/lib.rs | 11 +-- crates/openshell-sandbox/src/ssh.rs | 4 +- docs/sandboxes/manage-sandboxes.mdx | 21 ++++ 10 files changed, 116 insertions(+), 69 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index d592bc262..ac2b50392 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -2776,7 +2776,7 @@ async fn main() -> Result<()> { } else { None // auto-detect }; - let env_map = run::parse_key_value_pairs(&envs, "--env")?; + let env_map = run::parse_env_pairs(&envs)?; let exit_code = run::sandbox_exec_grpc( endpoint, &name, diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 7361c1acc..3b1e309a4 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -3919,6 +3919,18 @@ pub fn parse_key_value_pairs(items: &[String], flag: &str) -> Result Result> { + let map = parse_key_value_pairs(items, "--env")?; + for key in map.keys() { + if key.starts_with("OPENSHELL_") { + return Err(miette::miette!( + "--env keys starting with OPENSHELL_ are reserved; got '{key}'" + )); + } + } + Ok(map) +} + fn parse_credential_pairs(items: &[String]) -> Result> { let mut map = HashMap::new(); diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index c2d52f291..116ea45b8 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -1412,3 +1412,14 @@ async fn sandbox_create_env_rejects_invalid_format() { "error should mention the flag and bad value, got: {msg}" ); } + +#[tokio::test] +async fn sandbox_create_env_rejects_reserved_prefix() { + let err = run::parse_env_pairs(&["VALID=ok".to_string(), "OPENSHELL_SECRET=bad".to_string()]) + .unwrap_err(); + let msg = format!("{err}"); + assert!( + msg.contains("OPENSHELL_") && msg.contains("reserved"), + "error should mention reserved prefix, got: {msg}" + ); +} diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 2d69a1d88..11ffab7d6 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -1648,6 +1648,7 @@ fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig user_env.extend(template.environment.clone()); } user_env.extend(spec.environment.clone()); + environment.extend(user_env.clone()); if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) { @@ -1656,7 +1657,6 @@ fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig json, ); } - environment.extend(user_env); } environment.insert( diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index 8fb079f5c..583f3cb99 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -1630,7 +1630,11 @@ fn build_env_list( if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) { - upsert_env(&mut env, openshell_core::sandbox_env::USER_ENVIRONMENT, &json); + upsert_env( + &mut env, + openshell_core::sandbox_env::USER_ENVIRONMENT, + &json, + ); } apply_required_env( &mut env, diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 44f53a9ee..cdf074619 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -244,7 +244,13 @@ fn build_env( let mut env: BTreeMap = BTreeMap::new(); // 1. User-supplied environment (lowest priority). + // Template vars first, then spec overwrites (spec is user-specified). let mut user_env: BTreeMap = BTreeMap::new(); + if let Some(t) = template { + for (k, v) in &t.environment { + user_env.insert(k.clone(), v.clone()); + } + } if let Some(s) = spec { if !s.log_level.is_empty() { env.insert( @@ -256,20 +262,12 @@ fn build_env( user_env.insert(k.clone(), v.clone()); } } - if let Some(t) = template { - for (k, v) in &t.environment { - user_env.insert(k.clone(), v.clone()); - } - } + env.extend(user_env.clone()); if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) { - env.insert( - openshell_core::sandbox_env::USER_ENVIRONMENT.into(), - json, - ); + env.insert(openshell_core::sandbox_env::USER_ENVIRONMENT.into(), json); } - env.extend(user_env); // 2. Required driver vars (highest priority -- always overwrite). env.insert( diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index bd124abfb..5338c576e 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -3450,55 +3450,10 @@ fn build_guest_environment( || guest_visible_openshell_endpoint(&config.openshell_endpoint), String::from, ); - let mut environment = HashMap::from([ - ("HOME".to_string(), "/root".to_string()), - ( - "PATH".to_string(), - "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin".to_string(), - ), - ("TERM".to_string(), "xterm".to_string()), - ( - openshell_core::sandbox_env::ENDPOINT.to_string(), - openshell_endpoint, - ), - ( - openshell_core::sandbox_env::SANDBOX_ID.to_string(), - sandbox.id.clone(), - ), - ( - openshell_core::sandbox_env::SANDBOX.to_string(), - sandbox.name.clone(), - ), - ( - openshell_core::sandbox_env::SSH_SOCKET_PATH.to_string(), - GUEST_SSH_SOCKET_PATH.to_string(), - ), - ( - openshell_core::sandbox_env::SANDBOX_COMMAND.to_string(), - "tail -f /dev/null".to_string(), - ), - ( - openshell_core::sandbox_env::LOG_LEVEL.to_string(), - openshell_core::driver_utils::sandbox_log_level(sandbox, &config.log_level), - ), - ]); - if config.requires_tls_materials() { - environment.extend(HashMap::from([ - ( - openshell_core::sandbox_env::TLS_CA.to_string(), - GUEST_TLS_CA_PATH.to_string(), - ), - ( - openshell_core::sandbox_env::TLS_CERT.to_string(), - GUEST_TLS_CERT_PATH.to_string(), - ), - ( - openshell_core::sandbox_env::TLS_KEY.to_string(), - GUEST_TLS_KEY_PATH.to_string(), - ), - ])); - } + // 1. User-supplied environment (lowest priority). let user_env = merged_environment(sandbox); + let mut environment: HashMap = HashMap::new(); + environment.extend(user_env.clone()); if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) { @@ -3507,7 +3462,52 @@ fn build_guest_environment( json, ); } - environment.extend(user_env); + + // 2. Required driver vars (highest priority -- always overwrite). + environment.insert("HOME".to_string(), "/root".to_string()); + environment.insert( + "PATH".to_string(), + "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin".to_string(), + ); + environment.insert("TERM".to_string(), "xterm".to_string()); + environment.insert( + openshell_core::sandbox_env::ENDPOINT.to_string(), + openshell_endpoint, + ); + environment.insert( + openshell_core::sandbox_env::SANDBOX_ID.to_string(), + sandbox.id.clone(), + ); + environment.insert( + openshell_core::sandbox_env::SANDBOX.to_string(), + sandbox.name.clone(), + ); + environment.insert( + openshell_core::sandbox_env::SSH_SOCKET_PATH.to_string(), + GUEST_SSH_SOCKET_PATH.to_string(), + ); + environment.insert( + openshell_core::sandbox_env::SANDBOX_COMMAND.to_string(), + "tail -f /dev/null".to_string(), + ); + environment.insert( + openshell_core::sandbox_env::LOG_LEVEL.to_string(), + openshell_core::driver_utils::sandbox_log_level(sandbox, &config.log_level), + ); + if config.requires_tls_materials() { + environment.insert( + openshell_core::sandbox_env::TLS_CA.to_string(), + GUEST_TLS_CA_PATH.to_string(), + ); + environment.insert( + openshell_core::sandbox_env::TLS_CERT.to_string(), + GUEST_TLS_CERT_PATH.to_string(), + ); + environment.insert( + openshell_core::sandbox_env::TLS_KEY.to_string(), + GUEST_TLS_KEY_PATH.to_string(), + ); + } environment.insert( openshell_core::sandbox_env::TELEMETRY_ENABLED.to_string(), openshell_core::telemetry::enabled_env_value().to_string(), diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 90ba4fdb7..231b588ea 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -424,12 +424,11 @@ pub async fn run_sandbox( ); let provider_env = provider_credentials.snapshot().child_env.clone(); - let user_environment: std::collections::HashMap = std::env::var( - openshell_core::sandbox_env::USER_ENVIRONMENT, - ) - .ok() - .and_then(|json| serde_json::from_str(&json).ok()) - .unwrap_or_default(); + let user_environment: std::collections::HashMap = + std::env::var(openshell_core::sandbox_env::USER_ENVIRONMENT) + .ok() + .and_then(|json| serde_json::from_str(&json).ok()) + .unwrap_or_default(); // Create identity cache for SHA256 TOFU when OPA is active let identity_cache = opa_engine diff --git a/crates/openshell-sandbox/src/ssh.rs b/crates/openshell-sandbox/src/ssh.rs index 3eddc4b6d..9db2bf97d 100644 --- a/crates/openshell-sandbox/src/ssh.rs +++ b/crates/openshell-sandbox/src/ssh.rs @@ -702,7 +702,9 @@ fn apply_child_env( .env("TERM", term); for (key, value) in user_environment { - cmd.env(key, value); + if !key.starts_with("OPENSHELL_") { + cmd.env(key, value); + } } if let Some(url) = proxy_url { diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index 0db6d7678..ce2b923aa 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/docs/sandboxes/manage-sandboxes.mdx @@ -153,6 +153,27 @@ OpenShell allocates a TTY automatically when both stdin and stdout are terminals | `--timeout` | Command timeout in seconds. `0` disables the timeout. | | `--tty` | Force TTY allocation. | | `--no-tty` | Disable TTY allocation even when attached to a terminal. | +| `--env` | Set an environment variable for the command (`KEY=VALUE`, repeatable). | + +## Set Environment Variables + +Inject environment variables into the sandbox at creation time: + +```shell +openshell sandbox create --env API_KEY=sk-test --env DEBUG=1 -- my-agent +``` + +Variables set with `--env` are available to all processes in the sandbox, including interactive shells and exec commands. + +You can also set per-command environment variables with `sandbox exec`: + +```shell +openshell sandbox exec -n my-sandbox --env MY_VAR=hello -- printenv MY_VAR +``` + +Per-command variables override the sandbox-level environment for that command only. + +Environment variable names starting with `OPENSHELL_` are reserved and rejected by the CLI. ## Label a Sandbox From b5a9c125af07d6cb6708c2ff8b43315fed6b985c Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Fri, 5 Jun 2026 08:29:01 -0400 Subject: [PATCH 3/5] fix(server): reject OPENSHELL_* env keys at API boundary Add server-side validation to reject environment variable keys starting with OPENSHELL_ in SandboxSpec.environment, SandboxTemplate.environment, and ExecSandboxRequest.environment. This closes the gap where SDK/gRPC callers (bypassing the CLI) could inject reserved keys that affect supervisor-internal state. The CLI already rejects these keys via parse_env_pairs, but the API boundary is the authoritative enforcement point since all clients (CLI, SDK, direct gRPC) pass through it. --- .../sandbox_create_lifecycle_integration.rs | 2 +- .../openshell-server/src/grpc/validation.rs | 70 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 116ea45b8..bd495c4ea 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -1364,7 +1364,7 @@ async fn sandbox_create_sends_environment_variables() { Some("env-test"), None, "openshell", - None, + &[], true, false, None, diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 268c143d2..5d6c46d26 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -56,6 +56,7 @@ pub(super) fn validate_exec_request_fields(req: &ExecSandboxRequest) -> Result<( } reject_control_chars(value, &format!("environment value for '{key}'"))?; } + reject_reserved_env_keys(&req.environment, "environment")?; if !req.workdir.is_empty() { if req.workdir.len() > MAX_EXEC_WORKDIR_LEN { return Err(Status::invalid_argument(format!( @@ -125,10 +126,12 @@ pub(super) fn validate_sandbox_spec( MAX_MAP_VALUE_LEN, "spec.environment", )?; + reject_reserved_env_keys(&spec.environment, "spec.environment")?; // --- spec.template --- if let Some(ref tmpl) = spec.template { validate_sandbox_template(tmpl)?; + reject_reserved_env_keys(&tmpl.environment, "spec.template.environment")?; } // --- spec.policy serialized size --- @@ -243,6 +246,20 @@ pub(super) fn validate_string_map( Ok(()) } +fn reject_reserved_env_keys( + map: &std::collections::HashMap, + field_name: &str, +) -> Result<(), Status> { + for key in map.keys() { + if key.starts_with("OPENSHELL_") { + return Err(Status::invalid_argument(format!( + "{field_name} keys starting with OPENSHELL_ are reserved; got '{key}'" + ))); + } + } + Ok(()) +} + // --------------------------------------------------------------------------- // Provider field validation // --------------------------------------------------------------------------- @@ -894,6 +911,59 @@ mod tests { assert!(validate_sandbox_spec("my-sandbox", &spec).is_ok()); } + #[test] + fn validate_sandbox_spec_rejects_reserved_env_key() { + let spec = SandboxSpec { + environment: std::iter::once(("OPENSHELL_SECRET".to_string(), "val".to_string())) + .collect(), + ..Default::default() + }; + let err = validate_sandbox_spec("s", &spec).unwrap_err(); + assert!( + err.message().contains("OPENSHELL_") && err.message().contains("reserved"), + "expected reserved key error, got: {}", + err.message() + ); + } + + #[test] + fn validate_sandbox_spec_rejects_reserved_template_env_key() { + let spec = SandboxSpec { + template: Some(SandboxTemplate { + environment: std::iter::once(( + "OPENSHELL_ENDPOINT".to_string(), + "evil".to_string(), + )) + .collect(), + ..Default::default() + }), + ..Default::default() + }; + let err = validate_sandbox_spec("s", &spec).unwrap_err(); + assert!( + err.message().contains("OPENSHELL_") && err.message().contains("reserved"), + "expected reserved key error, got: {}", + err.message() + ); + } + + #[test] + fn validate_exec_request_rejects_reserved_env_key() { + let req = ExecSandboxRequest { + sandbox_id: "id".to_string(), + command: vec!["echo".to_string()], + environment: std::iter::once(("OPENSHELL_SANDBOX_ID".to_string(), "evil".to_string())) + .collect(), + ..Default::default() + }; + let err = validate_exec_request_fields(&req).unwrap_err(); + assert!( + err.message().contains("OPENSHELL_") && err.message().contains("reserved"), + "expected reserved key error, got: {}", + err.message() + ); + } + // ---- Provider field validation ---- fn one_credential() -> HashMap { From 2f226e86594b2acd30a904c35ed4389c3b907a2e Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Mon, 8 Jun 2026 11:38:32 -0400 Subject: [PATCH 4/5] fix(server): validate env key names and control chars at API boundary Add POSIX env key format validation (^[A-Za-z_][A-Za-z0-9_]*$) and control character rejection for SandboxSpec.environment, SandboxTemplate.environment, and ExecSandboxRequest.environment at the gRPC validation layer. This catches invalid keys like "1BAD" or "BAD-NAME" and values containing newlines at the API boundary rather than letting them surface as driver-specific container/pod failures. Also add matching CLI-side validation in parse_env_pairs so users get immediate feedback. Update docs to say "OpenShell rejects" rather than "the CLI rejects" since enforcement is now at both boundaries. --- crates/openshell-cli/src/run.rs | 16 +++++ .../sandbox_create_lifecycle_integration.rs | 19 ++++++ .../openshell-server/src/grpc/validation.rs | 61 +++++++++++++++++-- docs/sandboxes/manage-sandboxes.mdx | 2 +- 4 files changed, 92 insertions(+), 6 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 3b1e309a4..edbc9630c 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -3922,6 +3922,11 @@ pub fn parse_key_value_pairs(items: &[String], flag: &str) -> Result Result> { let map = parse_key_value_pairs(items, "--env")?; for key in map.keys() { + if !is_valid_env_name(key) { + return Err(miette::miette!( + "--env key must match [A-Za-z_][A-Za-z0-9_]*; got '{key}'" + )); + } if key.starts_with("OPENSHELL_") { return Err(miette::miette!( "--env keys starting with OPENSHELL_ are reserved; got '{key}'" @@ -3931,6 +3936,17 @@ pub fn parse_env_pairs(items: &[String]) -> Result> { Ok(map) } +fn is_valid_env_name(key: &str) -> bool { + let mut bytes = key.bytes(); + let Some(first) = bytes.next() else { + return false; + }; + if !(first == b'_' || first.is_ascii_alphabetic()) { + return false; + } + bytes.all(|b| b == b'_' || b.is_ascii_alphanumeric()) +} + fn parse_credential_pairs(items: &[String]) -> Result> { let mut map = HashMap::new(); diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index bd495c4ea..be71c0a36 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -917,6 +917,7 @@ async fn sandbox_create_sends_driver_config_json() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1371,6 +1372,7 @@ async fn sandbox_create_sends_environment_variables() { None, None, None, + None, &[], None, None, @@ -1423,3 +1425,20 @@ async fn sandbox_create_env_rejects_reserved_prefix() { "error should mention reserved prefix, got: {msg}" ); } + +#[tokio::test] +async fn sandbox_create_env_rejects_invalid_key_name() { + let err = run::parse_env_pairs(&["1BAD=value".to_string()]).unwrap_err(); + let msg = format!("{err}"); + assert!( + msg.contains("1BAD"), + "error should mention invalid key, got: {msg}" + ); + + let err = run::parse_env_pairs(&["BAD-NAME=value".to_string()]).unwrap_err(); + let msg = format!("{err}"); + assert!( + msg.contains("BAD-NAME"), + "error should mention invalid key, got: {msg}" + ); +} diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 5d6c46d26..eff4f00ad 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -56,7 +56,7 @@ pub(super) fn validate_exec_request_fields(req: &ExecSandboxRequest) -> Result<( } reject_control_chars(value, &format!("environment value for '{key}'"))?; } - reject_reserved_env_keys(&req.environment, "environment")?; + validate_env_entries(&req.environment, "environment")?; if !req.workdir.is_empty() { if req.workdir.len() > MAX_EXEC_WORKDIR_LEN { return Err(Status::invalid_argument(format!( @@ -126,12 +126,12 @@ pub(super) fn validate_sandbox_spec( MAX_MAP_VALUE_LEN, "spec.environment", )?; - reject_reserved_env_keys(&spec.environment, "spec.environment")?; + validate_env_entries(&spec.environment, "spec.environment")?; // --- spec.template --- if let Some(ref tmpl) = spec.template { validate_sandbox_template(tmpl)?; - reject_reserved_env_keys(&tmpl.environment, "spec.template.environment")?; + validate_env_entries(&tmpl.environment, "spec.template.environment")?; } // --- spec.policy serialized size --- @@ -246,16 +246,22 @@ pub(super) fn validate_string_map( Ok(()) } -fn reject_reserved_env_keys( +fn validate_env_entries( map: &std::collections::HashMap, field_name: &str, ) -> Result<(), Status> { - for key in map.keys() { + for (key, value) in map { + if !super::provider::is_valid_env_key(key) { + return Err(Status::invalid_argument(format!( + "{field_name} keys must match ^[A-Za-z_][A-Za-z0-9_]*$; got '{key}'" + ))); + } if key.starts_with("OPENSHELL_") { return Err(Status::invalid_argument(format!( "{field_name} keys starting with OPENSHELL_ are reserved; got '{key}'" ))); } + reject_control_chars(value, &format!("{field_name} value for '{key}'"))?; } Ok(()) } @@ -964,6 +970,51 @@ mod tests { ); } + #[test] + fn validate_sandbox_spec_rejects_invalid_env_key_name() { + let spec = SandboxSpec { + environment: std::iter::once(("1BAD".to_string(), "val".to_string())).collect(), + ..Default::default() + }; + let err = validate_sandbox_spec("s", &spec).unwrap_err(); + assert!( + err.message().contains("1BAD"), + "expected invalid key error, got: {}", + err.message() + ); + } + + #[test] + fn validate_sandbox_spec_rejects_env_value_with_control_chars() { + let spec = SandboxSpec { + environment: std::iter::once(("KEY".to_string(), "val\nue".to_string())).collect(), + ..Default::default() + }; + let err = validate_sandbox_spec("s", &spec).unwrap_err(); + assert!( + err.message().contains("newline"), + "expected control char error, got: {}", + err.message() + ); + } + + #[test] + fn validate_sandbox_spec_rejects_invalid_template_env_key_name() { + let spec = SandboxSpec { + template: Some(SandboxTemplate { + environment: std::iter::once(("BAD-NAME".to_string(), "val".to_string())).collect(), + ..Default::default() + }), + ..Default::default() + }; + let err = validate_sandbox_spec("s", &spec).unwrap_err(); + assert!( + err.message().contains("BAD-NAME"), + "expected invalid key error, got: {}", + err.message() + ); + } + // ---- Provider field validation ---- fn one_credential() -> HashMap { diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index ce2b923aa..469e04361 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/docs/sandboxes/manage-sandboxes.mdx @@ -173,7 +173,7 @@ openshell sandbox exec -n my-sandbox --env MY_VAR=hello -- printenv MY_VAR Per-command variables override the sandbox-level environment for that command only. -Environment variable names starting with `OPENSHELL_` are reserved and rejected by the CLI. +Environment variable names starting with `OPENSHELL_` are reserved. Keys must match `[A-Za-z_][A-Za-z0-9_]*`. ## Label a Sandbox From 69c18c796130cdf5d56548f59a3cd4cc019a3faa Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Mon, 8 Jun 2026 14:39:11 -0400 Subject: [PATCH 5/5] fix(server): allow exec helper keys and add env size limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split exec env validation so OPENSHELL_PYFUNC_B64 (used by the Python SDK's exec_python()) passes through while runtime-clobberable keys like OPENSHELL_SANDBOX_ID are still rejected. Create-time validation remains strict — all OPENSHELL_* keys are blocked. Add a 256 KiB total serialized-size limit on create-time environment maps. The drivers serialize the full map into a single OPENSHELL_USER_ENVIRONMENT env var; capping the input at the API boundary prevents driver/runtime-specific startup failures from oversized env blocks. Add template environment control-character test and exec helper key acceptance test. --- .../openshell-server/src/grpc/validation.rs | 64 ++++++++++++++++++- 1 file changed, 62 insertions(+), 2 deletions(-) diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index eff4f00ad..03a69d6e9 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -56,7 +56,7 @@ pub(super) fn validate_exec_request_fields(req: &ExecSandboxRequest) -> Result<( } reject_control_chars(value, &format!("environment value for '{key}'"))?; } - validate_env_entries(&req.environment, "environment")?; + validate_exec_env_entries(&req.environment, "environment")?; if !req.workdir.is_empty() { if req.workdir.len() > MAX_EXEC_WORKDIR_LEN { return Err(Status::invalid_argument(format!( @@ -246,9 +246,40 @@ pub(super) fn validate_string_map( Ok(()) } +/// OPENSHELL_* keys that are allowed in exec environment. The Python SDK's +/// `exec_python()` sends a serialized callable via this key. +const EXEC_ALLOWED_OPENSHELL_KEYS: &[&str] = &["OPENSHELL_PYFUNC_B64"]; + +/// Maximum total serialized size of user environment (bytes). The drivers +/// serialize the full map as JSON into a single `OPENSHELL_USER_ENVIRONMENT` +/// env var; capping the input prevents driver/runtime-specific startup +/// failures from oversized env blocks. +const MAX_ENV_SERIALIZED_SIZE: usize = 256 * 1024; // 256 KiB + fn validate_env_entries( map: &std::collections::HashMap, field_name: &str, +) -> Result<(), Status> { + let total_size: usize = map.iter().map(|(k, v)| k.len() + v.len()).sum(); + if total_size > MAX_ENV_SERIALIZED_SIZE { + return Err(Status::invalid_argument(format!( + "{field_name} total size exceeds {MAX_ENV_SERIALIZED_SIZE} byte limit ({total_size} bytes)" + ))); + } + validate_env_entries_inner(map, field_name, &[]) +} + +fn validate_exec_env_entries( + map: &std::collections::HashMap, + field_name: &str, +) -> Result<(), Status> { + validate_env_entries_inner(map, field_name, EXEC_ALLOWED_OPENSHELL_KEYS) +} + +fn validate_env_entries_inner( + map: &std::collections::HashMap, + field_name: &str, + allowed_openshell_keys: &[&str], ) -> Result<(), Status> { for (key, value) in map { if !super::provider::is_valid_env_key(key) { @@ -256,7 +287,7 @@ fn validate_env_entries( "{field_name} keys must match ^[A-Za-z_][A-Za-z0-9_]*$; got '{key}'" ))); } - if key.starts_with("OPENSHELL_") { + if key.starts_with("OPENSHELL_") && !allowed_openshell_keys.contains(&key.as_str()) { return Err(Status::invalid_argument(format!( "{field_name} keys starting with OPENSHELL_ are reserved; got '{key}'" ))); @@ -970,6 +1001,35 @@ mod tests { ); } + #[test] + fn validate_exec_request_allows_pyfunc_helper_key() { + let req = ExecSandboxRequest { + sandbox_id: "id".to_string(), + command: vec!["python".to_string()], + environment: std::iter::once(("OPENSHELL_PYFUNC_B64".to_string(), "data".to_string())) + .collect(), + ..Default::default() + }; + assert!(validate_exec_request_fields(&req).is_ok()); + } + + #[test] + fn validate_sandbox_spec_rejects_template_env_value_with_control_chars() { + let spec = SandboxSpec { + template: Some(SandboxTemplate { + environment: std::iter::once(("KEY".to_string(), "val\nue".to_string())).collect(), + ..Default::default() + }), + ..Default::default() + }; + let err = validate_sandbox_spec("s", &spec).unwrap_err(); + assert!( + err.message().contains("newline"), + "expected control char error, got: {}", + err.message() + ); + } + #[test] fn validate_sandbox_spec_rejects_invalid_env_key_name() { let spec = SandboxSpec {