diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index 340bddf3..50556a08 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -247,6 +247,12 @@ pub(super) async fn run_redirect( // re-lock (and delete) the user's lockfile as a side effect of a no-op run. let mut migration_warnings: Vec = Vec::new(); let mut migration_edits: Vec = Vec::new(); + // The pre-migration bun.lockb bytes, held so the migration can be undone + // when the subsequent rewrite lands NOTHING in the migrated bun.lock: an + // npm override whose version doesn't match the lock (or whose entry is + // refused) must not permanently convert the user's lockfile format as a + // side effect of a zero-redirect run. + let mut lockb_backup: Option> = None; let has_lockb = args.common.cwd.join("bun.lockb").exists(); let has_bun_lock = args.common.cwd.join("bun.lock").exists(); let has_npm_override = overrides.iter().any(|o| o.ecosystem == "npm"); @@ -259,6 +265,9 @@ pub(super) async fn run_redirect( re-run without --dry-run to apply", })); } else { + // Read the binary lock BEFORE bun deletes it, so a zero-rewrite + // run can restore it below. + let lockb_bytes = std::fs::read(args.common.cwd.join("bun.lockb")).ok(); // `.output()` (not `.status()`): bun's install chatter must not // interleave with the machine `--json` envelope on stdout. let output = std::process::Command::new("bun") @@ -273,6 +282,7 @@ pub(super) async fn run_redirect( let migrated = matches!(output, Ok(o) if o.status.success()) && args.common.cwd.join("bun.lock").exists(); if migrated { + lockb_backup = lockb_bytes; // bun deleted bun.lockb itself. Record the removal so `--revert` // knows the file was replaced (binary — git history is the // restore path, so no `original` bytes are captured). @@ -342,6 +352,37 @@ pub(super) async fn run_redirect( let rewrite = rewrite_registry_redirect(&files, &overrides); let rewritten: Vec = rewrite.files.keys().cloned().collect(); + // The lockb→text migration is only KEPT when the rewrite actually landed + // in the migrated bun.lock. Otherwise nothing was redirected there and the + // migration was pure side effect: restore the saved bun.lockb bytes, + // remove the generated text lock, and drop the ledger removal record so + // the no-op run leaves the lockfile format untouched. The rewriter's own + // warning (entry-not-found / unsupported) explains WHY nothing landed. + if !migration_edits.is_empty() && !rewrite.files.contains_key("bun.lock") { + let restored = lockb_backup + .as_deref() + .is_some_and(|bytes| std::fs::write(args.common.cwd.join("bun.lockb"), bytes).is_ok()); + if restored { + let _ = std::fs::remove_file(args.common.cwd.join("bun.lock")); + migration_edits.clear(); + migration_warnings.push(serde_json::json!({ + "code": "redirect_bun_lockb_migration_reverted", + "detail": "bun.lockb was migrated to a text bun.lock but no redirect landed \ + in it; the original bun.lockb was restored", + })); + } else { + // Restore failed (unreadable pre-migration or unwritable now): + // keep the migration record and say loudly that the format was + // converted by a run that redirected nothing. + migration_warnings.push(serde_json::json!({ + "code": "redirect_bun_lockb_migrated_without_redirect", + "detail": "bun.lockb was migrated to a text bun.lock but no redirect landed \ + in it, and the original bun.lockb could not be restored; git \ + history is the restore path", + })); + } + } + // Editing a Rush lock outside `rush update` desyncs the // pnpmShrinkwrapHash recorded in repo-state.json. When // preventManualShrinkwrapChanges is enabled, `rush install` then @@ -529,7 +570,9 @@ pub(super) async fn run_redirect( // deleting the other mode's ledger; reconciliation is deferred (see PR Scope). // Read after the ledger write above so a non-dry-run reflects this run. let mut takeover_warnings: Vec = Vec::new(); - let superseded = super::classify_overlap_takeover(&args.common.cwd).await.redirect; + let superseded = super::classify_overlap_takeover(&args.common.cwd) + .await + .redirect; if !superseded.is_empty() { takeover_warnings.push(serde_json::json!({ "code": super::REDIRECT_SUPERSEDES_VENDORED, diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index d302aa48..bceed838 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -910,6 +910,93 @@ async fn scan_redirect_migrates_bun_lockb_then_redirects() { ); } +/// The lockb migration must be UNDONE when the rewrite lands nothing in the +/// migrated bun.lock: here the shim's re-locked text lock holds a DIFFERENT +/// version of the dep than the granted override, so nothing is redirectable — +/// the run must restore the original bun.lockb bytes, remove the generated +/// bun.lock, and write no ledger, instead of permanently converting the +/// user's lockfile format as a side effect of a zero-redirect scan. +#[cfg(unix)] +#[tokio::test] +#[serial] +async fn zero_redirect_restores_bun_lockb_after_migration() { + let server = MockServer::start().await; + mock_discovery(&server).await; + mock_reference(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write( + tmp.path().join("package.json"), + format!( + r#"{{ "name": "consumer", "version": "0.0.0", "dependencies": {{ "{NAME}": "^{VERSION}" }} }}"# + ), + ) + .unwrap(); + let pkg = tmp.path().join("node_modules").join(NAME); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + format!(r#"{{ "name": "{NAME}", "version": "{VERSION}" }}"#), + ) + .unwrap(); + let lockb_bytes: &[u8] = b"BUN-BINARY-PLACEHOLDER"; + std::fs::write(tmp.path().join("bun.lockb"), lockb_bytes).unwrap(); + + // The shim's text lock resolves the dep to a version the override does + // NOT target, so the bun rewriter finds no rewritable tuple. + let bin_dir = tmp.path().join("fakebin"); + std::fs::create_dir_all(&bin_dir).unwrap(); + let shim = bin_dir.join("bun"); + let bun_lock_body = format!( + "{{\n \"lockfileVersion\": 1,\n \"packages\": {{\n \ + \"{NAME}\": [\"{NAME}@2.0.0\", \"\", {{}}, \"sha512-UPSTREAMupstream==\"],\n \ + }}\n}}\n" + ); + std::fs::write( + &shim, + format!( + "#!/bin/sh\n\ + cat > bun.lock <<'LOCK'\n{bun_lock_body}LOCK\n\ + rm -f bun.lockb\n\ + exit 0\n" + ), + ) + .unwrap(); + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&shim, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + let orig_path = std::env::var("PATH").unwrap_or_default(); + // SAFETY: single-threaded #[serial] test; PATH restored below. + unsafe { + std::env::set_var("PATH", format!("{}:{orig_path}", bin_dir.display())); + } + + let code = run(redirect_args(tmp.path(), server.uri())).await; + + unsafe { + std::env::set_var("PATH", orig_path); + } + assert_eq!(code, 0, "a zero-redirect run is not an error"); + + let restored = std::fs::read(tmp.path().join("bun.lockb")) + .expect("bun.lockb must be restored after a zero-redirect migration"); + assert_eq!( + restored, lockb_bytes, + "restored bun.lockb must carry the original bytes" + ); + assert!( + !tmp.path().join("bun.lock").exists(), + "the generated text lock must be removed with the migration undone" + ); + assert!( + !tmp.path() + .join(".socket/vendor/redirect-state.json") + .exists(), + "no ledger may record a migration that was undone" + ); +} + /// A `socket-patch` Command with the ambient `SOCKET_*` env surface scrubbed, /// for the subprocess tests below: the binary binds a wide clap env surface /// (SOCKET_DRY_RUN, SOCKET_OFFLINE, SOCKET_ECOSYSTEMS, SOCKET_PROXY_URL, ...), diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 0ff40eb5..f6467b41 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -1134,6 +1134,7 @@ fn rewrite_bun_lock( }; let target_spec = format!("{fname}@{}", dep.version); let url_spec = format!("{fname}@{}", dep.artifact_url); + let mut matched_any = false; for entry in &entries { let Some(spec) = entry.elems.first().and_then(|e| decode_json_string(e)) else { continue; @@ -1150,15 +1151,31 @@ fn rewrite_bun_lock( } else if entry.elems.len() == 3 && spec == url_spec { // Already one of our URL 3-tuples for this exact URL. Idempotent // if the integrity already matches; otherwise refresh it. + matched_any = true; if entry.elems[2] == format!("\"{sha512}\"") { continue; } deps_verbatim = entry.elems[1].clone(); + } else if entry.elems.len() == 3 + && entry.elems[1].starts_with('{') + && is_prior_hosted_bun_spec(&spec, &fname, &dep.artifact_url) + { + // A URL 3-tuple written by an EARLIER redirect whose artifact + // URL has since changed (a patch republish rotates the uuid + // path segment; grant-token rotation changes the token — the + // registry `name@version` spec was destroyed by that first + // rewrite, so exact-URL matching alone would strand the stale + // pin forever). Re-pin to the current URL. Ownership is + // claimed narrowly — same origin and same `- + // .tgz` leaf as the CURRENT artifact URL — so user URL deps + // and other-version entries never match (fail-closed). + deps_verbatim = entry.elems[1].clone(); } else { // Same-name-but-unowned entry (user file:/URL dep, other // version) — never touched. continue; } + matched_any = true; let original = lines[entry.line_idx].clone(); let rebuilt = format!( "{indent}{key}: [{url}, {deps}, {integrity}]{comma}", @@ -1183,12 +1200,54 @@ fn rewrite_bun_lock( }); changed = true; } + if !matched_any { + // Mirrors the pnpm/berry/uv rewriters: a granted dep that matched + // no rewritable tuple (lock re-resolved to another version, entry + // occupied by an unowned URL/file: spec) must be diagnosable, not + // a silent drop from the `redirected` count. + result.warnings.push(RewriteWarning { + code: "redirect_bun_entry_not_found".into(), + detail: format!("no rewritable bun.lock entry for {fname}@{}", dep.version), + }); + } } if changed { result.files.insert("bun.lock".into(), lines.join("\n")); } } +/// True when a bun.lock 3-tuple spec (`name@`) was written by an earlier +/// hosted redirect of this same dependency: the spec's URL shares both the +/// origin (`scheme://host[:port]`) and the trailing `-.tgz` +/// path leaf with the CURRENT artifact URL. Both halves come from the live +/// override — nothing about the patch server's URL layout is assumed — and +/// anything that fails to parse fails the match (closed): user URL deps live +/// on other origins, and another version's artifact has a different leaf. +fn is_prior_hosted_bun_spec(spec: &str, fname: &str, current_url: &str) -> bool { + let Some(old_url) = spec + .strip_prefix(fname) + .and_then(|rest| rest.strip_prefix('@')) + else { + return false; + }; + fn origin_and_leaf(url: &str) -> Option<(&str, &str)> { + if !url.starts_with("https://") && !url.starts_with("http://") { + return None; + } + let scheme_end = url.find("://").unwrap() + 3; + let path_start = url[scheme_end..].find('/')? + scheme_end; + let leaf = url[path_start..] + .rsplit('/') + .next() + .filter(|l| !l.is_empty())?; + Some((&url[..path_start], leaf)) + } + match (origin_and_leaf(old_url), origin_and_leaf(current_url)) { + (Some(old), Some(new)) => old == new, + _ => false, + } +} + // ── uv.lock ────────────────────────────────────────────────────────────────── fn rewrite_uv_lock( files: &BTreeMap, @@ -3354,6 +3413,170 @@ mod tests { rewrite_bun_lock(&files, &[no_sha], &mut r); assert!(r.files.is_empty()); assert_eq!(r.warnings[0].code, "redirect_bun_missing_sha512"); + assert_eq!( + r.warnings.len(), + 1, + "the sha512 refusal must not double-warn entry-not-found" + ); + } + + /// A bun.lock already redirected by an earlier run holds a URL 3-tuple — + /// the registry `name@version` spec is gone — so when the artifact URL + /// changes (patch republish rotates the uuid segment, token rotation + /// changes the token) the entry MUST still be re-pinned to the new URL; + /// exact-URL matching alone stranded the stale pin forever. Ownership is + /// origin + `-.tgz` leaf, so user URL deps and + /// other-version artifacts stay untouched. + #[test] + fn bun_lock_re_redirects_stale_hosted_url() { + let old_sha = format!("sha512-{}==", "O".repeat(86)); + let new_sha = format!("sha512-{}==", "N".repeat(86)); + let old_url = "https://patch.socket.dev/patch/npm/oldtoken-1111/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/left-pad-1.3.0.tgz"; + let new_url = "https://patch.socket.dev/patch/npm/newtoken-2222/bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb/left-pad-1.3.0.tgz"; + let ovr = npm_override("left-pad", "1.3.0", new_url, &new_sha); + + let mut files = BTreeMap::new(); + files.insert( + "bun.lock".to_string(), + bun_lock_file( + &format!("\"left-pad\": [\"left-pad@{old_url}\", {{}}, \"{old_sha}\"],"), + 1, + ), + ); + let mut r = RewriteResult::default(); + rewrite_bun_lock(&files, std::slice::from_ref(&ovr), &mut r); + let out = r + .files + .get("bun.lock") + .expect("stale URL must be re-pinned"); + assert!( + out.contains(&format!("\"left-pad@{new_url}\"")) && !out.contains(old_url), + "entry must carry the NEW artifact URL: {out}" + ); + assert!(out.contains(&new_sha) && !out.contains(&old_sha)); + assert!( + r.warnings.is_empty(), + "re-pin is not a warning case: {:?}", + r.warnings + ); + assert_eq!(r.edits.len(), 1); + + // Idempotent: a second run over the re-pinned lock is a no-op. + let mut files = BTreeMap::new(); + files.insert("bun.lock".to_string(), out.clone()); + let mut r = RewriteResult::default(); + rewrite_bun_lock(&files, std::slice::from_ref(&ovr), &mut r); + assert!(r.files.is_empty(), "same-URL rerun must stay a no-op"); + assert!(r.warnings.is_empty()); + + // A user's own URL dep (different origin, same leaf) is never claimed. + let mut files = BTreeMap::new(); + files.insert( + "bun.lock".to_string(), + bun_lock_file( + &format!( + "\"left-pad\": [\"left-pad@https://example.com/mirror/left-pad-1.3.0.tgz\", {{}}, \"{old_sha}\"]," + ), + 1, + ), + ); + let mut r = RewriteResult::default(); + rewrite_bun_lock(&files, std::slice::from_ref(&ovr), &mut r); + assert!( + r.files.is_empty(), + "foreign-origin URL dep must not be touched" + ); + assert_eq!(r.warnings[0].code, "redirect_bun_entry_not_found"); + + // Our origin but ANOTHER version's leaf is never claimed either. + let other_version_url = "https://patch.socket.dev/patch/npm/oldtoken-1111/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/left-pad-1.2.0.tgz"; + let mut files = BTreeMap::new(); + files.insert( + "bun.lock".to_string(), + bun_lock_file( + &format!("\"left-pad\": [\"left-pad@{other_version_url}\", {{}}, \"{old_sha}\"],"), + 1, + ), + ); + let mut r = RewriteResult::default(); + rewrite_bun_lock(&files, std::slice::from_ref(&ovr), &mut r); + assert!( + r.files.is_empty(), + "other-version tuple must not be touched" + ); + assert_eq!(r.warnings[0].code, "redirect_bun_entry_not_found"); + } + + /// A granted dep that matches no rewritable tuple (lock re-resolved to a + /// different version) must warn — mirroring pnpm/berry/uv — instead of + /// silently dropping out of the `redirected` count. + #[test] + fn bun_lock_entry_not_found_warns() { + let sha512 = format!("sha512-{}==", "A".repeat(86)); + let ovr = npm_override("left-pad", "1.3.0", "http://p.test/lp.tgz", &sha512); + + let mut files = BTreeMap::new(); + files.insert( + "bun.lock".to_string(), + bun_lock_file( + "\"left-pad\": [\"left-pad@1.2.0\", \"\", {}, \"sha512-OLD==\"],", + 1, + ), + ); + let mut r = RewriteResult::default(); + rewrite_bun_lock(&files, std::slice::from_ref(&ovr), &mut r); + assert!(r.files.is_empty() && r.edits.is_empty()); + assert_eq!(r.warnings[0].code, "redirect_bun_entry_not_found"); + assert!( + r.warnings[0].detail.contains("left-pad@1.3.0"), + "the warning must name the missing dep: {}", + r.warnings[0].detail + ); + + // A successful rewrite emits NO warning. + let mut files = BTreeMap::new(); + files.insert( + "bun.lock".to_string(), + bun_lock_file( + "\"left-pad\": [\"left-pad@1.3.0\", \"\", {}, \"sha512-OLD==\"],", + 1, + ), + ); + let mut r = RewriteResult::default(); + rewrite_bun_lock(&files, std::slice::from_ref(&ovr), &mut r); + assert!(r.files.contains_key("bun.lock")); + assert!(r.warnings.is_empty(), "{:?}", r.warnings); + } + + /// A packages header spelled any way other than bun's byte-exact emitted + /// shape must fail CLOSED with the unsupported warning — not parse as an + /// empty lock and silently skip the dep. + #[test] + fn bun_lock_noncanonical_packages_header_fails_closed() { + let sha512 = format!("sha512-{}==", "A".repeat(86)); + let ovr = npm_override("left-pad", "1.3.0", "http://p.test/lp.tgz", &sha512); + + for lock in [ + // Tab-indented header. + "{\n \"lockfileVersion\": 1,\n\t\"packages\": {\n \ + \"left-pad\": [\"left-pad@1.3.0\", \"\", {}, \"sha512-OLD==\"],\n\t}\n}\n", + // 4-space re-indent. + "{\n \"lockfileVersion\": 1,\n \"packages\": {\n \ + \"left-pad\": [\"left-pad@1.3.0\", \"\", {}, \"sha512-OLD==\"],\n }\n}\n", + // Space before the colon. + "{\n \"lockfileVersion\": 1,\n \"packages\" : {\n \ + \"left-pad\": [\"left-pad@1.3.0\", \"\", {}, \"sha512-OLD==\"],\n }\n}\n", + ] { + let mut files = BTreeMap::new(); + files.insert("bun.lock".to_string(), lock.to_string()); + let mut r = RewriteResult::default(); + rewrite_bun_lock(&files, std::slice::from_ref(&ovr), &mut r); + assert!(r.files.is_empty(), "must not rewrite: {lock}"); + assert_eq!( + r.warnings[0].code, "redirect_bun_lock_unsupported", + "non-canonical header must refuse, not read as empty: {lock}" + ); + } } /// A realistic uv.lock block carries BOTH an `sdist` entry and a `wheels` diff --git a/crates/socket-patch-core/src/vendor/bun_lock_text.rs b/crates/socket-patch-core/src/vendor/bun_lock_text.rs index ea076828..b2534a81 100644 --- a/crates/socket-patch-core/src/vendor/bun_lock_text.rs +++ b/crates/socket-patch-core/src/vendor/bun_lock_text.rs @@ -78,10 +78,22 @@ pub(crate) fn packages_bounds(lines: &[String]) -> Option<(usize, usize)> { /// is neither blank nor a single-line `"key": [tuple]` entry fails CLOSED. pub(crate) fn parse_packages_section(lines: &[String]) -> Result, String> { let Some((start, end)) = packages_bounds(lines) else { - // No (or unterminated) packages section: an empty lock simply has - // no entries; an unterminated one is malformed. + // Only a lock with NO `"packages"` object at all is an empty lock. + // Everything else fails CLOSED: an unterminated canonical section is + // malformed, and a header spelled ANY other way than bun's byte-exact + // emitted shape (tab/4-space re-indent, `"packages" : {`) must refuse + // rather than read as "no entries" — treating it as empty would make + // the caller silently skip a lock bun itself parses fine. return if lines.iter().any(|l| l.trim_end() == " \"packages\": {") { Err("unterminated \"packages\" section".to_string()) + } else if lines.iter().any(|l| { + l.trim_start() + .strip_prefix("\"packages\"") + .map(str::trim_start) + .and_then(|rest| rest.strip_prefix(':')) + .is_some_and(|rest| rest.trim_start().starts_with('{')) + }) { + Err("\"packages\" section header is not in bun's emitted shape".to_string()) } else { Ok(Vec::new()) }; @@ -302,4 +314,47 @@ mod tests { "trailing junk" ); } + + fn to_lines(text: &str) -> Vec { + text.split('\n').map(str::to_string).collect() + } + + /// A `"packages"` header spelled any way other than bun's byte-exact + /// emitted shape must parse as an ERROR (fail closed), never as an empty + /// lock — "empty" made the rewriters silently skip locks bun itself + /// parses fine. + #[test] + fn noncanonical_packages_header_is_an_error_not_empty() { + let entry = r#""left-pad": ["left-pad@1.3.0", "", {}, "sha512-X=="],"#; + for lock in [ + format!("{{\n \"lockfileVersion\": 1,\n\t\"packages\": {{\n {entry}\n\t}}\n}}\n"), + format!( + "{{\n \"lockfileVersion\": 1,\n \"packages\": {{\n {entry}\n }}\n}}\n" + ), + format!("{{\n \"lockfileVersion\": 1,\n \"packages\" : {{\n {entry}\n }}\n}}\n"), + ] { + assert!( + parse_packages_section(&to_lines(&lock)).is_err(), + "must fail closed, not read as empty: {lock}" + ); + } + + // Truly absent packages section: an empty lock, no error. + let empty = "{\n \"lockfileVersion\": 1,\n \"workspaces\": {\n }\n}\n"; + assert!(parse_packages_section(&to_lines(empty)).unwrap().is_empty()); + + // A dependency literally named "packages" in another section must not + // trip the fail-closed header detection (its value is a string, not + // an object opener). + let dep_named_packages = "{\n \"lockfileVersion\": 1,\n \"workspaces\": {\n \ + \"\": {\n \"dependencies\": {\n \ + \"packages\": \"^1.0.0\",\n },\n },\n }\n}\n"; + assert!(parse_packages_section(&to_lines(dep_named_packages)) + .unwrap() + .is_empty()); + + // The canonical-but-unterminated case still errors. + let unterminated = "{\n \"lockfileVersion\": 1,\n \"packages\": {\n"; + assert!(parse_packages_section(&to_lines(unterminated)).is_err()); + } }