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 57feb67cd..9529ae9f5 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs +++ b/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" + ); + } +} diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index 999651de1..7901f5043 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/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] diff --git a/crates/socket-patch-core/src/patch/npm_bin.rs b/crates/socket-patch-core/src/patch/npm_bin.rs index f245b5458..afdac2868 100644 --- a/crates/socket-patch-core/src/patch/npm_bin.rs +++ b/crates/socket-patch-core/src/patch/npm_bin.rs @@ -67,9 +67,12 @@ pub(crate) fn fixed_for_write( /// would leave a bin `env` can't run. /// /// 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 +/// it in `bin` and npm linked it ([`is_linked_bin`]), whatever shebang is +/// on disk now (a release before #1756 wrote the CRLF one, #1791). A bin +/// npm didn't link (`bin-links=false`, or a `directories.bin` file of a +/// non-registry install) keeps its published shebang, so it is written as +/// recorded (#1804). 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>( @@ -86,15 +89,14 @@ pub(crate) async fn form_for_write<'a>( 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))) - }; + let lf_shebang = read_prefix(&path) + .await + .is_some_and(|prefix| has_lf_shebang(&prefix)); + let npm_installed = (lf_shebang + && (is_npm(package_key) + || (!installed_source.is_empty() + && source_hash(&path).await.as_deref() == Some(installed_source)))) + || is_linked_bin(package_key, pkg_path, normalized).await; if !npm_installed { return as_recorded(content); } @@ -109,9 +111,22 @@ fn is_npm(package_key: &str) -> bool { } /// 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 { +/// the npm package `package_key` lists as a bin in its `package.json` and +/// npm linked: a file npm's `bin-links` ran `fixBin` on. +/// +/// npm rewrites only the bins it links, so the listing alone is not +/// enough: with `bin-links=false`, or for a `directories.bin` file of a +/// package installed from a tarball or folder (npm expands that field +/// only in registry packuments), the file keeps its published shebang +/// (#1804). The link is a `.bin/` entry in the `node_modules` the +/// package sits in, where npm links it, or a global prefix's +/// `bin/` or `.cmd`: a symlink resolving to this file, or a +/// cmd or shell shim naming it. A package in a virtual store (pnpm's +/// `.pnpm` or a relocated `virtualStoreDir`, npm's linked `.store`, vlt, +/// Bun, Yarn's `pnpmStoreFolder`) has its bins linked into each +/// dependent's `.bin`, wherever that is, so there the listing alone is +/// taken for the link, as before. +pub(crate) async fn is_linked_bin(package_key: &str, pkg_path: &Path, normalized: &str) -> bool { if !is_npm(package_key) { return false; } @@ -120,7 +135,107 @@ pub(crate) async fn is_listed_bin(package_key: &str, pkg_path: &Path, normalized else { return false; }; - lists_bin(&manifest, normalized) + let names = bin_names(&manifest, normalized); + if names.is_empty() { + return false; + } + let file = pkg_path.join(normalized); + let pkg_path = pkg_path.to_path_buf(); + crate::utils::fs::run_blocking(move || has_bin_link(&pkg_path, &file, &names)).await +} + +/// Whether an entry named one of `names` links the bin `file` of the +/// package at `pkg_path`, in the places [`is_linked_bin`] lists. +fn has_bin_link(pkg_path: &Path, file: &Path, names: &[String]) -> bool { + let mut ancestors = pkg_path.ancestors().skip(1); + // npm installs every package under a `node_modules`; a copy outside + // one is in a relocated store. + let Some(node_modules) = ancestors + .by_ref() + .find(|dir| dir.file_name().is_some_and(|n| n == "node_modules")) + else { + return true; + }; + if in_virtual_store(node_modules) { + return true; + } + // A shim names the file by its path below `node_modules`, from + // wherever it is (`..\crlfbin\cli.js`, `node_modules/crlfbin/cli.js`). + let Some(below) = file + .strip_prefix(node_modules) + .ok() + .and_then(Path::to_str) + .map(|p| p.replace('\\', "/")) + else { + return false; + }; + let target = std::fs::canonicalize(file).ok(); + let links = |entry: &Path| match std::fs::symlink_metadata(entry) { + Ok(meta) if meta.file_type().is_symlink() => { + target.is_some() && std::fs::canonicalize(entry).ok() == target + } + Ok(meta) if meta.is_file() => shim_names(entry, &below), + _ => false, + }; + let in_dir = |dir: &Path, suffix: &str| { + names + .iter() + .any(|name| links(&dir.join(format!("{name}{suffix}")))) + }; + if in_dir(&node_modules.join(".bin"), "") { + return true; + } + let Some(prefix) = node_modules.parent() else { + return false; + }; + // Windows global: `/node_modules`, shims in ``. + in_dir(prefix, ".cmd") + // Unix global: `/lib/node_modules`, links in `/bin`. + || (prefix.file_name().is_some_and(|n| n == "lib") + && prefix.parent().is_some_and(|p| in_dir(&p.join("bin"), ""))) +} + +/// Whether `node_modules` (the one a package sits in) is inside a virtual +/// store: below a dot folder of a `node_modules` (`.pnpm`, `.store`, +/// `.vlt`, `.bun`), or in a `@` store entry, which no +/// project folder is (a relocated pnpm `virtualStoreDir`). +fn in_virtual_store(node_modules: &Path) -> bool { + let entry = node_modules.parent(); + entry.is_some_and(|entry| { + entry + .file_name() + .and_then(|n| n.to_str()) + .is_some_and(|n| n.get(1..).is_some_and(|rest| rest.contains('@'))) + && !entry.join("package.json").exists() + }) || node_modules.ancestors().skip(1).any(|dir| { + dir.file_name() + .and_then(|n| n.to_str()) + .is_some_and(|n| n.starts_with('.')) + && dir + .parent() + .and_then(Path::file_name) + .is_some_and(|n| n == "node_modules") + }) +} + +/// Whether the shim at `path` (a cmd-shim or shell script) names the +/// file at `below`, its `/`-separated path below `node_modules`. +fn shim_names(path: &Path, below: &str) -> bool { + use std::io::Read as _; + let Ok((file, _)) = crate::utils::fs::open_regular_file_sync(path) else { + return false; + }; + let mut text = Vec::new(); + if file.take(8192).read_to_end(&mut text).is_err() { + return false; + } + let text = String::from_utf8_lossy(&text).replace('\\', "/"); + // A whole path segment run: `../crlfbin/cli.js`, not + // `../a-crlfbin/cli.js` or `../crlfbin/cli.json`. + let path_char = |c: char| c.is_alphanumeric() || matches!(c, '.' | '-' | '_' | '/' | '@'); + text.match_indices(below).any(|(at, _)| { + text[..at].ends_with('/') && !text[at + below.len()..].starts_with(path_char) + }) } /// Whether the file at `normalized` holds exactly the recorded content @@ -144,7 +259,7 @@ pub(crate) async fn needs_fix( }) { return false; } - if !is_listed_bin(package_key, pkg_path, normalized).await { + if !is_linked_bin(package_key, pkg_path, normalized).await { return false; } crate::utils::fs::read_regular_to_bytes(&path) @@ -154,46 +269,74 @@ pub(crate) async fn needs_fix( }) } -/// 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 { +/// The names npm links `normalized` under when the `package.json` bytes +/// `manifest` list it as a bin, as npm's `normalize-package-bin` and +/// `directories.bin` step read them: a `bin` path (a string, linked under +/// the unscoped package `name`, an object's values under their keys, or +/// an array's entries under their file names) or, when `bin` is missing +/// or empty, a non-hidden file under `directories.bin`, under its file +/// name. Paths are resolved as npm does (`./` and `..` folded, never +/// above the package). Empty when the file is not a listed bin. +fn bin_names(manifest: &[u8], normalized: &str) -> Vec { use serde_json::Value; let Ok(json) = serde_json::from_slice::(manifest) else { - return false; + return Vec::new(); }; 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) + let Some(file_name) = target.last().map(|n| n.to_string()) else { + return Vec::new(); + }; + // npm keeps only a key's last segment, `\` and `:` taken for `/`. + let link_name = |key: &str| { + key.trim_end_matches(['/', '\\', ':']) + .rsplit(['/', '\\', ':']) + .next() + .filter(|n| !n.is_empty() && *n != "." && *n != "..") + .map(str::to_string) }; 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::String(bin)) if !bin.is_empty() => json + .get("name") + .and_then(Value::as_str) + .filter(|_| clean_path(bin) == target) + .and_then(link_name) + .into_iter() + .collect(), + Some(Value::Object(bins)) => bins + .iter() + .filter(|(_, bin)| bin.as_str().is_some_and(|bin| clean_path(bin) == target)) + .filter_map(|(key, _)| link_name(key)) + .collect(), + Some(Value::Array(bins)) => { + let listed = bins + .iter() + .filter_map(Value::as_str) + .any(|bin| clean_path(bin) == target); + if listed { + vec![file_name] + } else { + Vec::new() + } + } + Some(Value::String(_) | Value::Null | Value::Bool(false)) | None => { + let listed = 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('.')) + }); + if listed { + vec![file_name] + } else { + Vec::new() + } } - 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, + Some(_) => Vec::new(), } } @@ -368,7 +511,7 @@ mod tests { #[test] fn lists_bin_reads_every_bin_shape() { - let listed = |manifest: &str, file: &str| lists_bin(manifest.as_bytes(), file); + let listed = |manifest: &str, file: &str| !bin_names(manifest.as_bytes(), file).is_empty(); assert!(listed(r#"{"name":"x","bin":"cli.js"}"#, "cli.js")); assert!(listed(r#"{"name":"x","bin":"./bin/../cli.js"}"#, "cli.js")); assert!(listed( @@ -403,6 +546,179 @@ mod tests { assert!(!listed("not json", "cli.js")); } + #[test] + fn bin_names_are_npms_link_names() { + let names = |manifest: &str, file: &str| bin_names(manifest.as_bytes(), file); + assert_eq!(names(r#"{"name":"@s/x","bin":"cli.js"}"#, "cli.js"), ["x"]); + assert_eq!( + names( + r#"{"bin":{"a":"cli.js","b":"other.js","c":"./cli.js"}}"#, + "cli.js" + ), + ["a", "c"] + ); + assert_eq!(names(r#"{"bin":{"../../a":"cli.js"}}"#, "cli.js"), ["a"]); + assert_eq!(names(r#"{"bin":{"a/":"cli.js"}}"#, "cli.js"), ["a"]); + assert_eq!(names(r#"{"bin":["bin/cli.js"]}"#, "bin/cli.js"), ["cli.js"]); + assert_eq!( + names(r#"{"directories":{"bin":"bin"}}"#, "bin/sub/run"), + ["run"] + ); + assert!(names(r#"{"bin":{"a":"x.js"}}"#, "cli.js").is_empty()); + } + + /// `node_modules//cli.js` under `root`, its package.json listing + /// it as the bin `crlfbin`. Returns the package dir. + fn installed_bin(root: &Path, under: &str) -> std::path::PathBuf { + let pkg = root.join(under).join("crlfbin"); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name":"crlfbin","bin":{"crlfbin":"cli.js"}}"#, + ) + .unwrap(); + std::fs::write(pkg.join("cli.js"), b"#!/usr/bin/env node\r\n").unwrap(); + pkg + } + + /// A cmd-shim at `path` running `target` (relative to the shim). + fn shim_to(path: std::path::PathBuf, target: &str) { + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + let target = target.replace('/', "\\"); + std::fs::write(path, format!("@\"%dp0%\\{target}\" %*\r\n")).unwrap(); + } + + fn shim(path: std::path::PathBuf) { + shim_to(path, "../crlfbin/cli.js"); + } + + const PURL: &str = "pkg:npm/crlfbin@1.0.0"; + + /// Regression (#1804): npm runs `fixBin` only on bins it links, so a + /// listed bin without a link (`bin-links=false`) is not npm's form. + #[tokio::test] + async fn is_linked_bin_wants_npms_link() { + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "node_modules"); + assert!( + !is_linked_bin(PURL, &pkg, "cli.js").await, + "bin-links=false" + ); + shim(root.path().join("node_modules/.bin/other")); + assert!(!is_linked_bin(PURL, &pkg, "cli.js").await, "another bin"); + shim(root.path().join("node_modules/.bin/crlfbin")); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await); + assert!(!is_linked_bin("pkg:pypi/crlfbin@1.0.0", &pkg, "cli.js").await); + } + + #[cfg(unix)] + #[tokio::test] + async fn is_linked_bin_follows_a_symlink_to_this_file() { + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "node_modules"); + let bin = root.path().join("node_modules/.bin"); + std::fs::create_dir_all(&bin).unwrap(); + std::fs::write(pkg.join("other.js"), b"x").unwrap(); + std::os::unix::fs::symlink("../crlfbin/other.js", bin.join("crlfbin")).unwrap(); + assert!( + !is_linked_bin(PURL, &pkg, "cli.js").await, + "links elsewhere" + ); + std::fs::remove_file(bin.join("crlfbin")).unwrap(); + std::os::unix::fs::symlink("../crlfbin/cli.js", bin.join("crlfbin")).unwrap(); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await); + } + + #[tokio::test] + async fn is_linked_bin_finds_global_and_pnpm_links() { + // Unix global prefix. + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "lib/node_modules"); + assert!(!is_linked_bin(PURL, &pkg, "cli.js").await); + shim(root.path().join("bin/crlfbin")); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await); + // Windows global prefix. + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "node_modules"); + shim(root.path().join("crlfbin.cmd")); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await); + // A virtual-store copy, whose bins are linked into its dependents'. + for store in [ + "node_modules/.pnpm/crlfbin@1.0.0/node_modules", + "node_modules/.store/crlfbin@1.0.0/node_modules", + ] { + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), store); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await, "{store}"); + } + } + + /// npm links a nested copy's bins into its own `node_modules/.bin`, so + /// the top-level copy's link says nothing about it. + #[tokio::test] + async fn is_linked_bin_looks_only_where_npm_links_the_copy() { + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "node_modules/a/node_modules"); + shim(root.path().join("node_modules/.bin/crlfbin")); + assert!(!is_linked_bin(PURL, &pkg, "cli.js").await); + shim(root.path().join("node_modules/a/node_modules/.bin/crlfbin")); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await); + // Scoped: `node_modules/@s/crlfbin` links into `node_modules/.bin`. + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "node_modules/@s"); + shim_to( + root.path().join("node_modules/.bin/crlfbin"), + "../@s/crlfbin/cli.js", + ); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await); + } + + /// A file that merely has a bin's name, where a global prefix would + /// keep its shim, is not npm's link unless it runs this file. + #[tokio::test] + async fn is_linked_bin_wants_the_shim_to_run_this_file() { + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "node_modules"); + shim_to(root.path().join("crlfbin.cmd"), "scripts/crlfbin.js"); + shim_to( + root.path().join("node_modules/.bin/crlfbin"), + "../a-crlfbin/cli.js", + ); + assert!(!is_linked_bin(PURL, &pkg, "cli.js").await, "a-crlfbin"); + shim_to( + root.path().join("node_modules/.bin/crlfbin"), + "../crlfbin/cli.json", + ); + assert!(!is_linked_bin(PURL, &pkg, "cli.js").await, "cli.json"); + shim_to( + root.path().join("node_modules/.bin/crlfbin"), + "../other/cli.js", + ); + assert!(!is_linked_bin(PURL, &pkg, "cli.js").await); + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "lib/node_modules"); + shim_to(root.path().join("bin/crlfbin"), "../tools/crlfbin"); + assert!(!is_linked_bin(PURL, &pkg, "cli.js").await); + } + + /// Stores outside a `node_modules` dot folder: a relocated pnpm + /// `virtualStoreDir`, and Yarn's `pnpmStoreFolder` (`/package`). + #[tokio::test] + async fn is_linked_bin_takes_the_listing_in_relocated_stores() { + let root = tempfile::tempdir().unwrap(); + let pkg = installed_bin(root.path(), "vstore/crlfbin@1.0.0/node_modules"); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await); + let root = tempfile::tempdir().unwrap(); + let pkg = root.path().join("store/crlfbin-npm-1.0.0-abc/package"); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name":"crlfbin","bin":{"crlfbin":"cli.js"}}"#, + ) + .unwrap(); + assert!(is_linked_bin(PURL, &pkg, "cli.js").await); + } + #[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 9ee5a73fe..f7cb2928c 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -3244,21 +3244,28 @@ mod tests { 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 root = tempfile::tempdir().unwrap(); + // npm linked the bin, so it ran `fixBin` on it at install. + let pkg = root.path().join("node_modules/x"); + std::fs::create_dir_all(root.path().join("node_modules/.bin")).unwrap(); + std::fs::write( + root.path().join("node_modules/.bin/x"), + b"node \"$basedir/../x/cli.js\" \"$@\"", + ) + .unwrap(); + std::fs::create_dir_all(&pkg).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"), + pkg.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(); + tokio::fs::write(pkg.join("cli.js"), after).await.unwrap(); let files = HashMap::from([( "package/cli.js".to_string(), PatchFileInfo { @@ -3267,11 +3274,10 @@ mod tests { }, )]); let res = - rollback_package_patch("pkg:npm/x@1.0.0", pkg.path(), &files, blobs.path(), false) - .await; + rollback_package_patch("pkg:npm/x@1.0.0", &pkg, &files, blobs.path(), false).await; assert!(res.success, "{res:?}"); assert_eq!( - tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(), + tokio::fs::read(pkg.join("cli.js")).await.unwrap(), b"#!/usr/bin/env node\nconsole.log(1);\r\n" ); } @@ -3292,7 +3298,16 @@ mod tests { }, )]); for with_blob in [true, false] { - 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/x"); + std::fs::create_dir_all(root.path().join("node_modules/.bin")).unwrap(); + std::fs::write( + root.path().join("node_modules/.bin/x"), + b"node \"$basedir/../x/cli.js\" \"$@\"", + ) + .unwrap(); + std::fs::create_dir_all(&pkg).unwrap(); let blobs = tempfile::tempdir().unwrap(); if with_blob { tokio::fs::write(blobs.path().join(&before_hash), before) @@ -3300,17 +3315,14 @@ mod tests { .unwrap(); } tokio::fs::write( - pkg.path().join("package.json"), + pkg.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(); + tokio::fs::write(pkg.join("cli.js"), before).await.unwrap(); let res = - rollback_package_patch("pkg:npm/x@1.0.0", pkg.path(), &files, blobs.path(), false) - .await; + rollback_package_patch("pkg:npm/x@1.0.0", &pkg, &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" @@ -3318,7 +3330,7 @@ mod tests { before }; assert_eq!( - tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(), + tokio::fs::read(pkg.join("cli.js")).await.unwrap(), expected, "{with_blob}" );