From 6b4b376bb01950162caaa0c5ae0d8c54027d53d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 10:52:19 +0000 Subject: [PATCH 1/4] Start PTY hermetic spawns for #823 Assisted-by: Claude Code:claude-opus-5-5 From bf66308f27c0c7bfaf74bfa395deae93e05a1d8e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 11:15:12 +0000 Subject: [PATCH 2/4] Spawn PTY test children hermetically Seven PTY test runners spawned socket-patch with their own SOCKET_* scrubs, and the copies had drifted: the scan runner stripped the SOCKET_NO_CONFIG opt-out (so a developer's socket login could reach the child), several left telemetry or the update notifier to the ambient shell, and none removed the flag-bound vars by name, so one set by a parallel in-process test could leak into the child. Add hermetic::pty_command / pty_binary_command, which put the same environment as hermetic::command on a portable_pty CommandBuilder, move the seven runners onto them, and teach the spawn ratchet to flag CommandBuilder::new(binary()) so new PTY suites can't drift again. SOCKET_YES joins the hostile seeds, since it skips every prompt the PTY suites drive. Refs #823 Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/cli/covgap_output.rs | 50 ++--------- .../tests/cli/interactive_prompts_e2e.rs | 50 ++--------- .../socket-patch-cli/tests/common/hermetic.rs | 58 ++++++++++-- .../tests/covgap_commands_rollback.rs | 27 +----- .../tests/covgap_commands_scan_mod.rs | 31 ++----- .../tests/remove/covgap_commands_remove.rs | 23 +---- .../tests/spawn_env_hygiene.rs | 89 +++++++++++++++++-- .../tests/update/covgap_commands_update.rs | 26 ++---- .../tests/update/update_notifier_e2e.rs | 23 ++--- 9 files changed, 173 insertions(+), 204 deletions(-) diff --git a/crates/socket-patch-cli/tests/cli/covgap_output.rs b/crates/socket-patch-cli/tests/cli/covgap_output.rs index 6286eff7d..b932330b9 100644 --- a/crates/socket-patch-cli/tests/cli/covgap_output.rs +++ b/crates/socket-patch-cli/tests/cli/covgap_output.rs @@ -19,7 +19,7 @@ use crate::common::binary; use std::path::Path; use std::time::Duration; -use portable_pty::{native_pty_system, CommandBuilder, PtySize}; +use portable_pty::{native_pty_system, PtySize}; use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -104,58 +104,24 @@ fn run_in_pty_inner( }) .expect("openpty"); + // The hermetic environment (`tests/common/hermetic.rs`): an ambient + // SOCKET_YES=true would skip the confirm prompt the bare-Enter test + // drives, and SOCKET_JSON=true flips select_one onto its + // JsonModeNeedsExplicit branch before the dialoguer menu renders. The + // `sh` wrapper `exec`s the binary, which inherits the same environment. let mut cmd = if sigint == Sigint::Ignored { - let mut cmd = CommandBuilder::new("/bin/sh"); + let mut cmd = common::hermetic::pty_command("/bin/sh"); cmd.arg("-c"); cmd.arg("trap '' INT; exec \"$0\" \"$@\""); cmd.arg(binary()); cmd } else { - CommandBuilder::new(binary()) + common::hermetic::pty_binary_command() }; for a in args { cmd.arg(a); } cmd.cwd(cwd); - // The binary binds a wide `SOCKET_*` env surface. An ambient value - // silently reroutes what these tests exercise — SOCKET_YES=true skips - // the very confirm prompt the bare-Enter test exists to drive, and - // SOCKET_JSON=true flips select_one onto its JsonModeNeedsExplicit - // branch before the dialoguer menu ever renders. The highest-risk vars - // are seeded with hostile values and then scrubbed — `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 this suite can't rely on) turns the tests red immediately. - cmd.env("SOCKET_YES", "true"); - cmd.env("SOCKET_JSON", "true"); - cmd.env("SOCKET_DRY_RUN", "true"); - cmd.env("SOCKET_SILENT", "true"); - cmd.env_remove("SOCKET_YES"); - cmd.env_remove("SOCKET_JSON"); - cmd.env_remove("SOCKET_DRY_RUN"); - cmd.env_remove("SOCKET_SILENT"); - // Prefix-scrub whatever else the ambient shell carries (SOCKET_CWD, - // SOCKET_MANIFEST_PATH, SOCKET_API_TOKEN, SOCKET_PROXY_URL — the get - // tests pin their API endpoint by flag and must not be rerouted). - // Telemetry opt-outs are deliberately kept so an opted-out dev stays - // opted out. - for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy(); - if name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - && name != "SOCKET_NO_UPDATE_CHECK" - { - cmd.env_remove(&key); - } - } - // A developer's real `socket login` (the socket-cli config.json token - // fallback) must never authenticate a test child. - cmd.env("SOCKET_NO_CONFIG", "1"); - // This suite hands children a REAL terminal, so the update notifier's - // stderr-TTY guard does not protect it. Force the kill-switch so no PTY - // child ever fetches release metadata mid-prompt. - cmd.env("SOCKET_NO_UPDATE_CHECK", "1"); // `dialoguer`/`console` key handling needs a terminal type; pin one so // an unset ambient TERM (bare CI shells) can't skew rendering. cmd.env("TERM", "xterm-256color"); diff --git a/crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs b/crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs index b8c392e74..1516063cc 100644 --- a/crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs +++ b/crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs @@ -7,12 +7,10 @@ #![cfg(unix)] -use crate::common::binary; - use std::path::Path; use std::time::Duration; -use portable_pty::{native_pty_system, CommandBuilder, PtySize}; +use portable_pty::{native_pty_system, PtySize}; /// Spawn the socket-patch binary inside a PTY, send `input`, and /// collect all output until the child exits. Returns `(exit_code, @@ -52,49 +50,17 @@ fn run_in_pty_bytes(args: &[&str], cwd: &Path, input: &[u8], timeout: Duration) }) .expect("openpty"); - let mut cmd = CommandBuilder::new(binary()); + // The hermetic environment (`tests/common/hermetic.rs`): an ambient + // SOCKET_YES=true would skip the very confirm prompts this file exists + // to drive, and SOCKET_SILENT=true would suppress the output the + // oracles match. It also forces the update notifier off: this suite + // hands the child a REAL terminal, so the stderr-TTY guard does not + // protect it. + let mut cmd = crate::common::hermetic::pty_binary_command(); for a in args { cmd.arg(a); } cmd.cwd(cwd); - // The binary binds a wide `SOCKET_*` env surface (SOCKET_YES, - // SOCKET_JSON, SOCKET_DRY_RUN, SOCKET_SILENT, SOCKET_MANIFEST_PATH, - // ...). An ambient value silently reroutes what these tests exercise — - // SOCKET_YES=true skips the very confirm prompts this file exists to - // drive, and SOCKET_SILENT=true suppresses the output the oracles - // match. The highest-risk vars are seeded with hostile values and then - // scrubbed — `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 this suite can't rely on) turns the - // tests red immediately. - cmd.env("SOCKET_YES", "true"); - cmd.env("SOCKET_JSON", "true"); - cmd.env("SOCKET_DRY_RUN", "true"); - cmd.env("SOCKET_SILENT", "true"); - cmd.env_remove("SOCKET_YES"); - cmd.env_remove("SOCKET_JSON"); - cmd.env_remove("SOCKET_DRY_RUN"); - cmd.env_remove("SOCKET_SILENT"); - // Prefix-scrub whatever else the ambient shell carries (SOCKET_CWD, - // SOCKET_MANIFEST_PATH, SOCKET_API_TOKEN — removing the token also - // forces the public proxy). Telemetry opt-outs are deliberately kept - // so an opted-out dev stays opted out. - for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy(); - if name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - && name != "SOCKET_NO_UPDATE_CHECK" - { - cmd.env_remove(&key); - } - } - // This suite is the one place test children get a REAL terminal, so the - // update notifier's stderr-TTY guard does not protect it. Force the - // kill-switch (mirroring the `.cargo/config.toml` `[env]` default the - // prefix scrub would otherwise strip) so no PTY child ever fetches - // release metadata mid-prompt. - cmd.env("SOCKET_NO_UPDATE_CHECK", "1"); let mut child = pair .slave diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index 167ecae5e..c2b95d49e 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. +//! 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`] //! plus whichever [`Extra`] scrubs their tool needs. //! @@ -40,6 +42,8 @@ pub const HOSTILE_SEEDS: &[(&str, &str)] = &[ ("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"), @@ -131,18 +135,60 @@ pub fn tool_command(name: &str) -> Command { ) } +/// [`command`] for a PTY child: the same hermetic `SOCKET_*` environment +/// on a `portable_pty::CommandBuilder` for `program`. The PTY suites hand +/// the binary a real terminal, so they can't use `std::process::Command`; +/// before this they each hand-rolled a scrub, and the copies drifted (one +/// stripped the opt-outs, several didn't force them, none removed the +/// flag-bound vars by name). `program` is usually the binary, or a shell +/// that `exec`s it, which inherits the same environment. +/// +/// `CommandBuilder::new` snapshots the process environment, so a var set +/// in-process before the snapshot and removed before [`command`]'s sweep +/// would survive in it; the snapshot is swept too. Callers add args, cwd +/// and env afterwards, as with [`command`]. +pub fn pty_command(program: impl AsRef) -> portable_pty::CommandBuilder { + let program = program.as_ref(); + let mut pty = portable_pty::CommandBuilder::new(program); + let snapshot: Vec = pty + .iter_full_env_as_str() + .map(|(key, _)| key.to_owned()) + .filter(|key| is_scrubbed_socket_var(key)) + .collect(); + for key in snapshot { + pty.env_remove(key); + } + for (key, value) in command(Path::new(program)).get_envs() { + match value { + Some(value) => pty.env(key, value), + None => pty.env_remove(key), + } + } + pty +} + +/// [`pty_command`] for the `socket-patch` binary cargo built for this run. +pub fn pty_binary_command() -> portable_pty::CommandBuilder { + pty_command(env!("CARGO_BIN_EXE_socket-patch")) +} + +/// Whether [`scrub_socket_vars`] removes the var `name`: every `SOCKET_*` +/// except the telemetry opt-outs (an opted-out developer stays opted out), +/// `SOCKET_NO_CONFIG` and `SOCKET_NO_UPDATE_CHECK`. +fn is_scrubbed_socket_var(name: &str) -> bool { + name.starts_with("SOCKET_") + && !name.contains("TELEMETRY") + && name != "SOCKET_NO_CONFIG" + && name != "SOCKET_NO_UPDATE_CHECK" +} + /// Remove every ambient `SOCKET_*` var from `cmd`, except the telemetry /// opt-outs (an opted-out developer stays opted out), `SOCKET_NO_CONFIG` /// and `SOCKET_NO_UPDATE_CHECK`. [`command`] runs this; package-manager /// children call it directly. pub fn scrub_socket_vars(cmd: &mut Command) { for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy(); - if name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - && name != "SOCKET_NO_UPDATE_CHECK" - { + if is_scrubbed_socket_var(&key.to_string_lossy()) { cmd.env_remove(&key); } } diff --git a/crates/socket-patch-cli/tests/covgap_commands_rollback.rs b/crates/socket-patch-cli/tests/covgap_commands_rollback.rs index 4aa75591d..fb3abd34e 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_rollback.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_rollback.rs @@ -2377,7 +2377,7 @@ fn manifest_write_failure_warns_and_exits_one() { #[cfg(unix)] mod interactive { use super::*; - use portable_pty::{native_pty_system, CommandBuilder, PtySize}; + use portable_pty::{native_pty_system, PtySize}; use std::time::Duration; /// Spawn the binary inside a PTY, send `input`, collect all output — @@ -2393,32 +2393,13 @@ mod interactive { }) .expect("openpty"); - let mut cmd = CommandBuilder::new(common::binary()); + // The hermetic environment: SOCKET_YES would skip the very prompt + // under test. + let mut cmd = common::hermetic::pty_binary_command(); for a in args { cmd.arg(a); } cmd.cwd(cwd); - // Seed-then-scrub the highest-risk vars (SOCKET_YES would skip the - // very prompt under test), then prefix-scrub the rest. - cmd.env("SOCKET_YES", "true"); - cmd.env("SOCKET_JSON", "true"); - cmd.env("SOCKET_DRY_RUN", "true"); - cmd.env("SOCKET_SILENT", "true"); - cmd.env_remove("SOCKET_YES"); - cmd.env_remove("SOCKET_JSON"); - cmd.env_remove("SOCKET_DRY_RUN"); - cmd.env_remove("SOCKET_SILENT"); - for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy(); - if name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - && name != "SOCKET_NO_UPDATE_CHECK" - { - cmd.env_remove(&key); - } - } - cmd.env("SOCKET_NO_UPDATE_CHECK", "1"); let mut child = pair .slave diff --git a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs index e041a3b48..b5519ac45 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs @@ -1581,7 +1581,7 @@ mod pty { use super::*; use std::time::Duration; - use portable_pty::{native_pty_system, CommandBuilder, PtySize}; + use portable_pty::{native_pty_system, PtySize}; /// Spawn the binary in a PTY, send `input`, and collect all output /// until exit (the `interactive_prompts_e2e.rs` harness pattern: @@ -1611,37 +1611,16 @@ mod pty { }) .expect("openpty"); - let mut cmd = CommandBuilder::new(binary()); + // The hermetic environment (`tests/common/hermetic.rs`), which also + // forces the update notifier off: this child gets a REAL terminal, + // so the stderr-TTY guard does not protect it. + let mut cmd = common::hermetic::pty_binary_command(); for a in args { cmd.arg(a); } cmd.cwd(cwd); // These tests assert VT rendering, even when the parent has TERM=dumb. cmd.env("TERM", "xterm-256color"); - // Scrub the flag-bound SOCKET_* surface so ambient configuration - // cannot reroute the run; keep telemetry disabled and the - // update notifier off — this child gets a REAL terminal, so the - // stderr-TTY guard does not protect it. - for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy(); - if name.starts_with("SOCKET_") { - cmd.env_remove(&key); - } - } - // In-process tests in this binary `std::env::set_var` these via - // `apply_env_toggles`; one set by a parallel test between the scan - // above and the spawn would be inherited, so remove them - // unconditionally (see in_process_vendor.rs `run_cli`). - for key in [ - "SOCKET_OFFLINE", - "SOCKET_DEBUG", - "SOCKET_API_URL", - "SOCKET_PROXY_URL", - ] { - cmd.env_remove(key); - } - cmd.env("SOCKET_TELEMETRY_DISABLED", "1"); - cmd.env("SOCKET_NO_UPDATE_CHECK", "1"); for (k, v) in env { cmd.env(k, v); } diff --git a/crates/socket-patch-cli/tests/remove/covgap_commands_remove.rs b/crates/socket-patch-cli/tests/remove/covgap_commands_remove.rs index 99d3e2198..aa7311d86 100644 --- a/crates/socket-patch-cli/tests/remove/covgap_commands_remove.rs +++ b/crates/socket-patch-cli/tests/remove/covgap_commands_remove.rs @@ -13,8 +13,6 @@ //! remove_invariants.rs / remove_duality_invariants.rs / //! interactive_prompts_e2e.rs. -use crate::common::binary; - use std::path::{Path, PathBuf}; use crate::common; @@ -1892,7 +1890,7 @@ fn remove_rollback_infrastructure_error_surfaces_rollback_failed() { #[cfg(unix)] mod pty { use super::*; - use portable_pty::{native_pty_system, CommandBuilder, PtySize}; + use portable_pty::{native_pty_system, PtySize}; use std::time::Duration; /// Spawn the binary inside a PTY, send `input`, collect all output @@ -1908,26 +1906,13 @@ mod pty { }) .expect("openpty"); - let mut cmd = CommandBuilder::new(binary()); + // The hermetic environment: SOCKET_YES would skip the very prompt + // this test drives. + let mut cmd = common::hermetic::pty_binary_command(); for a in args { cmd.arg(a); } cmd.cwd(cwd); - // Scrub the ambient SOCKET_* surface (SOCKET_YES would skip the - // very prompt this test drives); keep telemetry opt-outs. - for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy(); - if name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - && name != "SOCKET_NO_UPDATE_CHECK" - { - cmd.env_remove(&key); - } - } - cmd.env("SOCKET_NO_CONFIG", "1"); - cmd.env("SOCKET_NO_UPDATE_CHECK", "1"); - cmd.env("SOCKET_TELEMETRY_DISABLED", "1"); let mut child = pair.slave.spawn_command(cmd).expect("spawn in PTY"); drop(pair.slave); diff --git a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs index 906f6cbf7..786337e86 100644 --- a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs +++ b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs @@ -10,9 +10,9 @@ //! * a ratchet over the test tree: no private `scrub_socket_env` copy, and //! no file that spawns the binary with a bare `Command::new`; spawns go //! through `hermetic::command` / `common::run*`. The detector matches the -//! spellings in `has_raw_spawn`, not a path bound to a local first, and -//! doesn't see `portable_pty::CommandBuilder` spawns, which keep their -//! own scrubs. +//! spellings in `has_raw_spawn` (for `Command::new` and the PTY suites' +//! `portable_pty::CommandBuilder::new`), not a path bound to a local +//! first. #[path = "common/hermetic.rs"] mod hermetic; @@ -285,6 +285,66 @@ fn command_drops_flag_bound_vars_set_after_it_is_built() { ); } +/// The `SOCKET_*` part of the environment a PTY child would see. +fn pty_socket_env(cmd: &portable_pty::CommandBuilder) -> BTreeMap { + cmd.iter_full_env_as_str() + .filter(|(k, _)| k.starts_with("SOCKET_")) + .map(|(k, v)| (k.to_owned(), v.to_owned())) + .collect() +} + +/// The PTY suites used to hand-roll their scrubs, and the copies drifted. +/// `pty_command` must give a PTY child exactly the `SOCKET_*` environment +/// `command` gives a piped one, whatever the ambient shell holds. +#[test] +#[serial] +fn pty_command_carries_the_same_socket_env_as_command() { + with_ambient( + &[ + ("SOCKET_YES", "true"), + ("SOCKET_JSON", "true"), + ("SOCKET_DRY_RUN", "true"), + ("SOCKET_API_TOKEN", "ambient-token"), + ("SOCKET_CLI_API_TOKEN", "ambient-peer-token"), + ("SOCKET_SOME_FUTURE_FLAG", "1"), + ("SOCKET_NO_CONFIG", "0"), + ("SOCKET_NO_UPDATE_CHECK", "0"), + ("SOCKET_TELEMETRY_DISABLED", "0"), + ], + || { + let piped: BTreeMap = + effective_env(&hermetic::command(Path::new("socket-patch"))) + .into_iter() + .filter_map(|(k, v)| Some((k.into_string().ok()?, v.into_string().ok()?))) + .filter(|(k, _)| k.starts_with("SOCKET_")) + .collect(); + let pty = pty_socket_env(&hermetic::pty_command("socket-patch")); + assert_eq!(pty, piped); + assert_eq!(pty.get("SOCKET_YES"), None); + assert_eq!( + pty.get("SOCKET_NO_UPDATE_CHECK").map(String::as_str), + Some("1") + ); + assert_eq!( + pty.get("SOCKET_TELEMETRY_DISABLED").map(String::as_str), + Some("1") + ); + }, + ); +} + +#[test] +#[serial] +fn pty_command_never_leaks_its_hostile_seeds_and_keeps_caller_env() { + let mut cmd = hermetic::pty_binary_command(); + let env = pty_socket_env(&cmd); + for (key, _) in hermetic::HOSTILE_SEEDS { + assert_eq!(env.get(*key), None, "{key} seed must be scrubbed"); + } + cmd.env("SOCKET_NO_UPDATE_CHECK", "0"); + assert_eq!(cmd.get_env("SOCKET_NO_UPDATE_CHECK"), Some("0".as_ref())); +} + #[test] #[serial] fn caller_env_set_after_command_survives_the_scrub() { @@ -493,10 +553,14 @@ fn test_sources() -> Vec<(String, String)> { } /// Whether `text` spawns the `socket-patch` binary with a bare -/// `Command::new`: `binary()` (any path prefix), the `CARGO_BIN_EXE_*` -/// literal, `socket_bin()` or a `BINARY` const. +/// `Command::new` or `CommandBuilder::new`: `binary()` (any path prefix), +/// the `CARGO_BIN_EXE_*` literal, `socket_bin()` or a `BINARY` const. fn has_raw_spawn(text: &str) -> bool { - text.split("Command::new(").skip(1).any(|rest| { + let mut spawns = text + .split("Command::new(") + .skip(1) + .chain(text.split("CommandBuilder::new(").skip(1)); + spawns.any(|rest| { let arg = rest.trim_start_matches(|c: char| c.is_alphanumeric() || c == '_' || c == ':'); let head = &rest[..rest.len() - arg.len()]; let name = head.rsplit("::").next().unwrap_or(head); @@ -515,6 +579,9 @@ fn raw_spawn_detector_matches_the_spellings_in_the_tree() { "Command::new(env!(\"CARGO_BIN_EXE_socket-patch\"))", "Command::new(socket_bin())", "Command::new(BINARY)", + "CommandBuilder::new(binary())", + "portable_pty::CommandBuilder::new(common::binary())", + "CommandBuilder::new(env!(\"CARGO_BIN_EXE_socket-patch\"))", ] { assert!(has_raw_spawn(spawn), "{spawn}"); } @@ -524,6 +591,8 @@ fn raw_spawn_detector_matches_the_spellings_in_the_tree() { "Command::new(bin)", "Command::new(binary_path)", "Command::new(BINARY_DIR)", + "hermetic::pty_binary_command()", + "CommandBuilder::new(\"/bin/sh\")", ] { assert!(!has_raw_spawn(ok), "{ok}"); } @@ -556,9 +625,11 @@ fn no_new_bare_binary_spawns() { .collect(); assert!( found.is_empty(), - "these test files spawn the binary with a bare Command::new: \ + "these test files spawn the binary with a bare Command::new or \ + CommandBuilder::new: \ {found:?}. Use `hermetic::command(&bin)` / \ - `hermetic::binary_command()` or `common::run*` (tests/common/) so the \ - child gets the hermetic SOCKET_* environment." + `hermetic::binary_command()` or `common::run*` (tests/common/), or \ + `hermetic::pty_command` / `hermetic::pty_binary_command()` for a PTY \ + child, so the child gets the hermetic SOCKET_* environment." ); } diff --git a/crates/socket-patch-cli/tests/update/covgap_commands_update.rs b/crates/socket-patch-cli/tests/update/covgap_commands_update.rs index 16836e614..4f5846990 100644 --- a/crates/socket-patch-cli/tests/update/covgap_commands_update.rs +++ b/crates/socket-patch-cli/tests/update/covgap_commands_update.rs @@ -212,7 +212,7 @@ async fn update_dry_run_human_reports_update_available() { #[cfg(unix)] mod pty { use super::*; - use portable_pty::{native_pty_system, CommandBuilder, PtySize}; + use portable_pty::{native_pty_system, PtySize}; use std::path::Path; use std::time::Duration; @@ -237,29 +237,15 @@ mod pty { }) .expect("openpty"); - let mut cmd = CommandBuilder::new(bin); + // The hermetic environment: SOCKET_YES would skip the very prompt + // this test drives, and the update notifier is forced off (this + // suite hands the child a REAL terminal, so the stderr-TTY guard + // does not protect it). + let mut cmd = crate::common::hermetic::pty_command(bin); for a in args { cmd.arg(a); } cmd.cwd(cwd); - // Scrub the ambient SOCKET_* surface (SOCKET_YES would skip the - // very prompt this test drives); keep telemetry opt-outs. - for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy(); - if name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - && name != "SOCKET_NO_UPDATE_CHECK" - { - cmd.env_remove(&key); - } - } - cmd.env("SOCKET_NO_CONFIG", "1"); - // This suite hands the child a REAL terminal, so the update - // notifier's stderr-TTY guard does not protect it — force the - // kill-switch so no PTY child fetches release metadata mid-prompt. - cmd.env("SOCKET_NO_UPDATE_CHECK", "1"); - cmd.env("SOCKET_TELEMETRY_DISABLED", "1"); // Caller env lands last so explicit injections survive the scrub. for (k, v) in env { cmd.env(k, v); diff --git a/crates/socket-patch-cli/tests/update/update_notifier_e2e.rs b/crates/socket-patch-cli/tests/update/update_notifier_e2e.rs index 54889b6fb..a3196eda6 100644 --- a/crates/socket-patch-cli/tests/update/update_notifier_e2e.rs +++ b/crates/socket-patch-cli/tests/update/update_notifier_e2e.rs @@ -594,13 +594,13 @@ async fn update_command_never_notifies() { #[cfg(unix)] mod pty { use super::*; - use portable_pty::{native_pty_system, CommandBuilder, PtySize}; + use portable_pty::{native_pty_system, PtySize}; use std::io::Read; /// Minimal PTY runner (same shape as `interactive_prompts_e2e.rs`): /// reader thread to EOF, detached SIGKILL watchdog, no input. This - /// bypasses `run_installed`, so the hermetic scrub and the update kit - /// are reproduced by hand. + /// bypasses `run_installed`, so the update kit is applied by hand on + /// top of `hermetic::pty_command`. fn run_in_pty( bin: &Path, cwd: &Path, @@ -617,22 +617,11 @@ mod pty { }) .expect("openpty"); - let mut cmd = CommandBuilder::new(bin); + // The hermetic environment, then the caller's kit, which lands last + // (its `SOCKET_NO_UPDATE_CHECK=0` turns the notifier back on). + let mut cmd = common::hermetic::pty_command(bin); cmd.arg("apply"); cmd.cwd(cwd); - // Prefix-scrub the ambient SOCKET_* surface (keep telemetry - // opt-outs and the no-config hermeticity default), then land the - // caller's kit — mirrors run_bin_with_env for a PTY child. - for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy().into_owned(); - if name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - { - cmd.env_remove(name); - } - } - cmd.env("SOCKET_NO_CONFIG", "1"); for (k, v) in env { cmd.env(k, v); } From 4253e5a89c3febc262e706a3c91f5be98a87b347 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 11:22:59 +0000 Subject: [PATCH 3/4] Build the PTY env from one copy of the plan pty_command now replaces the builder's environment outright with what hermetic::command's child would see, instead of sweeping the builder's own environment snapshot and replaying the plan on top. That keeps one source for the environment, handles non-UTF-8 values, and can't miss a var removed between the snapshot and the scrub. The contract test now compares the full environment, not only SOCKET_*. Refs #823 Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/common/hermetic.rs | 53 ++++++++----------- .../tests/spawn_env_hygiene.rs | 17 +++--- 2 files changed, 31 insertions(+), 39 deletions(-) diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index c2b95d49e..d28c6e101 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -135,34 +135,32 @@ pub fn tool_command(name: &str) -> Command { ) } -/// [`command`] for a PTY child: the same hermetic `SOCKET_*` environment -/// on a `portable_pty::CommandBuilder` for `program`. The PTY suites hand -/// the binary a real terminal, so they can't use `std::process::Command`; +/// [`command`] for a PTY child: the same hermetic environment on a +/// `portable_pty::CommandBuilder` for `program`. The PTY suites hand the +/// binary a real terminal, so they can't use `std::process::Command`; /// before this they each hand-rolled a scrub, and the copies drifted (one /// stripped the opt-outs, several didn't force them, none removed the /// flag-bound vars by name). `program` is usually the binary, or a shell /// that `exec`s it, which inherits the same environment. /// -/// `CommandBuilder::new` snapshots the process environment, so a var set -/// in-process before the snapshot and removed before [`command`]'s sweep -/// would survive in it; the snapshot is swept too. Callers add args, cwd -/// and env afterwards, as with [`command`]. +/// The builder's environment is replaced outright with what [`command`]'s +/// child would see. `CommandBuilder::new` snapshots the process +/// environment, so editing that snapshot would miss a var a parallel +/// in-process test removed between the snapshot and the scrub. Callers add +/// args, cwd and env afterwards, as with [`command`]. pub fn pty_command(program: impl AsRef) -> portable_pty::CommandBuilder { let program = program.as_ref(); - let mut pty = portable_pty::CommandBuilder::new(program); - let snapshot: Vec = pty - .iter_full_env_as_str() - .map(|(key, _)| key.to_owned()) - .filter(|key| is_scrubbed_socket_var(key)) - .collect(); - for key in snapshot { - pty.env_remove(key); - } + let mut env: std::collections::BTreeMap<_, _> = std::env::vars_os().collect(); for (key, value) in command(Path::new(program)).get_envs() { match value { - Some(value) => pty.env(key, value), - None => pty.env_remove(key), - } + Some(value) => env.insert(key.to_owned(), value.to_owned()), + None => env.remove(key), + }; + } + let mut pty = portable_pty::CommandBuilder::new(program); + pty.env_clear(); + for (key, value) in env { + pty.env(key, value); } pty } @@ -172,23 +170,18 @@ pub fn pty_binary_command() -> portable_pty::CommandBuilder { pty_command(env!("CARGO_BIN_EXE_socket-patch")) } -/// Whether [`scrub_socket_vars`] removes the var `name`: every `SOCKET_*` -/// except the telemetry opt-outs (an opted-out developer stays opted out), -/// `SOCKET_NO_CONFIG` and `SOCKET_NO_UPDATE_CHECK`. -fn is_scrubbed_socket_var(name: &str) -> bool { - name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - && name != "SOCKET_NO_UPDATE_CHECK" -} - /// Remove every ambient `SOCKET_*` var from `cmd`, except the telemetry /// opt-outs (an opted-out developer stays opted out), `SOCKET_NO_CONFIG` /// and `SOCKET_NO_UPDATE_CHECK`. [`command`] runs this; package-manager /// children call it directly. pub fn scrub_socket_vars(cmd: &mut Command) { for (key, _) in std::env::vars_os() { - if is_scrubbed_socket_var(&key.to_string_lossy()) { + let name = key.to_string_lossy(); + if name.starts_with("SOCKET_") + && !name.contains("TELEMETRY") + && name != "SOCKET_NO_CONFIG" + && name != "SOCKET_NO_UPDATE_CHECK" + { cmd.env_remove(&key); } } diff --git a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs index 786337e86..671c95895 100644 --- a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs +++ b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs @@ -285,20 +285,19 @@ fn command_drops_flag_bound_vars_set_after_it_is_built() { ); } -/// The `SOCKET_*` part of the environment a PTY child would see. -fn pty_socket_env(cmd: &portable_pty::CommandBuilder) -> BTreeMap { +/// The environment a PTY child would see. +fn pty_env(cmd: &portable_pty::CommandBuilder) -> BTreeMap { cmd.iter_full_env_as_str() - .filter(|(k, _)| k.starts_with("SOCKET_")) .map(|(k, v)| (k.to_owned(), v.to_owned())) .collect() } /// The PTY suites used to hand-roll their scrubs, and the copies drifted. -/// `pty_command` must give a PTY child exactly the `SOCKET_*` environment -/// `command` gives a piped one, whatever the ambient shell holds. +/// `pty_command` must give a PTY child exactly the environment `command` +/// gives a piped one, whatever the ambient shell holds. #[test] #[serial] -fn pty_command_carries_the_same_socket_env_as_command() { +fn pty_command_carries_the_same_env_as_command() { with_ambient( &[ ("SOCKET_YES", "true"), @@ -316,11 +315,11 @@ fn pty_command_carries_the_same_socket_env_as_command() { effective_env(&hermetic::command(Path::new("socket-patch"))) .into_iter() .filter_map(|(k, v)| Some((k.into_string().ok()?, v.into_string().ok()?))) - .filter(|(k, _)| k.starts_with("SOCKET_")) .collect(); - let pty = pty_socket_env(&hermetic::pty_command("socket-patch")); + let pty = pty_env(&hermetic::pty_command("socket-patch")); assert_eq!(pty, piped); assert_eq!(pty.get("SOCKET_YES"), None); + assert_eq!(pty.get("SOCKET_SOME_FUTURE_FLAG"), None); assert_eq!( pty.get("SOCKET_NO_UPDATE_CHECK").map(String::as_str), Some("1") @@ -337,7 +336,7 @@ fn pty_command_carries_the_same_socket_env_as_command() { #[serial] fn pty_command_never_leaks_its_hostile_seeds_and_keeps_caller_env() { let mut cmd = hermetic::pty_binary_command(); - let env = pty_socket_env(&cmd); + let env = pty_env(&cmd); for (key, _) in hermetic::HOSTILE_SEEDS { assert_eq!(env.get(*key), None, "{key} seed must be scrubbed"); } From 029fbc19562ac992704c777daf0396d1dc957157 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 11:36:34 +0000 Subject: [PATCH 4/4] Sweep the PTY env copy with the scrub rule pty_command and the hermetic::command plan each read the process environment, so a SOCKET_* var a parallel in-process test cleared between the two reads could survive in the PTY child's environment. Sweep the copied environment once more with the same rule scrub_socket_vars uses (Bugbot finding on #1530). Refs #823 Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/common/hermetic.rs | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index d28c6e101..bf4773463 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -144,10 +144,11 @@ pub fn tool_command(name: &str) -> Command { /// that `exec`s it, which inherits the same environment. /// /// The builder's environment is replaced outright with what [`command`]'s -/// child would see. `CommandBuilder::new` snapshots the process -/// environment, so editing that snapshot would miss a var a parallel -/// in-process test removed between the snapshot and the scrub. Callers add -/// args, cwd and env afterwards, as with [`command`]. +/// child would see. The copy here and [`command`]'s own sweep each read the +/// process environment, so a var a parallel in-process test clears between +/// the two reads would survive the plan; the copy is swept once more with +/// the same rule. Callers add args, cwd and env +/// afterwards, as with [`command`]. pub fn pty_command(program: impl AsRef) -> portable_pty::CommandBuilder { let program = program.as_ref(); let mut env: std::collections::BTreeMap<_, _> = std::env::vars_os().collect(); @@ -157,6 +158,7 @@ pub fn pty_command(program: impl AsRef) -> portable_pty::Comman None => env.remove(key), }; } + env.retain(|key, _| !is_scrubbed_socket_var(key)); let mut pty = portable_pty::CommandBuilder::new(program); pty.env_clear(); for (key, value) in env { @@ -176,17 +178,21 @@ pub fn pty_binary_command() -> portable_pty::CommandBuilder { /// children call it directly. pub fn scrub_socket_vars(cmd: &mut Command) { for (key, _) in std::env::vars_os() { - let name = key.to_string_lossy(); - if name.starts_with("SOCKET_") - && !name.contains("TELEMETRY") - && name != "SOCKET_NO_CONFIG" - && name != "SOCKET_NO_UPDATE_CHECK" - { + if is_scrubbed_socket_var(&key) { cmd.env_remove(&key); } } } +/// Whether [`scrub_socket_vars`] removes the var `key`. +fn is_scrubbed_socket_var(key: &std::ffi::OsStr) -> bool { + let name = key.to_string_lossy(); + name.starts_with("SOCKET_") + && !name.contains("TELEMETRY") + && name != "SOCKET_NO_CONFIG" + && name != "SOCKET_NO_UPDATE_CHECK" +} + /// The in-process half, for test binaries that only parse argv with clap: /// clap reads every env-bound `SOCKET_*` flag at parse time, so an ambient /// value (or a `.cargo/config.toml` `[env]` default such as