diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index 8ce80d9f1..183a88e8f 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -35,17 +35,17 @@ use crate::ui::{self, plural, print_json, StatusLine}; use super::get::{download_and_apply_patches_with, DownloadParams, DownloadRun}; -pub use self::socket_yml_args::{SocketYmlArgs, MIN_SEVERITY_ENV}; use self::policy::{load_invocation_policy, InvocationPolicy, PolicyLoadError, ScanPolicy}; +pub use self::socket_yml_args::{SocketYmlArgs, MIN_SEVERITY_ENV}; mod discovery; mod gc; pub(crate) mod hosted; pub(crate) mod policy; -mod socket_yml_args; pub(crate) mod render; pub(crate) mod rollout; pub mod rollout_args; +mod socket_yml_args; pub(crate) mod vendor_flow; use self::discovery::{ @@ -65,13 +65,13 @@ use self::gc::gc_json; pub(crate) use self::hosted::boxed_run_redirect_selected; use self::hosted::run_redirect; pub(crate) use self::hosted::{vlt_rollback_heal, vlt_takeover_heal}; -pub(crate) use self::vendor_flow::{ - boxed_vendor_step, preview_vendor_json, print_dry_run_refusals, VendorStep, -}; use self::vendor_flow::{ boxed_vendor_interactive_path, boxed_vendor_json_path, fold_vendored_skips_into_apply, partition_skipped_selected, }; +pub(crate) use self::vendor_flow::{ + boxed_vendor_step, preview_vendor_json, print_dry_run_refusals, VendorStep, +}; /// Packages per batch request on the authenticated API when `--batch-size` /// is not given: the server's own per-request maximum @@ -318,11 +318,7 @@ pub struct ScanArgs { /// `requests`), or a purl with or without its version /// (`pkg:npm/lodash`, `pkg:pypi/requests@2.31.0`). Repeat the flag or /// separate with commas - #[arg( - long = "package", - env = "SOCKET_SCAN_PACKAGES", - value_delimiter = ',' - )] + #[arg(long = "package", env = "SOCKET_SCAN_PACKAGES", value_delimiter = ',')] pub packages: Vec, /// On a successful scan, also generate an OpenVEX 0.2.0 document. @@ -500,9 +496,10 @@ async fn discover_selected( telemetry.flush().await; let error_count = failures.len(); if error_count > 0 && error_count == packages.len() { - let err = failures - .last() - .map_or_else(|| "all patch-detail queries failed".to_string(), |(_, e)| e.clone()); + let err = failures.last().map_or_else( + || "all patch-detail queries failed".to_string(), + |(_, e)| e.clone(), + ); let message = format!("all {error_count} patch-detail queries failed: {err}"); if detail_error_line { eprintln!("{}", render::fetch_details_failed(&failures)); @@ -568,7 +565,11 @@ fn classified_rows( packages: &[BatchPackagePatches], result: Option<&mut serde_json::Value>, ) -> Vec { - let failed: Vec = discovered.failed.iter().map(|(purl, _)| purl.clone()).collect(); + let failed: Vec = discovered + .failed + .iter() + .map(|(purl, _)| purl.clone()) + .collect(); stage.incomplete = rollout::lookup_incomplete(&recorded.index, &failed, batch_failed); let rows = rollout::classify(&discovered.offers, &recorded.index, &stage.project); if let Some(result) = result { @@ -1317,7 +1318,8 @@ fn project_dirs(cwd: &Path, paths: &[String]) -> Result, St let joined = cwd.join(raw); if raw.contains(['*', '?', '[']) { let pattern = joined.to_string_lossy().into_owned(); - let matches = glob::glob(&pattern).map_err(|e| format!("invalid path pattern `{raw}`: {e}"))?; + let matches = + glob::glob(&pattern).map_err(|e| format!("invalid path pattern `{raw}`: {e}"))?; let before = dirs.len(); dirs.extend( matches @@ -1390,7 +1392,10 @@ async fn run_project_dirs( } // One budget per invocation (§5.2): the directories spend it in sorted // order, and a package admitted in one is admitted free in the next. - let configured = match args.rollout.resolve_from_env(invocation.policy.max_new_patches()) { + let configured = match args + .rollout + .resolve_from_env(invocation.policy.max_new_patches()) + { Ok(max) => max, Err(message) => { eprintln!("Error: {message}"); @@ -1491,7 +1496,10 @@ async fn run_scan( // error. let configured_cap = match args.rollout.carry.as_ref() { Some(carry) => carry.lock().configured, - None => match args.rollout.resolve_from_env(invocation.policy.max_new_patches()) { + None => match args + .rollout + .resolve_from_env(invocation.policy.max_new_patches()) + { Ok(max) => max, Err(message) => { eprintln!("Error: {message}"); @@ -1499,11 +1507,8 @@ async fn run_scan( } }, }; - let mut stage = rollout::Stage::new( - configured_cap, - args.rollout.carry.clone(), - &args.common.cwd, - ); + let mut stage = + rollout::Stage::new(configured_cap, args.rollout.carry.clone(), &args.common.cwd); // Strict airgap (CLI_CONTRACT.md `--offline`): scan's patch discovery // is remote data, so refuse before the crawl and before the API client @@ -1704,8 +1709,11 @@ async fn run_scan( .filter(|pkg| args.common.purl_ecosystem_selected(&pkg.purl)) .collect(); - let package_specs: Vec<&String> = - args.packages.iter().filter(|s| !s.trim().is_empty()).collect(); + let package_specs: Vec<&String> = args + .packages + .iter() + .filter(|s| !s.trim().is_empty()) + .collect(); let filtered_crawled: Vec<_> = if package_specs.is_empty() { filtered_crawled } else { @@ -1860,13 +1868,12 @@ async fn run_scan( // `redirectState` rides the empty-discovery envelope too // (same rule as the ≥1-package path). `wiringLive` is empty // by construction: this run covered zero packages. - let redirect_state = (!args.common.is_global()).then_some( - crate::commands::hosted_state_from_pins( + let redirect_state = + (!args.common.is_global()).then_some(crate::commands::hosted_state_from_pins( &socket_patch_core::patch::redirect::upstream::HostedPin::all( ctx.discovery().await, ), - ), - ); + )); if let Some(state) = redirect_state_json(redirect_state.as_ref(), &[]) { result["redirectState"] = state; } @@ -2222,7 +2229,8 @@ async fn run_scan( // A report-only run selects nothing, but a severity floor or // `enabled: false` still hides candidates; report them like the // human arm does (the detail fetch runs only then). - if !apply && !vendor && policy.reports_selection() && !all_packages_with_patches.is_empty() { + if !apply && !vendor && policy.reports_selection() && !all_packages_with_patches.is_empty() + { if let Err((code, message)) = discover_selected( &api_client, &all_packages_with_patches, @@ -2515,12 +2523,7 @@ async fn run_scan( &all_packages_with_patches, None, ); - updates = offer_updates( - &rows, - &discovered, - &recorded, - &all_packages_with_patches, - ); + updates = offer_updates(&rows, &discovered, &recorded, &all_packages_with_patches); rows } // `discover_selected` already printed the failure to stderr. @@ -2730,8 +2733,11 @@ async fn run_scan( plan_kept_rows(&mut stage, rows, selected) }; - // Drop selections the manifest already records at the same uuid. - // Agent mode only: vendored mode never reads the manifest. + // Set aside selections the manifest already records at the same uuid. + // A wet agent run still hands them to the download step below, whose + // nested apply re-applies them after a reinstall (#454, #732); a + // preview only names them. Agent mode only: vendored mode never reads + // the manifest. let recorded = |p: &PatchSearchResult| { existing_manifest .as_ref() @@ -2743,17 +2749,18 @@ async fn run_scan( } else { selected.into_iter().partition(|p| recorded(p)) }; + let reapply = !report_only && !args.common.dry_run; if !silent { for p in &already_recorded { open_paragraph(&mut skip_paragraph); println!( "{}", - render::already_recorded_line(&normalize_purl(&p.purl), &p.uuid) + render::already_recorded_line(&normalize_purl(&p.purl), &p.uuid, reapply) ); } } - if selected.is_empty() { + if selected.is_empty() && (!reapply || already_recorded.is_empty()) { if !silent { open_paragraph(&mut skip_paragraph); if !stage.deferred_keys().is_empty() { @@ -2764,6 +2771,8 @@ async fn run_scan( } } else if already_recorded.is_empty() { println!("No patches selected."); + } else if args.common.dry_run && !report_only { + println!("{}", render::ALL_ALREADY_RECORDED_DRY_RUN); } else { println!("{}", render::ALL_ALREADY_RECORDED); } @@ -2773,7 +2782,7 @@ async fn run_scan( } // Display detailed summary of selected patches (skipped under --silent). - if !silent { + if !silent && !selected.is_empty() { if vendor { println!("\nPatches to vendor:\n"); } else { @@ -2927,9 +2936,16 @@ async fn run_scan( ) .await } else { - let (code, _) = - download_and_apply_patches_with(&selected, ¶ms, &download_run(&args, &api_client)) - .await; + // The recorded selections ride along: the fetch loop skips their + // record and the nested apply re-applies them (a no-op on disk + // when the installed copy is still patched). + let to_download: Vec<_> = selected.iter().chain(&already_recorded).cloned().collect(); + let (code, _) = download_and_apply_patches_with( + &to_download, + ¶ms, + &download_run(&args, &api_client), + ) + .await; code }; @@ -2982,14 +2998,20 @@ mod tests { dirs.iter() .map(|(d, explicit)| { ( - d.strip_prefix(tmp.path()).unwrap().to_string_lossy().replace('\\', "/"), + d.strip_prefix(tmp.path()) + .unwrap() + .to_string_lossy() + .replace('\\', "/"), *explicit, ) }) .collect() }; - let got = project_dirs(tmp.path(), &["apps/*".into(), "libs/core".into(), "apps/web".into()]) - .unwrap(); + let got = project_dirs( + tmp.path(), + &["apps/*".into(), "libs/core".into(), "apps/web".into()], + ) + .unwrap(); // Named literally = explicit (also when a glob matches it too). assert_eq!( rel(got), diff --git a/crates/socket-patch-cli/src/commands/scan/render.rs b/crates/socket-patch-cli/src/commands/scan/render.rs index 2f881da3e..c58732214 100644 --- a/crates/socket-patch-cli/src/commands/scan/render.rs +++ b/crates/socket-patch-cli/src/commands/scan/render.rs @@ -300,18 +300,27 @@ pub(super) fn not_installed_skip_line(purl: &str) -> String { ) } -/// `[skip]` line for a selection the manifest already records. -pub(super) fn already_recorded_line(purl: &str, uuid: &str) -> String { +/// Line for a selection the manifest already records: `[re-apply]` on a +/// wet agent run (the nested apply re-applies it), `[skip]` in a preview. +pub(super) fn already_recorded_line(purl: &str, uuid: &str, reapply: bool) -> String { + let tag = if reapply { "re-apply" } else { "skip" }; format!( - " [skip] {purl} (already recorded: {})", + " [{tag}] {purl} (already recorded: {})", super::super::get::short_uuid(uuid) ) } -/// Printed when every selection is already recorded (agent mode). +/// Printed when every selection is already recorded and nothing is applied +/// (report-only scan). pub(super) const ALL_ALREADY_RECORDED: &str = "All selected patches are already recorded in the manifest; run `socket-patch apply` to re-apply them."; +/// [`ALL_ALREADY_RECORDED`] for an agent-mode `--dry-run`. A report-only +/// dry run keeps [`ALL_ALREADY_RECORDED`]: dropping `--dry-run` there still +/// only reports. +pub(super) const ALL_ALREADY_RECORDED_DRY_RUN: &str = + "All selected patches are already recorded in the manifest; a run without --dry-run re-applies them."; + /// The terminal error when no package's patch details could be fetched. pub(super) fn fetch_details_failed(failed: &[(String, String)]) -> String { match failed { @@ -746,7 +755,10 @@ mod tests { #[test] fn report_only_hint_names_agent_mode() { - assert_eq!(report_only_hint()[0], "To apply these patches in place, run:"); + assert_eq!( + report_only_hint()[0], + "To apply these patches in place, run:" + ); assert!(report_only_hint()[1].contains("--mode agent")); } @@ -760,9 +772,13 @@ mod tests { assert!(l.contains("`socket-patch scan --mode vendored`"), "{l}"); assert!(!l.contains("--vendor`"), "{l}"); assert_eq!( - already_recorded_line("pkg:npm/x@1", "884e9f6d-aaaa"), + already_recorded_line("pkg:npm/x@1", "884e9f6d-aaaa", false), " [skip] pkg:npm/x@1 (already recorded: 884e9f6d)" ); + assert_eq!( + already_recorded_line("pkg:npm/x@1", "884e9f6d-aaaa", true), + " [re-apply] pkg:npm/x@1 (already recorded: 884e9f6d)" + ); } #[test] diff --git a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs index 47fa71669..7d940fe31 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs @@ -1476,7 +1476,13 @@ async fn scan_hosted_paths_run_once_per_project_directory() { let header = format!("== {} ==", Path::new("apps").join(app).display()); assert!(stdout.contains(&header), "missing {header:?}: {stdout}"); } - assert_eq!(stdout.matches("Switched 0 packages to hosted patches").count(), 2, "{stdout}"); + assert_eq!( + stdout + .matches("Switched 0 packages to hosted patches") + .count(), + 2, + "{stdout}" + ); let reqs = recorded(&mock).await; assert_eq!(batch_bodies(&reqs).len(), 2, "one discovery per directory"); } @@ -1915,7 +1921,6 @@ mod pty { screen.join("\n") ); } - } // --------------------------------------------------------------------------- @@ -2262,10 +2267,153 @@ fn scan_mode_conflict_error_is_capitalized_and_names_no_hidden_flag() { assert!(!stderr.contains("--redirect"), "{stderr:?}"); } -/// A selection the manifest already records at the same uuid is not -/// offered again (it would only be downloaded to be skipped). +/// Mount batch, by-package and view endpoints for two patched packages +/// (each `index.js` goes from `before\n` to `after\n`). +async fn mount_two_patch_api(mock: &MockServer, pkgs: &[(&str, &str)], before: &[u8]) { + let patches = |purl: &str, uuid: &str| { + serde_json::json!({ + "purl": purl, + "patches": [{ + "uuid": uuid, "purl": purl, "tier": "free", + "cveIds": [], "ghsaIds": [], "severity": "high", + "title": "covgap test patch" + }] + }) + }; + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG_SLUG}/patches/batch"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "packages": pkgs.iter().map(|(p, u)| patches(p, u)).collect::>(), + "canAccessPaidPatches": false, + }))) + .mount(mock) + .await; + for (purl, uuid) in pkgs { + mount_by_package(mock, purl, uuid, serde_json::json!({})).await; + Mock::given(method("GET")) + .and(path(format!("/v0/orgs/{ORG_SLUG}/patches/view/{uuid}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "uuid": uuid, + "purl": purl, + "publishedAt": "2024-01-01T00:00:00Z", + "files": { + "package/index.js": { + "beforeHash": git_sha256(before), + "afterHash": git_sha256(b"after\n"), + "blobContent": "YWZ0ZXIK", + } + }, + "vulnerabilities": {}, + "description": "Covgap test patch", + "license": "MIT", + "tier": "free", + }))) + .mount(mock) + .await; + } +} + +/// #732: a human-output `scan --mode agent` / `scan --sync` re-applies a +/// patch the manifest already records once a reinstall has put the +/// pristine bytes back (the `--json` path has done so since #454). +#[tokio::test] +async fn scan_human_reapplies_an_already_recorded_patch_after_reinstall() { + for flags in [&["--mode", "agent"][..], &["--sync"][..]] { + let mock = MockServer::start().await; + let purl = "pkg:npm/minimist@1.2.2"; + let before = b"before\n"; + mount_one_patch_api(&mock, purl, before).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "minimist", "1.2.2", before); + let index = tmp.path().join("node_modules/minimist/index.js"); + + let (code, stdout, stderr) = run_scan_human(tmp.path(), &mock.uri(), flags); + assert_eq!(code, 0, "flags={flags:?}: stdout={stdout}; stderr={stderr}"); + assert_eq!( + std::fs::read(&index).unwrap(), + b"after\n", + "flags={flags:?}" + ); + + // `npm ci` / `rm -rf node_modules && npm install`. + std::fs::write(&index, before).unwrap(); + + let (code, stdout, stderr) = run_scan_human(tmp.path(), &mock.uri(), flags); + assert_eq!(code, 0, "flags={flags:?}: stdout={stdout}; stderr={stderr}"); + assert_eq!( + std::fs::read(&index).unwrap(), + b"after\n", + "flags={flags:?}: the recorded patch must be re-applied; stdout={stdout}; stderr={stderr}" + ); + assert!( + stdout.contains(&format!("[re-apply] {purl} (already recorded: 11111111)")), + "flags={flags:?}: {stdout:?}" + ); + assert!( + !stdout.contains("run `socket-patch apply` to re-apply them"), + "flags={flags:?}: {stdout:?}" + ); + let manifest = + std::fs::read_to_string(tmp.path().join(".socket/manifest.json")).expect("manifest"); + let v: serde_json::Value = serde_json::from_str(&manifest).unwrap(); + assert_eq!(v["patches"][purl]["uuid"], UUID, "flags={flags:?}: {v}"); + } +} + +/// #732: when a run also has a new patch, the recorded-but-reverted one +/// is re-applied alongside it rather than dropped from the selection. #[tokio::test] -async fn scan_human_does_not_offer_an_already_recorded_patch() { +async fn scan_human_reapplies_recorded_patch_alongside_a_new_one() { + const UUID_B: &str = "22222222-2222-4222-8222-222222222222"; + let a = "pkg:npm/minimist@1.2.2"; + let b = "pkg:npm/left-pad@1.3.0"; + let before = b"before\n"; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "minimist", "1.2.2", before); + write_npm_package(tmp.path(), "left-pad", "1.3.0", before); + let index_a = tmp.path().join("node_modules/minimist/index.js"); + let index_b = tmp.path().join("node_modules/left-pad/index.js"); + + // First run: only `a` has a patch. + let first = MockServer::start().await; + mount_two_patch_api(&first, &[(a, UUID)], before).await; + let (code, stdout, stderr) = run_scan_agent(tmp.path(), &first.uri(), &[]); + assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}"); + assert_eq!(std::fs::read(&index_a).unwrap(), b"after\n"); + assert_eq!(std::fs::read(&index_b).unwrap(), before); + + // Reinstall, then a patch for `b` is published. + std::fs::write(&index_a, before).unwrap(); + let second = MockServer::start().await; + mount_two_patch_api(&second, &[(a, UUID), (b, UUID_B)], before).await; + let (code, stdout, stderr) = run_scan_agent(tmp.path(), &second.uri(), &[]); + assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}"); + assert_eq!( + std::fs::read(&index_b).unwrap(), + b"after\n", + "new patch applied; stdout={stdout}; stderr={stderr}" + ); + assert_eq!( + std::fs::read(&index_a).unwrap(), + b"after\n", + "recorded patch re-applied; stdout={stdout}; stderr={stderr}" + ); + assert!(stdout.contains("Patches to apply:"), "{stdout:?}"); + assert!( + stdout.contains(&format!("[re-apply] {a} (already recorded: 11111111)")), + "{stdout:?}" + ); +} + +/// `--dry-run` stays a non-mutating preview: an already-recorded +/// selection is neither downloaded nor re-applied, and the message says a +/// wet run re-applies it. +#[tokio::test] +async fn scan_human_dry_run_previews_reapply_of_a_recorded_patch() { let mock = MockServer::start().await; let purl = "pkg:npm/minimist@1.2.2"; mount_batch_one(&mock, purl, UUID, "free", &[], false).await; @@ -2275,24 +2423,64 @@ async fn scan_human_does_not_offer_an_already_recorded_patch() { write_root_package_json(tmp.path()); write_npm_package(tmp.path(), "minimist", "1.2.2", b"x\n"); seed_manifest(tmp.path(), &[(purl, UUID)]); + let manifest_before = std::fs::read(tmp.path().join(".socket/manifest.json")).unwrap(); - let (code, stdout, stderr) = run_scan_agent(tmp.path(), &mock.uri(), &["--yes"]); + let (code, stdout, stderr) = run_scan_agent(tmp.path(), &mock.uri(), &["--dry-run"]); assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}"); assert!( stdout.contains(&format!("[skip] {purl} (already recorded: 11111111)")), "{stdout:?}" ); assert!( - stdout.contains("All selected patches are already recorded in the manifest"), + stdout.contains("a run without --dry-run re-applies them"), "{stdout:?}" ); assert!(!stdout.contains("Patches to apply:"), "{stdout:?}"); - assert!(!stderr.contains("Download and apply"), "{stderr:?}"); assert_eq!( view_gets(&recorded(&mock).await), 0, "nothing is downloaded" ); + assert_eq!( + std::fs::read(tmp.path().join("node_modules/minimist/index.js")).unwrap(), + b"x\n" + ); + assert_eq!( + std::fs::read(tmp.path().join(".socket/manifest.json")).unwrap(), + manifest_before + ); +} + +/// A report-only `scan --prune --dry-run` never applies, so it must not say that +/// dropping `--dry-run` re-applies a recorded patch; it points at +/// `socket-patch apply` instead. +#[tokio::test] +async fn scan_report_only_dry_run_points_recorded_patch_at_apply() { + let mock = MockServer::start().await; + let purl = "pkg:npm/minimist@1.2.2"; + mount_batch_one(&mock, purl, UUID, "free", &[], false).await; + mount_by_package(&mock, purl, UUID, serde_json::json!({})).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "minimist", "1.2.2", b"x\n"); + seed_manifest(tmp.path(), &[(purl, UUID)]); + + let (code, stdout, stderr) = run_scan_human(tmp.path(), &mock.uri(), &["--prune", "--dry-run"]); + assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}"); + assert!( + stdout.contains(&format!("[skip] {purl} (already recorded: 11111111)")), + "{stdout:?}" + ); + assert!( + !stdout.contains("a run without --dry-run re-applies them"), + "{stdout:?}" + ); + assert!(!stdout.contains("[re-apply]"), "{stdout:?}"); + assert_eq!( + std::fs::read(tmp.path().join("node_modules/minimist/index.js")).unwrap(), + b"x\n" + ); } /// The human table's PACKAGE column grows to fit the PURL (a fixed-width