Fix npm bin shebang on unlinked bins (#1804)

crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs

@@ -1174,15 +1174,27 @@ const NPM_BIN_AFTER: &[u8] = b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n// S
 /// `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 proj = crlf_bin_project_with(
+        tmp,
+        r#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"cli.js"}}"#,
+        "cli.js",
+        &[],
+    );
+    let cli = proj.join("node_modules/crlfbin/cli.js");
+    assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE, "npm's fixBin");
+    proj
+}
+
+/// [`crlf_bin_project`] with the package's `package.json` `manifest`, the
+/// CRLF-shebang file at `rel` in the package, and `flags` added to
+/// `npm install`.
+fn crlf_bin_project_with(tmp: &Path, manifest: &str, rel: &str, flags: &[&str]) -> 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"),
-        r#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"cli.js"}}"#,
-    )
-    .unwrap();
-    std::fs::write(pkg.join("cli.js"), before).unwrap();
+    let file = pkg.join(rel);
+    std::fs::create_dir_all(file.parent().unwrap()).unwrap();
+    std::fs::write(pkg.join("package.json"), manifest).unwrap();
+    std::fs::write(&file, before).unwrap();
     std::fs::write(
         tmp.join("crlfbin-1.0.0.tgz"),
         npm_e2e_common::npm_pack_like(&pkg),
@@ -1196,23 +1208,20 @@ fn crlf_bin_project(tmp: &Path) -> PathBuf {
     )
     .unwrap();
     let cache = tmp.join("cache");
-    let out = npm_e2e_common::npm(
-        &proj,
-        &[
-            "install",
-            "--no-audit",
-            "--no-fund",
-            "--cache",
-            cache.to_str().unwrap(),
-        ],
-    );
+    let mut args = vec![
+        "install",
+        "--no-audit",
+        "--no-fund",
+        "--cache",
+        cache.to_str().unwrap(),
+    ];
+    args.extend_from_slice(flags);
+    let out = npm_e2e_common::npm(&proj, &args);
     assert!(
         out.status.success(),
         "{}",
         npm_e2e_common::output_text(&out)
     );
-    let cli = proj.join("node_modules/crlfbin/cli.js");
-    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");
@@ -1223,7 +1232,7 @@ fn crlf_bin_project(tmp: &Path) -> PathBuf {
         proj.join(".socket/manifest.json"),
         format!(
             r#"{{"patches":{{"pkg:npm/crlfbin@1.0.0":{{"uuid":"e2f99e5d-3afd-5cbd-be80-6a0102bcd6c2",
-"exportedAt":"2026-01-01T00:00:00Z","files":{{"package/cli.js":{{"beforeHash":"{b}","afterHash":"{a}"}}}},
+"exportedAt":"2026-01-01T00:00:00Z","files":{{"package/{rel}":{{"beforeHash":"{b}","afterHash":"{a}"}}}},
 "vulnerabilities":{{"GHSA-xxxx-yyyy-zzzz":{{"cves":["CVE-2099-0001"],"summary":"t","severity":"high","description":"d"}}}},
 "description":"d","license":"MIT","tier":"free"}}}}}}"#
         ),
@@ -1345,3 +1354,65 @@ async fn npm_agent_recovers_crlf_bin_left_by_raw_write() {
     assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE);
     assert_crlf_bin_runs(&proj, "after rollback");
 }
+
+/// Regression (#1804): npm rewrites the shebang only of bins it links.
+/// With `--no-bin-links`, or for a `directories.bin`-only package
+/// installed from a `file:` tarball (npm doesn't expand that field from a
+/// tarball's `package.json`), the installed bin keeps its CRLF shebang.
+/// `apply` writes the patch's recorded bytes, `apply --check` agrees, and
+/// `rollback` restores what `npm install` wrote, byte for byte.
+#[cfg(unix)]
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "wall-bound real-npm install; runs on the e2e CI matrix leg"]
+async fn npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link() {
+    const SUITE: &str = "npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link";
+    if npm_e2e_common::npm_version().is_none() {
+        npm_e2e_common::skip(SUITE, "npm not on PATH");
+        return;
+    }
+    for (manifest, rel, flags) in [
+        (
+            r#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"cli.js"}}"#,
+            "cli.js",
+            &["--no-bin-links"][..],
+        ),
+        (
+            r#"{"name":"crlfbin","version":"1.0.0","directories":{"bin":"bin"}}"#,
+            "bin/cli.js",
+            &[][..],
+        ),
+    ] {
+        let tmp = tempfile::tempdir().unwrap();
+        let proj = crlf_bin_project_with(tmp.path(), manifest, rel, flags);
+        let cli = proj.join("node_modules/crlfbin").join(rel);
+        assert_eq!(
+            std::fs::read(&cli).unwrap(),
+            CRLF_BIN_BEFORE,
+            "{rel}: npm left the shebang alone"
+        );
+
+        let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]);
+        assert_eq!(
+            code, 0,
+            "{rel}: apply\nstdout:\n{stdout}\nstderr:\n{stderr}"
+        );
+        assert_eq!(std::fs::read(&cli).unwrap(), CRLF_BIN_AFTER, "{rel}: apply");
+
+        let (code, stdout, stderr) = run_socket(&proj, &["apply", "--check", "--offline"]);
+        assert_eq!(
+            code, 0,
+            "{rel}: check\nstdout:\n{stdout}\nstderr:\n{stderr}"
+        );
+
+        let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--offline"]);
+        assert_eq!(
+            code, 0,
+            "{rel}: rollback\nstdout:\n{stdout}\nstderr:\n{stderr}"
+        );
+        assert_eq!(
+            std::fs::read(&cli).unwrap(),
+            CRLF_BIN_BEFORE,
+            "{rel}: rollback is byte-exact"
+        );
+    }
+}

crates/socket-patch-core/src/patch/apply.rs

@@ -4180,21 +4180,30 @@ mod tests {
     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();
+        let root = tempfile::tempdir().unwrap();
+        // npm linked the bin, so it ran `fixBin` on it at install.
+        let pkg = root.path().join("node_modules/crlfbin");
+        std::fs::create_dir_all(root.path().join("node_modules/.bin")).unwrap();
+        std::fs::write(
+            root.path().join("node_modules/.bin/crlfbin"),
+            b"node \"$basedir/../crlfbin/cli.js\" \"$@\"",
+        )
+        .unwrap();
+        std::fs::create_dir_all(&pkg).unwrap();
         tokio::fs::write(
-            pkg.path().join("package.json"),
+            pkg.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)
+        tokio::fs::write(pkg.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(),
+                &pkg,
                 &files,
                 &sources,
                 false,
@@ -4205,7 +4214,7 @@ mod tests {
         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(),
+            tokio::fs::read(pkg.join("cli.js")).await.unwrap(),
             NPM_BIN_AFTER
         );
         let again = apply().await;
@@ -4214,7 +4223,7 @@ mod tests {
         assert!(
             !needs_npm_bin_rewrite(
                 "pkg:npm/crlfbin@1.0.0",
-                pkg.path(),
+                &pkg,
                 "package/cli.js",
                 &files["package/cli.js"]
             )
@@ -4239,25 +4248,29 @@ mod tests {
                 br#"{"name":"crlfbin","bin":"cli.js"}"#,
             ),
         ] {
-            let pkg = tempfile::tempdir().unwrap();
-            tokio::fs::write(pkg.path().join("package.json"), manifest)
+            // npm linked a `crlfbin` bin, but not to this file.
+            let root = tempfile::tempdir().unwrap();
+            let pkg = root.path().join("node_modules/crlfbin");
+            std::fs::create_dir_all(root.path().join("node_modules/.bin")).unwrap();
+            std::fs::write(
+                root.path().join("node_modules/.bin/crlfbin"),
+                b"node \"$basedir/../crlfbin/cli.js\" \"$@\"",
+            )
+            .unwrap();
+            std::fs::create_dir_all(&pkg).unwrap();
+            tokio::fs::write(pkg.join("package.json"), manifest)
                 .await
                 .unwrap();
-            tokio::fs::write(pkg.path().join("cli.js"), CRLF_BIN_AFTER)
+            tokio::fs::write(pkg.join("cli.js"), CRLF_BIN_AFTER)
                 .await
                 .unwrap();
             assert!(
-                !needs_npm_bin_rewrite(
-                    purl,
-                    pkg.path(),
-                    "package/cli.js",
-                    &files["package/cli.js"]
-                )
-                .await
+                !needs_npm_bin_rewrite(purl, &pkg, "package/cli.js", &files["package/cli.js"])
+                    .await
             );
             let res = apply_package_patch(
                 purl,
-                pkg.path(),
+                &pkg,
                 &files,
                 &PatchSources::blobs_only(blobs.path()),
                 false,
@@ -4267,12 +4280,100 @@ mod tests {
             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(),
+                tokio::fs::read(pkg.join("cli.js")).await.unwrap(),
                 CRLF_BIN_AFTER
             );
         }
     }
 
+    /// Regression (#1804): npm rewrites the shebang only of bins it
+    /// links. With `bin-links=false`, or for a `directories.bin` file of a
+    /// `file:` tarball install (npm doesn't expand that field from a
+    /// tarball), the installed bin keeps its CRLF shebang, so apply writes
+    /// the recorded bytes and leaves them alone, and rollback restores the
+    /// installed bytes exactly.
+    #[tokio::test]
+    async fn apply_writes_recorded_bytes_for_bin_npm_did_not_link() {
+        for (manifest, rel, linked) in [
+            // `npm install --no-bin-links`: no `.bin` at all.
+            (
+                &br#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"cli.js"}}"#[..],
+                "cli.js",
+                None,
+            ),
+            // `directories.bin` from a tarball: `.bin` holds other bins only.
+            (
+                br#"{"name":"crlfbin","version":"1.0.0","directories":{"bin":"bin"}}"#,
+                "bin/cli.js",
+                Some("other"),
+            ),
+        ] {
+            let key = format!("package/{rel}");
+            let key = key.as_str();
+            let mut files = crlf_bin_files();
+            let info = files.remove("package/cli.js").unwrap();
+            files.insert(key.to_string(), info);
+            let blobs = crlf_bin_blobs().await;
+            let root = tempfile::tempdir().unwrap();
+            let pkg = root.path().join("node_modules/crlfbin");
+            std::fs::create_dir_all(pkg.join("bin")).unwrap();
+            if let Some(name) = linked {
+                std::fs::create_dir_all(root.path().join("node_modules/.bin")).unwrap();
+                std::fs::write(root.path().join("node_modules/.bin").join(name), b"shim").unwrap();
+            }
+            tokio::fs::write(pkg.join("package.json"), manifest)
+                .await
+                .unwrap();
+            let cli = pkg.join(rel);
+            tokio::fs::write(&cli, CRLF_BIN_BEFORE).await.unwrap();
+            let sources = PatchSources::blobs_only(blobs.path());
+            let apply = || {
+                apply_package_patch(
+                    "pkg:npm/crlfbin@1.0.0",
+                    &pkg,
+                    &files,
+                    &sources,
+                    false,
+                    MismatchPolicy::Strict,
+                )
+            };
+            let res = apply().await;
+            assert!(res.success, "{res:?}");
+            assert_eq!(res.files_patched, vec![key.to_string()]);
+            assert_eq!(
+                tokio::fs::read(&cli).await.unwrap(),
+                CRLF_BIN_AFTER,
+                "{key}"
+            );
+            assert!(
+                !needs_npm_bin_rewrite("pkg:npm/crlfbin@1.0.0", &pkg, key, &files[key]).await,
+                "{key}"
+            );
+            let again = apply().await;
+            assert!(again.files_patched.is_empty(), "{again:?}");
+            assert_eq!(
+                tokio::fs::read(&cli).await.unwrap(),
+                CRLF_BIN_AFTER,
+                "{key}"
+            );
+
+            let rolled = crate::patch::rollback::rollback_package_patch(
+                "pkg:npm/crlfbin@1.0.0",
+                &pkg,
+                &files,
+                blobs.path(),
+                false,
+            )
+            .await;
+            assert!(rolled.success, "{rolled:?}");
+            assert_eq!(
+                tokio::fs::read(&cli).await.unwrap(),
+                CRLF_BIN_BEFORE,
+                "rollback is byte-exact: {key}"
+            );
+        }
+    }
+
     /// A non-npm package keeps the patch's exact bytes on a consent
     /// overwrite: only npm rewrites bin shebangs.
     #[tokio::test]