diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index dff38f2c8..212b49d0e 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -850,7 +850,7 @@ A bare `rollback` (or a scoped one, for its scope) restores the SYSTEM to unpatc 2. **Agent leg** — the existing in-place restore machinery, unchanged (v5.0 presentation: the human `No patches found in manifest` line prints only for an unscoped run with no work in ANY leg — a run whose work is all vendored/hosted stays quiet about the manifest): multi-copy restore, release-variant narrowing, the before-blob gate (+ on-demand download; a gate abort still exits 1 with per-package `missing_blob` failure results **and** skips manifest cleanup + GC entirely — nothing was restored, and the retry's revert data must survive), local-go redirect drop, and the `not_installed` exit-0 asymmetry verbatim. Vendor-owned purls are still excluded here (see the vendored-mode section) — they are handled by the next leg instead of being punted to other commands. 3. **Vendored leg** — each in-scope ledger entry (embedded-record entries included) is reverted through the vendor backends: lockfile wiring restored, artifact dir deleted (and its emptied `.socket/vendor//` husk pruned, v5.0), ledger entry dropped + persisted per purl (crash-consistent, like `vendor --revert`). A **drift-keep** (the backend refused a drifted lock) keeps the entry, the artifact, AND the manifest record (`vendoredKept`, exit 1 — the system is still patched); a failure is recorded and other entries proceed. 4. **Hosted leg** — each in-scope hosted pin is restored to its default upstream registry entry; see "Hosted unwind coverage" below. After a hosted leg with no failure, a wet run deletes a pre-v5 `redirect-state.json` once no lockfile pins a hosted patch any more (a failed delete is the `legacy_redirect_ledger_kept` warning). -5. **Manifest cleanup** — entries are removed ONLY for in-scope purls whose legs fully succeeded, were not-installed, or were release-variant siblings narrowed away by an attempted variant that succeeded (half a variant group never lingers — `remove` parity); drift-kept and failed purls keep their records, and a failed variant holds its whole group. No-op removals never rewrite the file. A failed write surfaces as `manifest_write_failed` (warning + `partial_failure` exit 1; GC still runs against the unchanged manifest). +5. **Manifest cleanup** — entries are removed ONLY for in-scope purls whose legs fully succeeded, were not-installed, or were release-variant siblings narrowed away by an attempted variant that succeeded (half a variant group never lingers — `remove` parity); drift-kept and failed purls keep their records, and a failed variant holds its whole group. A manifest record that a live hosted pin has superseded (the lockfile wires the same package release to a different patch uuid, e.g. an agent → hosted migration after the patch was replaced) and whose installed copy holds neither side of the record is not restored in place and does not fail the run: the hosted leg's lock restore and the next install unwind it, and the record leaves the manifest with the `rollback_record_superseded` warning (`remove` does the same instead of aborting before its hosted leg). A copy still holding the record's patched bytes is restored as usual. No-op removals never rewrite the file. A failed write surfaces as `manifest_write_failed` (warning + `partial_failure` exit 1; GC still runs against the unchanged manifest). 6. **GC** — blob, diff and legacy package-archive sweeps against the post-removal manifest, using the same artifact-reference policy as `remove`, retaining beforeHash blobs for (a) removed-but-not-installed entries (a crawler miss must not destroy the only local revert data — `remove` parity) and (b) EVERY entry remaining in the post-removal manifest — still-active patches (failed, drift-kept, eco-/path-excluded) keep their revert data, so a scoped or failed run never destroys the blobs a later rollback needs; only blobs referenced solely by genuinely-removed entries are swept. GC errors warn (`cleanup_failed`) and continue — they never affect the exit (repair's posture). **Confirmation prompt.** A wet, non-preserve run with work prompts once, remove-style, composing only the clauses that apply into one English list (`a and b`, `a, b, and c`) with counted nouns: `Roll back N patches`, `remove them from the local manifest`, `delete M vendored artifacts and their ledger records`, `restore H hosted packages to the upstream registry` (e.g. `Roll back 1 patch, remove it from the local manifest, and restore 1 hosted package to the upstream registry?`) — default yes, auto-accepted under `--yes`/`--json`/non-TTY (the shared `confirm` semantics; CI unaffected). Decline prints `Rollback cancelled.` and exits 0. `--dry-run` and `--preserve-state` runs are prompt-free (they delete no local state). @@ -886,7 +886,7 @@ v5.0 replaces v4's per-purl reverts and whole-ledger reverse replay (`revert_rem | Key | Shape | Meaning | |---|---|---| -| `warnings` | `[{code, detail}]` | Run-level warnings, now populated (previously always empty): `reinstall_required`, `hosted_state_not_preservable`, `out_of_scope_copies_restored`, `vendor_state_unreadable`, `cleanup_failed`, `manifest_write_failed`, `legacy_redirect_ledger_kept`, the upstream-restore advisories (`npm_allow_remote_left`, `pnpm_trust_lockfile_left`, `maven_trusted_checksums_left`, `nuget_default_config_left`, `upstream_uv_override_removed`), `ownership_not_restored` (a restored file whose ownership could not be put back — see the apply warnings), plus vendored/hosted leg advisories. New codes are additive (MINOR) | +| `warnings` | `[{code, detail}]` | Run-level warnings, now populated (previously always empty): `reinstall_required`, `hosted_state_not_preservable`, `out_of_scope_copies_restored`, `vendor_state_unreadable`, `cleanup_failed`, `manifest_write_failed`, `legacy_redirect_ledger_kept`, the upstream-restore advisories (`npm_allow_remote_left`, `pnpm_trust_lockfile_left`, `maven_trusted_checksums_left`, `nuget_default_config_left`, `upstream_uv_override_removed`), `ownership_not_restored` (a restored file whose ownership could not be put back — see the apply warnings), `rollback_record_superseded` (a manifest record superseded by a live hosted pin, left to the hosted leg — see Manifest cleanup), plus vendored/hosted leg advisories. New codes are additive (MINOR) | | `vendored` | `[purl]` | **Meaning narrowed (MAJOR)**: vendor-owned purls the run did NOT act on — today exactly the corrupt-vendor-ledger skip. | | `vendoredReverted` | `[purl]` | Ledger entries cleanly reverted this run (unwired + artifact deleted + entry dropped; previewed on dry-run) | | `vendoredPreserved` | `[purl]` | `--preserve-state`: unwired with artifact + ledger entry kept | diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index c5fbed562..d69b1183e 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -603,6 +603,7 @@ pub async fn run(args: RemoveArgs) -> i32 { &manifest, &vendored_keys, InnerSelection::Identifier(Some(&args.identifier)), + &super::rollback::superseded_by_hosted(&manifest, &hosted_pins), Some(&telemetry_client), ) .await diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 29369279c..ab654edb6 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -14,8 +14,10 @@ use socket_patch_core::patch::rollback::{ VerifyRollbackResult, VerifyRollbackStatus, }; use socket_patch_core::telemetry::{track_patch_rollback_failed, track_patch_rolled_back}; +use socket_patch_core::utils::composer_version::composer_purls_equivalent; use socket_patch_core::utils::purl::{patch_matches, strip_purl_qualifiers}; use socket_patch_core::vendor::{purl_keys_cover, RevertOpts, VendorState}; +use socket_patch_core::vex::discover::canonical_base_purl; use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::time::Duration; @@ -441,8 +443,13 @@ pub(crate) struct RollbackOutcome { /// the revert data the retry needs. pub(crate) aborted: bool, /// Run warnings `(code, detail)` for copies left alone without failing - /// the run (today: `gradle_m2_copy_not_restored`). + /// the run (`gradle_m2_copy_not_restored`, `rollback_record_superseded`). pub(crate) warnings: Vec<(String, String)>, + /// In-scope manifest entries superseded by a live hosted pin whose + /// installed copies hold neither side of the recorded patch (#933): + /// left to the hosted leg's lock restore instead of failing, and + /// removable from the manifest like a rolled-back entry. Sorted. + pub(crate) superseded: Vec, } /// How `rollback_patches_inner` selects manifest entries. @@ -1449,6 +1456,7 @@ pub async fn run(args: RollbackArgs) -> i32 { &manifest, &vendored_keys, selection, + &superseded_by_hosted(&manifest, &hosted_pins), Some(&telemetry_client), ) .await @@ -1461,6 +1469,7 @@ pub async fn run(args: RollbackArgs) -> i32 { narrowed_out, aborted, warnings: agent_warnings, + superseded, }) => { // Copies left alone without failing the run (an unconsumed // `~/.m2` copy: `gradle_m2_copy_not_restored`). @@ -1562,6 +1571,7 @@ pub async fn run(args: RollbackArgs) -> i32 { } succeeded_purls.contains(*purl) || not_installed.contains(purl) + || superseded.contains(purl) || (narrowed_out.contains(purl) && !failed_bases.contains(strip_purl_qualifiers(purl))) }) @@ -2015,6 +2025,10 @@ pub(crate) async fn rollback_patches_inner( manifest: &PatchManifest, vendored_keys: &HashSet, selection: InnerSelection<'_>, + // Manifest purl -> the hosted uuid a live lockfile pin superseded its + // record with ([`superseded_by_hosted`]); empty when no hosted pin + // replaces a recorded patch. + superseded: &HashMap, // The client the caller already built. Constructing one per phase // printed the core client's "No SOCKET_API_TOKEN set" notice once per // construction — twice in a single rollback. `None` builds one on @@ -2064,6 +2078,7 @@ pub(crate) async fn rollback_patches_inner( narrowed_out: Vec::new(), aborted: false, warnings: Vec::new(), + superseded: Vec::new(), }); } @@ -2091,6 +2106,7 @@ pub(crate) async fn rollback_patches_inner( narrowed_out: Vec::new(), aborted: false, warnings: Vec::new(), + superseded: Vec::new(), }); } @@ -2490,6 +2506,7 @@ pub(crate) async fn rollback_patches_inner( narrowed_out: Vec::new(), aborted: true, warnings: Vec::new(), + superseded: Vec::new(), }); } @@ -2570,6 +2587,7 @@ pub(crate) async fn rollback_patches_inner( narrowed_out: Vec::new(), aborted: true, warnings: Vec::new(), + superseded: Vec::new(), }); } } @@ -2590,6 +2608,7 @@ pub(crate) async fn rollback_patches_inner( narrowed_out: narrowed_out.clone(), aborted: false, warnings: Vec::new(), + superseded: Vec::new(), }); } @@ -2597,6 +2616,7 @@ pub(crate) async fn rollback_patches_inner( let mut results: Vec = Vec::new(); let mut has_errors = false; let mut warnings: Vec<(String, String)> = Vec::new(); + let mut superseded_left: Vec = Vec::new(); for target in &rollback_targets { let (purl, pkg_path) = (&target.purl, &target.dir); @@ -2638,6 +2658,12 @@ pub(crate) async fn rollback_patches_inner( warnings.push(warning); continue; } + let files = target.files.as_ref().unwrap_or(&patch.files); + if let Some(warning) = superseded_record_skip(target, &result, files, superseded).await { + warnings.push(warning); + superseded_left.push(purl.clone()); + continue; + } if !result.success { has_errors = true; // Under --silent (the summary muted) this line is the run's @@ -2685,6 +2711,8 @@ pub(crate) async fn rollback_patches_inner( results.push(result); } + superseded_left.sort(); + superseded_left.dedup(); Ok(RollbackOutcome { success: !has_errors, results, @@ -2693,6 +2721,7 @@ pub(crate) async fn rollback_patches_inner( narrowed_out, aborted: false, warnings, + superseded: superseded_left, }) } @@ -2771,6 +2800,123 @@ fn unconsumed_m2_skip(target: &CopyTarget, result: &RollbackResult) -> Option<(S }) } +/// Manifest records a live hosted pin has superseded (#933): purl -> the +/// hosted uuid the lockfiles wire for the same package release, when no +/// hosted pin for that release carries the record's own uuid. An agent → +/// hosted migration whose patch was replaced meanwhile leaves exactly this: +/// record A in the manifest, the lock pinning B. `vex` reports the same +/// state as `vex_record_superseded`. +pub(crate) fn superseded_by_hosted( + manifest: &PatchManifest, + pins: &[HostedPin], +) -> HashMap { + manifest + .patches + .iter() + .filter_map(|(purl, record)| { + let pkg = canonical_base_purl(purl); + let same: Vec<&HostedPin> = pins + .iter() + .filter(|pin| pin.purl == pkg || composer_purls_equivalent(&pin.purl, &pkg)) + .collect(); + if same.iter().any(|pin| pin.uuid == record.uuid) { + return None; + } + same.first().map(|pin| (purl.clone(), pin.uuid.clone())) + }) + .collect() +} + +/// The run warning that replaces a failed in-place restore of a manifest +/// record a live hosted pin superseded ([`superseded_by_hosted`]), when it +/// failed before writing anything because the installed copy holds bytes +/// that are neither side of the record (the superseding patch's, after a +/// reinstall), lacks a file, or is a Gradle hash directory this record +/// never patched (`gradle_rollback_hash_mismatch` with no file at the +/// record's patched bytes). The hosted leg's lock restore and the reinstall +/// it asks for unwind that copy; restoring the record's original bytes over +/// the superseding patch's would only mix the two. `None` for any other +/// result: a copy still holding the record's patched bytes (including a +/// swapped jar with no backup, `jvm_jar_backup_missing`) is restored or +/// fails as usual, and so does a file that cannot be read. +async fn superseded_record_skip( + target: &CopyTarget, + result: &RollbackResult, + files: &HashMap, + superseded: &HashMap, +) -> Option<(String, String)> { + let wired = superseded.get(&target.purl)?; + if result.success || !result.files_rolled_back.is_empty() { + return None; + } + if result + .files_verified + .iter() + .any(|v| v.status == VerifyRollbackStatus::NotFound && !v.is_absent()) + { + return None; + } + let mismatched = result + .files_verified + .iter() + .any(|v| v.status == VerifyRollbackStatus::HashMismatch || v.is_absent()); + // A Gradle hash directory is refused before verification when the + // record's before-blob does not hash to its name: normally the + // superseding patch's own download, which this record never patched. + // It is left only if no file there still holds the record's patched + // bytes (a corrupt blob for the directory the record DID patch fails + // as usual). + let foreign_gradle_dir = result + .error + .as_deref() + .is_some_and(|e| e.starts_with("gradle_rollback_hash_mismatch")) + && !holds_patched_bytes(target, files).await; + (mismatched || foreign_gradle_dir).then(|| { + ( + "rollback_record_superseded".to_string(), + format!( + "{}: the recorded patch is superseded by the lockfile-wired hosted patch {wired}; \ + left the installed copy at {} to the lockfile restore (the next \ + package-manager install puts the original files back)", + target.purl, + target.dir.display() + ), + ) + }) +} + +/// Whether any of `files` in `target`'s copy is at the record's patched +/// bytes, or cannot be checked (an unsafe key, a read error other than +/// "not found"), which may hide them. +async fn holds_patched_bytes(target: &CopyTarget, files: &HashMap) -> bool { + for (file, info) in files { + let key = maven_target_key(&target.purl, &target.dir, file); + let rel = Path::new(key.strip_prefix("package/").unwrap_or(&key)); + if rel.as_os_str().is_empty() + || !rel + .components() + .all(|c| matches!(c, std::path::Component::Normal(_))) + { + return true; + } + // FIFO-safe: a FIFO or device planted at the leaf is refused, not + // opened (a bare read would block forever), and counts as possibly + // patched below. + match socket_patch_core::utils::fs::read_regular_to_bytes(&target.dir.join(rel)).await { + Ok(bytes) => { + if socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes(&bytes) + == info.after_hash + { + return true; + } + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(_) => return true, + } + } + false +} + /// The key `file` of a Maven record as it is joined onto `dir`: a Gradle /// hash dir holds the bare file name (`package/` dropped). fn maven_target_key(purl: &str, dir: &Path, file: &str) -> String { @@ -3083,6 +3229,7 @@ mod tests { &manifest, &vendored_keys, InnerSelection::Identifier(identifier), + &HashMap::new(), None, ) .await?; @@ -3249,6 +3396,166 @@ mod tests { } } + fn superseded_map() -> HashMap { + HashMap::from([( + "pkg:npm/foo@1.0.0".to_string(), + "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb".to_string(), + )]) + } + + /// A record of one file, `package/index.js`, patched from `before` to + /// `after`, and a copy dir holding `installed` as that file. + fn superseded_copy( + installed: &[u8], + ) -> ( + tempfile::TempDir, + CopyTarget, + HashMap, + ) { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("index.js"), installed).unwrap(); + let files = HashMap::from([( + "package/index.js".to_string(), + PatchFileInfo { + before_hash: socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes( + b"original", + ), + after_hash: socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes( + b"patched by A", + ), + }, + )]); + let target = CopyTarget::plain("pkg:npm/foo@1.0.0", tmp.path()); + (tmp, target, files) + } + + fn refused(error: &str) -> RollbackResult { + let mut result = make_result(&[], &[]); + result.success = false; + result.error = Some(error.to_string()); + result + } + + #[tokio::test] + async fn superseded_skip_covers_a_copy_holding_neither_side() { + let (tmp, target, files) = superseded_copy(b"patched by B"); + let result = make_result(&[VerifyRollbackStatus::HashMismatch], &[]); + let (code, detail) = superseded_record_skip(&target, &result, &files, &superseded_map()) + .await + .expect("a superseded record's mismatched copy is left to the hosted leg"); + assert_eq!(code, "rollback_record_superseded"); + assert!( + detail.contains("bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb"), + "{detail}" + ); + } + + #[tokio::test] + async fn superseded_skip_covers_a_gradle_dir_the_record_never_patched() { + let (tmp, target, files) = superseded_copy(b"patched by B"); + let result = refused("gradle_rollback_hash_mismatch: the before-blob for x does not hash"); + assert!( + superseded_record_skip(&target, &result, &files, &superseded_map()) + .await + .is_some() + ); + } + + #[tokio::test] + async fn superseded_skip_keeps_jvm_copies_holding_the_patched_bytes() { + // The record's own patched bytes are still there: a corrupt blob for + // the directory it patched, or a swapped jar with no backup, fails. + let (tmp, target, files) = superseded_copy(b"patched by A"); + for error in [ + "gradle_rollback_hash_mismatch: the before-blob for x does not hash", + "jvm_jar_backup_missing: no original of lib-1.0.jar", + ] { + let result = refused(error); + assert!( + superseded_record_skip(&target, &result, &files, &superseded_map()) + .await + .is_none(), + "{error}" + ); + } + } + + #[cfg(unix)] + #[tokio::test] + async fn superseded_skip_never_blocks_on_a_fifo() { + // A FIFO where the record's file should be is unverifiable: the + // check must refuse it, not block in open(2), and must not skip. + let (tmp, target, files) = superseded_copy(b"patched by B"); + std::fs::remove_file(tmp.path().join("index.js")).unwrap(); + let made = std::process::Command::new("mkfifo") + .arg(tmp.path().join("index.js")) + .status() + .expect("run mkfifo"); + assert!(made.success()); + let result = refused("gradle_rollback_hash_mismatch: the before-blob for x does not hash"); + let skip = tokio::time::timeout( + std::time::Duration::from_secs(10), + superseded_record_skip(&target, &result, &files, &superseded_map()), + ) + .await + .expect("the patched-bytes probe must not block on a FIFO"); + assert!(skip.is_none()); + } + + #[tokio::test] + async fn superseded_skip_leaves_other_failures_and_records_alone() { + let (_tmp, target, files) = superseded_copy(b"patched by B"); + let mismatch = make_result(&[VerifyRollbackStatus::HashMismatch], &[]); + // Not superseded: the mismatch fails as before. + assert!( + superseded_record_skip(&target, &mismatch, &files, &HashMap::new()) + .await + .is_none() + ); + // A missing before-blob is a real failure even when superseded. + let missing = make_result(&[VerifyRollbackStatus::MissingBlob], &[]); + assert!( + superseded_record_skip(&target, &missing, &files, &superseded_map()) + .await + .is_none() + ); + // A file that exists but cannot be read may still hold A's bytes. + let mut unreadable = make_result(&[VerifyRollbackStatus::NotFound], &[]); + unreadable.files_verified[0].message = Some("Failed to hash file: EACCES".to_string()); + assert!( + superseded_record_skip(&target, &unreadable, &files, &superseded_map()) + .await + .is_none() + ); + } + + #[test] + fn superseded_by_hosted_needs_a_pin_with_another_uuid() { + let mut manifest = PatchManifest::new(); + manifest + .patches + .insert("pkg:npm/foo@1.0.0".to_string(), make_record("aaaa")); + manifest + .patches + .insert("pkg:npm/bar@2.0.0".to_string(), make_record("cccc")); + let pin = |purl: &str, uuid: &str| HostedPin { + purl: purl.to_string(), + uuid: uuid.to_string(), + files: vec!["package-lock.json".to_string()], + }; + let pins = vec![ + pin("pkg:npm/foo@1.0.0", "bbbb"), + // bar's hosted pin carries the record's own uuid: not superseded. + pin("pkg:npm/bar@2.0.0", "cccc"), + ]; + let map = superseded_by_hosted(&manifest, &pins); + assert_eq!( + map, + HashMap::from([("pkg:npm/foo@1.0.0".to_string(), "bbbb".to_string())]) + ); + assert!(superseded_by_hosted(&manifest, &[]).is_empty()); + } + #[test] fn all_files_already_original_true_when_every_file_matches() { let r = make_result( diff --git a/crates/socket-patch-cli/tests/in_process_rollback_hosted.rs b/crates/socket-patch-cli/tests/in_process_rollback_hosted.rs index d1b5b1607..5091adb59 100644 --- a/crates/socket-patch-cli/tests/in_process_rollback_hosted.rs +++ b/crates/socket-patch-cli/tests/in_process_rollback_hosted.rs @@ -1372,3 +1372,199 @@ async fn npm_hosted_round_trip_manifest_less_vex() { .expect("manifest-less VEX cells panicked"); }); } + +// --------------------------------------------------------------------------- +// Agent record superseded by a hosted pin (#933) +// --------------------------------------------------------------------------- +// +// An agent-mode apply recorded patch A in `.socket/manifest.json`; a later +// hosted scan pinned the same `name@version` to a SUPERSEDING patch (the +// fixture's UUID, B) and left record A in place. After a reinstall the tree +// holds B's bytes, which are neither of A's sides, so restoring A in place +// would fail "modified after patching". The hosted leg owns that package +// now: rollback and remove restore the lock, drop the superseded record +// with a warning, and exit 0. + +const SUPERSEDED_UUID: &str = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa"; +const ORIGINAL_INDEX: &[u8] = b"module.exports = 'original';\n"; +const A_PATCHED_INDEX: &[u8] = b"module.exports = 'patched by A';\n"; +const B_PATCHED_INDEX: &[u8] = b"module.exports = 'patched by B';\n"; + +/// The npm project wired by a real hosted scan to patch B, with agent +/// record A left in the manifest (its before-blob cached, as agent apply +/// leaves it) and `installed` as the installed `index.js`. Returns the +/// pristine lock bytes. +async fn write_superseded_agent_fixture( + root: &Path, + server: &MockServer, + installed: &[u8], +) -> String { + let pristine = write_npm_project(root); + let pkg = root.join("node_modules").join(NAME); + std::fs::write(pkg.join("index.js"), A_PATCHED_INDEX).unwrap(); + + let before = socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes(ORIGINAL_INDEX); + let after = socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes(A_PATCHED_INDEX); + let mut record = patch_record(SUPERSEDED_UUID, GHSA); + record.files.clear(); + record.files.insert( + "package/index.js".to_string(), + PatchFileInfo { + before_hash: before.clone(), + after_hash: after, + }, + ); + let mut manifest = socket_patch_core::manifest::schema::PatchManifest::new(); + manifest.patches.insert(PURL.to_string(), record); + std::fs::create_dir_all(root.join(".socket/blobs")).unwrap(); + std::fs::write(root.join(".socket/blobs").join(&before), ORIGINAL_INDEX).unwrap(); + std::fs::write( + root.join(".socket/manifest.json"), + serde_json::to_string_pretty(&manifest).unwrap(), + ) + .unwrap(); + + mock_discovery(server).await; + mock_reference(server).await; + mock_view(server).await; + let code = scan_run(hosted_scan_args(root, server.uri())).await; + assert_eq!(code, 0, "the superseding hosted scan should succeed"); + let wired = std::fs::read_to_string(root.join("package-lock.json")).unwrap(); + assert!( + wired.contains(HOSTED_URL), + "the lock must pin the superseding hosted patch; got:\n{wired}" + ); + let manifest = std::fs::read_to_string(root.join(".socket/manifest.json")).unwrap(); + assert!( + manifest.contains(SUPERSEDED_UUID), + "the hosted scan leaves the superseded agent record A in place; got:\n{manifest}" + ); + + // What the next `npm ci` (or no reinstall at all) leaves installed. + std::fs::write(pkg.join("index.js"), installed).unwrap(); + mock_npm_registry(server).await; + pristine +} + +fn manifest_patch_keys(root: &Path) -> Vec { + let raw = std::fs::read_to_string(root.join(".socket/manifest.json")).unwrap(); + let v: Value = serde_json::from_str(&raw).unwrap(); + v["patches"] + .as_object() + .map(|m| m.keys().cloned().collect()) + .unwrap_or_default() +} + +/// Run `remove --json --yes` online as a scrubbed subprocess. +fn run_remove_subprocess_online(cwd: &Path, server: &MockServer, identifier: &str) -> (i32, Value) { + let out = scrubbed_cli() + .env( + "SOCKET_NPM_REGISTRY", + format!("{}/npm-registry", server.uri()), + ) + .args([ + "remove", + identifier, + "--json", + "--yes", + "--patch-server-url", + "http://patch.test", + "--cwd", + cwd.to_str().unwrap(), + ]) + .output() + .expect("run socket-patch"); + let envelope: Value = serde_json::from_slice(&out.stdout).unwrap_or_else(|e| { + panic!( + "remove --json stdout must be a pure JSON envelope: {e}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ) + }); + (out.status.code().unwrap_or(-1), envelope) +} + +/// #933: after the reinstall the tree holds B's bytes. Rollback restores +/// the lock, drops record A with `rollback_record_superseded`, leaves the +/// installed bytes for the reinstall to replace, and exits 0 — twice. +#[tokio::test] +#[serial] +async fn rollback_drops_an_agent_record_superseded_by_a_hosted_pin() { + let server = MockServer::start().await; + let tmp = tempfile::tempdir().unwrap(); + let pristine = write_superseded_agent_fixture(tmp.path(), &server, B_PATCHED_INDEX).await; + + let (code, envelope) = run_rollback_subprocess_online(tmp.path(), &server, &[]); + assert_eq!( + code, 0, + "rollback must not fail on the superseded record:\n{envelope:#}" + ); + assert!( + warning_codes(&envelope).contains(&"rollback_record_superseded".to_string()), + "the superseded record is named in warnings[]:\n{envelope:#}" + ); + let restored = std::fs::read_to_string(tmp.path().join("package-lock.json")).unwrap(); + assert_eq!(restored, pristine, "the hosted leg restores the lock"); + assert!( + manifest_patch_keys(tmp.path()).is_empty(), + "the superseded record leaves the manifest" + ); + let installed = + std::fs::read(tmp.path().join("node_modules").join(NAME).join("index.js")).unwrap(); + assert_eq!( + installed, B_PATCHED_INDEX, + "B's installed bytes are the reinstall's to replace, never overwritten with A's original" + ); + + let (code, envelope) = run_rollback_subprocess_online(tmp.path(), &server, &[]); + assert_eq!(code, 0, "a re-run is a clean no-op:\n{envelope:#}"); +} + +/// #933 without a reinstall: the tree still holds A's patched bytes, so +/// the agent leg restores them in place as before. +#[tokio::test] +#[serial] +async fn rollback_restores_a_superseded_agent_record_still_installed() { + let server = MockServer::start().await; + let tmp = tempfile::tempdir().unwrap(); + let pristine = write_superseded_agent_fixture(tmp.path(), &server, A_PATCHED_INDEX).await; + + let (code, envelope) = run_rollback_subprocess_online(tmp.path(), &server, &[]); + assert_eq!(code, 0, "{envelope:#}"); + assert!( + !warning_codes(&envelope).contains(&"rollback_record_superseded".to_string()), + "A's bytes were restored in place, nothing was left to the reinstall:\n{envelope:#}" + ); + let restored = std::fs::read_to_string(tmp.path().join("package-lock.json")).unwrap(); + assert_eq!(restored, pristine); + assert!(manifest_patch_keys(tmp.path()).is_empty()); + let installed = + std::fs::read(tmp.path().join("node_modules").join(NAME).join("index.js")).unwrap(); + assert_eq!(installed, ORIGINAL_INDEX); +} + +/// #933: `remove ` un-hosts the package instead of aborting on the +/// superseded agent record before the hosted leg runs. +#[tokio::test] +#[serial] +async fn remove_unhosts_a_package_whose_agent_record_is_superseded() { + let server = MockServer::start().await; + let tmp = tempfile::tempdir().unwrap(); + let pristine = write_superseded_agent_fixture(tmp.path(), &server, B_PATCHED_INDEX).await; + + let (code, envelope) = run_remove_subprocess_online(tmp.path(), &server, PURL); + assert_eq!( + code, 0, + "remove must not refuse the superseded record:\n{envelope:#}" + ); + let restored = std::fs::read_to_string(tmp.path().join("package-lock.json")).unwrap(); + assert_eq!( + restored, pristine, + "remove must not leave the hosted pin live" + ); + assert!(manifest_patch_keys(tmp.path()).is_empty()); + assert!( + envelope.to_string().contains("rollback_record_superseded"), + "the superseded record is reported:\n{envelope:#}" + ); +} diff --git a/crates/socket-patch-core/src/crawlers/gradle_cache.rs b/crates/socket-patch-core/src/crawlers/gradle_cache.rs index ef295ee27..afd7c4fba 100644 --- a/crates/socket-patch-core/src/crawlers/gradle_cache.rs +++ b/crates/socket-patch-core/src/crawlers/gradle_cache.rs @@ -70,8 +70,7 @@ pub fn hash_eq(dir_name: &str, sha1_hex: &str) -> bool { /// Whether `bytes` are the pristine download Gradle stored in the hash /// directory `dir_name` (their sha1 names it). pub fn pristine(dir_name: &str, bytes: &[u8]) -> bool { - use sha1::{Digest, Sha1}; - hash_eq(dir_name, &hex::encode(Sha1::digest(bytes))) + hash_eq(dir_name, &crate::utils::digest::sha1_hex_of(bytes)) } /// Whether `path` is a version directory of a `files-2.1` tree @@ -432,8 +431,6 @@ impl DerivedIndex { /// The [`DerivedCopies`] of the jar `jar_leaf` whose pristine bytes /// hash to `pristine_sha1`. pub fn query(&self, jar_leaf: &str, pristine_sha1: &str) -> DerivedCopies { - use sha1::{Digest, Sha1}; - let instrumented = format!("instrumented-{jar_leaf}"); let mut out = DerivedCopies { incomplete: self.incomplete, @@ -460,7 +457,9 @@ impl DerivedIndex { out.stale.push(path.clone()); } else if name == jar_leaf || name == instrumented { match crate::utils::fs::read_regular_to_bytes_sync(path) { - Ok(bytes) if hash_eq(&hex::encode(Sha1::digest(&bytes)), pristine_sha1) => { + Ok(bytes) + if hash_eq(&crate::utils::digest::sha1_hex_of(&bytes), pristine_sha1) => + { out.stale.push(path.clone()) } Ok(_) => out.unknown.push(path.clone()), diff --git a/crates/socket-patch-core/src/patch/jvm_jar.rs b/crates/socket-patch-core/src/patch/jvm_jar.rs index 82d679406..f38a84403 100644 --- a/crates/socket-patch-core/src/patch/jvm_jar.rs +++ b/crates/socket-patch-core/src/patch/jvm_jar.rs @@ -25,8 +25,6 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use sha1::Digest as _; - use crate::crawlers::gradle_cache; use crate::hash::git_sha256::compute_git_sha256_from_bytes; use crate::manifest::schema::PatchFileInfo; @@ -353,12 +351,11 @@ fn unpatched_members( } fn sha256_hex(bytes: &[u8]) -> String { - use sha2::Digest as _; - hex::encode(sha2::Sha256::digest(bytes)) + crate::utils::digest::sha256_hex_of(bytes) } fn sha1_hex(bytes: &[u8]) -> String { - hex::encode(sha1::Sha1::digest(bytes)) + crate::utils::digest::sha1_hex_of(bytes) } /// `/jvm-originals/.jar`. diff --git a/crates/socket-patch-core/src/patch/sidecars/maven.rs b/crates/socket-patch-core/src/patch/sidecars/maven.rs index f2f5a2466..8798bfce6 100644 --- a/crates/socket-patch-core/src/patch/sidecars/maven.rs +++ b/crates/socket-patch-core/src/patch/sidecars/maven.rs @@ -17,8 +17,6 @@ use std::path::{Path, PathBuf}; -use sha1::Digest as _; - use super::{ SidecarAdvisory, SidecarAdvisoryCode, SidecarError, SidecarFile, SidecarFileAction, SidecarPayload, SidecarSeverity, @@ -44,7 +42,7 @@ impl Algo { fn digest(self, bytes: &[u8]) -> String { match self { - Algo::Sha1 => hex::encode(sha1::Sha1::digest(bytes)), + Algo::Sha1 => crate::utils::digest::sha1_hex_of(bytes), Algo::Md5 => hex::encode(md5(bytes)), } }