From b63a296775ec51368352c251c182edf0ab71c0fd Mon Sep 17 00:00:00 2001 From: jif Date: Fri, 25 Sep 2026 13:58:21 +0000 Subject: [PATCH] Honor shell environment policy in legacy snapshots (#48099) ## Why Legacy shell snapshots could retain exports excluded by `shell_environment_policy` and restore captured values over explicit overrides when replayed. ## What changed - Build snapshots with the resolved environment's policy and omit filtered exports and original values for variables overridden by `set`. - Skip eager snapshot creation when configuration is unavailable, and reject snapshots whose policy differs from the current turn's policy. ## Testing Add a regression test covering default and custom exclusions, `include_only`, preservation of multiline values, and `set` overrides that survive snapshot replay. Verify excluded and overridden captured values are absent from the snapshot file. GitOrigin-RevId: 4b785130f6c5a4d268ab86fd36ab9d77d8c4a3b9 --- codex-rs/core/src/environment_selection.rs | 13 ++- codex-rs/core/src/session/turn_context.rs | 4 +- codex-rs/core/src/shell_snapshot.rs | 26 ++++- codex-rs/core/src/shell_snapshot_tests.rs | 25 ++++- codex-rs/core/tests/suite/shell_snapshot.rs | 104 +++++++++++++++++- .../src/shell_snapshot_render.rs | 20 +++- .../shell-command/src/shell_snapshot_tests.rs | 2 +- 7 files changed, 180 insertions(+), 14 deletions(-) diff --git a/codex-rs/core/src/environment_selection.rs b/codex-rs/core/src/environment_selection.rs index ea3d31931b..34fc4a721e 100644 --- a/codex-rs/core/src/environment_selection.rs +++ b/codex-rs/core/src/environment_selection.rs @@ -17,7 +17,6 @@ use codex_exec_server::ExecutorFileSystem; use codex_exec_server::SelectedCapabilityRootsStatus; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; -use codex_protocol::config_types::ShellEnvironmentPolicy; use codex_protocol::error::CodexErr; use codex_protocol::error::Result as CodexResult; use codex_protocol::models::PermissionProfile; @@ -344,18 +343,22 @@ impl ThreadEnvironments { environment: Arc, cwd: PathUri, shell: Option, + config: &EnvironmentConfigState, ) -> ShellSnapshotTask { // Protected snapshots require the command's sandbox and are captured lazily. if shell_snapshot.should_rebuild_inherited() { return futures::future::ready(None).boxed().shared(); } + let EnvironmentConfigState::Ready(config) = config else { + return futures::future::ready(None).boxed().shared(); + }; let shell_snapshot = shell_snapshot .build( environment, cwd, shell, /*allow_login_shell*/ true, - ShellEnvironmentPolicy::default(), + config.shell_environment_policy.clone(), /*sandbox*/ None, ) .boxed() @@ -610,6 +613,7 @@ impl ThreadEnvironments { Arc::clone(&resolved.environment), selected.selection.cwd.clone(), resolved.shell.clone(), + &selected.selection.config, ); selected.resolution = futures::future::ready(Ok(ResolvedEnvironment { shell_snapshot, @@ -803,11 +807,16 @@ impl ThreadEnvironments { ) }; let shell_snapshot_builder = shell_snapshot.clone(); + let snapshot_config = installed_config + .clone() + .map(EnvironmentConfigState::Ready) + .unwrap_or(selection.config); let task = Self::start_shell_snapshot_task( shell_snapshot, Arc::clone(&environment), selection.cwd, shell.clone(), + &snapshot_config, ); let shell_snapshot_v2_supported = snapshot_v2 && (environment.is_remote() || !shell_snapshot_builder.should_rebuild_inherited()); diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 6043ad94fe..65cf7e8ceb 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -196,7 +196,9 @@ impl TurnEnvironment { } None } else { - self.shell_snapshot.peek()?.clone() + self.shell_snapshot.peek()?.clone().filter(|snapshot| { + &snapshot.shell_environment_policy == self.shell_environment_policy() + }) } } diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index 880474c65a..596bf8de5a 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/codex-rs/core/src/shell_snapshot.rs @@ -72,6 +72,7 @@ pub(crate) enum SnapshotCredentialBrokerState { pub(crate) struct ShellSnapshotFile { path: AbsolutePathBuf, credentials: Option, + pub(crate) shell_environment_policy: ShellEnvironmentPolicy, } struct SnapshotCredentials { @@ -205,7 +206,7 @@ impl ShellSnapshot { SnapshotCredentialBrokerState::Ready(network_proxy) if sandbox.is_some() => { Some(SnapshotCredentialBroker { network_proxy, - shell_environment_policy, + shell_environment_policy: shell_environment_policy.clone(), allow_login_shell, }) } @@ -221,6 +222,7 @@ impl ShellSnapshot { &config.codex_home, config.session_id, &cwd, + &shell_environment_policy, &shell, config.state_db.clone(), credential_broker, @@ -242,10 +244,12 @@ impl ShellSnapshot { .await } + #[allow(clippy::too_many_arguments)] async fn try_create( codex_home: &AbsolutePathBuf, session_id: ThreadId, session_cwd: &AbsolutePathBuf, + shell_environment_policy: &ShellEnvironmentPolicy, shell: &Shell, state_db: Option, credential_broker: Option, @@ -283,6 +287,7 @@ impl ShellSnapshot { shell, &temp_path, session_cwd, + shell_environment_policy, credential_broker.as_ref(), sandbox, ) @@ -319,7 +324,11 @@ impl ShellSnapshot { return Err("write_failed"); } - Ok(ShellSnapshotFile { path, credentials }) + Ok(ShellSnapshotFile { + path, + credentials, + shell_environment_policy: shell_environment_policy.clone(), + }) } } @@ -531,6 +540,7 @@ async fn write_shell_snapshot( shell: &Shell, output_path: &AbsolutePathBuf, cwd: &AbsolutePathBuf, + shell_environment_policy: &ShellEnvironmentPolicy, credential_broker: Option<&SnapshotCredentialBroker>, sandbox: Option<&ShellSnapshotSandbox>, ) -> Result> { @@ -538,7 +548,14 @@ async fn write_shell_snapshot( if shell_type == ShellType::PowerShell || shell_type == ShellType::Cmd { bail!("Shell snapshot not supported yet for {shell_type:?}"); } - let (snapshot, credentials) = capture_snapshot(shell, cwd, credential_broker, sandbox).await?; + let (snapshot, credentials) = capture_snapshot( + shell, + cwd, + shell_environment_policy, + credential_broker, + sandbox, + ) + .await?; if let Some(parent) = output_path.parent() { let parent_display = parent.display(); @@ -558,6 +575,7 @@ async fn write_shell_snapshot( async fn capture_snapshot( shell: &Shell, cwd: &AbsolutePathBuf, + shell_environment_policy: &ShellEnvironmentPolicy, credential_broker: Option<&SnapshotCredentialBroker>, sandbox: Option<&ShellSnapshotSandbox>, ) -> Result<(String, Option)> { @@ -597,7 +615,7 @@ async fn capture_snapshot( let capture = CapturedSnapshot::parse(shell_type, raw_snapshot.as_bytes()) .ok_or_else(|| anyhow!("invalid shell snapshot capture"))?; let Some(credential_broker) = credential_broker else { - return Ok((capture.render_script(), None)); + return Ok((capture.render_script(shell_environment_policy), None)); }; let original_env = std::str::from_utf8(capture.environment)? diff --git a/codex-rs/core/src/shell_snapshot_tests.rs b/codex-rs/core/src/shell_snapshot_tests.rs index 944a41f116..9c7bed4ee3 100644 --- a/codex-rs/core/src/shell_snapshot_tests.rs +++ b/codex-rs/core/src/shell_snapshot_tests.rs @@ -154,6 +154,7 @@ async fn get_snapshot(shell_type: ShellType) -> Result { &shell, &path.abs(), &dir.path().abs(), + &ShellEnvironmentPolicy::default(), /*credential_broker*/ None, /*sandbox*/ None, ) @@ -728,6 +729,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul &shell, &path, &dir.path().abs(), + &credential_broker.shell_environment_policy, Some(&credential_broker), /*sandbox*/ None, ) @@ -825,6 +827,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul &shell, &filtered_startup_path, &dir.path().abs(), + &filtered_startup_broker.shell_environment_policy, Some(&filtered_startup_broker), /*sandbox*/ None, ) @@ -847,6 +850,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul let (_, excluded_credentials) = capture_snapshot( &shell, &dir.path().abs(), + &excluded_credential_broker.shell_environment_policy, Some(&excluded_credential_broker), /*sandbox*/ None, ) @@ -916,7 +920,11 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul ); assert!(!inherited_snapshot.contains(inherited_secret)); - let snapshot_file = ShellSnapshotFile { path, credentials }; + let snapshot_file = ShellSnapshotFile { + path, + credentials, + shell_environment_policy: ShellEnvironmentPolicy::default(), + }; let unset_credential_keys = snapshot_file .credentials .as_ref() @@ -1212,6 +1220,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul &shell, &partially_filtered_path, &dir.path().abs(), + &partially_filtered_credential_broker.shell_environment_policy, Some(&partially_filtered_credential_broker), /*sandbox*/ None, ) @@ -1230,6 +1239,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul "Bearer ghp_filtered_dummy\nunset" ); let filtered_snapshot = ShellSnapshotFile { + shell_environment_policy: ShellEnvironmentPolicy::default(), path: partially_filtered_path, credentials: filtered_credentials, }; @@ -1270,6 +1280,7 @@ async fn snapshot_protects_posix_startup_only_when_it_contains_credentials() -> &posix_shell, &posix_snapshot_path, &dir.path().abs(), + &posix_credential_broker.shell_environment_policy, Some(&posix_credential_broker), /*sandbox*/ None, ) @@ -1293,6 +1304,7 @@ async fn snapshot_protects_posix_startup_only_when_it_contains_credentials() -> &posix_shell, &application_snapshot_path, &dir.path().abs(), + &posix_credential_broker.shell_environment_policy, Some(&posix_credential_broker), /*sandbox*/ None, ) @@ -1344,6 +1356,7 @@ async fn snapshot_discovers_and_restores_inherited_credential_aliases() -> Resul &shell, &inherited_path, &dir.path().abs(), + &inherited_credential_broker.shell_environment_policy, Some(&inherited_credential_broker), /*sandbox*/ None, ) @@ -1366,6 +1379,7 @@ async fn snapshot_discovers_and_restores_inherited_credential_aliases() -> Resul ); let snapshot_file = ShellSnapshotFile { + shell_environment_policy: ShellEnvironmentPolicy::default(), path: inherited_path, credentials: inherited_credentials, }; @@ -1434,6 +1448,7 @@ async fn snapshot_discovers_and_restores_inherited_credential_aliases() -> Resul let residual_credential = capture_snapshot( &shell, &dir.path().abs(), + &inherited_credential_broker.shell_environment_policy, Some(&inherited_credential_broker), /*sandbox*/ None, ) @@ -1450,6 +1465,7 @@ async fn snapshot_discovers_and_restores_inherited_credential_aliases() -> Resul let (unset_path_snapshot, _) = capture_snapshot( &shell, &dir.path().abs(), + &inherited_credential_broker.shell_environment_policy, Some(&inherited_credential_broker), /*sandbox*/ None, ) @@ -1471,12 +1487,14 @@ async fn snapshot_discovers_and_restores_inherited_credential_aliases() -> Resul let (snapshot, credentials) = capture_snapshot( &shell, &dir.path().abs(), + &inherited_credential_broker.shell_environment_policy, Some(&inherited_credential_broker), /*sandbox*/ None, ) .await?; assert!(!snapshot.contains(real)); let snapshot_file = ShellSnapshotFile { + shell_environment_policy: ShellEnvironmentPolicy::default(), path: dir.path().join("hostless-snapshot.sh").abs(), credentials, }; @@ -1527,12 +1545,14 @@ async fn snapshot_discovers_and_restores_inherited_credential_aliases() -> Resul let (snapshot, credentials) = capture_snapshot( &shell, &dir.path().abs(), + &broker.shell_environment_policy, Some(&broker), /*sandbox*/ None, ) .await?; assert!(!snapshot.contains(real)); let snapshot_file = ShellSnapshotFile { + shell_environment_policy: ShellEnvironmentPolicy::default(), path: dir.path().join("hostless-alias-snapshot.sh").abs(), credentials, }; @@ -1587,6 +1607,7 @@ async fn try_create_creates_and_deletes_snapshot_file() -> Result<()> { &dir.path().abs(), ThreadId::new(), &dir.path().abs(), + &ShellEnvironmentPolicy::default(), &shell, /*state_db*/ None, /*credential_broker*/ None, @@ -1618,6 +1639,7 @@ async fn try_create_uses_distinct_generation_paths() -> Result<()> { &dir.path().abs(), session_id, &dir.path().abs(), + &ShellEnvironmentPolicy::default(), &shell, /*state_db*/ None, /*credential_broker*/ None, @@ -1629,6 +1651,7 @@ async fn try_create_uses_distinct_generation_paths() -> Result<()> { &dir.path().abs(), session_id, &dir.path().abs(), + &ShellEnvironmentPolicy::default(), &shell, /*state_db*/ None, /*credential_broker*/ None, diff --git a/codex-rs/core/tests/suite/shell_snapshot.rs b/codex-rs/core/tests/suite/shell_snapshot.rs index acf1a2bd8c..f9795efb17 100644 --- a/codex-rs/core/tests/suite/shell_snapshot.rs +++ b/codex-rs/core/tests/suite/shell_snapshot.rs @@ -14,9 +14,13 @@ use codex_network_proxy::NetworkProxyConfig; #[cfg(unix)] use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::CollaborationMode; +#[cfg(unix)] +use codex_protocol::config_types::EnvironmentVariablePattern; use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Settings; #[cfg(unix)] +use codex_protocol::config_types::ShellEnvironmentPolicy; +#[cfg(unix)] use codex_protocol::config_types::TrustLevel; #[cfg(unix)] use codex_protocol::config_types::WindowsSandboxLevel; @@ -59,7 +63,7 @@ use core_test_support::wait_for_event_match; use pretty_assertions::assert_eq; use serde_json::json; use std::collections::HashMap; -#[cfg(target_os = "macos")] +#[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::path::Path; use std::path::PathBuf; @@ -752,6 +756,104 @@ async fn shell_snapshot_v2_guardian_uses_its_resolved_permissions_and_tools( Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn legacy_snapshot_omits_filtered_and_overridden_exports() -> Result<()> { + skip_if_remote!(Ok(()), "legacy snapshots require a local environment"); + let home = tempfile::tempdir()?; + let shell_path = home.path().join("bash"); + // Seed the capture shell without changing the test process environment. Replay uses -c. + fs::write( + &shell_path, + r#"#!/bin/sh +if [ "$1" = -lc ]; then + export HOME="${0%/*}" + export PROFILE_ALLOWED='first +second' + export PROFILE_TOKEN=token-sentinel + export PROFILE_DENIED=denied-sentinel + export OTHER=other-sentinel + export PROFILE_SECRET=original-secret-sentinel +fi +exec /bin/bash --noprofile --norc "$@" +"#, + ) + .await?; + fs::set_permissions(&shell_path, std::fs::Permissions::from_mode(/*mode*/ 0o755)).await?; + let builder = test_codex() + .with_user_shell( + codex_shell_command::shell_detect::DetectedShell { + shell_type: codex_shell_command::shell_detect::ShellType::Bash, + shell_path, + } + .into(), + ) + .with_config(|config| { + config.features.enable(Feature::ShellSnapshot).unwrap(); + config.features.enable(Feature::UnifiedExec).unwrap(); + config.features.disable(Feature::ShellSnapshotV2).unwrap(); + config + .permissions + .set_permission_profile(PermissionProfile::Disabled) + .unwrap(); + config.permissions.shell_environment_policy = ShellEnvironmentPolicy { + ignore_default_excludes: false, + exclude: vec![EnvironmentVariablePattern::new_case_insensitive( + "PROFILE_DENIED", + )], + include_only: ["PATH", "PROFILE_*"] + .map(EnvironmentVariablePattern::new_case_insensitive) + .to_vec(), + r#set: HashMap::from([("PROFILE_SECRET".to_string(), "dummy".to_string())]), + ..Default::default() + }; + }); + let harness = TestCodexHarness::with_auto_env_builder(builder).await?; + let snapshot_path = wait_for_snapshot(harness.test().home.path()).await?; + let snapshot = fs::read_to_string(&snapshot_path).await?; + for sentinel in [ + "token-sentinel", + "denied-sentinel", + "other-sentinel", + "original-secret-sentinel", + ] { + assert!(!snapshot.contains(sentinel)); + } + let snapshot_path = shlex::try_quote(snapshot_path.to_str().unwrap())?; + let args = json!({ + "cmd": format!("printf '%s|%s|%s|%s|%s' \"$PROFILE_ALLOWED\" \"${{PROFILE_TOKEN-missing}}\" \"${{PROFILE_DENIED-missing}}\" \"${{OTHER-missing}}\" \"$PROFILE_SECRET\"; . {snapshot_path}; printf '|%s' \"$PROFILE_SECRET\""), + "shell": "/bin/bash", + "yield_time_ms": 1_000, + }); + mount_sse_sequence( + harness.server(), + vec![ + sse(vec![ + ev_function_call("snapshot-policy", "exec_command", &args.to_string()), + ev_completed("tool"), + ]), + sse(vec![ev_completed("done")]), + ], + ) + .await; + // Keep the environment selection unchanged so the readiness wait refers to this turn's snapshot. + harness + .test() + .submit_text_turn("verify snapshot filtering") + .await?; + let output = harness.function_call_stdout("snapshot-policy").await; + assert_eq!( + normalize_newlines(&output) + .split_once("Output:\n") + .unwrap() + .1 + .trim(), + "first\nsecond|missing|missing|missing|dummy|dummy" + ); + harness.test().codex.shutdown_and_wait().await?; + Ok(()) +} + #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn shell_snapshot_v2_filters_profile_secrets_without_creating_files() -> Result<()> { diff --git a/codex-rs/shell-command/src/shell_snapshot_render.rs b/codex-rs/shell-command/src/shell_snapshot_render.rs index ead034d38c..1d8e48ccf2 100644 --- a/codex-rs/shell-command/src/shell_snapshot_render.rs +++ b/codex-rs/shell-command/src/shell_snapshot_render.rs @@ -1,7 +1,9 @@ -//! Serialize prepared snapshot values without inspecting credentials or executing shell code. +//! Serialize prepared snapshot values and apply export-name policy without executing shell code. //! Native declarations stay unchanged unless the credential stage supplies a replacement. use super::capture::CapturedSnapshot; +use codex_protocol::config_types::ShellEnvironmentPolicy; +use codex_protocol::shell_environment::create_env_from_vars; impl CapturedSnapshot<'_> { /// Render native functions, options, and aliases; the executor restores environment separately. @@ -9,12 +11,22 @@ impl CapturedSnapshot<'_> { format!("{}{}", self.state, self.aliases) } - /// Render a complete replay script without applying credential or environment policy. - pub fn render_script(&self) -> String { + /// Persist only exports admitted by the policy. Explicit overrides are supplied + /// by the command environment; never retain their original captured values. + pub fn render_script(&self, policy: &ShellEnvironmentPolicy) -> String { + let allowed = create_env_from_vars( + self.exports + .iter() + .map(|export| (export.key.to_string(), String::new())), + policy, + /*thread_id*/ None, + ); let mut script = self.render_state(); script.push_str("# exports (native declarations)\n"); for export in &self.exports { - script.push_str(&export.source); + if allowed.contains_key(export.key) && !policy.r#set.contains_key(export.key) { + script.push_str(&export.source); + } } script } diff --git a/codex-rs/shell-command/src/shell_snapshot_tests.rs b/codex-rs/shell-command/src/shell_snapshot_tests.rs index 3ab21128e3..9b6a2d8efa 100644 --- a/codex-rs/shell-command/src/shell_snapshot_tests.rs +++ b/codex-rs/shell-command/src/shell_snapshot_tests.rs @@ -2231,7 +2231,7 @@ fn rewrite_snapshot_credentials( fn captured_script(shell_type: ShellType, source: &str) -> Result { let captured = CapturedSnapshot::parse(shell_type, source.as_bytes()).context("invalid native capture")?; - Ok(captured.render_script()) + Ok(captured.render_script(&codex_protocol::config_types::ShellEnvironmentPolicy::default())) } #[test]