diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index 3b5fba8b1..df1087ffb 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -952,11 +952,14 @@ async fn run_redirect_selected_untracked( // the workspace root's pnpm-workspace.yaml, unless a // `pnpm_config_trust_lockfile` in this environment overrides that file // (#1514): adding the key there would not help, and the rewrite's - // trust warning names the variable instead. - let trust_env_off = socket_patch_core::utils::pnpm_workspace::trust_lockfile_env(None, |var| { - std::env::var(var).ok() - }) - .is_some_and(|found| found.as_bool() != Some(true)); + // trust warning names the variable instead. The variable is read at + // the project's pnpm major, as the rewrite reads it (#1557). + let trust_env_off = + socket_patch_core::utils::pnpm_workspace::trust_lockfile_env_for(&view, |var| { + std::env::var(var).ok() + }) + .await + .is_some_and(|found| found.as_bool() != Some(true)); if let Some(refusal) = socket_patch_core::hosted::governing_root::refusal( &view, &candidates, diff --git a/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs index 2303108a0..ebaf1109f 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs @@ -172,6 +172,14 @@ fn corepack(cwd: &Path, pm: &str, args: &[&str]) -> Output { /// registry.npmjs.org, where a clamp turned a single `read ECONNRESET` /// into a failed required leg. fn corepack_with_retries(cwd: &Path, pm: &str, args: &[&str], registry_retries: bool) -> Output { + corepack_command(cwd, pm, args, registry_retries) + .output() + .expect("failed to run corepack") +} + +/// The command [`corepack_with_retries`] runs, for a caller that adds +/// environment after the hermetic scrub. +fn corepack_command(cwd: &Path, pm: &str, args: &[&str], registry_retries: bool) -> Command { let mut cmd = pnpm_command(pm); let legacy = pm .strip_prefix("pnpm@") @@ -211,7 +219,7 @@ fn corepack_with_retries(cwd: &Path, pm: &str, args: &[&str], registry_retries: // would otherwise take the sandbox values back out again. cache_env::isolate(&mut cmd); cmd.env("COREPACK_ENABLE_DOWNLOAD_PROMPT", "0"); - cmd.output().expect("failed to run corepack") + cmd } fn run_socket(cwd: &Path, args: &[&str]) -> (i32, String, String) { @@ -818,6 +826,19 @@ fn fresh_checkout_install( label: &str, extra_args: &[&str], with_workspace_yaml: bool, +) -> (PathBuf, Output) { + fresh_checkout_install_env(fx, pm, label, extra_args, with_workspace_yaml, &[]) +} + +/// [`fresh_checkout_install`] with `env` set on the install, after the +/// hermetic scrub. +fn fresh_checkout_install_env( + fx: &PnpmRedirectFixture, + pm: &str, + label: &str, + extra_args: &[&str], + with_workspace_yaml: bool, + env: &[(&str, &str)], ) -> (PathBuf, Output) { let fresh = fx.tmp.path().join(format!("fresh-{label}")); std::fs::create_dir_all(&fresh).unwrap(); @@ -851,7 +872,9 @@ fn fresh_checkout_install( let store_flag = format!("--store-dir={}", fresh_store.display()); let mut args = vec!["install", "--frozen-lockfile", &store_flag]; args.extend_from_slice(extra_args); - let out = corepack(&fresh, pm, &args); + let mut cmd = corepack_command(&fresh, pm, &args, false); + cmd.envs(env.iter().copied()); + let out = cmd.output().expect("failed to run corepack"); (fresh, out) } @@ -1428,6 +1451,84 @@ async fn pnpm11_trust_lockfile_env_override_is_respected() { assert!(workspace.contains("trustLockfile: true"), "{workspace}"); } +/// #1557: pnpm 11+ ignore a `pnpm_config_trust_lockfile` that is not +/// exactly `true` or `false`, such as the empty string `${{ vars.X }}` +/// exports when the repo variable is unset, and read pnpm-workspace.yaml +/// instead. A scan whose +/// environment sets it so must heal the trust config as if it were unset, +/// and a fresh frozen install in that same environment then lands the +/// patched bytes from the written key. +#[tokio::test(flavor = "multi_thread")] +#[serial_test::serial] +#[ignore = "wall-bound real-pnpm install (~60s); runs on all 3 OSes as an e2e CI matrix leg"] +async fn pnpm11_empty_trust_lockfile_env_is_ignored() { + let Some(fx) = redirect_scanned_pnpm_project( + PNPM_TERTIARY, + "pnpm11-trust-env-empty", + false, + true, + HostedDriver::Scan, + ) + .await + else { + return; + }; + let ws_path = fx.proj.join("pnpm-workspace.yaml"); + assert!( + !ws_path.exists(), + "the opted-out fixture scan wrote no workspace file" + ); + + let api_url = fx._server.uri(); + let args = [ + "scan", + "--mode", + "hosted", + "--json", + "--yes", + "--cwd", + fx.proj.to_str().unwrap(), + "--api-url", + &api_url, + "--org", + ORG, + "--api-token", + "fake", + ]; + let env = [("pnpm_config_trust_lockfile".to_string(), String::new())]; + let (code, stdout, stderr) = run_socket_env(&fx.proj, &args, &env); + assert_eq!(code, 0, "stdout:\n{stdout}\nstderr:\n{stderr}"); + let envelope = parse_envelope(&stdout); + let workspace = std::fs::read_to_string(&ws_path).unwrap_or_default(); + assert!( + workspace.contains("trustLockfile: true"), + "an empty variable pnpm ignores must not stop the write:\n{envelope}" + ); + let detail = envelope["warnings"] + .as_array() + .into_iter() + .flatten() + .find(|w| w["code"] == "redirect_pnpm_trust_lockfile") + .and_then(|w| w["detail"].as_str()) + .unwrap_or_else(|| panic!("redirect_pnpm_trust_lockfile:\n{envelope}")) + .to_string(); + assert!( + detail.contains("no extra flags") && !detail.contains("in the environment"), + "the warning must not claim the empty variable was respected:\n{detail}" + ); + + // pnpm itself, run with the same empty variable, honours the key. + let (fresh, ci) = fresh_checkout_install_env( + &fx, + PNPM_TERTIARY, + "pnpm11-trust-env-empty", + &[], + true, + &[("pnpm_config_trust_lockfile", "")], + ); + assert_marker_landed(&fresh, &fx.patched, &ci, "pnpm11 empty trust env"); +} + /// Legacy pnpm@7 leg: the fixture install emits a lockfileVersion 5.4 lock /// (`/name/version:` path-style key), the scan splices its resolution like /// any other grammar, and the fresh dead-registry frozen install proves diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index f443bc187..af2a6f0ad 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -4228,6 +4228,35 @@ snapshots: ); } + /// #1557: pnpm 11+ ignore a `pnpm_config_trust_lockfile` (or + /// `PNPM_CONFIG_TRUST_LOCKFILE`) that is not exactly `true` or `false` + /// (an unset CI variable's empty string, `null`, `undefined`) and read + /// pnpm-workspace.yaml instead, so the scan writes `trustLockfile: + /// true` as if the variable were unset, and the warning does not claim + /// the variable was respected. + #[tokio::test] + async fn issue_1557_an_empty_trust_lockfile_variable_keeps_the_write() { + let mut root = MemoryProject::new(); + root.insert_text("pnpm-lock.yaml", LEFT_PAD_V9_LOCK); + for var in ["pnpm_config_trust_lockfile", "PNPM_CONFIG_TRUST_LOCKFILE"] { + for value in ["", "null", "undefined"] { + let env = move |name: &str| (name == var).then(|| value.to_string()); + let (_, done) = + npm_rewrite_in(&ProjectView::Memory(&root), &BTreeSet::new(), &env).await; + assert!(done.rewrite.files.contains_key("pnpm-lock.yaml")); + let workspace = done + .rewrite + .files + .get(PNPM_WORKSPACE_REL) + .unwrap_or_else(|| panic!("{:?}", done.rewrite.files.keys())); + assert!(workspace.contains("trustLockfile: true"), "{workspace}"); + let detail = pnpm_trust_detail(&done); + assert!(detail.contains("no extra flags"), "{detail}"); + assert!(!detail.contains("in the environment"), "{detail}"); + } + } + } + /// REGRESSION (#367), binary lock: a `bun.lockb`-only project's root /// manifest is read for its `patchedDependencies`, and a package the /// project patches itself with `bun patch` keeps its registry record, diff --git a/crates/socket-patch-core/src/utils/pnpm_workspace.rs b/crates/socket-patch-core/src/utils/pnpm_workspace.rs index 651470dec..b12928665 100644 --- a/crates/socket-patch-core/src/utils/pnpm_workspace.rs +++ b/crates/socket-patch-core/src/utils/pnpm_workspace.rs @@ -143,10 +143,11 @@ fn env_setting( Some(_) => &["npm_config_"], None => &["pnpm_config_", "npm_config_"], }; - prefixes - .iter() - .map(|prefix| format!("{prefix}{key}")) - .find_map(|var| { + prefixes.iter().find_map(|prefix| { + let var = format!("{prefix}{key}"); + if *prefix == "pnpm_config_" { + pnpm_env_setting(&var, major, &env) + } else { let value = env(&var)?.trim().to_string(); let source = format!("`{var}={value}` in the environment"); Some(PnpmSetting { @@ -154,13 +155,45 @@ fn env_setting( source, yaml: false, }) - }) + } + }) +} + +/// A boolean setting from `var`, a variable of the `pnpm_config_*` family +/// (either case) that pnpm 11 and newer read for a project on pnpm +/// `major`. pnpm takes only `true` or `false` there and ignores any other +/// value (empty, `null`, `undefined`, `0`, `FALSE`), reading the settings +/// file instead (#1557), so such a value sets nothing. pnpm 12 trims the +/// value first (` false` is false); pnpm 11 does not (` false` is +/// ignored). With the major unknown the value is trimmed, so a padded +/// `false` is respected: for the lock that withholds pins, the safe side. +fn pnpm_env_setting( + var: &str, + major: Option, + env: impl Fn(&str) -> Option, +) -> Option { + let raw = env(var)?; + let value = if major == Some(11) { + raw.as_str() + } else { + raw.trim() + }; + if !matches!(value, "true" | "false") { + return None; + } + let value = value.to_string(); + Some(PnpmSetting { + source: format!("`{var}={value}` in the environment"), + value, + yaml: false, + }) } /// pnpm's `trustLockfile` as the environment sets it, for a project on /// pnpm `major` (#1514): `pnpm_config_trust_lockfile`, or its uppercase -/// spelling, which pnpm 11 and newer read ahead of pnpm-workspace.yaml. -/// `None` on pnpm 10 and older, which never verify lock URLs, and for +/// spelling, which pnpm 11 and newer read ahead of pnpm-workspace.yaml +/// when it is `true` or `false` (see [`pnpm_env_setting`]). `None` on pnpm +/// 10 and older, which never verify lock URLs, and for /// `npm_config_trust_lockfile`, which no pnpm reads. pub fn trust_lockfile_env( major: Option, @@ -170,15 +203,20 @@ pub fn trust_lockfile_env( return None; } // pnpm 11's spelling, then the uppercase one pnpm 12 reads too. - env_setting("trust-lockfile", Some(major.unwrap_or(11)), &env).or_else(|| { - let var = "PNPM_CONFIG_TRUST_LOCKFILE"; - let value = env(var)?.trim().to_string(); - Some(PnpmSetting { - source: format!("`{var}={value}` in the environment"), - value, - yaml: false, - }) - }) + pnpm_env_setting("pnpm_config_trust_lockfile", major, &env) + .or_else(|| pnpm_env_setting("PNPM_CONFIG_TRUST_LOCKFILE", major, &env)) +} + +/// [`trust_lockfile_env`] for the project `view` shows, at the pnpm major +/// that installs it: whether pnpm 11 ignores a padded value or pnpm 12 +/// trims it depends on the major. The project's files are read for it only +/// when the environment sets the variable at all. +pub async fn trust_lockfile_env_for( + view: &ProjectView<'_>, + env: impl Fn(&str) -> Option, +) -> Option { + trust_lockfile_env(None, &env)?; + trust_lockfile_env(pnpm_major(view).await, &env) } /// [`pnpm_setting`], falling back to the environment ([`env_setting`], @@ -939,7 +977,7 @@ mod tests { None, env(&[ ("npm_config_git_branch_lockfile", "false"), - ("pnpm_config_git_branch_lockfile", "TRUE"), + ("pnpm_config_git_branch_lockfile", "true"), ]), ) .unwrap(); @@ -953,7 +991,7 @@ mod tests { #[test] fn the_environment_sets_trust_lockfile_only_for_pnpm_11_and_newer() { const LOWER: &[(&str, &str)] = &[("pnpm_config_trust_lockfile", "false")]; - const UPPER: &[(&str, &str)] = &[("PNPM_CONFIG_TRUST_LOCKFILE", " false ")]; + const UPPER: &[(&str, &str)] = &[("PNPM_CONFIG_TRUST_LOCKFILE", "false")]; const NPM: &[(&str, &str)] = &[("npm_config_trust_lockfile", "false")]; for major in [Some(11), Some(12), None] { let found = trust_lockfile_env(major, env(LOWER)).expect("lowercase"); @@ -972,6 +1010,122 @@ mod tests { ); } + /// #1557: pnpm 11 and newer read a `pnpm_config_*` boolean only when it + /// is `true` or `false` and otherwise ignore it for the settings file + /// (measured with `pnpm config get` on pnpm 11.28.5 and 12.11.2), so any + /// other value (an unset CI variable's empty string, `null`, + /// `undefined`, `0`, `FALSE`) sets nothing, for `trustLockfile` and + /// every other key alike. pnpm 12 trims the value first and pnpm 11 + /// does not, so a padded ` false` is ignored only on pnpm 11; with the + /// major unknown it is respected. npm's family keeps its own coercion + /// (pnpm 10 and older, see [`PnpmSetting::as_bool`]). + #[test] + fn a_pnpm_config_variable_counts_only_as_true_or_false() { + let one = |var: &'static str, value: &'static str| { + move |name: &str| (name == var).then(|| value.to_string()) + }; + const IGNORED: &[&str] = &["", " ", "null", "undefined", "0", "FALSE", "yes"]; + const PADDED: &[&str] = &[" false", "false ", " false "]; + for var in ["pnpm_config_trust_lockfile", "PNPM_CONFIG_TRUST_LOCKFILE"] { + for value in IGNORED { + for major in [Some(11), Some(12), None] { + assert_eq!( + trust_lockfile_env(major, one(var, value)), + None, + "{var}={value:?} on pnpm {major:?}" + ); + } + } + for (value, on) in [("true", true), ("false", false)] { + let found = trust_lockfile_env(Some(11), one(var, value)).unwrap(); + assert_eq!(found.as_bool(), Some(on), "{var}={value}"); + } + for value in PADDED { + assert_eq!(trust_lockfile_env(Some(11), one(var, value)), None); + for major in [Some(12), None] { + let found = trust_lockfile_env(major, one(var, value)).unwrap(); + assert_eq!(found.as_bool(), Some(false), "{var}={value:?}"); + assert!(found.source.contains(&format!("`{var}=false`"))); + } + } + } + for key in [ + "lockfile", + "shared-workspace-lockfile", + "git-branch-lockfile", + ] { + let var: &'static str = + Box::leak(format!("pnpm_config_{}", key.replace('-', "_")).into_boxed_str()); + for value in IGNORED { + for major in [Some(11), None] { + assert_eq!( + env_setting(key, major, one(var, value)), + None, + "{var}={value:?}" + ); + } + } + let found = env_setting(key, Some(11), one(var, "false")).unwrap(); + assert_eq!(found.as_bool(), Some(false), "{var}"); + for value in PADDED { + assert_eq!(env_setting(key, Some(11), one(var, value)), None); + for major in [Some(12), None] { + let found = env_setting(key, major, one(var, value)).unwrap(); + assert_eq!(found.as_bool(), Some(false), "{var}={value:?}"); + } + } + } + // An ignored pnpm spelling does not hide the next one pnpm reads. + let found = trust_lockfile_env( + Some(12), + env(&[ + ("pnpm_config_trust_lockfile", ""), + ("PNPM_CONFIG_TRUST_LOCKFILE", "false"), + ]), + ) + .unwrap(); + assert!(found.source.contains("PNPM_CONFIG_TRUST_LOCKFILE=false")); + // With no major known, an ignored pnpm spelling leaves npm's. + let found = env_setting( + "lockfile", + None, + env(&[ + ("pnpm_config_lockfile", ""), + ("npm_config_lockfile", "false"), + ]), + ) + .unwrap(); + assert!(found.source.contains("`npm_config_lockfile=false`")); + // npm's family is unchanged: its empty value still reads as false. + let found = env_setting("lockfile", Some(10), env(&[("npm_config_lockfile", "")])).unwrap(); + assert_eq!(found.as_bool(), Some(false)); + } + + /// #1557: the trust variable is read at the project's own pnpm major, + /// so the hosted member preflight and the rewrite agree on a padded + /// value: ignored on pnpm 11, `false` on pnpm 12. + #[tokio::test] + async fn the_trust_variable_follows_the_project_pnpm_major() { + const PADDED: &[(&str, &str)] = &[("pnpm_config_trust_lockfile", " false")]; + for (pm, off) in [("pnpm@11.28.5", false), ("pnpm@12.11.2", true), ("", true)] { + let tmp = tempfile::tempdir().unwrap(); + let manifest = if pm.is_empty() { + r#"{"name":"app"}"#.to_string() + } else { + format!(r#"{{"name":"app","packageManager":"{pm}"}}"#) + }; + write(tmp.path(), "package.json", &manifest); + let view = ProjectView::Disk(tmp.path()); + let found = trust_lockfile_env_for(&view, env(PADDED)).await; + assert_eq!( + found.and_then(|found| found.as_bool()), + off.then_some(false), + "{pm}" + ); + assert_eq!(trust_lockfile_env_for(&view, env(&[])).await, None); + } + } + /// #1460: pnpm 11 and newer read only `pnpm_config_*`, pnpm 10 and /// older only `npm_config_*`. #[test] diff --git a/docs/ecosystems.md b/docs/ecosystems.md index 9d6762c2a..b9959b28a 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -188,9 +188,13 @@ Each Legacy format has an upgrade path and an undo path. Both work in v5: For 9.0 root or member locks, the CLI configures `trustLockfile: true` in the root `pnpm-workspace.yaml` unless opted out with `--no-trust-lockfile-config` or explicitly disabled by the project or, for pnpm >=11, by a - `pnpm_config_trust_lockfile` environment variable the scan runs with (pnpm - reads it ahead of the file, so nothing is written and the warning names the - variable). pnpm >=11 needs this for hosted URLs. + `pnpm_config_trust_lockfile=false` environment variable the scan runs with + (pnpm reads it ahead of the file, so nothing is written and the warning names + the variable). pnpm >=11 reads a `pnpm_config_*` variable only when it is + `true` or `false` (pnpm 12 trims surrounding spaces first, pnpm 11 does + not); any other value (empty, `null`, `undefined`, `0`) is ignored for the + file, so the CLI ignores it too and writes the key. + pnpm >=11 needs this for hosted URLs. This skips registry re-verification for the whole lock; tarball integrity remains enforced. pnpm <=10 does not need the setting. An existing `pnpm-workspace.yaml` with no keys (empty or only comments) gets the