diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index 5c16e2423..2cf0ca5e8 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -599,6 +599,33 @@ async fn verify_installed_tree( .failed .retain(|f| !(f.reason == "package_not_found" && mismatched_keys.contains(&f.purl))); outcome.failed.extend(unmatched); + + // An npm bin holding the patched bytes with a CRLF shebang `env` + // can't run (written by a release before #1756) is one `apply` + // rewrites, so it is not in sync (#1791). + let mut crlf_bins: Vec = Vec::new(); + for purl in &outcome.applied { + let Some(record) = tree.patches.get(purl) else { + continue; + }; + 'copies: for copy in copies.get(purl).into_iter().flatten() { + for (file, info) in &record.files { + if socket_patch_core::patch::apply::needs_npm_bin_rewrite(purl, copy, file, info) + .await + { + crlf_bins.push(purl.clone()); + break 'copies; + } + } + } + } + outcome.applied.retain(|purl| !crlf_bins.contains(purl)); + outcome + .failed + .extend(crlf_bins.into_iter().map(|purl| FailedPatch { + purl, + reason: "not_applied".to_string(), + })); outcome } diff --git a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs index f09dbb836..57feb67cd 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs @@ -1161,30 +1161,21 @@ async fn npm_get_ghsa_hosted_narrows_and_installs() { manifestless_tail(&fx, &fresh, installed, &[]); } -/// Regression (#1756): on Linux and macOS npm's `bin-links` rewrites a -/// `bin` file's CRLF shebang to LF at install. Agent-mode `apply` patches -/// that install (it was refused as drift), keeps the bin runnable (a -/// written-back `#!/usr/bin/env node\r` makes `env` exit 127), re-applies -/// as a no-op, `vex` attests it, and `rollback` restores npm's form of the -/// original. Offline: the package is a local tarball. -#[cfg(unix)] -#[tokio::test(flavor = "multi_thread")] -#[ignore = "wall-bound real-npm install; runs on the e2e CI matrix leg"] -async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { - const SUITE: &str = "npm_agent_patches_crlf_shebang_bin_npm_rewrote"; - if npm_e2e_common::npm_version().is_none() { - npm_e2e_common::skip(SUITE, "npm not on PATH"); - return; - } - let before: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(\"hi\");\r\n"; - let after: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(\"hi\");\r\n// SOCKET-PATCHED\r\n"; - // What npm leaves on disk for each: the first line's `\r` dropped. - let installed_before: &[u8] = b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n"; - let installed_after: &[u8] = - b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n// SOCKET-PATCHED\r\n"; +/// The tarball bytes of a bin published from Windows (CRLF shebang), its +/// patched twin, and what npm's `fixBin` leaves on disk for each: the +/// first line's `\r` dropped (#1756). +const CRLF_BIN_BEFORE: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(\"hi\");\r\n"; +const CRLF_BIN_AFTER: &[u8] = + b"#!/usr/bin/env node\r\nconsole.log(\"hi\");\r\n// SOCKET-PATCHED\r\n"; +const NPM_BIN_BEFORE: &[u8] = b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n"; +const NPM_BIN_AFTER: &[u8] = b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n// SOCKET-PATCHED\r\n"; - let tmp = tempfile::tempdir().unwrap(); - let pkg = tmp.path().join("pkg"); +/// A project under `tmp` with the `crlfbin` tarball (a CRLF-shebang +/// `bin`) npm-installed, and an agent manifest patching that bin with +/// its blobs. Returns the project dir. +fn crlf_bin_project(tmp: &Path) -> PathBuf { + let (before, after) = (CRLF_BIN_BEFORE, CRLF_BIN_AFTER); + let pkg = tmp.join("pkg"); std::fs::create_dir_all(&pkg).unwrap(); std::fs::write( pkg.join("package.json"), @@ -1193,18 +1184,18 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { .unwrap(); std::fs::write(pkg.join("cli.js"), before).unwrap(); std::fs::write( - tmp.path().join("crlfbin-1.0.0.tgz"), + tmp.join("crlfbin-1.0.0.tgz"), npm_e2e_common::npm_pack_like(&pkg), ) .unwrap(); - let proj = tmp.path().join("proj"); + let proj = tmp.join("proj"); std::fs::create_dir_all(&proj).unwrap(); std::fs::write( proj.join("package.json"), r#"{"name":"p","version":"1.0.0","dependencies":{"crlfbin":"file:../crlfbin-1.0.0.tgz"}}"#, ) .unwrap(); - let cache = tmp.path().join("cache"); + let cache = tmp.join("cache"); let out = npm_e2e_common::npm( &proj, &[ @@ -1221,11 +1212,7 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { npm_e2e_common::output_text(&out) ); let cli = proj.join("node_modules/crlfbin/cli.js"); - assert_eq!( - std::fs::read(&cli).unwrap(), - installed_before, - "npm's fixBin" - ); + assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE, "npm's fixBin"); let (b, a) = (git_sha256(before), git_sha256(after)); let blobs = proj.join(".socket/blobs"); @@ -1243,12 +1230,42 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { ) .unwrap(); - let run_bin = || { - std::process::Command::new(proj.join("node_modules/.bin/crlfbin")) - .current_dir(&proj) - .output() - .unwrap() - }; + proj +} + +/// Runs the project's `crlfbin` bin, asserting it prints `hi`. +fn assert_crlf_bin_runs(proj: &Path, what: &str) { + let ran = std::process::Command::new(proj.join("node_modules/.bin/crlfbin")) + .current_dir(proj) + .output() + .unwrap(); + assert!( + ran.status.success(), + "{what}: {}", + npm_e2e_common::output_text(&ran) + ); + assert_eq!(String::from_utf8_lossy(&ran.stdout).trim(), "hi", "{what}"); +} + +/// Regression (#1756): on Linux and macOS npm's `bin-links` rewrites a +/// `bin` file's CRLF shebang to LF at install. Agent-mode `apply` patches +/// that install (it was refused as drift), keeps the bin runnable (a +/// written-back `#!/usr/bin/env node\r` makes `env` exit 127), re-applies +/// as a no-op, `vex` attests it, and `rollback` restores npm's form of the +/// original. Offline: the package is a local tarball. +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread")] +#[ignore = "wall-bound real-npm install; runs on the e2e CI matrix leg"] +async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { + const SUITE: &str = "npm_agent_patches_crlf_shebang_bin_npm_rewrote"; + if npm_e2e_common::npm_version().is_none() { + npm_e2e_common::skip(SUITE, "npm not on PATH"); + return; + } + let (installed_before, installed_after) = (NPM_BIN_BEFORE, NPM_BIN_AFTER); + let tmp = tempfile::tempdir().unwrap(); + let proj = crlf_bin_project(tmp.path()); + let cli = proj.join("node_modules/crlfbin/cli.js"); let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]); assert_eq!(code, 0, "apply\nstdout:\n{stdout}\nstderr:\n{stderr}"); @@ -1257,13 +1274,7 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { "no drift warning: {stderr}" ); assert_eq!(std::fs::read(&cli).unwrap(), installed_after); - let ran = run_bin(); - assert!( - ran.status.success(), - "{}", - npm_e2e_common::output_text(&ran) - ); - assert_eq!(String::from_utf8_lossy(&ran.stdout).trim(), "hi"); + assert_crlf_bin_runs(&proj, "after apply"); let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]); assert_eq!(code, 0, "re-apply\nstdout:\n{stdout}\nstderr:\n{stderr}"); @@ -1291,10 +1302,46 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--offline"]); assert_eq!(code, 0, "rollback\nstdout:\n{stdout}\nstderr:\n{stderr}"); assert_eq!(std::fs::read(&cli).unwrap(), installed_before); - let ran = run_bin(); + assert_crlf_bin_runs(&proj, "after rollback"); +} + +/// Regression (#1791): v5.0.1 wrote the raw CRLF patched bytes over the +/// bin npm had fixed, so it exits 127. `apply --check` reports that drift, +/// `apply` rewrites the bin in npm's form, and from the same broken state +/// `rollback` restores npm's runnable form of the original. +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread")] +#[ignore = "wall-bound real-npm install; runs on the e2e CI matrix leg"] +async fn npm_agent_recovers_crlf_bin_left_by_raw_write() { + const SUITE: &str = "npm_agent_recovers_crlf_bin_left_by_raw_write"; + if npm_e2e_common::npm_version().is_none() { + npm_e2e_common::skip(SUITE, "npm not on PATH"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let proj = crlf_bin_project(tmp.path()); + let cli = proj.join("node_modules/crlfbin/cli.js"); + // What v5.0.1's apply left: the recorded bytes, CRLF shebang and all. + std::fs::write(&cli, CRLF_BIN_AFTER).unwrap(); + + let (code, stdout, stderr) = run_socket(&proj, &["apply", "--check", "--offline"]); + assert_eq!(code, 1, "check\nstdout:\n{stdout}\nstderr:\n{stderr}"); assert!( - ran.status.success(), - "{}", - npm_e2e_common::output_text(&ran) + format!("{stdout}{stderr}").contains("pkg:npm/crlfbin@1.0.0"), + "names the drifted patch\nstdout:\n{stdout}\nstderr:\n{stderr}" ); + + let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]); + assert_eq!(code, 0, "apply\nstdout:\n{stdout}\nstderr:\n{stderr}"); + assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_AFTER); + assert_crlf_bin_runs(&proj, "after apply"); + + let (code, stdout, stderr) = run_socket(&proj, &["apply", "--check", "--offline"]); + assert_eq!(code, 0, "re-check\nstdout:\n{stdout}\nstderr:\n{stderr}"); + + std::fs::write(&cli, CRLF_BIN_AFTER).unwrap(); + let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--offline"]); + assert_eq!(code, 0, "rollback\nstdout:\n{stdout}\nstderr:\n{stderr}"); + assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE); + assert_crlf_bin_runs(&proj, "after rollback"); } diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index 9ab742e01..999651de1 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -1011,6 +1011,56 @@ fn mark_new_file_collision(verify: &mut VerifyResult, file_info: &PatchFileInfo) } } +/// A bin of an npm package holding the raw patched bytes with their CRLF +/// shebang, which a release before #1756 wrote over npm's form, can't be +/// run by `env`, so it is not patched yet: `Ready`, and the write puts it +/// in npm's form (#1791). [`verify_file_patch`] keeps calling it +/// `AlreadyPatched`, since a vendored tarball's staging copy rightly +/// holds those bytes. +async fn mark_crlf_bin_rewrite( + package_key: &str, + pkg_path: &Path, + file_name: &str, + file_info: &PatchFileInfo, + verify: &mut VerifyResult, +) { + if verify.status == VerifyStatus::AlreadyPatched + && verify.current_hash.as_deref() == Some(file_info.after_hash.as_str()) + && crate::patch::npm_bin::needs_fix( + package_key, + pkg_path, + normalize_file_path(file_name), + &file_info.after_hash, + ) + .await + { + verify.status = VerifyStatus::Ready; + verify.target_hash = Some(file_info.after_hash.clone()); + } +} + +/// Whether `apply` would rewrite `file_name` (a manifest key) of the +/// installed `package_key` copy at `pkg_path` although its bytes hash to +/// the patched content `file_info.after_hash`: an npm bin left with a +/// CRLF shebang `env` can't run (#1791). `apply --check` reports it as +/// not applied. +pub async fn needs_npm_bin_rewrite( + package_key: &str, + pkg_path: &Path, + file_name: &str, + file_info: &PatchFileInfo, +) -> bool { + let normalized = normalize_file_path(file_name); + is_safe_relative_subpath(normalized) + && crate::patch::npm_bin::needs_fix( + package_key, + pkg_path, + normalized, + &file_info.after_hash, + ) + .await +} + /// The single-copy apply engine behind [`apply_package_patch`]: verifies /// and patches the package at exactly the one `pkg_path` it is given. async fn apply_package_patch_at( @@ -1062,6 +1112,14 @@ async fn apply_package_patch_at( let mut verify_result = verify_file_patch(pkg_path, file_name, file_info).await; mark_new_file_collision(&mut verify_result, file_info); + mark_crlf_bin_rewrite( + package_key, + pkg_path, + file_name, + file_info, + &mut verify_result, + ) + .await; if verify_result.status != VerifyStatus::Ready && verify_result.status != VerifyStatus::AlreadyPatched @@ -1250,7 +1308,8 @@ async fn apply_package_patch_at( // that form too (see `npm_bin`). let (patched_content, written_hash) = match crate::patch::npm_bin::form_for_write( package_key, - &pkg_path.join(normalized), + pkg_path, + normalized, &file_info.before_hash, patched_content, &file_info.after_hash, @@ -4114,6 +4173,106 @@ mod tests { ); } + /// Regression (#1791): v5.0.1 wrote the raw CRLF `afterHash` bytes + /// over a bin npm had fixed, so it can't run. Apply rewrites it in + /// npm's form, once. + #[tokio::test] + async fn apply_repairs_crlf_bin_left_by_raw_write() { + let files = crlf_bin_files(); + let blobs = crlf_bin_blobs().await; + let pkg = tempfile::tempdir().unwrap(); + tokio::fs::write( + pkg.path().join("package.json"), + br#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"./cli.js"}}"#, + ) + .await + .unwrap(); + tokio::fs::write(pkg.path().join("cli.js"), CRLF_BIN_AFTER) + .await + .unwrap(); + let sources = PatchSources::blobs_only(blobs.path()); + let apply = || { + apply_package_patch( + "pkg:npm/crlfbin@1.0.0", + pkg.path(), + &files, + &sources, + false, + MismatchPolicy::Strict, + ) + }; + let res = apply().await; + assert!(res.success, "{res:?}"); + assert_eq!(res.files_patched, vec!["package/cli.js".to_string()]); + assert_eq!( + tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(), + NPM_BIN_AFTER + ); + let again = apply().await; + assert!(again.success, "{again:?}"); + assert!(again.files_patched.is_empty(), "{again:?}"); + assert!( + !needs_npm_bin_rewrite( + "pkg:npm/crlfbin@1.0.0", + pkg.path(), + "package/cli.js", + &files["package/cli.js"] + ) + .await + ); + } + + /// The #1791 rewrite is for files the package lists as bins, in npm + /// packages: npm leaves any other CRLF shebang alone. + #[tokio::test] + async fn apply_keeps_crlf_shebang_npm_does_not_fix() { + let files = crlf_bin_files(); + let blobs = crlf_bin_blobs().await; + for (purl, manifest) in [ + ( + "pkg:npm/crlfbin@1.0.0", + &br#"{"name":"crlfbin","version":"1.0.0","bin":{"other":"other.js"}}"#[..], + ), + ("pkg:npm/crlfbin@1.0.0", br#"{"name":"crlfbin"}"#), + ( + "pkg:composer/a/crlfbin@1.0.0", + br#"{"name":"crlfbin","bin":"cli.js"}"#, + ), + ] { + let pkg = tempfile::tempdir().unwrap(); + tokio::fs::write(pkg.path().join("package.json"), manifest) + .await + .unwrap(); + tokio::fs::write(pkg.path().join("cli.js"), CRLF_BIN_AFTER) + .await + .unwrap(); + assert!( + !needs_npm_bin_rewrite( + purl, + pkg.path(), + "package/cli.js", + &files["package/cli.js"] + ) + .await + ); + let res = apply_package_patch( + purl, + pkg.path(), + &files, + &PatchSources::blobs_only(blobs.path()), + false, + MismatchPolicy::Strict, + ) + .await; + assert!(res.success, "{purl}: {res:?}"); + assert!(res.files_patched.is_empty(), "{purl}: {res:?}"); + assert_eq!( + tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(), + CRLF_BIN_AFTER + ); + } + } + /// A non-npm package keeps the patch's exact bytes on a consent /// overwrite: only npm rewrites bin shebangs. #[tokio::test] diff --git a/crates/socket-patch-core/src/patch/npm_bin.rs b/crates/socket-patch-core/src/patch/npm_bin.rs index 35a1f0822..f245b5458 100644 --- a/crates/socket-patch-core/src/patch/npm_bin.rs +++ b/crates/socket-patch-core/src/patch/npm_bin.rs @@ -60,31 +60,41 @@ pub(crate) fn fixed_for_write( Ok(Some((fixed, hash))) } -/// The bytes to write over the file at `path` for `content` (recorded as -/// `expected_hash`), and their hash: `content` in `fixBin`'s form when -/// the file now on disk is a bin npm installed with an LF shebang, else -/// `content` as recorded. Writing a CRLF shebang there would leave a bin -/// `env` can't run. +/// The bytes to write over `normalized` (a safe path relative to +/// `pkg_path`) for `content` (recorded as `expected_hash`), and their +/// hash: `content` in `fixBin`'s form when the file is a bin npm +/// installed, else `content` as recorded. Writing a CRLF shebang there +/// would leave a bin `env` can't run. /// -/// The file is taken for an npm-installed bin when it has an LF shebang -/// and either belongs to an npm package, or hashes, in npm's original -/// form, to `installed_source` (the content this write replaces). +/// The file is taken for an npm-installed bin when its npm package lists +/// it in `bin` ([`is_listed_bin`]), whatever shebang is on disk now (a +/// release before #1756 wrote the CRLF one, #1791). Without that +/// evidence, when the file now on disk has an LF shebang and either +/// belongs to an npm package, or hashes, in npm's original form, to +/// `installed_source` (the content this write replaces). pub(crate) async fn form_for_write<'a>( package_key: &str, - path: &Path, + pkg_path: &Path, + normalized: &str, installed_source: &str, content: std::borrow::Cow<'a, [u8]>, expected_hash: &str, ) -> Result<(std::borrow::Cow<'a, [u8]>, String), std::io::Error> { let as_recorded = |content| Ok((content, expected_hash.to_string())); - let lf_shebang = read_prefix(path) - .await - .is_some_and(|prefix| has_lf_shebang(&prefix)); - let npm_installed = lf_shebang - && (crate::crawlers::Ecosystem::from_purl(package_key) - == Some(crate::crawlers::Ecosystem::Npm) - || (!installed_source.is_empty() - && source_hash(path).await.as_deref() == Some(installed_source))); + // Content `fixBin` leaves alone needs no evidence read from disk. + if fix_bin(&content).is_none() { + return as_recorded(content); + } + let path = pkg_path.join(normalized); + let npm_installed = is_listed_bin(package_key, pkg_path, normalized).await || { + let lf_shebang = read_prefix(&path) + .await + .is_some_and(|prefix| has_lf_shebang(&prefix)); + lf_shebang + && (is_npm(package_key) + || (!installed_source.is_empty() + && source_hash(&path).await.as_deref() == Some(installed_source))) + }; if !npm_installed { return as_recorded(content); } @@ -94,6 +104,115 @@ pub(crate) async fn form_for_write<'a>( } } +fn is_npm(package_key: &str) -> bool { + crate::crawlers::Ecosystem::from_purl(package_key) == Some(crate::crawlers::Ecosystem::Npm) +} + +/// Whether `normalized` (a safe path relative to `pkg_path`) is a file +/// the npm package `package_key` lists as a bin in its `package.json`: +/// a file npm's `bin-links` runs `fixBin` on. +pub(crate) async fn is_listed_bin(package_key: &str, pkg_path: &Path, normalized: &str) -> bool { + if !is_npm(package_key) { + return false; + } + let Ok(manifest) = + crate::utils::fs::read_regular_to_bytes(&pkg_path.join("package.json")).await + else { + return false; + }; + lists_bin(&manifest, normalized) +} + +/// Whether the file at `normalized` holds exactly the recorded content +/// `expected_hash`, in bytes `fixBin` would change (a CRLF shebang npm +/// never leaves in a bin), and is a bin of the npm package `package_key`: +/// the state a release before #1756 left by writing the recorded bytes +/// over npm's form, which `env` can't run (#1791). +pub(crate) async fn needs_fix( + package_key: &str, + pkg_path: &Path, + normalized: &str, + expected_hash: &str, +) -> bool { + if !is_npm(package_key) { + return false; + } + let path = pkg_path.join(normalized); + // Cheap probe first: only a `#!…\r\n` first line can need it. + if !read_prefix(&path).await.is_some_and(|prefix| { + first_newline(&prefix).is_some_and(|lf| lf >= 4 && prefix[lf - 1] == b'\r') + }) { + return false; + } + if !is_listed_bin(package_key, pkg_path, normalized).await { + return false; + } + crate::utils::fs::read_regular_to_bytes(&path) + .await + .is_ok_and(|bytes| { + fix_bin(&bytes).is_some() && compute_git_sha256_from_bytes(&bytes) == expected_hash + }) +} + +/// Whether the `package.json` bytes `manifest` list `normalized` as a +/// bin, as npm's `normalize-package-bin` and `directories.bin` step read +/// them: a `bin` path (a string, when the package has a `name`, an +/// object's values, or an array's entries) or, when `bin` is missing or +/// empty, a non-hidden file under `directories.bin`. Paths are resolved +/// as npm does (`./` and `..` folded, never above the package). +fn lists_bin(manifest: &[u8], normalized: &str) -> bool { + use serde_json::Value; + let Ok(json) = serde_json::from_slice::(manifest) else { + return false; + }; + let target = clean_path(normalized); + if target.is_empty() { + return false; + } + let lists = |bins: &mut dyn Iterator| { + bins.filter_map(Value::as_str) + .any(|bin| clean_path(bin) == target) + }; + match json.get("bin") { + Some(Value::String(bin)) if !bin.is_empty() => { + json.get("name") + .and_then(Value::as_str) + .is_some_and(|n| !n.is_empty()) + && clean_path(bin) == target + } + Some(Value::Object(bins)) => lists(&mut bins.values()), + Some(Value::Array(bins)) => lists(&mut bins.iter()), + Some(Value::String(_) | Value::Null | Value::Bool(false)) | None => json + .get("directories") + .and_then(|d| d.get("bin")) + .and_then(serde_json::Value::as_str) + .map(clean_path) + .is_some_and(|dir| { + !dir.is_empty() + && target.len() > dir.len() + && target.starts_with(&dir[..]) + && !target[dir.len()..].iter().any(|seg| seg.starts_with('.')) + }), + Some(_) => false, + } +} + +/// `path`'s segments with `.`, empty segments and `..` folded, rooted at +/// the package as npm's `path.join('/', …)` roots it. +fn clean_path(path: &str) -> Vec<&str> { + let mut out: Vec<&str> = Vec::new(); + for seg in path.split(['/', '\\']) { + match seg { + "" | "." => {} + ".." => { + out.pop(); + } + seg => out.push(seg), + } + } + out +} + /// The content `fixBin` turned into `bytes`, or `None` when `bytes` is not /// something `fixBin` writes (a `#!` first line of at least one byte, /// ending in `\n` early enough that its `\r\n` original fit the probe). @@ -247,6 +366,43 @@ mod tests { assert!(!has_lf_shebang(b"echo\n")); } + #[test] + fn lists_bin_reads_every_bin_shape() { + let listed = |manifest: &str, file: &str| lists_bin(manifest.as_bytes(), file); + assert!(listed(r#"{"name":"x","bin":"cli.js"}"#, "cli.js")); + assert!(listed(r#"{"name":"x","bin":"./bin/../cli.js"}"#, "cli.js")); + assert!(listed( + r#"{"bin":{"a":"x.js","b":"./bin/cli.js"}}"#, + "bin/cli.js" + )); + assert!(listed( + r#"{"directories":{"bin":"./bin"}}"#, + "bin/sub/cli.js" + )); + // `bin` wins over `directories.bin`, as in npm's normalize. + assert!(!listed( + r#"{"bin":{"a":"x.js"},"directories":{"bin":"bin"}}"#, + "bin/cli.js" + )); + assert!(!listed(r#"{"bin":{"a":"x.js"}}"#, "cli.js")); + assert!(listed(r#"{"bin":["x.js","./cli.js"]}"#, "cli.js")); + // npm drops a string `bin` of a package with no name. + assert!(!listed(r#"{"bin":"cli.js"}"#, "cli.js")); + // A missing or empty `bin` falls back to `directories.bin`, whose + // glob skips hidden files. + for bin in ["\"\"", "null"] { + let manifest = format!(r#"{{"bin":{bin},"directories":{{"bin":"bin"}}}}"#); + assert!(listed(&manifest, "bin/cli.js"), "{manifest}"); + } + assert!(!listed( + r#"{"directories":{"bin":"bin"}}"#, + "bin/.hidden/cli.js" + )); + assert!(!listed(r#"{"name":"x"}"#, "cli.js")); + assert!(!listed(r#"{"directories":{"bin":"."}}"#, "cli.js")); + assert!(!listed("not json", "cli.js")); + } + #[tokio::test] async fn source_hash_is_the_tarball_hash() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index 8a21b8ef7..9ee5a73fe 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -573,6 +573,38 @@ async fn remove_dir_if_empty(dir: &Path) -> bool { /// The single-copy rollback engine behind [`rollback_package_patch`]: /// verifies and rolls back the package at exactly the one `pkg_path` it is /// given. +/// A bin of an npm package holding the raw original bytes with their CRLF +/// shebang, which a rollback before #1756 wrote over npm's form, can't be +/// run by `env`: `Ready`, so the restore writes npm's form of the +/// original (#1791). Only with the before blob at hand: a finished +/// rollback never fails for want of one. +async fn mark_crlf_bin_rewrite( + package_key: &str, + pkg_path: &Path, + file_name: &str, + file_info: &PatchFileInfo, + blobs_path: &Path, + verify: &mut VerifyRollbackResult, +) { + if verify.status == VerifyRollbackStatus::AlreadyOriginal + && !file_info.before_hash.is_empty() + && verify.current_hash.as_deref() == Some(file_info.before_hash.as_str()) + && crate::patch::apply::is_valid_blob_hash(&file_info.before_hash) + && tokio::fs::symlink_metadata(blobs_path.join(&file_info.before_hash)) + .await + .is_ok_and(|m| m.is_file()) + && crate::patch::npm_bin::needs_fix( + package_key, + pkg_path, + normalize_file_path(file_name), + &file_info.before_hash, + ) + .await + { + verify.status = VerifyRollbackStatus::Ready; + } +} + async fn rollback_package_patch_at( package_key: &str, pkg_path: &Path, @@ -592,7 +624,17 @@ async fn rollback_package_patch_at( // First, verify all files for (file_name, file_info) in files_in_order(files) { - let verify_result = verify_file_rollback(pkg_path, file_name, file_info, blobs_path).await; + let mut verify_result = + verify_file_rollback(pkg_path, file_name, file_info, blobs_path).await; + mark_crlf_bin_rewrite( + package_key, + pkg_path, + file_name, + file_info, + blobs_path, + &mut verify_result, + ) + .await; // If any file has issues (not ready and not already original), we can't proceed if verify_result.status != VerifyRollbackStatus::Ready @@ -772,7 +814,8 @@ async fn rollback_package_patch_at( // form, the bytes `npm install` leaves (see `npm_bin`). let (original_content, restored_hash) = match crate::patch::npm_bin::form_for_write( package_key, - &pkg_path.join(normalize_file_path(file_name)), + pkg_path, + normalize_file_path(file_name), &file_info.after_hash, std::borrow::Cow::Owned(original_content), &file_info.before_hash, @@ -3194,6 +3237,94 @@ mod tests { assert!(again.files_rolled_back.is_empty(), "{again:?}"); } + /// Regression (#1791): a bin v5.0.1 patched with the raw CRLF bytes + /// rolls back to npm's runnable form of the original, the bytes + /// `npm install` wrote, not the CRLF tarball bytes. + #[tokio::test] + async fn rollback_of_raw_crlf_bin_restores_npm_form() { + let before: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(1);\r\n"; + let after: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(2);\r\n"; + let pkg = tempfile::tempdir().unwrap(); + let blobs = tempfile::tempdir().unwrap(); + let before_hash = compute_git_sha256_from_bytes(before); + tokio::fs::write(blobs.path().join(&before_hash), before) + .await + .unwrap(); + tokio::fs::write( + pkg.path().join("package.json"), + br#"{"name":"x","version":"1.0.0","bin":"cli.js"}"#, + ) + .await + .unwrap(); + tokio::fs::write(pkg.path().join("cli.js"), after) + .await + .unwrap(); + let files = HashMap::from([( + "package/cli.js".to_string(), + PatchFileInfo { + before_hash, + after_hash: compute_git_sha256_from_bytes(after), + }, + )]); + let res = + rollback_package_patch("pkg:npm/x@1.0.0", pkg.path(), &files, blobs.path(), false) + .await; + assert!(res.success, "{res:?}"); + assert_eq!( + tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(), + b"#!/usr/bin/env node\nconsole.log(1);\r\n" + ); + } + + /// Regression (#1791): a bin an earlier rollback left with the raw + /// CRLF original is rewritten in npm's runnable form when the before + /// blob is there, and left alone, as already original, when it isn't. + #[tokio::test] + async fn rollback_rewrites_raw_crlf_original_bin() { + let before: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(1);\r\n"; + let after: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(2);\r\n"; + let before_hash = compute_git_sha256_from_bytes(before); + let files = HashMap::from([( + "package/cli.js".to_string(), + PatchFileInfo { + before_hash: before_hash.clone(), + after_hash: compute_git_sha256_from_bytes(after), + }, + )]); + for with_blob in [true, false] { + let pkg = tempfile::tempdir().unwrap(); + let blobs = tempfile::tempdir().unwrap(); + if with_blob { + tokio::fs::write(blobs.path().join(&before_hash), before) + .await + .unwrap(); + } + tokio::fs::write( + pkg.path().join("package.json"), + br#"{"name":"x","version":"1.0.0","bin":"cli.js"}"#, + ) + .await + .unwrap(); + tokio::fs::write(pkg.path().join("cli.js"), before) + .await + .unwrap(); + let res = + rollback_package_patch("pkg:npm/x@1.0.0", pkg.path(), &files, blobs.path(), false) + .await; + assert!(res.success, "{with_blob}: {res:?}"); + let expected: &[u8] = if with_blob { + b"#!/usr/bin/env node\nconsole.log(1);\r\n" + } else { + before + }; + assert_eq!( + tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(), + expected, + "{with_blob}" + ); + } + } + /// Regression (#1756): a bin the patch added, installed by npm in its /// LF-shebang form, rolls back (is deleted) like the exact bytes. #[tokio::test]