From 8770d4aa0db218d7c7c605c2725dd6aa5ff8fef7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 00:57:09 +0000 Subject: [PATCH 1/3] Start #1958 hermetic color env Assisted-by: Claude Code:claude-opus-5-5 From 90fc905c6794fb52703e15c1ebe8359653ffbec0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 00:59:42 +0000 Subject: [PATCH 2/3] Scrub color env in hermetic test children hermetic::command now removes NO_COLOR, CLICOLOR, CLICOLOR_FORCE and COLUMNS, so an ambient CLICOLOR_FORCE=1 no longer puts ANSI codes into captured stdout (it failed a cli_parse_list test). TERM is left to the PTY suites. The HOSTILE_SEEDS list and the Yarn/Pnpm/Bundle seeds are deleted: each seed was removed right after it was set, so a dropped scrub could never show. spawn_env_hygiene now sets the hostile values in the test process and asserts the child (piped and PTY) never sees them. The module doc now says which builder pins HOME. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/common/hermetic.rs | 63 +++++-------------- .../tests/spawn_env_hygiene.rs | 61 ++++++++++++------ .../tests/vendor/e2e_golang_redirect.rs | 4 +- 3 files changed, 59 insertions(+), 69 deletions(-) diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index d2421c62b..62f64c771 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -11,6 +11,8 @@ //! //! [`command`] (and [`binary_command`] for the built binary) is the only way //! a test spawns `socket-patch`; `common::run_bin_with_env` is built on it. +//! `command` does not move `HOME`: only `run_bin_with_env` (and suites that +//! call `jvm_env::isolate_cli` themselves) point the child at a stand-in. //! PTY suites use [`pty_command`] / [`pty_binary_command`], which carry the //! same environment onto a `portable_pty::CommandBuilder`. //! Package-manager children the same tests spawn get [`scrub_socket_vars`] @@ -31,34 +33,11 @@ use std::path::Path; use std::process::Command; -/// The highest-risk `SOCKET_*` vars. [`command`] seeds each with a hostile -/// value and then removes it: `env_remove` clears the seed too, so the child -/// never sees it, but if a scrub line is ever dropped the seed (rather than a -/// developer's ambient shell, which the suites can't rely on) turns the tests -/// red immediately. -pub const HOSTILE_SEEDS: &[(&str, &str)] = &[ - ("SOCKET_CWD", "/nonexistent"), - ("SOCKET_GLOBAL", "true"), - ("SOCKET_GLOBAL_PREFIX", "/nonexistent"), - ("SOCKET_DRY_RUN", "true"), - ("SOCKET_SAVE_ONLY", "true"), - // Skips every confirm prompt the PTY suites exist to drive. - ("SOCKET_YES", "true"), - ("SOCKET_ECOSYSTEMS", "cargo"), - ("SOCKET_MANIFEST_PATH", "/nonexistent/manifest.json"), - ("SOCKET_JSON", "true"), - ("SOCKET_SILENT", "true"), - ("SOCKET_VERBOSE", "true"), - ("SOCKET_UPDATE_BASE_URL", "http://127.0.0.1:1"), - ("SOCKET_UPDATE_STATE_DIR", "/nonexistent"), - // An ambient token or API/proxy URL would move a child off the public - // proxy, or point the production suites at staging while every - // assertion still passed. - ("SOCKET_API_TOKEN", "hostile-seed-must-be-scrubbed"), - ("SOCKET_API_URL", "http://127.0.0.1:1/hostile"), - ("SOCKET_PROXY_URL", "http://127.0.0.1:1/hostile"), - ("SOCKET_OFFLINE", "true"), -]; +/// The env the CLI's color and width policy reads (`ui::color_enabled`, +/// `ui::width_of`) besides `TERM`. An ambient `CLICOLOR_FORCE=1` puts ANSI +/// codes into every captured stdout. `TERM` is left alone: the PTY suites +/// hand the binary a real terminal and pick their own. +pub const COLOR_ENV: &[&str] = &["NO_COLOR", "CLICOLOR", "CLICOLOR_FORCE", "COLUMNS"]; /// The opt-outs [`command`] forces on in every child: the config layer /// (a developer's real `socket login` must never authenticate a test @@ -75,19 +54,16 @@ pub const FORCED_OPT_OUTS: &[(&str, &str)] = &[ ("SOCKET_TELEMETRY_DISABLED", "1"), ]; -/// A `Command` for `bin` with the hermetic `SOCKET_*` environment: the -/// hostile seeds scrubbed, every flag-bound `SOCKET_*` and every other -/// ambient `SOCKET_*` removed (removing `SOCKET_API_TOKEN` also forces the +/// A `Command` for `bin` with the hermetic `SOCKET_*` environment: every +/// flag-bound `SOCKET_*` and every other ambient `SOCKET_*` removed (removing `SOCKET_API_TOKEN` also forces the /// public proxy), and the three opt-outs (config layer, update notifier, -/// telemetry) forced on. Callers add args, cwd and their own env +/// telemetry) forced on, and [`COLOR_ENV`] removed. Callers add args, cwd +/// and their own env /// afterwards; caller env lands last, so explicit injections survive the /// scrub. pub fn command(bin: &Path) -> Command { let mut cmd = Command::new(bin); - for (k, v) in HOSTILE_SEEDS { - cmd.env(k, v); - } - for (k, _) in HOSTILE_SEEDS { + for k in COLOR_ENV { cmd.env_remove(k); } // The flag-bound vars are removed by name, not only when the sweep below @@ -229,14 +205,13 @@ pub enum Extra { Venv, /// Every `YARN_*` var. Yarn berry lets any `.yarnrc.yml` setting be /// overridden by env, so an ambient `YARN_NODE_LINKER=pnp` would build a - /// PnP tree and void the node_modules probes. Seeded with that value and - /// then scrubbed. + /// PnP tree and void the node_modules probes. Yarn, /// Every `PNPM_*` and `npm_config_*` var (any case). pnpm lets any /// `.npmrc` setting be overridden by an `npm_config_*` env var (env /// outranks the project npmrc), so an ambient /// `npm_config_node_linker=pnp` makes pnpm emit a `.pnp.cjs` that - /// `vendor` refuses. Seeded with that value and then scrubbed. + /// `vendor` refuses. Pnpm, /// Yarn 1's ambient config: the cache folder in any spelling /// (`YARN_CACHE_FOLDER`, `yarn_cache_folder`, `npm_config_cache_folder`; @@ -256,8 +231,8 @@ pub enum Extra { /// lib dir, which pin every child ruby to the outer Gemfile and bundler. /// `GEM_HOME`/`GEM_PATH` are kept on purpose: rvm/chruby /// export them and `bundle` itself resolves through them (see - /// `cache_env.rs`). Seeded with a bogus `BUNDLE_APP_CONFIG` and then - /// scrubbed; run it before the suite sets its own `BUNDLE_*` values. + /// `cache_env.rs`). Run it before the suite sets its own `BUNDLE_*` + /// values. Bundle, /// Every `PDM_*` var plus `UV_PROJECT_ENVIRONMENT`: PDM reads any /// setting from `PDM_`, and its uv backend follows @@ -309,16 +284,13 @@ pub fn scrub_extra(cmd: &mut Command, extras: &[Extra]) { cmd.env_remove("VIRTUAL_ENV"); } Extra::Yarn => { - cmd.env("YARN_NODE_LINKER", "pnp"); for (key, _) in std::env::vars_os() { if key.to_string_lossy().starts_with("YARN_") { cmd.env_remove(&key); } } - cmd.env_remove("YARN_NODE_LINKER"); } Extra::Pnpm => { - cmd.env("npm_config_node_linker", "pnp"); for (key, _) in std::env::vars_os() { let name = key.to_string_lossy(); if name.starts_with("PNPM_") @@ -327,7 +299,6 @@ pub fn scrub_extra(cmd: &mut Command, extras: &[Extra]) { cmd.env_remove(&key); } } - cmd.env_remove("npm_config_node_linker"); } Extra::YarnClassic => { cmd.env_remove("YARN_CACHE_FOLDER"); @@ -345,13 +316,11 @@ pub fn scrub_extra(cmd: &mut Command, extras: &[Extra]) { cmd.env_remove("PREFIX"); } Extra::Bundle => { - cmd.env("BUNDLE_APP_CONFIG", "/nonexistent/.bundle"); for (key, _) in std::env::vars_os() { if key.to_string_lossy().starts_with("BUNDLE_") { cmd.env_remove(&key); } } - cmd.env_remove("BUNDLE_APP_CONFIG"); cmd.env_remove("RUBYOPT"); cmd.env_remove("RUBYLIB"); } diff --git a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs index 39d422430..40596de71 100644 --- a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs +++ b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs @@ -239,13 +239,41 @@ fn workflow_env_copies_carry_every_opt_out() { ); } +/// The highest-risk ambient values: each flips a mode, a target or the +/// output a test asserts on. Set in this process, so a dropped scrub fails. +const HOSTILE_AMBIENT: &[(&str, &str)] = &[ + ("SOCKET_CWD", "/nonexistent"), + ("SOCKET_GLOBAL", "true"), + ("SOCKET_GLOBAL_PREFIX", "/nonexistent"), + ("SOCKET_DRY_RUN", "true"), + ("SOCKET_SAVE_ONLY", "true"), + ("SOCKET_YES", "true"), + ("SOCKET_ECOSYSTEMS", "cargo"), + ("SOCKET_MANIFEST_PATH", "/nonexistent/manifest.json"), + ("SOCKET_JSON", "true"), + ("SOCKET_SILENT", "true"), + ("SOCKET_VERBOSE", "true"), + ("SOCKET_UPDATE_BASE_URL", "http://127.0.0.1:1"), + ("SOCKET_UPDATE_STATE_DIR", "/nonexistent"), + ("SOCKET_API_TOKEN", "ambient-token"), + ("SOCKET_API_URL", "http://127.0.0.1:1/hostile"), + ("SOCKET_PROXY_URL", "http://127.0.0.1:1/hostile"), + ("SOCKET_OFFLINE", "true"), + ("NO_COLOR", "1"), + ("CLICOLOR", "1"), + ("CLICOLOR_FORCE", "1"), + ("COLUMNS", "20"), +]; + #[test] #[serial] -fn command_never_leaks_its_hostile_seeds() { - let env = effective_env(&hermetic::command(Path::new("socket-patch"))); - for (key, _) in hermetic::HOSTILE_SEEDS { - assert_eq!(get(&env, key), None, "{key} seed must be scrubbed"); - } +fn command_scrubs_hostile_ambient_env() { + with_ambient(HOSTILE_AMBIENT, || { + let env = effective_env(&hermetic::command(Path::new("socket-patch"))); + for (key, _) in HOSTILE_AMBIENT { + assert_eq!(get(&env, key), None, "ambient {key} must be scrubbed"); + } + }); } /// In-process tests set flag-bound vars in the test binary's own @@ -334,11 +362,15 @@ fn pty_command_carries_the_same_env_as_command() { #[test] #[serial] -fn pty_command_never_leaks_its_hostile_seeds_and_keeps_caller_env() { - let mut cmd = hermetic::pty_binary_command(); +fn pty_command_scrubs_hostile_ambient_env_and_keeps_caller_env() { + let mut cmd = None; + with_ambient(HOSTILE_AMBIENT, || { + cmd = Some(hermetic::pty_binary_command()); + }); + let mut cmd = cmd.unwrap(); let env = pty_env(&cmd); - for (key, _) in hermetic::HOSTILE_SEEDS { - assert_eq!(env.get(*key), None, "{key} seed must be scrubbed"); + for (key, _) in HOSTILE_AMBIENT { + assert_eq!(env.get(*key), None, "ambient {key} must be scrubbed"); } cmd.env("SOCKET_NO_UPDATE_CHECK", "0"); assert_eq!(cmd.get_env("SOCKET_NO_UPDATE_CHECK"), Some("0".as_ref())); @@ -516,17 +548,6 @@ fn composer_extra_sweeps_overrides_and_pins_an_empty_home() { ); } -#[test] -#[serial] -fn extra_seeds_are_scrubbed_whatever_the_ambient_value() { - let mut cmd = Command::new("tool"); - hermetic::scrub_extra(&mut cmd, &[Extra::Yarn, Extra::Pnpm, Extra::Bundle]); - let env = effective_env(&cmd); - assert_eq!(get(&env, "YARN_NODE_LINKER"), None); - assert_eq!(get(&env, "npm_config_node_linker"), None); - assert_eq!(get(&env, "BUNDLE_APP_CONFIG"), None); -} - /// Every `.rs` file under `tests/`, as `/`-separated paths relative to it. fn test_sources() -> Vec<(String, String)> { fn walk(dir: &Path, root: &Path, out: &mut Vec<(String, String)>) { diff --git a/crates/socket-patch-cli/tests/vendor/e2e_golang_redirect.rs b/crates/socket-patch-cli/tests/vendor/e2e_golang_redirect.rs index f257cc636..b4dd7aab4 100644 --- a/crates/socket-patch-cli/tests/vendor/e2e_golang_redirect.rs +++ b/crates/socket-patch-cli/tests/vendor/e2e_golang_redirect.rs @@ -64,8 +64,8 @@ fn stage(root: &Path) -> (std::path::PathBuf, std::path::PathBuf) { (gomodcache, cache_dir) } -/// Run the CLI binary through the shared hermetic runner. Its hostile seeds -/// cover the vars verified to break this suite when inherited: +/// Run the CLI binary through the shared hermetic runner. Its scrub +/// covers the vars verified to break this suite when inherited: /// `SOCKET_GLOBAL` / `SOCKET_GLOBAL_PREFIX` take the redirect backend out of /// local scope (apply patches the fake module cache IN PLACE and `--check` /// exits 0 having checked nothing), `SOCKET_DRY_RUN` makes every apply a From df7d599bde11417b8194bd6f8534b5626db59b24 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 01:14:07 +0000 Subject: [PATCH 3/3] Seed every flag-bound var in the env canary The canary now sets every GLOBAL/LOCAL_ARG_ENV_VARS name, so a new flag-bound var is covered without a hand-kept list. command keeps the by-name removal of SOCKET_UPDATE_BASE_URL and SOCKET_UPDATE_STATE_DIR that the deleted seed loop used to give. Stale seed comments in the golang and vendored-production suites now point at spawn_env_hygiene. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/common/hermetic.rs | 16 +++-- .../tests/e2e_golang_build.rs | 2 +- .../tests/e2e_vendored_production.rs | 10 +-- .../tests/spawn_env_hygiene.rs | 69 +++++++++---------- 4 files changed, 47 insertions(+), 50 deletions(-) diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index 62f64c771..777b74b90 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -54,13 +54,12 @@ pub const FORCED_OPT_OUTS: &[(&str, &str)] = &[ ("SOCKET_TELEMETRY_DISABLED", "1"), ]; -/// A `Command` for `bin` with the hermetic `SOCKET_*` environment: every -/// flag-bound `SOCKET_*` and every other ambient `SOCKET_*` removed (removing `SOCKET_API_TOKEN` also forces the -/// public proxy), and the three opt-outs (config layer, update notifier, -/// telemetry) forced on, and [`COLOR_ENV`] removed. Callers add args, cwd -/// and their own env -/// afterwards; caller env lands last, so explicit injections survive the -/// scrub. +/// A `Command` for `bin` with the hermetic environment: every flag-bound +/// and every other ambient `SOCKET_*` removed (removing `SOCKET_API_TOKEN` +/// also forces the public proxy), [`COLOR_ENV`] removed, and the three +/// opt-outs (config layer, update notifier, telemetry) forced on. Callers +/// add args, cwd and their own env afterwards; caller env lands last, so +/// explicit injections survive the scrub. pub fn command(bin: &Path) -> Command { let mut cmd = Command::new(bin); for k in COLOR_ENV { @@ -77,6 +76,9 @@ pub fn command(bin: &Path) -> Command { cmd.env_remove(k); } cmd.env_remove("SOCKET_API_TOKEN"); + // Not flag-bound; removed by name for the same reason. + cmd.env_remove("SOCKET_UPDATE_BASE_URL"); + cmd.env_remove("SOCKET_UPDATE_STATE_DIR"); // The `SOCKET_CLI_*` peer names too: the binary promotes each onto its // canonical `SOCKET_*` name, so a peer token set mid-run would put the // child on the authenticated path. diff --git a/crates/socket-patch-cli/tests/e2e_golang_build.rs b/crates/socket-patch-cli/tests/e2e_golang_build.rs index e40e4fcd4..c18b2e6be 100644 --- a/crates/socket-patch-cli/tests/e2e_golang_build.rs +++ b/crates/socket-patch-cli/tests/e2e_golang_build.rs @@ -194,7 +194,7 @@ fn go_build_links_patch_via_replace_redirect() { // this suite fails deterministically if the leak returns. `GOTOOLCHAIN` // must lose to go_env's `local` pin (or every `go` command chases a // nonexistent toolchain through the file proxy). `SOCKET_DRY_RUN` needs - // no guard here: `hermetic::command` seeds and scrubs it itself. + // no guard here: `spawn_env_hygiene` pins `hermetic::command`'s scrub. std::env::set_var("GOTOOLCHAIN", "go1.99.99"); let tmp = tempfile::tempdir().unwrap(); let (consumer, modcache, proxy_url) = stage(tmp.path()); diff --git a/crates/socket-patch-cli/tests/e2e_vendored_production.rs b/crates/socket-patch-cli/tests/e2e_vendored_production.rs index 724c5fbd0..bf0b05683 100644 --- a/crates/socket-patch-cli/tests/e2e_vendored_production.rs +++ b/crates/socket-patch-cli/tests/e2e_vendored_production.rs @@ -87,8 +87,8 @@ //! No API token is used or needed — the suite deliberately runs against the //! **free public proxy** with `SOCKET_NO_CONFIG=true` so a developer's //! socket-cli login cannot move the run onto the org catalog. Every ambient -//! `SOCKET_*` var is scrubbed and hostile seeds are planted so a dropped scrub -//! turns the suite red immediately. +//! `SOCKET_*` var is scrubbed; `spawn_env_hygiene` turns red if a scrub is +//! dropped. //! //! # Running //! @@ -324,9 +324,9 @@ fn has_corepack_pm(pm: &str) -> bool { /// would silently point the run at staging; an ambient `SOCKET_API_TOKEN` (or a /// socket-cli login on disk) would move it off the free public proxy this suite /// exists to cover. The shared builder (`hermetic::binary_command`) removes -/// every ambient `SOCKET_*`, seeds those three with hostile values and removes -/// them (so a dropped scrub reddens the suite instead of leaking a developer's -/// shell), and forces `SOCKET_NO_CONFIG=1` to block the socket-cli +/// every ambient `SOCKET_*` (`spawn_env_hygiene` seeds those three in the +/// test process and fails if one reaches the child), and forces +/// `SOCKET_NO_CONFIG=1` to block the socket-cli /// config.json. The crawler's own ambient inputs are scrubbed too /// (`hermetic::CRAWLER_EXTRAS`: an activated venv, `BUNDLE_*`, `COMPOSER_*`), /// so an ambient BUNDLE_PATH can't move the gem leg's crawl off diff --git a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs index 40596de71..b70b95097 100644 --- a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs +++ b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs @@ -239,38 +239,34 @@ fn workflow_env_copies_carry_every_opt_out() { ); } -/// The highest-risk ambient values: each flips a mode, a target or the -/// output a test asserts on. Set in this process, so a dropped scrub fails. -const HOSTILE_AMBIENT: &[(&str, &str)] = &[ - ("SOCKET_CWD", "/nonexistent"), - ("SOCKET_GLOBAL", "true"), - ("SOCKET_GLOBAL_PREFIX", "/nonexistent"), - ("SOCKET_DRY_RUN", "true"), - ("SOCKET_SAVE_ONLY", "true"), - ("SOCKET_YES", "true"), - ("SOCKET_ECOSYSTEMS", "cargo"), - ("SOCKET_MANIFEST_PATH", "/nonexistent/manifest.json"), - ("SOCKET_JSON", "true"), - ("SOCKET_SILENT", "true"), - ("SOCKET_VERBOSE", "true"), - ("SOCKET_UPDATE_BASE_URL", "http://127.0.0.1:1"), - ("SOCKET_UPDATE_STATE_DIR", "/nonexistent"), - ("SOCKET_API_TOKEN", "ambient-token"), - ("SOCKET_API_URL", "http://127.0.0.1:1/hostile"), - ("SOCKET_PROXY_URL", "http://127.0.0.1:1/hostile"), - ("SOCKET_OFFLINE", "true"), - ("NO_COLOR", "1"), - ("CLICOLOR", "1"), - ("CLICOLOR_FORCE", "1"), - ("COLUMNS", "20"), -]; +/// Every flag-bound var but the forced opt-outs, plus the other high-risk +/// `SOCKET_*` values and the color env, set in this process so a dropped +/// scrub fails. +fn hostile_ambient() -> Vec<(&'static str, &'static str)> { + let args = socket_patch_cli::args::GLOBAL_ARG_ENV_VARS + .iter() + .chain(socket_patch_cli::args::LOCAL_ARG_ENV_VARS) + .filter(|k| !hermetic::FORCED_OPT_OUTS.iter().any(|(f, _)| f == *k)) + .map(|k| (*k, "true")); + let other = [ + ("SOCKET_API_TOKEN", "ambient-token"), + ("SOCKET_UPDATE_BASE_URL", "http://127.0.0.1:1"), + ("SOCKET_UPDATE_STATE_DIR", "/nonexistent"), + ("NO_COLOR", "1"), + ("CLICOLOR", "1"), + ("CLICOLOR_FORCE", "1"), + ("COLUMNS", "20"), + ]; + args.chain(other).collect() +} #[test] #[serial] fn command_scrubs_hostile_ambient_env() { - with_ambient(HOSTILE_AMBIENT, || { + let hostile = hostile_ambient(); + with_ambient(&hostile, || { let env = effective_env(&hermetic::command(Path::new("socket-patch"))); - for (key, _) in HOSTILE_AMBIENT { + for (key, _) in &hostile { assert_eq!(get(&env, key), None, "ambient {key} must be scrubbed"); } }); @@ -363,17 +359,16 @@ fn pty_command_carries_the_same_env_as_command() { #[test] #[serial] fn pty_command_scrubs_hostile_ambient_env_and_keeps_caller_env() { - let mut cmd = None; - with_ambient(HOSTILE_AMBIENT, || { - cmd = Some(hermetic::pty_binary_command()); + let hostile = hostile_ambient(); + with_ambient(&hostile, || { + let mut cmd = hermetic::pty_binary_command(); + let env = pty_env(&cmd); + for (key, _) in &hostile { + assert_eq!(env.get(*key), None, "ambient {key} must be scrubbed"); + } + cmd.env("SOCKET_NO_UPDATE_CHECK", "0"); + assert_eq!(cmd.get_env("SOCKET_NO_UPDATE_CHECK"), Some("0".as_ref())); }); - let mut cmd = cmd.unwrap(); - let env = pty_env(&cmd); - for (key, _) in HOSTILE_AMBIENT { - assert_eq!(env.get(*key), None, "ambient {key} must be scrubbed"); - } - cmd.env("SOCKET_NO_UPDATE_CHECK", "0"); - assert_eq!(cmd.get_env("SOCKET_NO_UPDATE_CHECK"), Some("0".as_ref())); } #[test]