diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler.rs b/crates/socket-patch-core/src/crawlers/npm_crawler.rs index 2d6acefd5..7a24fd60c 100644 --- a/crates/socket-patch-core/src/crawlers/npm_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/npm_crawler.rs @@ -111,7 +111,7 @@ fn skipped_members_of(root: &Path) -> Vec { let Some(patterns) = crate::utils::fs::read_regular_to_string_sync(&root.join("package.json")) .ok() - .and_then(|text| crate::hosted::governing_root::workspace_patterns(&text)) + .and_then(|text| crate::formats::package_json::Workspaces::listed_patterns(&text)) else { return Vec::new(); }; @@ -915,20 +915,17 @@ fn bun_workspace_pattern_members_sync(root: &Path) -> Option> { Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Some(Vec::new()), Err(_) => return None, }; - let text = strip_bom(&text); - let doc: serde_json::Value = serde_json::from_str(text).ok()?; - let patterns = match doc.get("workspaces") { - None | Some(serde_json::Value::Null) => Vec::new(), - Some(serde_json::Value::Array(list)) => list.clone(), - Some(serde_json::Value::Object(map)) => match map.get("packages") { - None => Vec::new(), - Some(packages) => packages.as_array()?.clone(), - }, - Some(_) => return None, + use crate::formats::package_json::Workspaces; + + // Bun refuses to install from a malformed field (#1900), so it names + // no member set. + let patterns = match Workspaces::parse(&text)? { + Workspaces::None => Vec::new(), + Workspaces::Patterns(patterns) => patterns, + Workspaces::Malformed(_) => return None, }; let mut members = Vec::new(); for pattern in &patterns { - let pattern = pattern.as_str()?; if pattern.starts_with('!') { continue; } @@ -8130,6 +8127,19 @@ mod tests { ) .unwrap(); assert_eq!(bun_workspace_pattern_members_sync(dir.path()), None); + + // #1900: Bun refuses to install from a `null` or non-string + // `workspaces`, so the patterns cannot name the member set. + for manifest in [r#"{"workspaces":null}"#, r#"{"workspaces":["a",1]}"#] { + std::fs::write(dir.path().join("package.json"), manifest).unwrap(); + assert_eq!( + bun_workspace_pattern_members_sync(dir.path()), + None, + "{manifest}" + ); + } + std::fs::write(dir.path().join("package.json"), r#"{"name":"x"}"#).unwrap(); + assert_eq!(bun_workspace_pattern_members_sync(dir.path()), Some(vec![])); } /// `.yarnrc` `--modules-folder` parsing: bare and quoted keys and diff --git a/crates/socket-patch-core/src/formats/mod.rs b/crates/socket-patch-core/src/formats/mod.rs index c738c0be2..3f8fcb874 100644 --- a/crates/socket-patch-core/src/formats/mod.rs +++ b/crates/socket-patch-core/src/formats/mod.rs @@ -34,6 +34,7 @@ pub mod governing_locks; pub(crate) mod json; pub(crate) mod maven; pub(crate) mod nuget; +pub(crate) mod package_json; pub(crate) mod pipenv; pub mod pnpm; pub mod registry; diff --git a/crates/socket-patch-core/src/formats/package_json.rs b/crates/socket-patch-core/src/formats/package_json.rs new file mode 100644 index 000000000..dd57cbe03 --- /dev/null +++ b/crates/socket-patch-core/src/formats/package_json.rs @@ -0,0 +1,169 @@ +//! The `workspaces` field of an npm-family `package.json`, read one way +//! for every caller (#1900). +//! +//! The field takes two shapes: the array form (npm, yarn, Bun) and the +//! object form whose `packages` array holds the patterns (yarn classic's +//! `nohoist` shape, Bun's catalogs shape). What the package managers do +//! with anything else was probed on npm 10.9, yarn 1.22.22 and Bun 1.4.4: +//! +//! | `workspaces` | npm | yarn 1 | Bun | +//! |---------------------------|--------|---------------|--------| +//! | `["a", 1]` | fails | fails | fails | +//! | `null` | fails | no workspaces | fails | +//! | `"a"` | fails | no workspaces | fails | +//! | `{"packages": "a"}` | fails | no workspaces | fails | +//! | `{"nohoist": []}` | fails | no workspaces | no workspaces | +//! +//! A list with a non-string item fails every install, so it is +//! [`Workspaces::Malformed`], as are the other shapes npm and Bun refuse +//! (yarn 1 installs those as if no field were set). The object form +//! without `packages` installs under the two package managers that write +//! it, so it declares no patterns. +//! +//! Readers that only widen (a governing-root refusal, an extra crawl +//! root, a warning, a patch search) take a malformed field's string items +//! through [`Workspaces::into_listed`]. A reader that must name an exact +//! member set (the Bun member walk) treats a malformed field as +//! unreadable. + +use serde_json::Value; + +/// What a `package.json` `workspaces` field declares. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum Workspaces { + /// No field, or the object form without `packages`. + None, + /// The patterns of a well-formed field, in order (possibly empty). + /// A leading `!` is kept: each caller applies negations the way its + /// package manager does. + Patterns(Vec), + /// A field the package managers refuse to install from (see the + /// module table), with the string items it does hold. + Malformed(Vec), +} + +impl Workspaces { + /// The field of a parsed `package.json`. + pub(crate) fn of(manifest: &Value) -> Self { + let list = match manifest.get("workspaces") { + None => return Workspaces::None, + Some(Value::Array(list)) => list, + Some(Value::Object(map)) => match map.get("packages") { + None => return Workspaces::None, + Some(Value::Array(list)) => list, + Some(_) => return Workspaces::Malformed(Vec::new()), + }, + Some(_) => return Workspaces::Malformed(Vec::new()), + }; + let strings: Vec = list + .iter() + .filter_map(Value::as_str) + .map(str::to_owned) + .collect(); + if strings.len() == list.len() { + Workspaces::Patterns(strings) + } else { + Workspaces::Malformed(strings) + } + } + + /// The field of a `package.json` text (a leading BOM is ignored); + /// `None` when the text is not JSON. + pub(crate) fn parse(package_json: &str) -> Option { + serde_json::from_str(super::text::strip_bom(package_json)) + .ok() + .map(|doc| Self::of(&doc)) + } + + /// The patterns a widening reader takes: every string item of a + /// declared field, well-formed or not; `None` when no field is + /// declared. A refusal, an extra crawl root or a warning that reads a + /// malformed root's patterns errs on the side the package managers' + /// failure cannot make unsafe. + pub(crate) fn into_listed(self) -> Option> { + match self { + Workspaces::None => None, + Workspaces::Patterns(list) | Workspaces::Malformed(list) => Some(list), + } + } + + /// [`Self::parse`] then [`Self::into_listed`]: the patterns a widening + /// reader takes from a `package.json` text. + pub(crate) fn listed_patterns(package_json: &str) -> Option> { + Self::parse(package_json)?.into_listed() + } + + /// Whether the field names at least one pattern ([`Self::into_listed`] + /// non-empty). + pub(crate) fn lists_any(&self) -> bool { + matches!( + self, + Workspaces::Patterns(list) | Workspaces::Malformed(list) if !list.is_empty() + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn strings(items: &[&str]) -> Vec { + items.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn classifies_every_shape() { + let cases: &[(&str, Workspaces)] = &[ + (r#"{"name":"x"}"#, Workspaces::None), + (r#"{"workspaces":{"nohoist":["**"]}}"#, Workspaces::None), + (r#"{"workspaces":[]}"#, Workspaces::Patterns(Vec::new())), + ( + r#"{"workspaces":["a/*","!a/skip","b"]}"#, + Workspaces::Patterns(strings(&["a/*", "!a/skip", "b"])), + ), + ( + r#"{"workspaces":{"packages":["a/*"],"nohoist":["**"]}}"#, + Workspaces::Patterns(strings(&["a/*"])), + ), + ( + r#"{"workspaces":["a",1]}"#, + Workspaces::Malformed(strings(&["a"])), + ), + ( + r#"{"workspaces":{"packages":[null,"b"]}}"#, + Workspaces::Malformed(strings(&["b"])), + ), + (r#"{"workspaces":null}"#, Workspaces::Malformed(Vec::new())), + (r#"{"workspaces":"a"}"#, Workspaces::Malformed(Vec::new())), + ( + r#"{"workspaces":{"packages":"a"}}"#, + Workspaces::Malformed(Vec::new()), + ), + ]; + for (text, want) in cases { + assert_eq!(Workspaces::parse(text).as_ref(), Some(want), "{text}"); + } + } + + #[test] + fn parse_ignores_a_bom_and_rejects_non_json() { + assert_eq!( + Workspaces::parse("\u{feff}{\"workspaces\":[\"a\"]}"), + Some(Workspaces::Patterns(strings(&["a"]))) + ); + assert_eq!(Workspaces::parse("not json"), None); + } + + #[test] + fn widening_readers_keep_a_malformed_fields_strings() { + assert_eq!(Workspaces::None.into_listed(), None); + assert_eq!( + Workspaces::Malformed(strings(&["a"])).into_listed(), + Some(strings(&["a"])) + ); + assert!(Workspaces::Malformed(strings(&["a"])).lists_any()); + assert!(!Workspaces::Malformed(Vec::new()).lists_any()); + assert!(!Workspaces::Patterns(Vec::new()).lists_any()); + assert!(!Workspaces::None.lists_any()); + } +} diff --git a/crates/socket-patch-core/src/hosted/governing_root.rs b/crates/socket-patch-core/src/hosted/governing_root.rs index 8d3d37075..4f7a88cae 100644 --- a/crates/socket-patch-core/src/hosted/governing_root.rs +++ b/crates/socket-patch-core/src/hosted/governing_root.rs @@ -24,6 +24,7 @@ use std::path::{Path, PathBuf}; use crate::constants::npm_family::{BUN_LOCK, BUN_LOCKB, VLT_LOCK}; use crate::formats::governing_locks::{npm_lock_files, NpmLockFamily}; +use crate::formats::package_json::Workspaces; use crate::formats::pnpm::workspace::read_package_globs; use crate::formats::text::strip_bom; use crate::formats::yarn::berry_gates::YARN_LOCK; @@ -649,7 +650,7 @@ async fn listing_workspace(base: &Path, dir: &Path) -> Option { let listed = ancestor.join("rush.json").exists() || package_json .ok() - .and_then(|text| workspace_patterns(&text)) + .and_then(|text| Workspaces::listed_patterns(&text)) .is_some_and(|patterns| lists(&patterns, &rel)) || read_regular_to_string(&ancestor.join(VLT_JSON)) .await @@ -730,7 +731,7 @@ async fn package_json_workspace_root(root: &Path) -> Option<(PathBuf, Vec<&'stat let Ok(text) = read_regular_to_string(&ancestor.join("package.json")).await else { continue; }; - let Some(patterns) = workspace_patterns(&text) else { + let Some(patterns) = Workspaces::listed_patterns(&text) else { continue; }; let Ok(rel) = member.strip_prefix(ancestor) else { @@ -1202,27 +1203,6 @@ fn vlt_workspace_patterns(vlt_json: &str) -> Option>> { }) } -/// The `workspaces` patterns of a `package.json`: the array form (npm, -/// yarn, Bun) or the object form's `packages` array (yarn classic's -/// `nohoist` shape, Bun's catalogs shape). `None` when the field is absent -/// or the manifest does not parse. -pub(crate) fn workspace_patterns(package_json: &str) -> Option> { - let text = strip_bom(package_json); - let doc: serde_json::Value = serde_json::from_str(text).ok()?; - let field = doc.get("workspaces")?; - let list = match field { - serde_json::Value::Array(list) => list, - serde_json::Value::Object(map) => map.get("packages")?.as_array()?, - _ => return None, - }; - Some( - list.iter() - .filter_map(|v| v.as_str()) - .map(str::to_string) - .collect(), - ) -} - async fn npmrc_lockfile_dir(root: &Path) -> Option { let npmrc = read_regular_to_string(&root.join(".npmrc")).await.ok()?; npmrc_top_level_value(&npmrc, "lockfile-dir") @@ -2553,11 +2533,14 @@ mod tests { vlt_workspace_patterns(&text), Some(vec![vec!["a/*".to_string()]]) ); - assert_eq!(workspace_patterns(&text), Some(vec!["a/*".to_string()])); + assert_eq!( + Workspaces::listed_patterns(&text), + Some(vec!["a/*".to_string()]) + ); } let text = format!("\u{feff}\u{feff}{json}"); assert_eq!(vlt_workspace_patterns(&text), None); - assert_eq!(workspace_patterns(&text), None); + assert_eq!(Workspaces::listed_patterns(&text), None); } /// #942: vlt's `workspaces` in `vlt.json` is a string, an array or an @@ -3493,20 +3476,6 @@ mod tests { assert!(refusal.message.contains(VLT_LOCK), "{}", refusal.message); } - #[test] - fn workspace_patterns_reads_both_field_shapes() { - assert_eq!( - workspace_patterns(r#"{"workspaces":["a/*","b"]}"#), - Some(vec!["a/*".to_string(), "b".to_string()]) - ); - assert_eq!( - workspace_patterns("\u{feff}{\"workspaces\":{\"packages\":[\"a/*\"]}}"), - Some(vec!["a/*".to_string()]) - ); - assert_eq!(workspace_patterns(r#"{"name":"x"}"#), None); - assert_eq!(workspace_patterns("not json"), None); - } - /// #417: a cargo workspace member is refused with the vendored code; the /// root and a standalone crate are not. #[tokio::test] diff --git a/crates/socket-patch-core/src/patch/redirect/yarnrc.rs b/crates/socket-patch-core/src/patch/redirect/yarnrc.rs index 5e40f0f17..2c679323b 100644 --- a/crates/socket-patch-core/src/patch/redirect/yarnrc.rs +++ b/crates/socket-patch-core/src/patch/redirect/yarnrc.rs @@ -667,7 +667,7 @@ pub fn yarn_classic_workspace_members(root: &Path) -> Vec { } let Some(patterns) = read_regular_to_string_sync(&root.join("package.json")) .ok() - .and_then(|text| crate::hosted::governing_root::workspace_patterns(&text)) + .and_then(|text| crate::formats::package_json::Workspaces::listed_patterns(&text)) .filter(|patterns| !patterns.is_empty()) else { return Vec::new(); diff --git a/crates/socket-patch-core/src/patch/user_patches.rs b/crates/socket-patch-core/src/patch/user_patches.rs index 0ee7c9d2d..3bf200a29 100644 --- a/crates/socket-patch-core/src/patch/user_patches.rs +++ b/crates/socket-patch-core/src/patch/user_patches.rs @@ -11,6 +11,7 @@ use std::sync::LazyLock; use regex::Regex; use serde_json::Value; +use crate::formats::package_json::Workspaces; use crate::formats::text::strip_bom; use crate::utils::fs::read_regular_to_string; use crate::utils::npm_range::npm_range_admits; @@ -90,17 +91,9 @@ impl UserPatchIndex { } } } - let workspaces = value["workspaces"] - .as_array() - .or_else(|| value["workspaces"]["packages"].as_array()); - if let Some(workspaces) = workspaces { - globs.extend( - workspaces - .iter() - .filter_map(Value::as_str) - .map(str::to_owned), - ); - } + // A malformed field still names its string items (#1900): + // walking them can only find more patches. + globs.extend(Workspaces::of(&value).into_listed().into_iter().flatten()); index.patch_package(&dir, &value).await; } else { index.patch_directory(&dir.join("patches"), false).await; @@ -1149,6 +1142,35 @@ mod tests { assert!(index.conflicts("pkg:npm/x@1.0.0").is_empty(), "{index:?}"); } + /// #1900: a `workspaces` list with a non-string item is read through + /// the shared package.json reader, which keeps its string patterns, + /// so the members they name are still searched for patches, and + /// unrelated npm packages are not marked as user-patched. + #[tokio::test] + async fn malformed_workspaces_still_walk_their_string_patterns() { + let tmp = tempfile::tempdir().unwrap(); + write( + tmp.path(), + "package.json", + r#"{"workspaces":["packages/*", 1]}"#, + ); + write(tmp.path(), "packages/a/package.json", "{}"); + write( + tmp.path(), + "packages/a/patches/one+1.0.0.patch", + "user diff", + ); + let index = UserPatchIndex::discover(tmp.path()).await; + assert!( + !index.conflicts("pkg:npm/one@1.0.0").is_empty(), + "{index:?}" + ); + assert!( + index.conflicts("pkg:npm/other@1.0.0").is_empty(), + "{index:?}" + ); + } + #[tokio::test] async fn dot_dot_workspace_globs_resolve_lexically() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/socket-patch-core/src/update/channel.rs b/crates/socket-patch-core/src/update/channel.rs index 7794e6526..d2ecca5d4 100644 --- a/crates/socket-patch-core/src/update/channel.rs +++ b/crates/socket-patch-core/src/update/channel.rs @@ -386,16 +386,10 @@ fn read_package_json(dir: &Path) -> Option { serde_json::from_str(crate::formats::text::strip_bom(&text)).ok() } -/// Whether a `package.json` declares workspaces: a non-empty array, or the -/// `{ "packages": [...] }` form with a non-empty `packages`. +/// Whether a `package.json` declares workspaces: its `workspaces` field +/// names at least one pattern ([`Workspaces::lists_any`](crate::formats::package_json::Workspaces::lists_any)). fn declares_workspaces(pkg: &serde_json::Value) -> bool { - let globs = match pkg.get("workspaces") { - Some(serde_json::Value::Object(obj)) => obj.get("packages"), - other => other, - }; - globs - .and_then(|g| g.as_array()) - .is_some_and(|g| !g.is_empty()) + crate::formats::package_json::Workspaces::of(pkg).lists_any() } /// Whether a `package.json` itself declares socket-patch in any dependency @@ -577,6 +571,24 @@ mod tests { upgrade_hint_for(channel, exe, &ChannelEnv::default()) } + /// #1900: `workspaces` is read through the shared package.json + /// reader, so a field that names no pattern string declares none. + #[test] + fn declares_workspaces_reads_the_shared_field_rule() { + for (text, want) in [ + (r#"{"workspaces":["a"]}"#, true), + (r#"{"workspaces":{"packages":["a"]}}"#, true), + (r#"{"workspaces":["a",1]}"#, true), + (r#"{"workspaces":[1]}"#, false), + (r#"{"workspaces":[]}"#, false), + (r#"{"workspaces":null}"#, false), + (r#"{"name":"x"}"#, false), + ] { + let pkg: serde_json::Value = serde_json::from_str(text).unwrap(); + assert_eq!(declares_workspaces(&pkg), want, "{text}"); + } + } + /// [`channel_label_for`] with no yarn `global-folder` configured. fn label(channel: InstallChannel, exe: &Path) -> &'static str { channel_label_for(channel, exe, &ChannelEnv::default()) diff --git a/crates/socket-patch-core/src/vendor/mod.rs b/crates/socket-patch-core/src/vendor/mod.rs index 481effdb7..be645cd0d 100644 --- a/crates/socket-patch-core/src/vendor/mod.rs +++ b/crates/socket-patch-core/src/vendor/mod.rs @@ -232,15 +232,7 @@ pub fn yarn_classic_workspace_member_risk(project_root: &Path) -> Option