diff --git a/crates/socket-patch-cli/tests/e2e_vendor_pypi_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_pypi_build.rs index 42fbfee62..24fe51166 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_pypi_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_pypi_build.rs @@ -1787,6 +1787,126 @@ fn pip_requirements_vendor_fresh_checkout_no_index_and_revert() { ); } +/// #977: the `# socket-patch vendor:` comment is bookkeeping pip never +/// reads. With it edited (respaced, a note appended) or stripped, a re-run +/// of `vendor` is in sync instead of appending a second line for the same +/// wheel, and `vendor --revert` still restores the registry pin. +#[test] +#[serial_test::serial] +fn pip_requirements_revert_after_the_vendor_comment_is_edited() { + let Some(python) = find_python() else { + println!("SKIP e2e_vendor_pypi_build(pip #977): no python3/python on PATH"); + return; + }; + bake_leak_guards(); + let tmp = tempfile::tempdir().unwrap(); + let proj = tmp.path().join("proj"); + std::fs::create_dir_all(&proj).unwrap(); + std::fs::write(proj.join("requirements.txt"), "idna==3.7\nsix==1.16.0\n").unwrap(); + let venv = proj.join(".venv"); + let mkvenv = tool(Path::new(python), &proj, &["-m", "venv", ".venv"], &[]); + assert_tool_ok(&mkvenv, "python -m venv"); + let install = tool( + &venv.join("bin/pip"), + &proj, + &[ + "install", + "--disable-pip-version-check", + "--quiet", + "--no-cache-dir", + "-r", + "requirements.txt", + ], + &[], + ); + if !install.status.success() { + println!( + "SKIP e2e_vendor_pypi_build(pip #977): `pip install` failed (PyPI \ + unreachable?):\n{}", + String::from_utf8_lossy(&install.stderr) + ); + return; + } + stage_patch(&proj, &site_packages(&venv).join("six.py")); + let requirements_before = std::fs::read(proj.join("requirements.txt")).unwrap(); + let blobs = proj.join(".socket/blobs"); + let blobs_before = tmp.path().join("blobs"); + copy_dir_recursive(&blobs, &blobs_before); + + let edits: [(&str, fn(&str) -> String); 3] = [ + ("respaced", |l| { + l.replace(" # socket-patch vendor", " # socket-patch vendor") + }), + ("note appended", |l| format!("{l} # CVE fix, do not bump")), + ("stripped", |l| { + l.split(" # socket-patch vendor") + .next() + .unwrap() + .to_string() + }), + ]; + for (edit, apply_edit) in edits { + if !blobs.exists() { + copy_dir_recursive(&blobs_before, &blobs); + } + let (code, stdout, stderr) = run_vendored(&VendorDriver::VendorOffline, &proj); + assert_eq!( + code, 0, + "{edit}: vendor failed.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let wired = std::fs::read_to_string(proj.join("requirements.txt")).unwrap(); + let line = wired + .lines() + .find(|l| l.contains(".socket/vendor/pypi/")) + .unwrap_or_else(|| panic!("{edit}: no vendor line:\n{wired}")) + .to_string(); + let edited = wired.replace(&line, &apply_edit(&line)); + assert_ne!(edited, wired, "{edit}"); + std::fs::write(proj.join("requirements.txt"), &edited).unwrap(); + + // Re-vendor: in sync, no second wheel line. + let (code, stdout, stderr) = run_vendored(&VendorDriver::VendorOffline, &proj); + assert_eq!( + code, 0, + "{edit}: re-vendor failed.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let after = std::fs::read_to_string(proj.join("requirements.txt")).unwrap(); + assert_eq!( + after, edited, + "{edit}: re-vendor must leave the wired file alone" + ); + + let (code, stdout, stderr) = run_socket( + &proj, + &[ + "vendor", + "--revert", + "--json", + "--cwd", + proj.to_str().unwrap(), + ], + ); + assert_eq!( + code, 0, + "{edit}: revert failed.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let renv = parse_envelope(&stdout); + assert_eq!( + renv["summary"]["removed"], 1, + "{edit}: one entry reverted: {renv}" + ); + assert_eq!( + std::fs::read(proj.join("requirements.txt")).unwrap(), + requirements_before, + "{edit}: revert must restore the registry pin" + ); + assert!( + !proj.join(".socket/vendor").exists(), + "{edit}: .socket/vendor must be removed after revert" + ); + } +} + #[test] #[serial_test::serial] fn pip_vendored_requirements_evaluate_environment_markers() { diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/pypi.rs b/crates/socket-patch-core/src/vendor/lock_inventory/pypi.rs index 0a9bdd3af..b1822066a 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/pypi.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/pypi.rs @@ -629,9 +629,9 @@ async fn inventory_pdm_lock(view: &ProjectView<'_>) -> Option /// `./.socket/vendor/pypi// --hash=sha256:… /// # socket-patch vendor: ==` /// (`vendor::pypi_requirements::vendor_line`), whose requirement name -/// lives ONLY in that comment tag -/// (`utils::requirements::vendor_tag`, the reader -/// `vex::discover::pypi_other` already uses). +/// lives in that comment tag (`utils::requirements::vendor_tag`, the +/// reader `vex::discover::pypi_other` already uses), or, with the tag +/// edited away, in the wheel's own file name (#977). /// /// A user's OWN file/url/path reference is not ours to resolve and stays /// out. @@ -689,14 +689,20 @@ async fn inventory_requirements_txt(view: &ProjectView<'_>) -> Option { let Some((raw_name, reference)) = crate::utils::requirements::direct_reference(t) - .and_then(|(n, r)| Some((n, socket_reference_coords(r)?))) + .and_then(|(n, r)| Some((n.to_string(), socket_reference_coords(r)?))) .or_else(|| { // The VENDORED writer's shape: a bare path line - // whose requirement name lives only in the + // whose requirement name lives in the // `socket-patch vendor:` comment tag it appends. + // pip never reads that comment, so a line whose tag + // was edited away or stripped is named by its + // wheel (#977). let coords = socket_reference_coords(t.split_whitespace().next()?)?; - let (tag_name, _) = crate::utils::requirements::vendor_tag(comment?)?; - Some((tag_name, coords)) + let name = match comment.and_then(crate::utils::requirements::vendor_tag) { + Some((tag_name, _)) => tag_name.to_string(), + None => coords.0.clone(), + }; + Some((name, coords)) }) else { continue; @@ -704,7 +710,7 @@ async fn inventory_requirements_txt(view: &ProjectView<'_>) -> Option==` tag ([`vendor_tag`]) and whose -/// first token is a pypi vendor path — the exact shape [`vendor_line`] -/// writes, lexed like lockfile discovery reads it. +/// for every line whose first token is a pypi vendor path — the shape +/// [`vendor_line`] writes, lexed like lockfile discovery reads it — for +/// `canon_name`. The `# socket-patch vendor: ==` tag +/// ([`vendor_tag`]) names the package; pip never reads that comment, so a +/// line whose tag was edited away or stripped still installs our wheel and +/// is named by the wheel's file name instead (#977). fn vendor_lines<'a>( content: &'a str, canon_name: &'a str, ) -> impl Iterator + 'a { logical_lines(content).into_iter().filter_map(move |ll| { let (code, comment) = split_comment(&ll.text); - let (name, _) = comment.and_then(vendor_tag)?; - if name != canon_name { - return None; - } let token = code.split_whitespace().next()?; let parts = super::path::parse_vendor_path(token).filter(|p| p.eco == "pypi")?; - Some((parts, token.to_string(), code.to_string())) + let named = match comment.and_then(vendor_tag) { + Some((name, _)) => name == canon_name, + None => super::lock_inventory::pypi::socket_reference_coords(token) + .is_some_and(|(name, _)| name == canon_name), + }; + named.then(|| (parts, token.to_string(), code.to_string())) }) } +/// Pair each recorded vendor line (`recorded`, bottom-up record order) with +/// a distinct line of `lines`, or `None` when none matches. An exact copy +/// of the recorded line is paired first; then a line whose code part (what +/// pip reads) is the recorded one's, so an edit to the trailing +/// `# socket-patch vendor:` comment alone — respaced, a note appended, the +/// comment stripped — is not drift (#977). Among those, a line still tagged +/// as a vendor line wins over a bare copy of the same wheel line. Records that wrote the same line +/// then take their paired lines in file order, so identical vendor lines +/// pair with their own records whichever of them the user edited. +fn pair_recorded_lines(lines: &[Line], recorded: &[Option<&str>]) -> Vec> { + fn code_of(text: &str) -> &str { + split_comment(text).0.trim() + } + let mut paired: Vec> = vec![None; recorded.len()]; + // Pass 0: exact copy. Pass 1: same code part, still tagged as a vendor + // line (its comment respaced or extended), so a bare manual copy of the + // same wheel line is not taken for it. Pass 2: same code part. + for pass in 0..3 { + for i in 0..recorded.len() { + let Some(rec) = recorded[i].filter(|_| paired[i].is_none()) else { + continue; + }; + let rec_code = code_of(rec); + paired[i] = (0..lines.len()).rev().find(|idx| { + let line = lines[*idx].text.as_str(); + !paired.contains(&Some(*idx)) + && match pass { + 0 => line.trim() == rec.trim(), + _ => { + !rec_code.is_empty() + && code_of(line) == rec_code + && (pass == 2 + || split_comment(line).1.and_then(vendor_tag).is_some()) + } + } + }); + } + } + // Bottom-up record order: the first record of a group of identical + // lines takes the group's bottom-most line. + for i in 0..recorded.len() { + let group: Vec = (0..recorded.len()) + .filter(|j| recorded[*j].is_some() && recorded[*j] == recorded[i]) + .collect(); + if group.first() != Some(&i) { + continue; + } + let mut idxs: Vec = group.iter().filter_map(|j| paired[*j]).collect(); + idxs.sort_unstable_by(|a, b| b.cmp(a)); + let members: Vec = group.into_iter().filter(|j| paired[*j].is_some()).collect(); + for (j, idx) in members.into_iter().zip(idxs) { + paired[j] = Some(idx); + } + } + paired +} + /// Whether one file's content holds a socket vendor line for `canon_name` /// ([`vendor_lines`]). pub(super) fn has_vendor_line(content: &str, canon_name: &str) -> bool { @@ -576,15 +636,26 @@ pub(super) async fn revert_requirements( // Reverse order = bottom-up matching, so identical vendor lines pair // with their own originals (records were emitted top-down). - for rec in entry.wiring.iter().rev().filter(|r| &r.file == file) { - let Some(new_line) = rec.new.as_ref().and_then(serde_json::Value::as_str) else { - warnings.push(drift_warning(file, rec)); - continue; - }; - let Some(idx) = lines.iter().rposition(|l| l.text.trim() == new_line.trim()) else { - warnings.push(drift_warning(file, rec)); - continue; - }; + let recs: Vec<&WiringRecord> = entry + .wiring + .iter() + .rev() + .filter(|r| &r.file == file) + .collect(); + let recorded: Vec> = recs + .iter() + .map(|r| r.new.as_ref().and_then(serde_json::Value::as_str)) + .collect(); + let mut edits: Vec<(usize, &WiringRecord)> = Vec::new(); + for (rec, idx) in recs.iter().zip(pair_recorded_lines(&lines, &recorded)) { + match idx { + Some(idx) => edits.push((idx, rec)), + None => warnings.push(drift_warning(file, rec)), + } + } + // Splice bottom-up so every paired index still names its line. + edits.sort_by(|a, b| b.0.cmp(&a.0)); + for (idx, rec) in edits { match rec.action { WiringAction::Added => { lines.remove(idx); @@ -984,20 +1055,19 @@ fn plan_rewire( return Err(format!("{rel} is outside the project root")); } let mut lines = split_lines(&file.content); - let mut taken: HashSet = HashSet::new(); let mut records = Vec::new(); // Bottom-up, pairing identical lines with their own records exactly // as the revert does. - for rec in prev.wiring.iter().rev().filter(|r| r.file == rel) { - let old = rec - .new - .as_ref() - .and_then(serde_json::Value::as_str) - .ok_or_else(|| format!("{rel}: a recorded vendor line is empty"))?; - let idx = (0..lines.len()) - .rev() - .find(|i| !taken.contains(i) && lines[*i].text.trim() == old.trim()) - .ok_or_else(|| format!("{rel}: the vendor line changed since vendoring"))?; + let recs: Vec<&WiringRecord> = prev.wiring.iter().rev().filter(|r| r.file == rel).collect(); + let recorded: Vec> = recs + .iter() + .map(|r| r.new.as_ref().and_then(serde_json::Value::as_str)) + .collect(); + let paired = pair_recorded_lines(&lines, &recorded); + for ((rec, old), idx) in recs.into_iter().zip(recorded).zip(paired) { + let old = old.ok_or_else(|| format!("{rel}: a recorded vendor line is empty"))?; + let idx = + idx.ok_or_else(|| format!("{rel}: the vendor line changed since vendoring"))?; let marker = vendor_line_marker(split_comment(old).0); let line = vendor_line( rel_wheel, @@ -1008,7 +1078,6 @@ fn plan_rewire( rec.action == WiringAction::Added, ); lines[idx].text = line.clone(); - taken.insert(idx); records.push(WiringRecord { new: Some(serde_json::Value::String(line)), ..rec.clone() @@ -2895,6 +2964,211 @@ mod tests { ); } + /// #977: the `# socket-patch vendor:` comment is bookkeeping pip never + /// reads. A comment-only edit (respaced, a note appended after it, or + /// stripped by a formatter) leaves a line pip installs exactly as + /// vendor wrote it, so the revert still restores the original pin. + fn comment_edits(line: &str) -> [(&'static str, String); 3] { + [ + ( + "respaced", + line.replace(" # socket-patch vendor", " # socket-patch vendor"), + ), + ("note appended", format!("{line} # CVE fix, do not bump")), + ( + "stripped", + line.split(" # socket-patch vendor") + .next() + .unwrap() + .to_string(), + ), + ] + } + + #[tokio::test] + async fn revert_ignores_edits_to_the_vendor_comment() { + for original in [ + "idna==3.7\nsix==1.16.0\n", + "idna==3.7\nsix==1.16.0 --hash=sha256:aaaa\n", + "idna==3.7\n", + ] { + let tmp = write_root(original).await; + let wiring = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap(); + let wired = read_root(tmp.path()).await; + let line = wired.lines().nth(1).unwrap().to_string(); + for (edit, edited) in comment_edits(&line) { + assert_ne!(edited, line, "{edit}"); + tokio::fs::write( + tmp.path().join("requirements.txt"), + wired.replace(&line, &edited), + ) + .await + .unwrap(); + let outcome = + revert_requirements(&entry_for(wiring.clone()), tmp.path(), false).await; + assert!(outcome.success, "{original:?} {edit}: {:?}", outcome.error); + assert!( + outcome.warnings.is_empty(), + "{original:?} {edit}: {:?}", + outcome.warnings + ); + assert_eq!(read_root(tmp.path()).await, original, "{edit}"); + } + } + } + + /// #977: an exact copy of the recorded line still pairs with its own + /// record first, so a bare manual reference to the same wheel is the + /// line left behind (and warned about), not the managed one. + #[tokio::test] + async fn revert_prefers_the_exact_vendor_line_over_a_bare_copy() { + let tmp = write_root("six==1.16.0\n").await; + let wiring = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap(); + let wired = read_root(tmp.path()).await; + let content = format!("./{REL_WHEEL}\n{wired}"); + tokio::fs::write(tmp.path().join("requirements.txt"), &content) + .await + .unwrap(); + let outcome = revert_requirements(&entry_for(wiring), tmp.path(), false).await; + assert!(outcome.success); + assert_eq!( + read_root(tmp.path()).await, + format!("./{REL_WHEEL}\nsix==1.16.0\n") + ); + } + + /// #977: identical vendor lines whose originals differ pair with their + /// own records when only one of them had its comment edited. + #[tokio::test] + async fn revert_pairs_identical_vendor_lines_in_order_after_an_edit() { + let original = "six==1.16.0\nidna==3.7\nsix == 1.16.0\n"; + let tmp = write_root(original).await; + let wiring = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap(); + assert_eq!(wiring.len(), 2); + let wired = read_root(tmp.path()).await; + let line = wired.lines().next().unwrap().to_string(); + let edited = format!("{line}\nidna==3.7\n{line} # CVE fix\n"); + assert_eq!(wired, format!("{line}\nidna==3.7\n{line}\n")); + tokio::fs::write(tmp.path().join("requirements.txt"), &edited) + .await + .unwrap(); + let outcome = revert_requirements(&entry_for(wiring), tmp.path(), false).await; + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + assert_eq!(read_root(tmp.path()).await, original); + } + + /// #977: a vendor line with a note appended pairs with its record even + /// when a bare manual copy of the same wheel line sits below it; the + /// copy is the user's own line, left behind and warned about. + #[tokio::test] + async fn revert_prefers_an_edited_vendor_line_over_a_bare_copy() { + let tmp = write_root("six==1.16.0\n").await; + let wiring = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap(); + let line = read_root(tmp.path()).await.trim_end().to_string(); + let content = format!("{line} # CVE fix\n./{REL_WHEEL}\n"); + tokio::fs::write(tmp.path().join("requirements.txt"), &content) + .await + .unwrap(); + let outcome = revert_requirements(&entry_for(wiring), tmp.path(), false).await; + assert!(outcome.success); + assert!( + !outcome + .warnings + .iter() + .any(|w| w.code == "vendor_revert_line_drifted"), + "{:?}", + outcome.warnings + ); + assert_eq!( + read_root(tmp.path()).await, + format!("six==1.16.0\n./{REL_WHEEL}\n") + ); + } + + /// #977: a vendor line whose comment was edited or stripped still routes + /// the package to this patch's wheel, so a re-run of `vendor` is in sync + /// rather than appending a second `(transitive)` line for the same wheel. + #[tokio::test] + async fn an_edited_vendor_comment_is_still_in_sync() { + for original in ["idna==3.7\nsix==1.16.0\n", "idna==3.7\n"] { + let tmp = write_root(original).await; + wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap(); + let wired = read_root(tmp.path()).await; + let line = wired.lines().nth(1).unwrap().to_string(); + for (edit, edited) in comment_edits(&line) { + let content = wired.replace(&line, &edited); + tokio::fs::write(tmp.path().join("requirements.txt"), &content) + .await + .unwrap(); + assert!( + matches!( + preflight_requirements(tmp.path(), "six", "1.16.0", UUID).await, + Ok(RequirementsTarget::InSync { pin: Some((path, _)) }) + if path == REL_WHEEL + ), + "{original:?} {edit}" + ); + assert!(has_vendor_line(&content, "six"), "{edit}"); + assert!(!has_vendor_line(&content, "idna"), "{edit}"); + } + } + } + + /// #977: a superseding patch re-wires a vendor line whose comment was + /// edited, and reverting the new entry restores the user's own pin. + #[tokio::test] + async fn rewire_and_revert_after_an_edited_vendor_comment() { + let original = "idna==3.7\nsix==1.16.0\n"; + let newer = format!(".socket/vendor/pypi/{OTHER_UUID}/six-1.16.0-py2.py3-none-any.whl"); + let tmp = write_root(original).await; + let wiring = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap(); + let wired = read_root(tmp.path()).await; + let line = wired.lines().nth(1).unwrap().to_string(); + for (edit, edited) in comment_edits(&line) { + tokio::fs::write( + tmp.path().join("requirements.txt"), + wired.replace(&line, &edited), + ) + .await + .unwrap(); + let rewired = rewire_requirements( + tmp.path(), + &entry_for(wiring.clone()), + "six", + "1.16.0", + &newer, + SHA, + ) + .await + .unwrap_or_else(|e| panic!("{edit}: {e:?}")); + assert_eq!( + read_root(tmp.path()).await, + format!("idna==3.7\n./{newer} # socket-patch vendor: six==1.16.0\n"), + "{edit}" + ); + let outcome = revert_requirements(&entry_for(rewired), tmp.path(), false).await; + assert!(outcome.success, "{edit}: {:?}", outcome.error); + assert!( + outcome.warnings.is_empty(), + "{edit}: {:?}", + outcome.warnings + ); + assert_eq!(read_root(tmp.path()).await, original, "{edit}"); + } + } + #[tokio::test] async fn revert_dry_run_writes_nothing() { let tmp = write_root("six==1.16.0\n").await; @@ -3084,7 +3358,8 @@ mod tests { // ── preflight / vendored_uuid_for ──────────────────────────────────── /// `vendored_uuid_for` must recognize ONLY this module's own line shape - /// for the queried package: another package's tag, a name-prefix tag + /// for the queried package (its tag, or with the tag edited away the + /// wheel's own name, #977): another package's tag, a name-prefix tag /// (`sixty`), a non-vendor path token, and a foreign-ecosystem vendor /// path are all invisible. Each false positive would misroute preflight /// into InSync (skipping the wire) or an already-vendored refusal for a