Skip to content

Commit 5c52a32

Browse files
committed
Distrust upgrades a project config can redirect
Two more ways an edited project could make the revert delete a vendored copy while something else gets installed: - npm: a package.json override can swap a registry edge for a git, URL or file: spec, which npm ci installs instead of the lock's resolved tarball. While any override names the vendored package, a version change now stays drift. - Bun: an empty registry field means the default registry, which a committed bunfig.toml or .npmrc can rebind. When either file names a registry, an upgrade from the default registry now stays drift. Both fall back to the pre-#1155 behavior, which keeps the vendored copy, whenever the project's own config could change the install source. Assisted-by: Claude Code:claude-opus-5-5
1 parent f379b2d commit 5c52a32

3 files changed

Lines changed: 159 additions & 6 deletions

File tree

‎crates/socket-patch-core/src/vendor/bun_lock.rs‎

Lines changed: 77 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -928,10 +928,22 @@ pub(crate) async fn revert_bun_opts(
928928
}
929929
}
930930

931+
// An empty registry field means "the default registry", which a
932+
// committed bunfig.toml or .npmrc can rebind; with either naming a
933+
// registry, an empty field proves nothing about where an upgrade
934+
// installs from (#1155).
935+
let default_registry_pinned = !project_configures_a_registry(project_root).await;
931936
let mut dirty = false;
932937
if let Some(lines) = lines.as_mut() {
933938
for rec in entry.wiring.iter().rev().filter(|r| r.file == BUN_LOCK) {
934-
revert_one_record(lines, rec, &entry.uuid, &mut dirty, &mut outcome.warnings);
939+
revert_one_record(
940+
lines,
941+
rec,
942+
&entry.uuid,
943+
default_registry_pinned,
944+
&mut dirty,
945+
&mut outcome.warnings,
946+
);
935947
}
936948
if dirty {
937949
if let Err(e) = atomic_write_bytes_preserving_mode(
@@ -986,6 +998,7 @@ fn revert_one_record(
986998
lines: &mut [String],
987999
rec: &WiringRecord,
9881000
entry_uuid: &str,
1001+
default_registry_pinned: bool,
9891002
dirty: &mut bool,
9901003
warnings: &mut Vec<VendorWarning>,
9911004
) {
@@ -1043,7 +1056,7 @@ fn revert_one_record(
10431056
// exactly as after `bun remove`. Nothing to restore; the caller
10441057
// keeps the artifact only while the lock still resolves through
10451058
// it. A same-version re-resolution stays drift.
1046-
if let Some(live_version) = version_moved_off(rec, &parsed) {
1059+
if let Some(live_version) = version_moved_off(rec, &parsed, default_registry_pinned) {
10471060
warnings.push(VendorWarning::new(
10481061
super::LOCK_ENTRY_REMOVED_CODE,
10491062
format!(
@@ -1104,16 +1117,36 @@ fn registry_name_version(entry: &BunEntry) -> Option<(String, String)> {
11041117
/// keeps the caller's drift verdict: an `original: None` record, a URL or
11051118
/// `file:` spec, another package or another registry is not a plain upgrade
11061119
/// and must keep the artifact and leave `vendor --check` red.
1107-
fn version_moved_off(rec: &WiringRecord, live: &BunEntry) -> Option<String> {
1120+
fn version_moved_off(
1121+
rec: &WiringRecord,
1122+
live: &BunEntry,
1123+
default_registry_pinned: bool,
1124+
) -> Option<String> {
11081125
let (live_name, live_version) = registry_name_version(live)?;
11091126
let original = parse_entry_line(rec.original.as_ref().and_then(Value::as_str)?).ok()?;
11101127
let (original_name, original_version) = registry_name_version(&original)?;
1111-
let same_registry = matches!((live.elems.get(1), original.elems.get(1)),
1112-
(Some(l), Some(o)) if decode_json_string(l).is_some() && decode_json_string(l) == decode_json_string(o));
1128+
let live_registry = decode_json_string(live.elems.get(1)?)?;
1129+
let same_registry = Some(&live_registry) == decode_json_string(original.elems.get(1)?).as_ref()
1130+
&& (!live_registry.is_empty() || default_registry_pinned);
11131131
(same_registry && live_name == original_name && live_version != original_version)
11141132
.then_some(live_version)
11151133
}
11161134

1135+
/// True when the project's own `bunfig.toml` or `.npmrc` mentions a
1136+
/// registry at all (or can't be read), so Bun's default registry may not
1137+
/// be npmjs. Deliberately coarse: any doubt keeps the drift verdict.
1138+
async fn project_configures_a_registry(project_root: &Path) -> bool {
1139+
for name in ["bunfig.toml", ".npmrc"] {
1140+
match read_regular_to_string(&project_root.join(name)).await {
1141+
Ok(text) if text.to_ascii_lowercase().contains("registry") => return true,
1142+
Ok(_) => {}
1143+
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
1144+
Err(_) => return true,
1145+
}
1146+
}
1147+
false
1148+
}
1149+
11171150
// ───────────────────────── vendor-specific classification ─────────────────
11181151
// The conservative line grammar (`BunEntry`, `parse_*`, `scan_*`, …) lives in
11191152
// `crate::vendor::bun_lock_text`; this module keeps only the vendor tuple
@@ -3256,6 +3289,45 @@ mod tests {
32563289
}
32573290
}
32583291

3292+
/// #1155 provenance guard: an empty registry field means "the default
3293+
/// registry", and a committed bunfig.toml or .npmrc can rebind that.
3294+
/// With either naming a registry, a version change from `""` is not
3295+
/// proven to come from the pre-vendor registry, so it stays drift.
3296+
#[tokio::test]
3297+
async fn revert_keeps_default_registry_upgrade_as_drift_when_the_registry_is_configured() {
3298+
for (file, text) in [
3299+
(
3300+
"bunfig.toml",
3301+
"[install]\nregistry = \"https://evil.example.com/\"\n",
3302+
),
3303+
(".npmrc", "registry=https://evil.example.com/\n"),
3304+
] {
3305+
let fx = fixture_with(BN3_BEFORE_LOCK, "node_modules/left-pad").await;
3306+
let (_, entry, _) = expect_done(fx.vendor(false).await);
3307+
let entry = entry.unwrap();
3308+
let upgraded_line =
3309+
" \"left-pad\": [\"left-pad@1.3.1\", \"\", {}, \"sha512-other==\"],";
3310+
let live = fx.read_lock().await;
3311+
let new_line = entry.wiring[0]
3312+
.new
3313+
.as_ref()
3314+
.and_then(Value::as_str)
3315+
.unwrap();
3316+
tokio::fs::write(
3317+
fx.root().join(BUN_LOCK),
3318+
live.replace(new_line, upgraded_line),
3319+
)
3320+
.await
3321+
.unwrap();
3322+
tokio::fs::write(fx.root().join(file), text).await.unwrap();
3323+
3324+
let outcome = revert_bun(&entry, fx.root(), false).await;
3325+
assert!(outcome.success, "{file}: {:?}", outcome.error);
3326+
assert!(outcome.drift_skipped(), "{file}: {:?}", outcome.warnings);
3327+
assert!(outcome.kept_artifact, "{file}: {:?}", outcome.warnings);
3328+
}
3329+
}
3330+
32593331
#[tokio::test]
32603332
async fn revert_leaves_drifted_entries_alone_with_warning() {
32613333
let fx = fixture_with(BN3_BEFORE_LOCK, "node_modules/left-pad").await;

‎crates/socket-patch-core/src/vendor/npm_lock.rs‎

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -777,7 +777,26 @@ pub async fn revert_npm_opts(
777777
}
778778
};
779779

780-
let non_registry = npm_non_registry_entries(&lock, &overrides);
780+
let mut non_registry = npm_non_registry_entries(&lock, &overrides);
781+
// An override can also swap a registry edge for a git / URL /
782+
// `file:` spec, which that set does not report. Any override that
783+
// names the vendored package keeps every record off the upgrade
784+
// path; the revert then drift-keeps as before #1155.
785+
let overridden = super::npm_common::parse_npm_purl(&entry.base_purl)
786+
.is_none_or(|(name, _)| overrides.mentions(&name));
787+
if overridden {
788+
for rec in entry.wiring.iter().filter(|r| r.file == lock_name) {
789+
if let Some(key) = rec.key.as_deref() {
790+
let key = match rec.kind.as_str() {
791+
KIND_LOCK_LEGACY_ENTRY => legacy_pointer_packages_key(key),
792+
_ => Some(key.to_string()),
793+
};
794+
if let Some(key) = key {
795+
non_registry.insert(key, "an override names the package".to_string());
796+
}
797+
}
798+
}
799+
}
781800
let mut changed = false;
782801
// Reverse application order, like every backend's revert.
783802
for rec in entry.wiring.iter().rev().filter(|r| r.file == lock_name) {
@@ -3620,6 +3639,51 @@ mod tests {
36203639
}
36213640
}
36223641

3642+
/// #1155 provenance guard: an override can swap a registry edge for a
3643+
/// git / URL / `file:` spec that npm ci installs instead of the lock's
3644+
/// `resolved`. While any override names the vendored package, a version
3645+
/// change is not trusted as a registry upgrade and stays drift.
3646+
#[tokio::test]
3647+
async fn revert_keeps_version_change_as_drift_while_an_override_names_the_package() {
3648+
for overrides in [
3649+
json!({ "left-pad": "file:../left-pad" }),
3650+
json!({ "foo": { "left-pad": "github:evil/left-pad" } }),
3651+
json!({ "left-pad@1.3.0": "https://example.com/left-pad.tgz" }),
3652+
] {
3653+
let fx = fixture().await;
3654+
let (_, entry, _) = expect_done(fx.vendor(false).await);
3655+
let entry = entry.unwrap();
3656+
let manifest = json!({ "name": "fixture", "version": "1.0.0", "overrides": overrides });
3657+
tokio::fs::write(
3658+
fx.root().join("package.json"),
3659+
serialize_json(&manifest, " ").unwrap(),
3660+
)
3661+
.await
3662+
.unwrap();
3663+
3664+
let upgraded = json!({
3665+
"version": "1.3.1",
3666+
"resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz",
3667+
"integrity": "sha512-upgraded=="
3668+
});
3669+
let mut live = fx.read_lock().await;
3670+
live["packages"]["node_modules/left-pad"] = upgraded.clone();
3671+
live["packages"]["node_modules/foo/node_modules/left-pad"] = upgraded;
3672+
tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap())
3673+
.await
3674+
.unwrap();
3675+
3676+
let outcome = revert_npm(&entry, fx.root(), false).await;
3677+
assert!(outcome.success, "{overrides}: {:?}", outcome.error);
3678+
assert!(
3679+
outcome.drift_skipped(),
3680+
"{overrides}: {:?}",
3681+
outcome.warnings
3682+
);
3683+
assert!(outcome.kept_artifact, "{overrides}: {:?}", outcome.warnings);
3684+
}
3685+
}
3686+
36233687
#[test]
36243688
fn legacy_pointer_maps_to_its_packages_key() {
36253689
assert_eq!(

‎crates/socket-patch-core/src/vendor/npm_origin.rs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,23 @@ impl NpmOverrides {
8989
}
9090
}
9191

92+
/// True when any override rule, at any nesting depth, names the package
93+
/// `name` (bare or with a selector). Coarser than [`Self::replacement`]
94+
/// on purpose: a caller that must not trust an edge's registry
95+
/// resolution while an override might swap it uses this.
96+
pub(crate) fn mentions(&self, name: &str) -> bool {
97+
fn walk(rules: &Map<String, Value>, name: &str) -> bool {
98+
rules.iter().any(|(key, value)| {
99+
let at = key
100+
.char_indices()
101+
.find_map(|(i, c)| (c == '@' && i > 0).then_some(i))
102+
.unwrap_or(key.len());
103+
&key[..at] == name || value.as_object().is_some_and(|nested| walk(nested, name))
104+
})
105+
}
106+
walk(&self.rules, name)
107+
}
108+
92109
/// The spec an override makes npm install for the edge `dep_name@spec`
93110
/// whose dependent sits at the lock key `from`, or `None` when no
94111
/// override clearly applies.

0 commit comments

Comments
 (0)