diff --git a/crates/socket-patch-cli/tests/mode_migration_pypi.rs b/crates/socket-patch-cli/tests/mode_migration_pypi.rs index 5535a27d8..433123419 100644 --- a/crates/socket-patch-cli/tests/mode_migration_pypi.rs +++ b/crates/socket-patch-cli/tests/mode_migration_pypi.rs @@ -3289,6 +3289,47 @@ async fn pip_lock_vendor_wires_requirements_with_the_pylock() { assert!(!root.join(format!(".socket/vendor/pypi/{UUID}")).exists()); } +/// #1873: pip expands `${NAME}` on every requirements line where it runs, +/// so what `six==${SIXV}` installs is unknowable to vendor. Beside a `pip +/// lock` pylock.toml, vendor refuses it and writes nothing (it used to wire +/// only the lock, name requirements.txt an unpatched loser and exit 0), +/// whatever this process's environment holds. A pylock-only wiring the +/// templated line later contests fails `vendor --check`. +#[tokio::test] +async fn pip_lock_vendor_refuses_an_env_templated_requirements_pin() { + const TEMPLATED: &str = "idna==3.7\nsix==${SP_E2E_1873_SIXV}\n"; + for env in [&[("SP_E2E_1873_SIXV", "1.16.0")][..], &[][..]] { + let (_tmp, root) = project(); + stage_pip_lock(&root); + std::fs::write(root.join("requirements.txt"), TEMPLATED).unwrap(); + stage_manifest(&root); + let before = tree(&root); + let (code, out) = run_cli(&root, &["vendor"], env); + assert_ne!(code, 0, "{env:?}: {out:#}"); + assert!( + out.to_string().contains("pypi_requirement_env_templated"), + "{env:?}: {out:#}" + ); + assert_eq!(tree(&root), before, "{env:?}: nothing written"); + } + + // A pylock-only wiring (vendored before six was named) is contested + // once the templated line names six. + let env = [("SP_E2E_1873_SIXV", "1.16.0")]; + let (_tmp, root) = project(); + stage_pip_lock(&root); + std::fs::write(root.join("requirements.txt"), "idna==3.7\n").unwrap(); + stage_manifest(&root); + let (code, out) = run_cli(&root, &["vendor"], &env); + assert_eq!(code, 0, "{out:#}"); + let (code, out) = run_cli(&root, &["vendor", "--check"], &env); + assert_eq!(code, 0, "{out:#}"); + std::fs::write(root.join("requirements.txt"), TEMPLATED).unwrap(); + let (code, out) = run_cli(&root, &["vendor", "--check"], &env); + assert_eq!(code, 1, "pip may install six from PyPI: {out:#}"); + assert!(out.to_string().contains("contested"), "{out:#}"); +} + /// #1851: requirements.txt pins six only through a `-r` include outside /// the project root, beside the `pip lock` pylock.toml. `pip install -r /// requirements.txt` installs that pin, and vendor can't edit the diff --git a/crates/socket-patch-core/src/utils/requirements.rs b/crates/socket-patch-core/src/utils/requirements.rs index cb1851497..ffed89d1d 100644 --- a/crates/socket-patch-core/src/utils/requirements.rs +++ b/crates/socket-patch-core/src/utils/requirements.rs @@ -288,6 +288,15 @@ pub(crate) fn strip_comment(text: &str) -> &str { split_comment(text).0 } +/// Whether pip's [`expand_env_vars`] would substitute anything in `code`. +/// pip expands every line, not only `-r` / `-c` targets, so `six==${SIXV}` +/// pins whatever `SIXV` holds where pip runs: a Docker `ARG`, a CI step, +/// none of which this process sees. Such a line pins no knowable release +/// (#1873). +pub(crate) fn has_env_reference(code: &str) -> bool { + code.contains("${") && expand_env_vars(code, |_| Some("_".to_string())) != code +} + /// pip's `expand_env_variables`: each `${NAME}` whose `NAME` is /// `[A-Z0-9_]+` is replaced by `lookup(NAME)`; an unset or empty variable /// leaves the reference as written. pip expands after stripping comments diff --git a/crates/socket-patch-core/src/vendor/pypi.rs b/crates/socket-patch-core/src/vendor/pypi.rs index c52d494cc..737c51343 100644 --- a/crates/socket-patch-core/src/vendor/pypi.rs +++ b/crates/socket-patch-core/src/vendor/pypi.rs @@ -263,6 +263,24 @@ async fn refuse_pin_outside_root( } } +/// #1873: refuse wiring a lock beside a requirements set that names the +/// target through a `${NAME}` pip expands where it runs (`six==${SIXV}`). +/// The release it installs is unknowable here, so naming requirements.txt +/// a loser would let the lock wiring report the package patched while pip +/// installs it from the registry. +async fn refuse_templated_pin( + project_root: &Path, + target: Option<(&str, &str)>, +) -> Result<(), (&'static str, String)> { + let Some((name, _)) = target else { + return Ok(()); + }; + match super::pypi_requirements::templated_pin(project_root, name).await { + Some(refusal) => Err(refusal), + None => Ok(()), + } +} + /// Route the project to a wiring flavor, first match wins. Lockfiles are the /// authoritative "this tool manages installs" signal, so locks are compared /// with locks (precedence follows migration direction / ecosystem currency: @@ -416,6 +434,7 @@ async fn detect_pypi_flavor( None => false, }; refuse_pin_outside_root(project_root, target).await?; + refuse_templated_pin(project_root, target).await?; if !wired_with_lock && exists(PYPI_REQUIREMENTS).await { present.push(PYPI_REQUIREMENTS); } @@ -471,6 +490,7 @@ async fn detect_pypi_flavor( }; if governing.is_some() { refuse_pin_outside_root(project_root, target).await?; + refuse_templated_pin(project_root, target).await?; } if governing.is_some() && !wired_with_lock @@ -4349,6 +4369,21 @@ mod tests { } } + /// #1873: beside a governing tool lock, a requirements pin templated + /// with pip's `${VAR}` refuses instead of being named a loser: the + /// release it installs is unknown. + #[tokio::test] + async fn governing_lock_refuses_an_env_templated_requirements_pin() { + let tmp = tempfile::tempdir().unwrap(); + touch(tmp.path(), "Pipfile", "[packages]\nsix = \"==1.16.0\"\n").await; + touch(tmp.path(), "Pipfile.lock", "{}").await; + touch(tmp.path(), "requirements.txt", "six==${SIXV}\n").await; + let (code, detail) = detect_pypi_flavor(tmp.path(), Some(("six", "1.16.0"))) + .await + .unwrap_err(); + assert_eq!(code, "pypi_requirement_env_templated", "{detail}"); + } + /// #912: a pylock-only Pipenv checkout can't be vendored — Pipenv drops /// the `archive` entry the wiring writes and installs the upstream /// release — so it refuses with a `pipenv lock` pointer. Without a diff --git a/crates/socket-patch-core/src/vendor/pypi_requirements.rs b/crates/socket-patch-core/src/vendor/pypi_requirements.rs index eacda979e..490ef46ec 100644 --- a/crates/socket-patch-core/src/vendor/pypi_requirements.rs +++ b/crates/socket-patch-core/src/vendor/pypi_requirements.rs @@ -27,8 +27,8 @@ use crate::crawlers::python_crawler::canonicalize_pypi_name; use crate::utils::fs::{atomic_write_bytes_preserving_mode, read_regular_to_string}; use crate::utils::line_endings::{self, terminator}; use crate::utils::requirements::{ - expand_env_vars, hash_options, logical_lines, parse_requirement, requirement_name, - requires_hashes, shlex_split, split_comment, strip_comment, vendor_tag, + expand_env_vars, has_env_reference, hash_options, logical_lines, parse_requirement, + requirement_name, requires_hashes, shlex_split, split_comment, strip_comment, vendor_tag, }; use super::common::refuse_symlinked; @@ -83,6 +83,8 @@ struct PinScan { /// environment-marker branch; whether that is ambiguous depends on the /// target pins of the whole requirement set ([`marker_split_ok`]). other_branches: bool, + /// Some occurrence carries a `${NAME}` pip expands where it runs. + templated: bool, } /// Whether the other version's marker branches (if any) in a requirement @@ -109,13 +111,16 @@ fn marker_split_ok(scans: &[PinScan]) -> bool { /// all carry a marker (`split_ok` from [`marker_split_ok`]). #[derive(Debug, PartialEq, Eq)] enum PinRefusal { + Templated, Extras, NotPinned, UnmarkedSplit, } fn pin_refusal(scan: &PinScan, split_ok: bool) -> Option { - if scan.extras { + if scan.templated { + Some(PinRefusal::Templated) + } else if scan.extras { Some(PinRefusal::Extras) } else if scan.range { Some(PinRefusal::NotPinned) @@ -136,15 +141,25 @@ fn scan_pins(content: &str, canon_name: &str, version: &str) -> PinScan { let mut exact_extras = false; let mut found_range = false; let mut other_branches = false; + let mut found_templated = false; // Hosted mode also rewrites a file's one line for the package when it // is unpinned (`six[foo]`), so that extras line counts as exact too. let mut named_lines = 0usize; let mut unpinned_extras = false; for ll in logical_lines(content) { let code = strip_comment(&ll.text); - if requirement_name(code).is_some_and(|name| canonicalize_pypi_name(name) == canon_name) { + let names_target = + requirement_name(code).is_some_and(|name| canonicalize_pypi_name(name) == canon_name); + if names_target { named_lines += 1; } + if has_env_reference(code) && names_target { + // pip expands `${NAME}` where it runs, so the release this + // line installs is unknowable here, and rewriting it would + // drop the template for every other environment (#1873). + found_templated = true; + continue; + } let Some(req) = parse_requirement(code) else { // A line that names the package but is no requirement pip // reads cleanly is never a pin to rewrite (fail closed). @@ -202,6 +217,7 @@ fn scan_pins(content: &str, canon_name: &str, version: &str) -> PinScan { exact_extras: exact_extras || (unpinned_extras && named_lines == 1), range: found_range, other_branches, + templated: found_templated, } } @@ -225,7 +241,9 @@ fn find_pin(content: &str, canon_name: &str, version: &str) -> PinSearch { let split_ok = marker_split_ok(std::slice::from_ref(&scan)); match pin_refusal(&scan, split_ok) { Some(PinRefusal::Extras) => return PinSearch::Extras, - Some(PinRefusal::NotPinned | PinRefusal::UnmarkedSplit) => return PinSearch::Range, + Some(PinRefusal::Templated | PinRefusal::NotPinned | PinRefusal::UnmarkedSplit) => { + return PinSearch::Range + } None => {} } match scan.exact.into_iter().next() { @@ -465,6 +483,22 @@ pub(super) async fn sibling_pin_wirable(root: &Path, canon_name: &str, version: && refuse_symlinked(root, &rels).await.is_ok() } +/// #1873: refuse a requirements line naming `canon_name` through a +/// `${NAME}` pip expands where it runs (`six==${SIXV}`). What it installs +/// is unknowable here, so the vendor neither rewrites it nor names the file +/// a loser beside a lock that it wires alone. +fn templated_refusal(rel: &str, canon_name: &str) -> (&'static str, String) { + ( + "pypi_requirement_env_templated", + format!( + "{rel}: the {canon_name} requirement uses a ${{NAME}} environment variable pip \ + expands at install time, so the release it installs is unknown; pin it \ + literally or use agent mode (`scan --mode agent` + `socket-patch apply`) \ + instead" + ), + ) +} + /// The out-of-root `-r` include (`../shared/base.txt`) through which pip /// installs `canon_name==version` from somewhere other than a vendored /// wheel, if the requirements set has one (#1851). Vendor cannot edit that @@ -577,6 +611,21 @@ fn extras_refusal(rel: &str, canon_name: &str) -> (&'static str, String) { ) } +/// [`templated_refusal`] for the first requirements file in the tree pip +/// reads from `root` that names `canon_name` through a `${NAME}`. +pub(super) async fn templated_pin(root: &Path, canon_name: &str) -> Option<(&'static str, String)> { + let files = collect_requirements_files(root).await.ok()?; + files.iter().find_map(|f| { + logical_lines(&f.content).into_iter().find_map(|ll| { + let code = strip_comment(&ll.text); + (has_env_reference(code) + && requirement_name(code) + .is_some_and(|name| canonicalize_pypi_name(name) == canon_name)) + .then(|| templated_refusal(&f.rel, canon_name)) + }) + }) +} + /// [`sibling_pin_wirable`] for a lock about to be wired fresh, except /// that a requirements set pinning exactly `canon_name==version` with /// extras (`six[foo]==1.16.0`) refuses the package, as the @@ -1027,6 +1076,7 @@ async fn plan_requirements( for (file, scan) in files.iter().zip(scans) { match pin_refusal(&scan, split_ok) { + Some(PinRefusal::Templated) => return Err(templated_refusal(&file.rel, canon_name)), Some(PinRefusal::Extras) => return Err(extras_refusal(&file.rel, canon_name)), Some(PinRefusal::NotPinned) => { return Err(( @@ -2431,6 +2481,56 @@ mod tests { } } + /// #1873: pip expands `${NAME}` on every line where it runs, so what + /// `six==${SIXV}` installs is unknowable here. Vendor refuses it with + /// its own code (it used to say "not pinned", or beside a pylock leave + /// it on PyPI while the lock wiring reported the package patched) and + /// never rewrites the template away, in the root or an include. A + /// `${...}` pip does not expand (lowercase) is no template. + #[tokio::test] + async fn refuses_an_env_templated_pin() { + for (root, base) in [ + ("requests==2.31.0\nsix==${SIXV}\n", None), + ("six == ${SIXV} # templated\n", None), + ("six==1.16.0 --hash=sha256:${SIX_SHA}\n", None), + ("-r base.txt\nidna==3.7\n", Some("six==${SIXV}\n")), + ] { + let tmp = write_root(root).await; + if let Some(base) = base { + tokio::fs::write(tmp.path().join("base.txt"), base) + .await + .unwrap(); + } + let err = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap_err(); + assert_eq!( + err.0, "pypi_requirement_env_templated", + "{root:?}: {}", + err.1 + ); + assert_eq!(read_root(tmp.path()).await, root, "nothing written"); + assert!(!sibling_pin_wirable(tmp.path(), "six", "1.16.0").await); + assert_eq!( + templated_pin(tmp.path(), "six").await.map(|r| r.0), + Some("pypi_requirement_env_templated"), + "{root:?}" + ); + // Another package's line is untouched by the refusal. + assert!(templated_pin(tmp.path(), "idna").await.is_none()); + } + let tmp = write_root("requests==${REQV}\nsix==1.16.0\n").await; + assert!(templated_pin(tmp.path(), "six").await.is_none()); + wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap(); + assert_eq!( + find_pin("six==${sixv}\n", "six", "1.16.0"), + PinSearch::Range, + "a lowercase reference stays literal and is no pin" + ); + } + #[tokio::test] async fn rewrites_hash_pinned_continuation_and_preserves_crlf() { // A hash-pinned requirement spanning two physical lines, CRLF file. diff --git a/crates/socket-patch-core/src/vex/discover/pypi_locks.rs b/crates/socket-patch-core/src/vex/discover/pypi_locks.rs index 8e7a78166..97bfe31c6 100644 --- a/crates/socket-patch-core/src/vex/discover/pypi_locks.rs +++ b/crates/socket-patch-core/src/vex/discover/pypi_locks.rs @@ -1420,10 +1420,16 @@ mod tests { ("-e git+https://example.com/click.git#egg=click\n", true), ("git+https://example.com/click.git#egg=click\n", true), ("../wheels/click-8.1.7-py3-none-any.whl\n", true), + // #1873: pip expands `${NAME}` where it installs, so a + // templated pin may install the wired release from PyPI. + ("click==${CLICK_VERSION}\n", true), + ("click${CLICK_SPEC}\n", true), ( "click @ https://files.pythonhosted.org/packages/click-8.1.6-py3-none-any.whl\n", false, ), + ("click==${click_version}\n", false), + ("idna==${IDNA_VERSION}\n", false), ("click==8.1.6\n", false), ("idna==3.7\n", false), ] { diff --git a/crates/socket-patch-core/src/vex/discover/pypi_other.rs b/crates/socket-patch-core/src/vex/discover/pypi_other.rs index 376b38626..9a724b3e6 100644 --- a/crates/socket-patch-core/src/vex/discover/pypi_other.rs +++ b/crates/socket-patch-core/src/vex/discover/pypi_other.rs @@ -305,7 +305,7 @@ fn outside_root_line(ctx: &DiscoverCtx<'_>, file: &str, line: &str, out: &mut Di return; } let Some((name, location)) = requirement_location(code) else { - registry_pin(file, code, out); + registry_line(file, code, out); return; }; if matches!(classify(ctx, location, url_form(location)), Ok(None)) { @@ -342,9 +342,7 @@ fn requirements_line(ctx: &DiscoverCtx<'_>, file: &str, line: &str, out: &mut Di return; } let Some((name, location)) = requirement_location(code) else { - // A registry requirement; a pin that can only install one release - // is evidence against another lock's wiring of that release. - registry_pin(file, code, out); + registry_line(file, code, out); return; }; let tag = comment.and_then(vendor_tag); @@ -364,6 +362,21 @@ fn requirements_line(ctx: &DiscoverCtx<'_>, file: &str, line: &str, out: &mut Di ); } +/// A registry requirement line, in-root or in an out-of-root include. pip +/// expands `${NAME}` where it runs, so a line carrying one may install any +/// release of the package it names: evidence against every wiring of it +/// (#1873). Otherwise a pin that can only install one release is evidence +/// against another lock's wiring of that release ([`registry_pin`]). +fn registry_line(file: &str, code: &str, out: &mut Discovery) { + if requirements::has_env_reference(code) { + if let Some(name) = requirements::requirement_name(code) { + out.resolved_elsewhere_any_version(file, pypi_name_purl(name)); + } + return; + } + registry_pin(file, code, out); +} + /// Record a registry requirement that can only install one release as /// evidence against another lock's wiring of it: an exact /// `name[extras]==X` pin in any PEP 440 spelling (`==1.16.0.0`, @@ -1913,6 +1926,51 @@ mod tests { } } + /// REGRESSION (#1873): pip expands `${NAME}` on every requirements + /// line where it runs (a Docker `ARG`, a CI step), so `six==${SIXV}` + /// may install any release, including the wired one, from PyPI. It + /// contests a lock's wiring of six whatever this process's environment + /// holds. A lowercase `${...}` stays literal in pip and pins nothing. + #[tokio::test] + async fn issue_1873_env_templated_pin_contests_wiring() { + const WHL: &str = "six-1.16.0-py2.py3-none-any.whl"; + let vendored = vendored_wheel(UUID_A, WHL); + let wired = pipfile_lock(serde_json::json!({ "default": { + "six": { "file": format!("./{vendored}"), "hashes": [format!("sha256:{SHA}")] }, + }})); + let discover = |requirements: &'static str| { + let wired = wired.clone(); + async move { + let p = Project::new(); + p.write("Pipfile.lock", &wired) + .write("Pipfile", "[packages]\nsix = \"*\"\n") + .write("requirements.txt", requirements); + p.discover().await + } + }; + for line in [ + "six==${SP_TEST_1873_SIXV}\n", + "idna==3.7\nSix == ${SIXV} # templated\n", + "six${SIX_SPEC}\n", + "six==1.16.0 --hash=sha256:${SIX_SHA}\n", + ] { + let out = discover(line).await; + assert!(out.refs.is_empty(), "{line:?}: {:#?}", out.refs); + let c = out + .vendored_contest("pkg:pypi/six@1.16.0", UUID_A) + .unwrap_or_else(|| panic!("{line:?}: not contested")); + assert_eq!( + c.other, + std::path::Path::new("requirements.txt"), + "{line:?}" + ); + } + for line in ["six==${sixv}\n", "idna==${IDNAV}\n"] { + let out = discover(line).await; + assert_eq!(out.refs.len(), 1, "{line:?}: {:#?}", out.diagnostics); + } + } + /// #1602: a lock tree that wires the release itself, under another /// PEP 440 spelling, is not a contester even when it also pins it: /// pip resolves the include's `==1.16.0` to the tree's own vendored