Skip to content

Commit b73b080

Browse files
Fix npm bin shebang on unlinked bins (#1804) (#1805)
* Start fix for #1804 Assisted-by: Claude Code:claude-opus-5-5 * Keep CRLF shebang on bins npm didn't link Since #1792, apply and rollback treated every file a package lists as a bin as one npm had rewritten to an LF shebang at install. npm only does that for bins it links. With bin-links=false (--no-bin-links), or for a directories.bin file of a package installed from a tarball or folder, the installed file keeps its CRLF shebang, so apply wrote bytes that matched neither the patch nor a fresh install, and rollback no longer restored the installed file byte for byte. A listed bin now counts as npm's form only when npm linked it: a .bin/<name> entry in a node_modules the package sits under (a symlink must point at this file), a pnpm dependent's .bin entry, or a global prefix bin or .cmd shim. Bins npm did link, including ones an older release left with a CRLF shebang (#1791), are still repaired. Fixes #1804 Assisted-by: Claude Code:claude-opus-5-5 * Look for npm's bin link only where npm puts it npm links a package's bins into the .bin folder of the node_modules the package sits in, so a nested copy is no longer taken as linked because the top-level copy of the same package has a .bin entry. Packages in a virtual store (pnpm, npm's linked strategy, vlt, Bun) get their bins linked into each dependent instead, so there the bin listing alone still counts, as it did before. Also check the on-disk shebang before the link, which is cheaper, and read a bin key with a trailing slash the way npm does. Refs #1804 Assisted-by: Claude Code:claude-opus-5-5 * Only trust a bin shim that runs this file A regular file that merely had a bin's name, such as a project's own <name>.cmd or a lib/../bin script, counted as npm's link, so a bin npm never linked could still be rewritten with an LF shebang. A shim now has to name this file's path below node_modules, as npm's and pnpm's shims do; a symlink still has to resolve to it. Copies in a store npm doesn't lay out (outside any node_modules, as in Yarn's pnpmStoreFolder, or in a name@version entry of a relocated pnpm virtualStoreDir) keep taking the bin listing as the link, as before. Refs #1804 Assisted-by: Claude Code:claude-opus-5-5 * Match a bin shim's path on whole segments A shim for another package whose folder name ends the same way, such as a-crlfbin/cli.js, or a sibling file like cli.json, counted as running this package's bin. The shim now has to name the path as whole segments. Refs #1804 Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: socket-patch-agent <noreply@anthropic.com>
1 parent 34f13e0 commit b73b080

4 files changed

Lines changed: 605 additions & 105 deletions

File tree

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

Lines changed: 91 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1174,15 +1174,27 @@ const NPM_BIN_AFTER: &[u8] = b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n// S
11741174
/// `bin`) npm-installed, and an agent manifest patching that bin with
11751175
/// its blobs. Returns the project dir.
11761176
fn crlf_bin_project(tmp: &Path) -> PathBuf {
1177+
let proj = crlf_bin_project_with(
1178+
tmp,
1179+
r#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"cli.js"}}"#,
1180+
"cli.js",
1181+
&[],
1182+
);
1183+
let cli = proj.join("node_modules/crlfbin/cli.js");
1184+
assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE, "npm's fixBin");
1185+
proj
1186+
}
1187+
1188+
/// [`crlf_bin_project`] with the package's `package.json` `manifest`, the
1189+
/// CRLF-shebang file at `rel` in the package, and `flags` added to
1190+
/// `npm install`.
1191+
fn crlf_bin_project_with(tmp: &Path, manifest: &str, rel: &str, flags: &[&str]) -> PathBuf {
11771192
let (before, after) = (CRLF_BIN_BEFORE, CRLF_BIN_AFTER);
11781193
let pkg = tmp.join("pkg");
1179-
std::fs::create_dir_all(&pkg).unwrap();
1180-
std::fs::write(
1181-
pkg.join("package.json"),
1182-
r#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"cli.js"}}"#,
1183-
)
1184-
.unwrap();
1185-
std::fs::write(pkg.join("cli.js"), before).unwrap();
1194+
let file = pkg.join(rel);
1195+
std::fs::create_dir_all(file.parent().unwrap()).unwrap();
1196+
std::fs::write(pkg.join("package.json"), manifest).unwrap();
1197+
std::fs::write(&file, before).unwrap();
11861198
std::fs::write(
11871199
tmp.join("crlfbin-1.0.0.tgz"),
11881200
npm_e2e_common::npm_pack_like(&pkg),
@@ -1196,23 +1208,20 @@ fn crlf_bin_project(tmp: &Path) -> PathBuf {
11961208
)
11971209
.unwrap();
11981210
let cache = tmp.join("cache");
1199-
let out = npm_e2e_common::npm(
1200-
&proj,
1201-
&[
1202-
"install",
1203-
"--no-audit",
1204-
"--no-fund",
1205-
"--cache",
1206-
cache.to_str().unwrap(),
1207-
],
1208-
);
1211+
let mut args = vec![
1212+
"install",
1213+
"--no-audit",
1214+
"--no-fund",
1215+
"--cache",
1216+
cache.to_str().unwrap(),
1217+
];
1218+
args.extend_from_slice(flags);
1219+
let out = npm_e2e_common::npm(&proj, &args);
12091220
assert!(
12101221
out.status.success(),
12111222
"{}",
12121223
npm_e2e_common::output_text(&out)
12131224
);
1214-
let cli = proj.join("node_modules/crlfbin/cli.js");
1215-
assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE, "npm's fixBin");
12161225

12171226
let (b, a) = (git_sha256(before), git_sha256(after));
12181227
let blobs = proj.join(".socket/blobs");
@@ -1223,7 +1232,7 @@ fn crlf_bin_project(tmp: &Path) -> PathBuf {
12231232
proj.join(".socket/manifest.json"),
12241233
format!(
12251234
r#"{{"patches":{{"pkg:npm/crlfbin@1.0.0":{{"uuid":"e2f99e5d-3afd-5cbd-be80-6a0102bcd6c2",
1226-
"exportedAt":"2026-01-01T00:00:00Z","files":{{"package/cli.js":{{"beforeHash":"{b}","afterHash":"{a}"}}}},
1235+
"exportedAt":"2026-01-01T00:00:00Z","files":{{"package/{rel}":{{"beforeHash":"{b}","afterHash":"{a}"}}}},
12271236
"vulnerabilities":{{"GHSA-xxxx-yyyy-zzzz":{{"cves":["CVE-2099-0001"],"summary":"t","severity":"high","description":"d"}}}},
12281237
"description":"d","license":"MIT","tier":"free"}}}}}}"#
12291238
),
@@ -1345,3 +1354,65 @@ async fn npm_agent_recovers_crlf_bin_left_by_raw_write() {
13451354
assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE);
13461355
assert_crlf_bin_runs(&proj, "after rollback");
13471356
}
1357+
1358+
/// Regression (#1804): npm rewrites the shebang only of bins it links.
1359+
/// With `--no-bin-links`, or for a `directories.bin`-only package
1360+
/// installed from a `file:` tarball (npm doesn't expand that field from a
1361+
/// tarball's `package.json`), the installed bin keeps its CRLF shebang.
1362+
/// `apply` writes the patch's recorded bytes, `apply --check` agrees, and
1363+
/// `rollback` restores what `npm install` wrote, byte for byte.
1364+
#[cfg(unix)]
1365+
#[tokio::test(flavor = "multi_thread")]
1366+
#[ignore = "wall-bound real-npm install; runs on the e2e CI matrix leg"]
1367+
async fn npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link() {
1368+
const SUITE: &str = "npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link";
1369+
if npm_e2e_common::npm_version().is_none() {
1370+
npm_e2e_common::skip(SUITE, "npm not on PATH");
1371+
return;
1372+
}
1373+
for (manifest, rel, flags) in [
1374+
(
1375+
r#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"cli.js"}}"#,
1376+
"cli.js",
1377+
&["--no-bin-links"][..],
1378+
),
1379+
(
1380+
r#"{"name":"crlfbin","version":"1.0.0","directories":{"bin":"bin"}}"#,
1381+
"bin/cli.js",
1382+
&[][..],
1383+
),
1384+
] {
1385+
let tmp = tempfile::tempdir().unwrap();
1386+
let proj = crlf_bin_project_with(tmp.path(), manifest, rel, flags);
1387+
let cli = proj.join("node_modules/crlfbin").join(rel);
1388+
assert_eq!(
1389+
std::fs::read(&cli).unwrap(),
1390+
CRLF_BIN_BEFORE,
1391+
"{rel}: npm left the shebang alone"
1392+
);
1393+
1394+
let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]);
1395+
assert_eq!(
1396+
code, 0,
1397+
"{rel}: apply\nstdout:\n{stdout}\nstderr:\n{stderr}"
1398+
);
1399+
assert_eq!(std::fs::read(&cli).unwrap(), CRLF_BIN_AFTER, "{rel}: apply");
1400+
1401+
let (code, stdout, stderr) = run_socket(&proj, &["apply", "--check", "--offline"]);
1402+
assert_eq!(
1403+
code, 0,
1404+
"{rel}: check\nstdout:\n{stdout}\nstderr:\n{stderr}"
1405+
);
1406+
1407+
let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--offline"]);
1408+
assert_eq!(
1409+
code, 0,
1410+
"{rel}: rollback\nstdout:\n{stdout}\nstderr:\n{stderr}"
1411+
);
1412+
assert_eq!(
1413+
std::fs::read(&cli).unwrap(),
1414+
CRLF_BIN_BEFORE,
1415+
"{rel}: rollback is byte-exact"
1416+
);
1417+
}
1418+
}

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

Lines changed: 119 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4180,21 +4180,30 @@ mod tests {
41804180
async fn apply_repairs_crlf_bin_left_by_raw_write() {
41814181
let files = crlf_bin_files();
41824182
let blobs = crlf_bin_blobs().await;
4183-
let pkg = tempfile::tempdir().unwrap();
4183+
let root = tempfile::tempdir().unwrap();
4184+
// npm linked the bin, so it ran `fixBin` on it at install.
4185+
let pkg = root.path().join("node_modules/crlfbin");
4186+
std::fs::create_dir_all(root.path().join("node_modules/.bin")).unwrap();
4187+
std::fs::write(
4188+
root.path().join("node_modules/.bin/crlfbin"),
4189+
b"node \"$basedir/../crlfbin/cli.js\" \"$@\"",
4190+
)
4191+
.unwrap();
4192+
std::fs::create_dir_all(&pkg).unwrap();
41844193
tokio::fs::write(
4185-
pkg.path().join("package.json"),
4194+
pkg.join("package.json"),
41864195
br#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"./cli.js"}}"#,
41874196
)
41884197
.await
41894198
.unwrap();
4190-
tokio::fs::write(pkg.path().join("cli.js"), CRLF_BIN_AFTER)
4199+
tokio::fs::write(pkg.join("cli.js"), CRLF_BIN_AFTER)
41914200
.await
41924201
.unwrap();
41934202
let sources = PatchSources::blobs_only(blobs.path());
41944203
let apply = || {
41954204
apply_package_patch(
41964205
"pkg:npm/crlfbin@1.0.0",
4197-
pkg.path(),
4206+
&pkg,
41984207
&files,
41994208
&sources,
42004209
false,
@@ -4205,7 +4214,7 @@ mod tests {
42054214
assert!(res.success, "{res:?}");
42064215
assert_eq!(res.files_patched, vec!["package/cli.js".to_string()]);
42074216
assert_eq!(
4208-
tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(),
4217+
tokio::fs::read(pkg.join("cli.js")).await.unwrap(),
42094218
NPM_BIN_AFTER
42104219
);
42114220
let again = apply().await;
@@ -4214,7 +4223,7 @@ mod tests {
42144223
assert!(
42154224
!needs_npm_bin_rewrite(
42164225
"pkg:npm/crlfbin@1.0.0",
4217-
pkg.path(),
4226+
&pkg,
42184227
"package/cli.js",
42194228
&files["package/cli.js"]
42204229
)
@@ -4239,25 +4248,29 @@ mod tests {
42394248
br#"{"name":"crlfbin","bin":"cli.js"}"#,
42404249
),
42414250
] {
4242-
let pkg = tempfile::tempdir().unwrap();
4243-
tokio::fs::write(pkg.path().join("package.json"), manifest)
4251+
// npm linked a `crlfbin` bin, but not to this file.
4252+
let root = tempfile::tempdir().unwrap();
4253+
let pkg = root.path().join("node_modules/crlfbin");
4254+
std::fs::create_dir_all(root.path().join("node_modules/.bin")).unwrap();
4255+
std::fs::write(
4256+
root.path().join("node_modules/.bin/crlfbin"),
4257+
b"node \"$basedir/../crlfbin/cli.js\" \"$@\"",
4258+
)
4259+
.unwrap();
4260+
std::fs::create_dir_all(&pkg).unwrap();
4261+
tokio::fs::write(pkg.join("package.json"), manifest)
42444262
.await
42454263
.unwrap();
4246-
tokio::fs::write(pkg.path().join("cli.js"), CRLF_BIN_AFTER)
4264+
tokio::fs::write(pkg.join("cli.js"), CRLF_BIN_AFTER)
42474265
.await
42484266
.unwrap();
42494267
assert!(
4250-
!needs_npm_bin_rewrite(
4251-
purl,
4252-
pkg.path(),
4253-
"package/cli.js",
4254-
&files["package/cli.js"]
4255-
)
4256-
.await
4268+
!needs_npm_bin_rewrite(purl, &pkg, "package/cli.js", &files["package/cli.js"])
4269+
.await
42574270
);
42584271
let res = apply_package_patch(
42594272
purl,
4260-
pkg.path(),
4273+
&pkg,
42614274
&files,
42624275
&PatchSources::blobs_only(blobs.path()),
42634276
false,
@@ -4267,12 +4280,100 @@ mod tests {
42674280
assert!(res.success, "{purl}: {res:?}");
42684281
assert!(res.files_patched.is_empty(), "{purl}: {res:?}");
42694282
assert_eq!(
4270-
tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(),
4283+
tokio::fs::read(pkg.join("cli.js")).await.unwrap(),
42714284
CRLF_BIN_AFTER
42724285
);
42734286
}
42744287
}
42754288

4289+
/// Regression (#1804): npm rewrites the shebang only of bins it
4290+
/// links. With `bin-links=false`, or for a `directories.bin` file of a
4291+
/// `file:` tarball install (npm doesn't expand that field from a
4292+
/// tarball), the installed bin keeps its CRLF shebang, so apply writes
4293+
/// the recorded bytes and leaves them alone, and rollback restores the
4294+
/// installed bytes exactly.
4295+
#[tokio::test]
4296+
async fn apply_writes_recorded_bytes_for_bin_npm_did_not_link() {
4297+
for (manifest, rel, linked) in [
4298+
// `npm install --no-bin-links`: no `.bin` at all.
4299+
(
4300+
&br#"{"name":"crlfbin","version":"1.0.0","bin":{"crlfbin":"cli.js"}}"#[..],
4301+
"cli.js",
4302+
None,
4303+
),
4304+
// `directories.bin` from a tarball: `.bin` holds other bins only.
4305+
(
4306+
br#"{"name":"crlfbin","version":"1.0.0","directories":{"bin":"bin"}}"#,
4307+
"bin/cli.js",
4308+
Some("other"),
4309+
),
4310+
] {
4311+
let key = format!("package/{rel}");
4312+
let key = key.as_str();
4313+
let mut files = crlf_bin_files();
4314+
let info = files.remove("package/cli.js").unwrap();
4315+
files.insert(key.to_string(), info);
4316+
let blobs = crlf_bin_blobs().await;
4317+
let root = tempfile::tempdir().unwrap();
4318+
let pkg = root.path().join("node_modules/crlfbin");
4319+
std::fs::create_dir_all(pkg.join("bin")).unwrap();
4320+
if let Some(name) = linked {
4321+
std::fs::create_dir_all(root.path().join("node_modules/.bin")).unwrap();
4322+
std::fs::write(root.path().join("node_modules/.bin").join(name), b"shim").unwrap();
4323+
}
4324+
tokio::fs::write(pkg.join("package.json"), manifest)
4325+
.await
4326+
.unwrap();
4327+
let cli = pkg.join(rel);
4328+
tokio::fs::write(&cli, CRLF_BIN_BEFORE).await.unwrap();
4329+
let sources = PatchSources::blobs_only(blobs.path());
4330+
let apply = || {
4331+
apply_package_patch(
4332+
"pkg:npm/crlfbin@1.0.0",
4333+
&pkg,
4334+
&files,
4335+
&sources,
4336+
false,
4337+
MismatchPolicy::Strict,
4338+
)
4339+
};
4340+
let res = apply().await;
4341+
assert!(res.success, "{res:?}");
4342+
assert_eq!(res.files_patched, vec![key.to_string()]);
4343+
assert_eq!(
4344+
tokio::fs::read(&cli).await.unwrap(),
4345+
CRLF_BIN_AFTER,
4346+
"{key}"
4347+
);
4348+
assert!(
4349+
!needs_npm_bin_rewrite("pkg:npm/crlfbin@1.0.0", &pkg, key, &files[key]).await,
4350+
"{key}"
4351+
);
4352+
let again = apply().await;
4353+
assert!(again.files_patched.is_empty(), "{again:?}");
4354+
assert_eq!(
4355+
tokio::fs::read(&cli).await.unwrap(),
4356+
CRLF_BIN_AFTER,
4357+
"{key}"
4358+
);
4359+
4360+
let rolled = crate::patch::rollback::rollback_package_patch(
4361+
"pkg:npm/crlfbin@1.0.0",
4362+
&pkg,
4363+
&files,
4364+
blobs.path(),
4365+
false,
4366+
)
4367+
.await;
4368+
assert!(rolled.success, "{rolled:?}");
4369+
assert_eq!(
4370+
tokio::fs::read(&cli).await.unwrap(),
4371+
CRLF_BIN_BEFORE,
4372+
"rollback is byte-exact: {key}"
4373+
);
4374+
}
4375+
}
4376+
42764377
/// A non-npm package keeps the patch's exact bytes on a consent
42774378
/// overwrite: only npm rewrites bin shebangs.
42784379
#[tokio::test]

0 commit comments

Comments
 (0)