diff --git a/AGENTS.md b/AGENTS.md index ab20e377..1d422af1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -191,8 +191,27 @@ A change is not done without a test. Use the highest-fidelity tool per layer. which masks the empty-body bug the rule exists to catch. - Pin exact exit codes for the `CliError` contract; add a regression test for every fixed failure mode (exit code, message, missing-field tolerance). -- Use `temp_env::with_var` for env-dependent tests — never the unsound - process-global `std::env::set_var` (`unsafe_code = "forbid"` bans it anyway). +- **Nothing mutates the process environment, tests included.** Read it through + `mergify_core::env` (`var`, `var_os`, `var_non_empty`); give the code under + test one with `mergify_core::env::testing::with_vars`, which installs a + thread-local overlay and touches no global state. `clippy.toml` disallows + `std::env::var*`, `set_var` and `remove_var`, and `deny.toml` bans + `temp-env`. + + Why, in one line: `setenv` racing `getenv` on another thread is a + use-after-free, libtest runs tests on many threads, and the concurrent + reader is usually not even ours (`std::env::temp_dir` behind every + `tempfile::tempdir()`, `Command::spawn` building a child's environment). + `crates/mergify-core/src/env.rs` carries the full argument and the + overlay's limits — read it before reaching for an exception. + + Two consequences worth knowing before you write the test. An overlay **is** + the environment while installed, so a variable you do not list reads as + unset whatever the host exports: name what the case is about and nothing + else, and it behaves the same on a laptop and on a CI runner. And it covers + our reads only — a dependency, or a process you spawn, still sees the real + environment. Giving a **child** a variable is a different job with a + different tool, `Command::env`, and is untouched by any of this. ## Dependencies diff --git a/Cargo.lock b/Cargo.lock index fda3f33a..dc65e64c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1753,7 +1753,6 @@ dependencies = [ "mergify-tui", "serde", "serde_json", - "temp-env", "tempfile", "tokio", "tracing", @@ -1811,7 +1810,6 @@ dependencies = [ "serde_json", "serde_yaml_ng", "sha2 0.11.0", - "temp-env", "tempfile", "tokio", "tracing", @@ -3100,15 +3098,6 @@ dependencies = [ "syn 2.0.117", ] -[[package]] -name = "temp-env" -version = "0.3.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "96374855068f47402c3121c6eed88d29cb1de8f3ab27090e273e420bdabcf050" -dependencies = [ - "parking_lot", -] - [[package]] name = "tempfile" version = "3.27.0" diff --git a/Cargo.toml b/Cargo.toml index e104e3f8..0ca501db 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -72,7 +72,6 @@ tracing-subscriber = { version = "0.3", default-features = false, features = ["f unicode-width = "0.2" url = "2" insta = { version = "1", features = ["json", "redactions"] } -temp-env = "0.3" wiremock = "0.6" [workspace.lints.rust] diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 00000000..19891017 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,27 @@ +# The process environment is read through `mergify_core::env` and +# written nowhere. `crates/mergify-core/src/env.rs` carries the +# argument; these entries carry the replacement. +# +# `set_var` / `remove_var` are `unsafe fn`, so the workspace's +# `unsafe_code = "forbid"` is what actually stops a mutation — these +# two lines are documentation, and a contributor who relaxes that lint +# is unlocking more than they think. `set_current_dir` is the safe +# process-global mutator, banned for the same reason: it races every +# relative path in every other test thread. +# +# `std::env::args` and `current_exe` are not the environment and are +# not listed. `temp_dir` is a `getenv` for `TMPDIR` and is unlisted +# for a different reason: it takes no variable name, so there is +# nothing to route through the funnel, and it is a read, which was +# never the unsound half. A build script reads cargo's environment, +# not the process's, and cannot depend on `mergify-core`; +# `crates/mergify-cli/build.rs` allows the lint in place. +disallowed-methods = [ + { path = "std::env::var", reason = "read it through `mergify_core::env::var` / `var_non_empty`" }, + { path = "std::env::var_os", reason = "read it through `mergify_core::env::var_os` / `var_os_non_empty`" }, + { path = "std::env::vars", reason = "read it through `mergify_core::env`; iterating the whole environment is only right when building a child process's, and needs an explicit allow" }, + { path = "std::env::vars_os", reason = "read it through `mergify_core::env`; iterating the whole environment is only right when building a child process's, and needs an explicit allow" }, + { path = "std::env::set_var", reason = "nothing mutates the process environment; give the code under test one with `mergify_core::env::testing::with_vars`" }, + { path = "std::env::remove_var", reason = "nothing mutates the process environment; give the code under test one with `mergify_core::env::testing::with_vars`" }, + { path = "std::env::set_current_dir", reason = "process-global, so it races every relative path in every other test thread; take the directory as a parameter" }, +] diff --git a/crates/mergify-auth/Cargo.toml b/crates/mergify-auth/Cargo.toml index 60d09e4b..7bba6753 100644 --- a/crates/mergify-auth/Cargo.toml +++ b/crates/mergify-auth/Cargo.toml @@ -25,7 +25,6 @@ url = { workspace = true } mergify-core = { path = "../mergify-core", features = ["test-support"] } mergify-test-support = { path = "../mergify-test-support" } serde_json = { workspace = true } -temp-env = { workspace = true } tempfile = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread"] } wiremock = { workspace = true } diff --git a/crates/mergify-auth/src/browser.rs b/crates/mergify-auth/src/browser.rs index 827328d1..c5776570 100644 --- a/crates/mergify-auth/src/browser.rs +++ b/crates/mergify-auth/src/browser.rs @@ -231,7 +231,7 @@ mod tests { #[cfg(target_os = "macos")] #[test] fn macos_opens_the_url_with_open() { - let command = temp_env::with_vars( + let command = mergify_core::env::testing::with_vars( [("SSH_CONNECTION", None::<&str>), ("SSH_TTY", None::<&str>)], || command_for("https://dashboard.mergify.com/device"), ) @@ -246,7 +246,7 @@ mod tests { #[cfg(all(unix, not(target_os = "macos")))] #[test] fn a_graphical_session_gets_xdg_open() { - let command = temp_env::with_vars( + let command = mergify_core::env::testing::with_vars( [("DISPLAY", Some(":0")), ("WAYLAND_DISPLAY", None::<&str>)], || command_for("https://dashboard.mergify.com/device"), ) @@ -263,7 +263,7 @@ mod tests { #[cfg(target_os = "macos")] #[test] fn an_ssh_session_to_a_mac_opens_nothing() { - let opened = temp_env::with_vars( + let opened = mergify_core::env::testing::with_vars( [ ("SSH_CONNECTION", Some("10.0.0.1 52000 10.0.0.2 22")), ("SSH_TTY", None), @@ -281,7 +281,7 @@ mod tests { #[cfg(all(unix, not(target_os = "macos")))] #[test] fn a_headless_session_opens_nothing() { - let opened = temp_env::with_vars( + let opened = mergify_core::env::testing::with_vars( [("DISPLAY", None::<&str>), ("WAYLAND_DISPLAY", None::<&str>)], || command_for("https://dashboard.mergify.com/device").is_ok(), ); diff --git a/crates/mergify-auth/src/lib.rs b/crates/mergify-auth/src/lib.rs index dc4e4eb7..863f7186 100644 --- a/crates/mergify-auth/src/lib.rs +++ b/crates/mergify-auth/src/lib.rs @@ -41,17 +41,18 @@ mod testing { /// Run `body` to completion with `MERGIFY_TOKEN` forced to /// `value`. /// - /// `temp_env` cannot wrap an `.await`, so the future is driven - /// inside the closure instead. Without this the wiring that - /// reads the variable is untestable, and untestable wiring is - /// wiring a future edit can delete with the suite still green: - /// asserting on the renderer alone proves only that the renderer - /// can print a note, never that anything asks it to. + /// The overlay is installed on this thread and the future is + /// driven on it, by a `current_thread` runtime built here. + /// Without this the wiring that reads the variable is + /// untestable, and untestable wiring is wiring a future edit can + /// delete with the suite still green: asserting on the renderer + /// alone proves only that the renderer can print a note, never + /// that anything asks it to. pub fn with_mergify_token(value: Option<&str>, body: F) -> F::Output { let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() .unwrap(); - temp_env::with_var("MERGIFY_TOKEN", value, || runtime.block_on(body)) + mergify_core::env::testing::with_var("MERGIFY_TOKEN", value, || runtime.block_on(body)) } } diff --git a/crates/mergify-auth/src/machine.rs b/crates/mergify-auth/src/machine.rs index ec730038..1672b077 100644 --- a/crates/mergify-auth/src/machine.rs +++ b/crates/mergify-auth/src/machine.rs @@ -111,7 +111,7 @@ mod tests { // the variable instead. #[test] fn the_variables_are_the_fallback() { - let from_windows = temp_env::with_vars( + let from_windows = mergify_core::env::testing::with_vars( [ ("COMPUTERNAME", Some("WIN-BOX")), ("HOSTNAME", Some("ignored")), @@ -120,13 +120,13 @@ mod tests { ); assert_eq!(from_windows.as_deref(), Some("WIN-BOX")); - let from_shell = temp_env::with_vars( + let from_shell = mergify_core::env::testing::with_vars( [("COMPUTERNAME", None), ("HOSTNAME", Some("build-42"))], from_env, ); assert_eq!(from_shell.as_deref(), Some("build-42")); - let from_nothing = temp_env::with_vars( + let from_nothing = mergify_core::env::testing::with_vars( [("COMPUTERNAME", None::<&str>), ("HOSTNAME", None)], from_env, ); diff --git a/crates/mergify-cli/Cargo.toml b/crates/mergify-cli/Cargo.toml index f10d5762..3bbdf97d 100644 --- a/crates/mergify-cli/Cargo.toml +++ b/crates/mergify-cli/Cargo.toml @@ -57,7 +57,6 @@ url = { workspace = true } insta = { workspace = true } regex = { workspace = true } serde_yaml_ng = { workspace = true } -temp-env = { workspace = true } tempfile = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread"] } wiremock = { workspace = true } diff --git a/crates/mergify-cli/build.rs b/crates/mergify-cli/build.rs index 45c22235..7ec0db2e 100644 --- a/crates/mergify-cli/build.rs +++ b/crates/mergify-cli/build.rs @@ -14,6 +14,11 @@ fn main() { // Rebuild when the env var changes so a release rebuild after a // dev build actually picks up the new value. println!("cargo:rerun-if-env-changed=MERGIFY_RELEASE_VERSION"); + // A build script's environment is cargo's, handed to it for this + // one invocation — not the process environment `clippy.toml` + // guards, and not reachable through `mergify_core::env`, which a + // build script cannot depend on. + #[allow(clippy::disallowed_methods)] let resolved = std::env::var("MERGIFY_RELEASE_VERSION") .ok() .filter(|v| !v.is_empty()) diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index 42c6daa4..3fbe63f0 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -8,7 +8,7 @@ //! ``?" suggestion off clap's built-in Levenshtein //! distance. -use std::env; +use mergify_core::env; use std::io::IsTerminal; use std::path::PathBuf; use std::process::ExitCode; @@ -53,7 +53,7 @@ mod self_update; const VERSION: &str = env!("MERGIFY_CLI_VERSION"); fn main() -> ExitCode { - let argv: Vec = env::args().skip(1).collect(); + let argv: Vec = std::env::args().skip(1).collect(); // Test hook used by `test_binary_build.py` to verify the // wheel-installed binary produces UTF-8 output (especially on @@ -2812,12 +2812,12 @@ enum ColorArg { /// `true` when `name` is exported to something other than the empty /// string, which is what means by "present". /// -/// The rule itself lives in `mergify_core::env::var_non_empty`, which -/// is where it is documented and tested; spelling it out a second time -/// here is how the color variables would drift away from every other -/// variable this CLI reads. +/// The rule itself lives in `mergify_core::env`, which is where it is +/// documented and tested; spelling it out a second time here is how +/// the color variables would drift away from every other variable +/// this CLI reads. fn non_empty(name: &str) -> bool { - mergify_core::env::var_non_empty(name).is_some() + env::var_os_non_empty(name).is_some() } /// Whether the log subscriber may emit ANSI on stderr. @@ -3984,8 +3984,9 @@ struct ScopesCliArgs { // `mergify_ci::scopes_detect::resolve_config_path` instead, // where empty correctly falls through to auto-detect. The // matching regression tests are - // `ci_scopes_parses_when_mergify_config_path_env_var_is_empty` - // (clap parse) and + // `no_argument_takes_its_value_from_the_environment` (asks the + // built `Command` whether *any* argument carries an `env` + // attribute, which covers this one) and // `resolve_config_path_treats_empty_env_var_as_unset` // (lower-level resolver). #[arg(long)] @@ -4897,72 +4898,75 @@ mod tests { // or test the terminal the suite happens to run under. } + /// No argument anywhere in the tree may take its value from the + /// environment through clap's `env = "…"` attribute. + /// + /// Twice now that attribute broke a caller who exports the + /// variable empty. `gha-mergify-ci` sets `MERGIFY_CONFIG_PATH=""` + /// when the user pinned no path, and clap read the empty string + /// as a present-but-empty `--config`, aborting with "a value is + /// required for '--config'" (monorepo#33423). Same shape for + /// `MERGIFY_TEST_EXIT_CODE=""` and `--test-exit-code`: "cannot + /// parse integer from empty string". Env lookup belongs in the + /// resolver, where `mergify_core::env::var_non_empty` treats + /// empty as unset. + /// + /// This replaces two tests that each exported one variable empty + /// and parsed one argv. Asking the built `Command` covers every + /// argument rather than those two, and needs no process + /// environment to mutate. #[test] - fn ci_scopes_parses_when_mergify_config_path_env_var_is_empty() { - // Regression for monorepo#33423 / gha-mergify-ci: - // the action sets `MERGIFY_CONFIG_PATH=""` (empty) when - // the caller didn't pin a config path, expecting - // auto-detect. The previous `ScopesCliArgs::config` - // declaration used `env = "MERGIFY_CONFIG_PATH"` on - // clap's side, which interpreted the empty env value as - // a present-but-empty `--config` flag and exited parsing - // with `a value is required for '--config'`. The clap - // env hook has been dropped — env lookup lives inside - // `scopes_detect::resolve_config_path` where empty is - // correctly treated as unset. Pin that here so the hook - // can't sneak back in. - let parsed = temp_env::with_var("MERGIFY_CONFIG_PATH", Some(""), || { - CliRoot::try_parse_from([ - "mergify".to_string(), - "ci".to_string(), - "scopes".to_string(), - "--write".to_string(), - "scopes.json".to_string(), - ]) - .expect("argv parses with empty MERGIFY_CONFIG_PATH") - }); + fn no_argument_takes_its_value_from_the_environment() { + fn walk(cmd: &clap::Command, path: &str, found: &mut Vec) { + for arg in cmd.get_arguments() { + if let Some(var) = arg.get_env() { + found.push(format!( + "{path} {} <- {}", + arg.get_id(), + var.to_string_lossy() + )); + } + } + for sub in cmd.get_subcommands() { + walk(sub, &format!("{path} {}", sub.get_name()), found); + } + } + + let mut found = Vec::new(); + walk(&CliRoot::command(), "mergify", &mut found); + assert!(found.is_empty(), "clap env hooks found: {found:#?}"); + } + + /// The observable half of the rule above, for the two arguments + /// it was reported on. + /// + /// The walk asks clap whether an `env = "…"` hook is declared, + /// which is the spelling that caused both regressions but not the + /// only one: `default_value_t = std::env::var(…).unwrap_or_default()` + /// or a `value_parser` that reads the environment reproduce it + /// exactly and declare no hook. This asserts what the user sees + /// instead. It needs no environment of its own — with the + /// variable unset, any of those spellings still surfaces a + /// present-but-empty value where `None` is required. + #[test] + fn an_omitted_flag_stays_omitted() { + let parsed = CliRoot::try_parse_from(["mergify", "ci", "scopes", "--write", "scopes.json"]) + .expect("argv parses"); let Dispatch::Native(NativeCommand::CiScopes(opts)) = dispatch_from_parsed(parsed) else { panic!("ci scopes must dispatch natively"); }; - // `--config` was never supplied; the empty env var must - // not surface as a value (which would change the - // downstream resolver's branch). assert!(opts.config.is_none(), "got: {:?}", opts.config); - } - #[test] - fn ci_junit_process_parses_when_mergify_test_exit_code_env_var_is_empty() { - // Second instance of the same class of regression as - // `ci_scopes_parses_when_…`: `gha-mergify-ci` exports - // `MERGIFY_TEST_EXIT_CODE=""` when the previous step - // didn't produce a runner exit code. Previously the clap - // `env = "MERGIFY_TEST_EXIT_CODE"` attribute on - // `--test-exit-code` tried to parse `""` as `i32` and - // exited parsing with `invalid value '' for - // '--test-exit-code': cannot parse integer from empty - // string`. The clap env hook has been dropped — env - // lookup lives in `junit_process::command::resolve_test_exit_code` - // where empty is correctly treated as `None`. Pin that - // here so the hook can't sneak back in. - let parsed = temp_env::with_var("MERGIFY_TEST_EXIT_CODE", Some(""), || { - CliRoot::try_parse_from([ - "mergify".to_string(), - "ci".to_string(), - "junit-process".to_string(), - "report.xml".to_string(), - ]) - .expect("argv parses with empty MERGIFY_TEST_EXIT_CODE") - }); + let parsed = CliRoot::try_parse_from(["mergify", "ci", "junit-process", "report.xml"]) + .expect("argv parses"); let Dispatch::Native(NativeCommand::CiJunitProcess(opts)) = dispatch_from_parsed(parsed) else { panic!("ci junit-process must dispatch natively"); }; - // `--test-exit-code` was never supplied; the empty env - // var must not surface as a value. assert!( opts.test_exit_code.is_none(), "got: {:?}", - opts.test_exit_code, + opts.test_exit_code ); } diff --git a/crates/mergify-cli/src/self_update.rs b/crates/mergify-cli/src/self_update.rs index 7c0b0000..f3e96d03 100644 --- a/crates/mergify-cli/src/self_update.rs +++ b/crates/mergify-cli/src/self_update.rs @@ -35,6 +35,7 @@ use std::path::Path; use std::time::Duration; use mergify_core::CliError; +use mergify_core::env; use serde::Deserialize; use sha2::Digest; use sha2::Sha256; @@ -58,7 +59,7 @@ const BASE_URL_ENV: &str = "MERGIFY_BASE_URL"; /// read from the response, never reconstructed, so we only need to /// know where the metadata lives. fn latest_release_url() -> String { - if let Ok(base) = std::env::var(BASE_URL_ENV) { + if let Some(base) = env::var_non_empty(BASE_URL_ENV) { format!("{base}/latest-release.json") } else { format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") @@ -443,6 +444,40 @@ mod tests { } } + #[test] + fn latest_release_url_falls_back_when_the_base_url_is_empty() { + // `gha-mergify-ci` exports unset variables as `""`, so an + // empty `MERGIFY_BASE_URL` must mean "no fixture" and not + // "fetch from `/latest-release.json`". This is the + // `var_non_empty` half of the empty-string rule; the + // overlay is what lets the case be written at all, since + // nothing may mutate the process environment. + let url = env::testing::with_var(BASE_URL_ENV, Some(""), latest_release_url); + assert_eq!( + url, + format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") + ); + } + + #[test] + fn latest_release_url_uses_a_non_empty_base_url() { + let url = env::testing::with_var( + BASE_URL_ENV, + Some("https://example.test"), + latest_release_url, + ); + assert_eq!(url, "https://example.test/latest-release.json"); + } + + #[test] + fn latest_release_url_defaults_when_the_base_url_is_unset() { + let url = env::testing::with_no_vars(latest_release_url); + assert_eq!( + url, + format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") + ); + } + #[test] fn select_asset_matches_versioned_name() { let assets = [ diff --git a/crates/mergify-cli/tests/live_smoke.rs b/crates/mergify-cli/tests/live_smoke.rs index 67f993f6..c0859e9b 100644 --- a/crates/mergify-cli/tests/live_smoke.rs +++ b/crates/mergify-cli/tests/live_smoke.rs @@ -109,8 +109,18 @@ fn cli_with(args: &[&str], extra_env: &[(&str, &str)], cwd: Option<&Path>) -> Cl .stdout(Stdio::piped()) .stderr(Stdio::piped()) .env_clear(); - for (k, v) in std::env::vars() { - if !scrub.contains(k.as_str()) { + // The one place the whole environment is read on purpose: this + // builds the *child's* environment out of ours, minus the CI + // variables a runner exports. Not the process-wide read the lint + // is aimed at. + // + // `vars_os`, not `vars`: the latter panics on a single variable + // that is not valid Unicode, and one Latin-1 export on the host + // would take down every case here before it spawned anything. + // `Command::env` takes `OsStr`, so nothing is lost. + #[allow(clippy::disallowed_methods)] + for (k, v) in std::env::vars_os() { + if !k.to_str().is_some_and(|k| scrub.contains(k)) { cmd.env(k, v); } } @@ -185,16 +195,21 @@ fn wait_timeout( } } +/// A token from the environment, or `None` when it is unset, empty, +/// or whitespace — the CI secret is empty on a fork's build, and a +/// stray newline in a locally exported one would otherwise be sent +/// as the bearer. +fn non_blank_env(name: &str) -> Option { + let value = mergify_core::env::var(name)?.trim().to_string(); + (!value.is_empty()).then_some(value) +} + /// Look up `LIVE_TEST_MERGIFY_TOKEN_CI`, the key scoped to what a /// CI job does. Empty / unset = skip the test (early return with /// `SKIP:` printed to stderr so the cargo test log shows what was /// skipped). fn live_token() -> Option { - let token = std::env::var("LIVE_TEST_MERGIFY_TOKEN_CI") - .unwrap_or_default() - .trim() - .to_string(); - (!token.is_empty()).then_some(token) + non_blank_env("LIVE_TEST_MERGIFY_TOKEN_CI") } /// Token for endpoints the CI-scoped key cannot reach: the @@ -210,11 +225,7 @@ fn live_token() -> Option { /// details fetch. A rename or wire-format drift on that route /// still ships green. fn live_admin_token() -> Option { - let token = std::env::var("LIVE_TEST_MERGIFY_TOKEN_ADMIN") - .unwrap_or_default() - .trim() - .to_string(); - (!token.is_empty()).then_some(token) + non_blank_env("LIVE_TEST_MERGIFY_TOKEN_ADMIN") } /// 8 random hex-ish chars, so concurrent or repeated runs never diff --git a/deny.toml b/deny.toml index 43158f36..9c276ba6 100644 --- a/deny.toml +++ b/deny.toml @@ -16,6 +16,15 @@ multiple-versions = "warn" # requirement and would register as wildcards, so exempt them. wildcards = "deny" allow-wildcard-paths = true +# `unsafe_code = "forbid"` stops our own code calling `setenv`, but it +# does not reach into a dependency that wraps it in a safe API. This +# blocks the one we used. It is a crate name, not a property — +# cargo-deny cannot express "nothing that calls setenv" — so a +# different wrapper would still get through; the rule itself lives in +# AGENTS.md. +deny = [ + { crate = "temp-env", reason = "use mergify_core::env::testing::with_vars — see crates/mergify-core/src/env.rs" }, +] [licenses] # SPDX identifiers we accept for anything we redistribute. Keep this