From f0ecb17eb1f1cda115151dea435fcdc1422938fc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 19:24:59 +0000 Subject: [PATCH 1/3] Start fix for #1791 Assisted-by: Claude Code:claude-opus-5-5 From 2d07dc0f9c61b8268754a4165c81931fb6f77a3f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 19:37:53 +0000 Subject: [PATCH 2/3] Recover npm bins v5.0.1 left with CRLF shebangs v5.0.1 could write a patch's raw bytes over an npm bin that npm had installed with an LF shebang, leaving "#!/usr/bin/env node\r" that env can't run (exit 127). After #1757, upgrading did not recover it: apply and apply --check called it already patched, and rollback wrote the CRLF original back, because the shebang form was chosen from whatever was on disk. Decide from the package's own package.json bin field (bin string or object, or directories.bin) whether a file is a bin npm rewrites. Apply now rewrites such a bin in npm's form, apply --check reports it as not applied, and rollback restores npm's runnable form of the original. Fixes #1791 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/src/commands/apply.rs | 25 +++ .../tests/e2e_redirect_npm_build.rs | 127 +++++++++++---- crates/socket-patch-core/src/patch/apply.rs | 130 ++++++++++++++- crates/socket-patch-core/src/patch/npm_bin.rs | 152 ++++++++++++++++-- .../socket-patch-core/src/patch/rollback.rs | 42 ++++- 5 files changed, 426 insertions(+), 50 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index 5c16e2423..63f5b29e6 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -599,6 +599,31 @@ 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 in record.files.keys() { + if socket_patch_core::patch::apply::needs_npm_bin_rewrite(purl, copy, file).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..5610537a7 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,6 +1230,43 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { ) .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 run_bin = || { std::process::Command::new(proj.join("node_modules/.bin/crlfbin")) .current_dir(&proj) @@ -1298,3 +1322,44 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { npm_e2e_common::output_text(&ran) ); } + +/// 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!( + 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..24174ea4c 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -1011,6 +1011,39 @@ 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)) + .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: 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) -> bool { + let normalized = normalize_file_path(file_name); + is_safe_relative_subpath(normalized) + && crate::patch::npm_bin::needs_fix(package_key, pkg_path, normalized).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 +1095,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 +1291,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 +4156,92 @@ 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").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").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..541e2797a 100644 --- a/crates/socket-patch-core/src/patch/npm_bin.rs +++ b/crates/socket-patch-core/src/patch/npm_bin.rs @@ -60,31 +60,37 @@ 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))); + 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 +100,94 @@ 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 holding raw bytes `fixBin` would change (a CRLF +/// shebang npm never leaves in a bin) 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) -> 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()) +} + +/// Whether the `package.json` bytes `manifest` list `normalized` as a +/// bin: a `bin` path (string or object values) or, with no `bin`, a file +/// under `directories.bin`, as npm's `normalize` reads them. Paths are +/// resolved as npm does (`./` and `..` folded, never above the package). +fn lists_bin(manifest: &[u8], normalized: &str) -> bool { + let Ok(json) = serde_json::from_slice::(manifest) else { + return false; + }; + let target = clean_path(normalized); + if target.is_empty() { + return false; + } + match json.get("bin") { + Some(serde_json::Value::String(bin)) => clean_path(bin) == target, + Some(serde_json::Value::Object(bins)) => bins + .values() + .filter_map(serde_json::Value::as_str) + .any(|bin| clean_path(bin) == target), + Some(_) => 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[..]) + }), + } +} + +/// `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 +341,30 @@ 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#"{"bin":"cli.js"}"#, "cli.js")); + assert!(listed(r#"{"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#"{"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..81cb58afc 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -772,7 +772,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 +3195,45 @@ 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 (#1756): a bin the patch added, installed by npm in its /// LF-shebang form, rolls back (is deleted) like the exact bytes. #[tokio::test] From bb8b488df44201cbcde147b82c651a42aae05478 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 19:50:53 +0000 Subject: [PATCH 3/3] Match npm's bin rules and fix old CRLF rollbacks Read package.json bins the way npm does: array bins, a string bin only when the package has a name, and directories.bin (skipping hidden files) when bin is missing or empty. apply --check now flags a bin only when its bytes are exactly the patched content, the same test apply uses, so the two always agree. Rollback also rewrites a bin that an older rollback left holding the raw CRLF original, when the original's blob is available. Writes skip reading package.json for content that has no CRLF shebang. Refs #1791 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/src/commands/apply.rs | 6 +- .../tests/e2e_redirect_npm_build.rs | 22 +---- crates/socket-patch-core/src/patch/apply.rs | 47 ++++++++-- crates/socket-patch-core/src/patch/npm_bin.rs | 78 ++++++++++++---- .../socket-patch-core/src/patch/rollback.rs | 93 ++++++++++++++++++- 5 files changed, 195 insertions(+), 51 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index 63f5b29e6..2cf0ca5e8 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -609,8 +609,10 @@ async fn verify_installed_tree( continue; }; 'copies: for copy in copies.get(purl).into_iter().flatten() { - for file in record.files.keys() { - if socket_patch_core::patch::apply::needs_npm_bin_rewrite(purl, copy, file).await { + 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; } 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 5610537a7..57feb67cd 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs @@ -1267,13 +1267,6 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() { let proj = crlf_bin_project(tmp.path()); let cli = proj.join("node_modules/crlfbin/cli.js"); - let run_bin = || { - std::process::Command::new(proj.join("node_modules/.bin/crlfbin")) - .current_dir(&proj) - .output() - .unwrap() - }; - let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]); assert_eq!(code, 0, "apply\nstdout:\n{stdout}\nstderr:\n{stderr}"); assert!( @@ -1281,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}"); @@ -1315,12 +1302,7 @@ 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!( - ran.status.success(), - "{}", - npm_e2e_common::output_text(&ran) - ); + assert_crlf_bin_runs(&proj, "after rollback"); } /// Regression (#1791): v5.0.1 wrote the raw CRLF patched bytes over the diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index 24174ea4c..999651de1 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -1026,8 +1026,13 @@ async fn mark_crlf_bin_rewrite( ) { 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)) - .await + && 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()); @@ -1036,12 +1041,24 @@ async fn mark_crlf_bin_rewrite( /// 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: 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) -> bool { +/// 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).await + && 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 @@ -4195,7 +4212,13 @@ mod tests { 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").await + !needs_npm_bin_rewrite( + "pkg:npm/crlfbin@1.0.0", + pkg.path(), + "package/cli.js", + &files["package/cli.js"] + ) + .await ); } @@ -4223,7 +4246,15 @@ mod tests { tokio::fs::write(pkg.path().join("cli.js"), CRLF_BIN_AFTER) .await .unwrap(); - assert!(!needs_npm_bin_rewrite(purl, pkg.path(), "package/cli.js").await); + assert!( + !needs_npm_bin_rewrite( + purl, + pkg.path(), + "package/cli.js", + &files["package/cli.js"] + ) + .await + ); let res = apply_package_patch( purl, pkg.path(), diff --git a/crates/socket-patch-core/src/patch/npm_bin.rs b/crates/socket-patch-core/src/patch/npm_bin.rs index 541e2797a..f245b5458 100644 --- a/crates/socket-patch-core/src/patch/npm_bin.rs +++ b/crates/socket-patch-core/src/patch/npm_bin.rs @@ -81,6 +81,10 @@ pub(crate) async fn form_for_write<'a>( expected_hash: &str, ) -> Result<(std::borrow::Cow<'a, [u8]>, String), std::io::Error> { let as_recorded = |content| Ok((content, expected_hash.to_string())); + // 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) @@ -119,11 +123,17 @@ pub(crate) async fn is_listed_bin(package_key: &str, pkg_path: &Path, normalized lists_bin(&manifest, normalized) } -/// Whether the file holding raw bytes `fixBin` would change (a CRLF -/// shebang npm never leaves in a bin) 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) -> bool { +/// 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; } @@ -139,36 +149,51 @@ pub(crate) async fn needs_fix(package_key: &str, pkg_path: &Path, normalized: &s } crate::utils::fs::read_regular_to_bytes(&path) .await - .is_ok_and(|bytes| fix_bin(&bytes).is_some()) + .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: a `bin` path (string or object values) or, with no `bin`, a file -/// under `directories.bin`, as npm's `normalize` reads them. Paths are -/// resolved as npm does (`./` and `..` folded, never above the package). +/// 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 { - let Ok(json) = serde_json::from_slice::(manifest) else { + 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(serde_json::Value::String(bin)) => clean_path(bin) == target, - Some(serde_json::Value::Object(bins)) => bins - .values() - .filter_map(serde_json::Value::as_str) - .any(|bin| clean_path(bin) == target), - Some(_) => false, - None => json + 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[..]) + !dir.is_empty() + && target.len() > dir.len() + && target.starts_with(&dir[..]) + && !target[dir.len()..].iter().any(|seg| seg.starts_with('.')) }), + Some(_) => false, } } @@ -344,8 +369,8 @@ mod tests { #[test] fn lists_bin_reads_every_bin_shape() { let listed = |manifest: &str, file: &str| lists_bin(manifest.as_bytes(), file); - assert!(listed(r#"{"bin":"cli.js"}"#, "cli.js")); - assert!(listed(r#"{"bin":"./bin/../cli.js"}"#, "cli.js")); + 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" @@ -360,6 +385,19 @@ mod tests { "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")); diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index 81cb58afc..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 @@ -3234,6 +3276,55 @@ mod tests { ); } + /// 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]