Skip to content

Commit bb8b488

Browse files
committed
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
1 parent 2d07dc0 commit bb8b488

5 files changed

Lines changed: 195 additions & 51 deletions

File tree

‎crates/socket-patch-cli/src/commands/apply.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -609,8 +609,10 @@ async fn verify_installed_tree(
609609
continue;
610610
};
611611
'copies: for copy in copies.get(purl).into_iter().flatten() {
612-
for file in record.files.keys() {
613-
if socket_patch_core::patch::apply::needs_npm_bin_rewrite(purl, copy, file).await {
612+
for (file, info) in &record.files {
613+
if socket_patch_core::patch::apply::needs_npm_bin_rewrite(purl, copy, file, info)
614+
.await
615+
{
614616
crlf_bins.push(purl.clone());
615617
break 'copies;
616618
}

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

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1267,27 +1267,14 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() {
12671267
let proj = crlf_bin_project(tmp.path());
12681268
let cli = proj.join("node_modules/crlfbin/cli.js");
12691269

1270-
let run_bin = || {
1271-
std::process::Command::new(proj.join("node_modules/.bin/crlfbin"))
1272-
.current_dir(&proj)
1273-
.output()
1274-
.unwrap()
1275-
};
1276-
12771270
let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]);
12781271
assert_eq!(code, 0, "apply\nstdout:\n{stdout}\nstderr:\n{stderr}");
12791272
assert!(
12801273
!stderr.contains("did not match"),
12811274
"no drift warning: {stderr}"
12821275
);
12831276
assert_eq!(std::fs::read(&cli).unwrap(), installed_after);
1284-
let ran = run_bin();
1285-
assert!(
1286-
ran.status.success(),
1287-
"{}",
1288-
npm_e2e_common::output_text(&ran)
1289-
);
1290-
assert_eq!(String::from_utf8_lossy(&ran.stdout).trim(), "hi");
1277+
assert_crlf_bin_runs(&proj, "after apply");
12911278

12921279
let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]);
12931280
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() {
13151302
let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--offline"]);
13161303
assert_eq!(code, 0, "rollback\nstdout:\n{stdout}\nstderr:\n{stderr}");
13171304
assert_eq!(std::fs::read(&cli).unwrap(), installed_before);
1318-
let ran = run_bin();
1319-
assert!(
1320-
ran.status.success(),
1321-
"{}",
1322-
npm_e2e_common::output_text(&ran)
1323-
);
1305+
assert_crlf_bin_runs(&proj, "after rollback");
13241306
}
13251307

13261308
/// Regression (#1791): v5.0.1 wrote the raw CRLF patched bytes over the

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

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1026,8 +1026,13 @@ async fn mark_crlf_bin_rewrite(
10261026
) {
10271027
if verify.status == VerifyStatus::AlreadyPatched
10281028
&& verify.current_hash.as_deref() == Some(file_info.after_hash.as_str())
1029-
&& crate::patch::npm_bin::needs_fix(package_key, pkg_path, normalize_file_path(file_name))
1030-
.await
1029+
&& crate::patch::npm_bin::needs_fix(
1030+
package_key,
1031+
pkg_path,
1032+
normalize_file_path(file_name),
1033+
&file_info.after_hash,
1034+
)
1035+
.await
10311036
{
10321037
verify.status = VerifyStatus::Ready;
10331038
verify.target_hash = Some(file_info.after_hash.clone());
@@ -1036,12 +1041,24 @@ async fn mark_crlf_bin_rewrite(
10361041

10371042
/// Whether `apply` would rewrite `file_name` (a manifest key) of the
10381043
/// installed `package_key` copy at `pkg_path` although its bytes hash to
1039-
/// the patched content: an npm bin left with a CRLF shebang `env` can't
1040-
/// run (#1791). `apply --check` reports it as not applied.
1041-
pub async fn needs_npm_bin_rewrite(package_key: &str, pkg_path: &Path, file_name: &str) -> bool {
1044+
/// the patched content `file_info.after_hash`: an npm bin left with a
1045+
/// CRLF shebang `env` can't run (#1791). `apply --check` reports it as
1046+
/// not applied.
1047+
pub async fn needs_npm_bin_rewrite(
1048+
package_key: &str,
1049+
pkg_path: &Path,
1050+
file_name: &str,
1051+
file_info: &PatchFileInfo,
1052+
) -> bool {
10421053
let normalized = normalize_file_path(file_name);
10431054
is_safe_relative_subpath(normalized)
1044-
&& crate::patch::npm_bin::needs_fix(package_key, pkg_path, normalized).await
1055+
&& crate::patch::npm_bin::needs_fix(
1056+
package_key,
1057+
pkg_path,
1058+
normalized,
1059+
&file_info.after_hash,
1060+
)
1061+
.await
10451062
}
10461063

10471064
/// The single-copy apply engine behind [`apply_package_patch`]: verifies
@@ -4195,7 +4212,13 @@ mod tests {
41954212
assert!(again.success, "{again:?}");
41964213
assert!(again.files_patched.is_empty(), "{again:?}");
41974214
assert!(
4198-
!needs_npm_bin_rewrite("pkg:npm/crlfbin@1.0.0", pkg.path(), "package/cli.js").await
4215+
!needs_npm_bin_rewrite(
4216+
"pkg:npm/crlfbin@1.0.0",
4217+
pkg.path(),
4218+
"package/cli.js",
4219+
&files["package/cli.js"]
4220+
)
4221+
.await
41994222
);
42004223
}
42014224

@@ -4223,7 +4246,15 @@ mod tests {
42234246
tokio::fs::write(pkg.path().join("cli.js"), CRLF_BIN_AFTER)
42244247
.await
42254248
.unwrap();
4226-
assert!(!needs_npm_bin_rewrite(purl, pkg.path(), "package/cli.js").await);
4249+
assert!(
4250+
!needs_npm_bin_rewrite(
4251+
purl,
4252+
pkg.path(),
4253+
"package/cli.js",
4254+
&files["package/cli.js"]
4255+
)
4256+
.await
4257+
);
42274258
let res = apply_package_patch(
42284259
purl,
42294260
pkg.path(),

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

Lines changed: 58 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ pub(crate) async fn form_for_write<'a>(
8181
expected_hash: &str,
8282
) -> Result<(std::borrow::Cow<'a, [u8]>, String), std::io::Error> {
8383
let as_recorded = |content| Ok((content, expected_hash.to_string()));
84+
// Content `fixBin` leaves alone needs no evidence read from disk.
85+
if fix_bin(&content).is_none() {
86+
return as_recorded(content);
87+
}
8488
let path = pkg_path.join(normalized);
8589
let npm_installed = is_listed_bin(package_key, pkg_path, normalized).await || {
8690
let lf_shebang = read_prefix(&path)
@@ -119,11 +123,17 @@ pub(crate) async fn is_listed_bin(package_key: &str, pkg_path: &Path, normalized
119123
lists_bin(&manifest, normalized)
120124
}
121125

122-
/// Whether the file holding raw bytes `fixBin` would change (a CRLF
123-
/// shebang npm never leaves in a bin) is a bin of the npm package
124-
/// `package_key`: the state a release before #1756 left by writing the
125-
/// recorded bytes over npm's form, which `env` can't run (#1791).
126-
pub(crate) async fn needs_fix(package_key: &str, pkg_path: &Path, normalized: &str) -> bool {
126+
/// Whether the file at `normalized` holds exactly the recorded content
127+
/// `expected_hash`, in bytes `fixBin` would change (a CRLF shebang npm
128+
/// never leaves in a bin), and is a bin of the npm package `package_key`:
129+
/// the state a release before #1756 left by writing the recorded bytes
130+
/// over npm's form, which `env` can't run (#1791).
131+
pub(crate) async fn needs_fix(
132+
package_key: &str,
133+
pkg_path: &Path,
134+
normalized: &str,
135+
expected_hash: &str,
136+
) -> bool {
127137
if !is_npm(package_key) {
128138
return false;
129139
}
@@ -139,36 +149,51 @@ pub(crate) async fn needs_fix(package_key: &str, pkg_path: &Path, normalized: &s
139149
}
140150
crate::utils::fs::read_regular_to_bytes(&path)
141151
.await
142-
.is_ok_and(|bytes| fix_bin(&bytes).is_some())
152+
.is_ok_and(|bytes| {
153+
fix_bin(&bytes).is_some() && compute_git_sha256_from_bytes(&bytes) == expected_hash
154+
})
143155
}
144156

145157
/// Whether the `package.json` bytes `manifest` list `normalized` as a
146-
/// bin: a `bin` path (string or object values) or, with no `bin`, a file
147-
/// under `directories.bin`, as npm's `normalize` reads them. Paths are
148-
/// resolved as npm does (`./` and `..` folded, never above the package).
158+
/// bin, as npm's `normalize-package-bin` and `directories.bin` step read
159+
/// them: a `bin` path (a string, when the package has a `name`, an
160+
/// object's values, or an array's entries) or, when `bin` is missing or
161+
/// empty, a non-hidden file under `directories.bin`. Paths are resolved
162+
/// as npm does (`./` and `..` folded, never above the package).
149163
fn lists_bin(manifest: &[u8], normalized: &str) -> bool {
150-
let Ok(json) = serde_json::from_slice::<serde_json::Value>(manifest) else {
164+
use serde_json::Value;
165+
let Ok(json) = serde_json::from_slice::<Value>(manifest) else {
151166
return false;
152167
};
153168
let target = clean_path(normalized);
154169
if target.is_empty() {
155170
return false;
156171
}
172+
let lists = |bins: &mut dyn Iterator<Item = &Value>| {
173+
bins.filter_map(Value::as_str)
174+
.any(|bin| clean_path(bin) == target)
175+
};
157176
match json.get("bin") {
158-
Some(serde_json::Value::String(bin)) => clean_path(bin) == target,
159-
Some(serde_json::Value::Object(bins)) => bins
160-
.values()
161-
.filter_map(serde_json::Value::as_str)
162-
.any(|bin| clean_path(bin) == target),
163-
Some(_) => false,
164-
None => json
177+
Some(Value::String(bin)) if !bin.is_empty() => {
178+
json.get("name")
179+
.and_then(Value::as_str)
180+
.is_some_and(|n| !n.is_empty())
181+
&& clean_path(bin) == target
182+
}
183+
Some(Value::Object(bins)) => lists(&mut bins.values()),
184+
Some(Value::Array(bins)) => lists(&mut bins.iter()),
185+
Some(Value::String(_) | Value::Null | Value::Bool(false)) | None => json
165186
.get("directories")
166187
.and_then(|d| d.get("bin"))
167188
.and_then(serde_json::Value::as_str)
168189
.map(clean_path)
169190
.is_some_and(|dir| {
170-
!dir.is_empty() && target.len() > dir.len() && target.starts_with(&dir[..])
191+
!dir.is_empty()
192+
&& target.len() > dir.len()
193+
&& target.starts_with(&dir[..])
194+
&& !target[dir.len()..].iter().any(|seg| seg.starts_with('.'))
171195
}),
196+
Some(_) => false,
172197
}
173198
}
174199

@@ -344,8 +369,8 @@ mod tests {
344369
#[test]
345370
fn lists_bin_reads_every_bin_shape() {
346371
let listed = |manifest: &str, file: &str| lists_bin(manifest.as_bytes(), file);
347-
assert!(listed(r#"{"bin":"cli.js"}"#, "cli.js"));
348-
assert!(listed(r#"{"bin":"./bin/../cli.js"}"#, "cli.js"));
372+
assert!(listed(r#"{"name":"x","bin":"cli.js"}"#, "cli.js"));
373+
assert!(listed(r#"{"name":"x","bin":"./bin/../cli.js"}"#, "cli.js"));
349374
assert!(listed(
350375
r#"{"bin":{"a":"x.js","b":"./bin/cli.js"}}"#,
351376
"bin/cli.js"
@@ -360,6 +385,19 @@ mod tests {
360385
"bin/cli.js"
361386
));
362387
assert!(!listed(r#"{"bin":{"a":"x.js"}}"#, "cli.js"));
388+
assert!(listed(r#"{"bin":["x.js","./cli.js"]}"#, "cli.js"));
389+
// npm drops a string `bin` of a package with no name.
390+
assert!(!listed(r#"{"bin":"cli.js"}"#, "cli.js"));
391+
// A missing or empty `bin` falls back to `directories.bin`, whose
392+
// glob skips hidden files.
393+
for bin in ["\"\"", "null"] {
394+
let manifest = format!(r#"{{"bin":{bin},"directories":{{"bin":"bin"}}}}"#);
395+
assert!(listed(&manifest, "bin/cli.js"), "{manifest}");
396+
}
397+
assert!(!listed(
398+
r#"{"directories":{"bin":"bin"}}"#,
399+
"bin/.hidden/cli.js"
400+
));
363401
assert!(!listed(r#"{"name":"x"}"#, "cli.js"));
364402
assert!(!listed(r#"{"directories":{"bin":"."}}"#, "cli.js"));
365403
assert!(!listed("not json", "cli.js"));

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

Lines changed: 92 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -573,6 +573,38 @@ async fn remove_dir_if_empty(dir: &Path) -> bool {
573573
/// The single-copy rollback engine behind [`rollback_package_patch`]:
574574
/// verifies and rolls back the package at exactly the one `pkg_path` it is
575575
/// given.
576+
/// A bin of an npm package holding the raw original bytes with their CRLF
577+
/// shebang, which a rollback before #1756 wrote over npm's form, can't be
578+
/// run by `env`: `Ready`, so the restore writes npm's form of the
579+
/// original (#1791). Only with the before blob at hand: a finished
580+
/// rollback never fails for want of one.
581+
async fn mark_crlf_bin_rewrite(
582+
package_key: &str,
583+
pkg_path: &Path,
584+
file_name: &str,
585+
file_info: &PatchFileInfo,
586+
blobs_path: &Path,
587+
verify: &mut VerifyRollbackResult,
588+
) {
589+
if verify.status == VerifyRollbackStatus::AlreadyOriginal
590+
&& !file_info.before_hash.is_empty()
591+
&& verify.current_hash.as_deref() == Some(file_info.before_hash.as_str())
592+
&& crate::patch::apply::is_valid_blob_hash(&file_info.before_hash)
593+
&& tokio::fs::symlink_metadata(blobs_path.join(&file_info.before_hash))
594+
.await
595+
.is_ok_and(|m| m.is_file())
596+
&& crate::patch::npm_bin::needs_fix(
597+
package_key,
598+
pkg_path,
599+
normalize_file_path(file_name),
600+
&file_info.before_hash,
601+
)
602+
.await
603+
{
604+
verify.status = VerifyRollbackStatus::Ready;
605+
}
606+
}
607+
576608
async fn rollback_package_patch_at(
577609
package_key: &str,
578610
pkg_path: &Path,
@@ -592,7 +624,17 @@ async fn rollback_package_patch_at(
592624

593625
// First, verify all files
594626
for (file_name, file_info) in files_in_order(files) {
595-
let verify_result = verify_file_rollback(pkg_path, file_name, file_info, blobs_path).await;
627+
let mut verify_result =
628+
verify_file_rollback(pkg_path, file_name, file_info, blobs_path).await;
629+
mark_crlf_bin_rewrite(
630+
package_key,
631+
pkg_path,
632+
file_name,
633+
file_info,
634+
blobs_path,
635+
&mut verify_result,
636+
)
637+
.await;
596638

597639
// If any file has issues (not ready and not already original), we can't proceed
598640
if verify_result.status != VerifyRollbackStatus::Ready
@@ -3234,6 +3276,55 @@ mod tests {
32343276
);
32353277
}
32363278

3279+
/// Regression (#1791): a bin an earlier rollback left with the raw
3280+
/// CRLF original is rewritten in npm's runnable form when the before
3281+
/// blob is there, and left alone, as already original, when it isn't.
3282+
#[tokio::test]
3283+
async fn rollback_rewrites_raw_crlf_original_bin() {
3284+
let before: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(1);\r\n";
3285+
let after: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(2);\r\n";
3286+
let before_hash = compute_git_sha256_from_bytes(before);
3287+
let files = HashMap::from([(
3288+
"package/cli.js".to_string(),
3289+
PatchFileInfo {
3290+
before_hash: before_hash.clone(),
3291+
after_hash: compute_git_sha256_from_bytes(after),
3292+
},
3293+
)]);
3294+
for with_blob in [true, false] {
3295+
let pkg = tempfile::tempdir().unwrap();
3296+
let blobs = tempfile::tempdir().unwrap();
3297+
if with_blob {
3298+
tokio::fs::write(blobs.path().join(&before_hash), before)
3299+
.await
3300+
.unwrap();
3301+
}
3302+
tokio::fs::write(
3303+
pkg.path().join("package.json"),
3304+
br#"{"name":"x","version":"1.0.0","bin":"cli.js"}"#,
3305+
)
3306+
.await
3307+
.unwrap();
3308+
tokio::fs::write(pkg.path().join("cli.js"), before)
3309+
.await
3310+
.unwrap();
3311+
let res =
3312+
rollback_package_patch("pkg:npm/x@1.0.0", pkg.path(), &files, blobs.path(), false)
3313+
.await;
3314+
assert!(res.success, "{with_blob}: {res:?}");
3315+
let expected: &[u8] = if with_blob {
3316+
b"#!/usr/bin/env node\nconsole.log(1);\r\n"
3317+
} else {
3318+
before
3319+
};
3320+
assert_eq!(
3321+
tokio::fs::read(pkg.path().join("cli.js")).await.unwrap(),
3322+
expected,
3323+
"{with_blob}"
3324+
);
3325+
}
3326+
}
3327+
32373328
/// Regression (#1756): a bin the patch added, installed by npm in its
32383329
/// LF-shebang form, rolls back (is deleted) like the exact bytes.
32393330
#[tokio::test]

0 commit comments

Comments
 (0)