Fix CRLF npm bins left broken by v5.0.1 (#1791)

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<String> = 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
 }
 

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");
 }