diff --git a/CHANGELOG.md b/CHANGELOG.md index b85d8bf44..1b3bacabf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -914,6 +914,15 @@ into the new version's section — see docs/releasing.md. ### Fixed +- **Hosted PDM `rollback` removes the patch after `pdm add` and a re-scan.** + `pdm add ` and `pdm lock --update-reuse` re-lay the redirected + `pdm.lock` unit but keep Socket's `url` and patched hash (PDM 2.26+), or + just the patched hash (2.12–2.20). A re-scan then recorded that + still-patched unit as the pristine one, so `rollback` reported success + and deleted the ledger while the lock stayed patched, or, on 2.12–2.20, + stopped installing. The re-scan now keeps the recorded pristine unit + whenever the re-laid one still carries the patch. + - **Hosted nuget redirects survive a `` in `nuget.config`.** The Socket source (and, in an existing ``, its mapping) was inserted ahead of the section's ``, which NuGet diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index c66d56fa5..165ce8c19 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -37,6 +37,84 @@ pub(crate) const REBASE_KINDS: &[&str] = &[ socket_patch_core::patch::redirect::vlt::KIND, ]; +/// The `original` a rebased PDM lock edit keeps: `recorded` is the ledger's +/// edit, `fresh` this run's edit for the same drifted unit. +/// +/// `pdm lock` re-resolves to the registry (and may reflow CRLF → LF), so the +/// fresh `original` IS the relocked-registry rollback target. But `pdm add +/// ` and `pdm lock --update-reuse` re-lay the unit while keeping the +/// patch: PDM 2.26+ keeps the Socket `url` and patched sha256, 2.12–2.20 keep +/// just the patched sha256. Adopting that still-patched fragment would make +/// rollback "revert" patched → patched and report success (#331). While the +/// fresh `original` still carries any string the recorded edit introduced +/// (a quoted value in its `new` that its `original` lacks: the url, the +/// patched hash), keep the recorded pristine unit, re-laid in the fresh +/// fragment's line endings and ending in the fresh fragment's BOUNDARY (a +/// unit fragment carries its blank lines plus the next top-level header, or +/// EOF, and the relock may have added a unit after this one), so it splices +/// into the relocked text without eating the successor's header. +pub(crate) fn rebased_pdm_original( + recorded: &socket_patch_core::patch::redirect::FileEdit, + fresh: &socket_patch_core::patch::redirect::FileEdit, +) -> Option { + let as_str = + |v: &Option| v.as_ref().and_then(|v| v.as_str().map(str::to_owned)); + let (Some(pristine), Some(wired), Some(current)) = ( + as_str(&recorded.original), + as_str(&recorded.new), + as_str(&fresh.original), + ) else { + return fresh.original.clone(); + }; + let introduced = ['"', '\''].into_iter().flat_map(|quote| { + wired + .split(quote) + .skip(1) + .step_by(2) + .map(str::to_owned) + .collect::>() + }); + let still_patched = introduced + .filter(|token| !token.is_empty() && !pristine.contains(token.as_str())) + .any(|token| current.contains(token.as_str())); + if !still_patched { + return fresh.original.clone(); + } + let lf = split_pdm_boundary(&pristine).0.replace("\r\n", "\n"); + let body = if current.contains("\r\n") { + lf.replace('\n', "\r\n") + } else { + lf + }; + let boundary = split_pdm_boundary(¤t).1; + Some(serde_json::Value::String(format!("{body}{boundary}"))) +} + +/// Split a PDM lock fragment into its body and its trailing boundary: the +/// line break ending the body's last line, then any blank or comment lines +/// and the next top-level header (`utils::pdm_lock`'s `next_header_end`), +/// or trailing blank lines up to EOF. A fragment with no boundary (a legacy +/// `[metadata.files]` entry) returns an empty one. +fn split_pdm_boundary(fragment: &str) -> (&str, &str) { + let lines: Vec<&str> = fragment.split_inclusive('\n').collect(); + let content = |line: &str| line.trim_end_matches(['\r', '\n']).trim().to_owned(); + let mut keep = lines.len(); + if keep > 1 && lines[keep - 1].starts_with('[') { + keep -= 1; + } + while keep > 1 && { + let line = content(lines[keep - 1]); + line.is_empty() || line.starts_with('#') + } { + keep -= 1; + } + let body: usize = lines[..keep].iter().map(|line| line.len()).sum(); + let body = fragment[..body] + .trim_end_matches('\n') + .trim_end_matches('\r'); + fragment.split_at(body.len()) +} + pub(crate) const REDIRECT_CANDIDATE_FILES: &[&str] = &[ "package-lock.json", "npm-shrinkwrap.json", @@ -2997,17 +3075,14 @@ pub(crate) async fn run_redirect_selected( .unwrap_or(0); if let Some(&target) = siblings.get(nth) { if !rebased.contains(&target) { - // `pdm lock` fully un-patches the lock (registry source - // restored) and may reflow line endings (CRLF → LF), so - // the fresh run's `original` IS the correct - // relocked-registry rollback target and the stale - // recorded one would restore a mismatched fragment. - // Poetry's relock instead KEEPS the Socket source (it - // only drops the inserted `files` line), so its oldest + // Poetry's relock KEEPS the Socket source (it only + // drops the inserted `files` line), so its oldest // `original` — the true pre-patch fragment — must - // survive; only its `new` is refreshed. + // survive; only its `new` is refreshed. PDM's depends + // on what the relock did: see `rebased_pdm_original`. if edit.kind == "redirect_pdm_lock_package" { - ledger.edits[target].original = edit.original.clone(); + ledger.edits[target].original = + rebased_pdm_original(&ledger.edits[target], edit); } ledger.edits[target].new = edit.new.clone(); ledger.edits[target].action = edit.action.clone(); diff --git a/crates/socket-patch-cli/src/hosted_memory/ledger.rs b/crates/socket-patch-cli/src/hosted_memory/ledger.rs index 2961d107d..ea04218f6 100644 --- a/crates/socket-patch-cli/src/hosted_memory/ledger.rs +++ b/crates/socket-patch-cli/src/hosted_memory/ledger.rs @@ -12,7 +12,7 @@ use socket_patch_core::patch::redirect::{ }; use socket_patch_core::vendor::lock_inventory::{MemoryEntry, MemoryProject}; -use crate::commands::scan::hosted::{rebase_vlt_edits, REBASE_KINDS}; +use crate::commands::scan::hosted::{rebase_vlt_edits, rebased_pdm_original, REBASE_KINDS}; /// Load the project's ledger: `Ok(None)` when absent, `Err` (the disk /// message) when present but unreadable or malformed. @@ -92,7 +92,8 @@ pub(crate) fn merge( if let Some(&target) = siblings.get(nth) { if !rebased.contains(&target) { if edit.kind == "redirect_pdm_lock_package" { - ledger.edits[target].original = edit.original.clone(); + ledger.edits[target].original = + rebased_pdm_original(&ledger.edits[target], edit); } ledger.edits[target].new = edit.new.clone(); ledger.edits[target].action = edit.action.clone(); @@ -184,4 +185,92 @@ mod tests { let text = serialize(&ledger).unwrap(); assert!(text.ends_with("}\n")); } + + fn pdm(original: &str, new: &str) -> FileEdit { + FileEdit { + path: "pdm.lock".into(), + kind: "redirect_pdm_lock_package".into(), + action: "rewritten".into(), + key: Some("urllib3".into()), + original: Some(serde_json::json!(original)), + new: Some(serde_json::json!(new)), + } + } + + /// #331: a drifted PDM unit that still carries the Socket url or the + /// patched sha256 (`pdm add`, `pdm lock --update-reuse`) keeps the + /// recorded pristine `original`, re-laid in the relocked line endings; + /// a clean relock (`pdm lock`) still adopts the fresh one. + #[test] + fn pdm_rebase_keeps_the_pristine_original_while_the_patch_survives() { + let url = "url = \"https://patch.test/u.whl\"\r\n"; + let pristine = "files = [\"sha256:1111\"]\r\n"; + let wired = format!("files = [\"sha256:cccc\"]\r\n{url}"); + for (current, fresh_new, keeps) in [ + // url + patched hash kept, re-laid. + ( + "files = [ \"sha256:cccc\" ]\nurl = \"https://patch.test/u.whl\"\n", + "files = [\"sha256:cccc\"]\nurl = \"https://patch.test/u.whl\"\n", + true, + ), + // url dropped, patched hash kept. + ( + "files = [ \"sha256:cccc\" ]\n", + "files = [\"sha256:cccc\"]\nurl = \"https://patch.test/u.whl\"\n", + true, + ), + // `pdm lock`: back on the registry. + ( + "files = [ \"sha256:2222\" ]\n", + "files = [\"sha256:cccc\"]\nurl = \"https://patch.test/u.whl\"\n", + false, + ), + ] { + let mut ledger = RedirectState::new(); + ledger.edits.push(pdm(pristine, &wired)); + let files = BTreeMap::from([("pdm.lock".to_string(), current.to_string())]); + merge( + &mut ledger, + &[pdm(current, fresh_new)], + BTreeMap::new(), + &files, + ); + assert_eq!(ledger.edits.len(), 1, "rebased, not appended"); + let original = ledger.edits[0].original.as_ref().unwrap().as_str().unwrap(); + let want = if keeps { + "files = [\"sha256:1111\"]\n" + } else { + current + }; + assert_eq!(original, want, "current={current:?}"); + assert_eq!( + ledger.edits[0].new.as_ref().unwrap().as_str().unwrap(), + fresh_new + ); + } + } + + /// A relock that appends a unit after this one moves the fragment's + /// boundary from EOF to the successor's header: the kept pristine body + /// takes the fresh boundary, so rollback cannot eat that header. + #[test] + fn pdm_rebase_keeps_the_fresh_fragment_boundary() { + let pristine = "[[package]]\nfiles = [\"sha256:1111\"]\n"; + let wired = "[[package]]\nfiles = [\"sha256:cccc\"]\nurl = \"https://patch.test/u.whl\"\n"; + let current = "[[package]]\nfiles = [ \"sha256:cccc\" ]\n# note\n\n[[package]]"; + let fresh_new = "[[package]]\nfiles = [\"sha256:cccc\"]\nurl = \"https://patch.test/u.whl\"\n# note\n\n[[package]]"; + let mut ledger = RedirectState::new(); + ledger.edits.push(pdm(pristine, wired)); + let files = BTreeMap::from([("pdm.lock".to_string(), current.to_string())]); + merge( + &mut ledger, + &[pdm(current, fresh_new)], + BTreeMap::new(), + &files, + ); + assert_eq!( + ledger.edits[0].original.as_ref().unwrap().as_str().unwrap(), + "[[package]]\nfiles = [\"sha256:1111\"]\n# note\n\n[[package]]" + ); + } } diff --git a/crates/socket-patch-cli/tests/in_process_redirect_pdm.rs b/crates/socket-patch-cli/tests/in_process_redirect_pdm.rs index a42a8f6c9..999633706 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect_pdm.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect_pdm.rs @@ -556,3 +556,158 @@ async fn assert_relock_roundtrip(lock: &str, relocked: &str) { "rollback restores the relocked lock byte for byte" ); } + +/// #331: `pdm add ` / `pdm lock --update-reuse` re-render the +/// redirected unit but KEEP the patch data. PDM 2.26+ keeps the Socket `url` +/// and the patched sha256; PDM 2.12–2.20 drop the `url` but keep the patched +/// sha256. The re-scan then records that still-patched unit as its +/// `original`, so a rebase that adopted it would make `rollback` "revert" +/// patched → patched, report success and delete the ledger. The rebase must +/// keep the ledger's pristine `original` instead (with the relocked line +/// endings), so `rollback` lands on the upstream registry unit inside the +/// user's re-rendered lock. +#[tokio::test] +#[serial] +async fn rerender_keeping_the_patch_then_rescan_rolls_back_to_the_registry() { + for start in ["\r\n", "\n"] { + for keep_url in [true, false] { + for later in [false, true] { + let lock = LOCK.replace("\r\n", "\n").replace('\n', start); + assert_rerender_roundtrip(&lock, keep_url, later).await; + } + } + } +} + +/// A `pdm add`ed package that sorts AFTER `urllib3`: PDM appends its unit, +/// which moves the `urllib3` fragment's boundary from EOF to a header. +const ZIPP: &str = "\n[[package]]\nname = \"zipp\"\nversion = \"3.20.2\"\nrequires_python = \">=3.8\"\nsummary = \"Backport of pathlib-compatible object wrapper for zip files\"\ngroups = [\"default\"]\nfiles = [\n {file = \"zipp-3.20.2-py3-none-any.whl\", hash = \"sha256:a817ac80d6cf4b23bf7f2828b7cabf326f15a001bea8b1f9b49631780ba28350\"},\n]\n"; + +/// The end of the `urllib3` unit starting at `at`: the next unit's header +/// (keeping the blank line before it with the successor) or EOF. +fn unit_end(lock: &str, at: usize) -> usize { + lock[at + 1..] + .find("\n\n[[package]]") + .map_or(lock.len(), |i| at + 1 + i + 1) +} + +/// Where the `urllib3` unit starts in `lock` (it is the last unit). +fn urllib3_unit(lock: &str) -> usize { + lock.find("[[package]]\nname = \"urllib3\"") + .expect("urllib3 unit") +} + +/// What `pdm add six==1.16.0` leaves: a new content hash, a `six` unit ahead +/// of `urllib3`, and the redirected `urllib3` unit re-laid by PDM — its +/// `files` back in PDM's one-entry-per-line layout, the Socket `url` moved +/// after `version` (2.26+) or dropped (2.12–2.20), and the patched sha256 +/// kept either way. PDM always writes LF. (Measured with real PDM 2.29.2.) +fn pdm_add_rerender(redirected: &str, keep_url: bool) -> String { + const SIX: &str = "[[package]]\nname = \"six\"\nversion = \"1.16.0\"\nrequires_python = \">=2.7, !=3.0.*, !=3.1.*, !=3.2.*\"\nsummary = \"Python 2 and 3 compatibility utilities\"\ngroups = [\"default\"]\nfiles = [\n {file = \"six-1.16.0-py2.py3-none-any.whl\", hash = \"sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254\"},\n {file = \"six-1.16.0.tar.gz\", hash = \"sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926\"},\n]\n\n"; + const WHEEL: &str = "urllib3-1.26.18-py2.py3-none-any.whl"; + let text = redirected.replace("\r\n", "\n").replace( + "68a0e962e677b7f765a49a6df99753b78d8a99dfe60e5694e6994dcc8efb44bc", + &"d".repeat(64), + ); + let at = urllib3_unit(&text); + let (head, unit) = text.split_at(at); + let socket_files = format!( + "files = [{{ file = \"{WHEEL}\", hash = \"sha256:{}\" }}]\n", + sha256() + ); + let pdm_files = format!( + "files = [\n {{file = \"{WHEEL}\", hash = \"sha256:{}\"}},\n]\n", + sha256() + ); + assert!(unit.contains(&socket_files), "{unit}"); + let url_line = format!("url = \"{HOSTED_URL}\"\n"); + assert!(unit.contains(&url_line), "{unit}"); + let mut unit = unit + .replace(&socket_files, &pdm_files) + .replace(&url_line, ""); + if keep_url { + // PDM's own key order puts the url right after the version. + let version = "version = \"1.26.18\"\n"; + unit = unit.replacen(version, &format!("{version}{url_line}"), 1); + } + format!("{head}{SIX}{unit}") +} + +/// [`pdm_add_rerender`], optionally also adding [`ZIPP`] after `urllib3`. +fn pdm_add_rerender_with(redirected: &str, keep_url: bool, later: bool) -> String { + let rerendered = pdm_add_rerender(redirected, keep_url); + if later { + format!("{rerendered}{ZIPP}") + } else { + rerendered + } +} + +async fn assert_rerender_roundtrip(lock: &str, keep_url: bool, later: bool) { + let server = MockServer::start().await; + mock_api(&server).await; + let tmp = tempfile::tempdir().unwrap(); + write_project(tmp.path(), lock); + let lock_path = tmp.path().join("pdm.lock"); + let ledger_path = tmp.path().join(".socket/vendor/redirect-state.json"); + let case = format!( + "keep_url={keep_url} later={later} crlf={}", + lock.contains('\r') + ); + + assert_eq!( + run(hosted_args(tmp.path(), server.uri(), None)).await, + 0, + "{case}" + ); + let redirected = read(&lock_path); + assert!(redirected.contains(HOSTED_URL), "{case}"); + + let rerendered = pdm_add_rerender_with(&redirected, keep_url, later); + std::fs::write(&lock_path, &rerendered).unwrap(); + + // The re-scan converges the re-laid unit back onto the Socket wiring. + assert_eq!( + run(hosted_args(tmp.path(), server.uri(), None)).await, + 0, + "{case}" + ); + let rescanned = read(&lock_path); + assert!(rescanned.contains(HOSTED_URL), "{case}: {rescanned}"); + let ledger: serde_json::Value = serde_json::from_str(&read(&ledger_path)).unwrap(); + for edit in ledger["edits"].as_array().unwrap() { + let original = edit["original"].as_str().unwrap(); + assert!( + !original.contains(HOSTED_URL) && !original.contains(&sha256()), + "{case}: the ledger's original must stay the pristine unit: {ledger}" + ); + } + + let code = rollback::run(RollbackArgs { + targets: Vec::new(), + common: global(tmp.path(), server.uri()), + one_off: false, + preserve_state: false, + }) + .await; + assert_eq!( + code, 0, + "{case}: rollback after re-render + re-scan must succeed" + ); + let rolled_back = read(&lock_path); + assert!( + !rolled_back.contains(HOSTED_URL) && !rolled_back.contains(&sha256()), + "{case}: rollback must remove the Socket url and patched hash: {rolled_back}" + ); + // Exactly the user's re-rendered lock with the pristine (LF) unit back, + // every other unit (a later one's header included) untouched. + let pristine = LOCK.replace("\r\n", "\n"); + let at = urllib3_unit(&rerendered); + let expected = format!( + "{}{}{}", + &rerendered[..at], + &pristine[urllib3_unit(&pristine)..], + &rerendered[unit_end(&rerendered, at)..] + ); + assert_eq!(rolled_back, expected, "{case}"); +} diff --git a/docs/testing/pdm-compatibility.md b/docs/testing/pdm-compatibility.md index 78a5a8c9d..a70425af8 100644 --- a/docs/testing/pdm-compatibility.md +++ b/docs/testing/pdm-compatibility.md @@ -66,6 +66,14 @@ Measured details: (pristine → current, never an appended chain), so `rollback` still lands on the pristine lock afterwards. This matters most for CRLF locks, which PDM re-renders to a different byte layout on relock. +- **`pdm add` and `pdm lock --update-reuse` keep the patch but re-lay its + unit.** PDM 2.26+ keeps the Socket `url` and patched hash; 2.12–2.20 drop + the `url` and keep the patched hash (a registry unit pinned to the patched + wheel, which `pdm sync` refuses). The re-scan rewires the unit and keeps + the ledger's recorded pristine unit, in the relocked line endings, as the + rollback target. It adopts the re-laid unit only once that unit no longer + carries anything the redirect introduced. So `rollback` restores the + upstream unit inside the user's re-rendered lock. - **Hosted mode verifies the lock's file hash** on install for every supported release (tamper the hash and `pdm sync` fails closed). Vendored mode's protection is the committed wheel bytes, verified by the same hash.