From 266b694b771b37bd6b31a5d81b7328a8bc0c39a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 13:59:27 +0000 Subject: [PATCH 1/2] Start refactor for #905 Assisted-by: Claude Code:claude-opus-5-5 From abe79b99ddf8212d73a9a062ddae9411e072c757 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 14:14:59 +0000 Subject: [PATCH 2/2] Move BOM handling in 8 more files to formats::text The yarn .yarnrc and Bun workspace readers in the npm crawler, the vlt and package.json workspace readers in governing_root, the .npmrc allow-remote splice, the berry restore's package.json and npmScopes reads, VEX discovery of pnpm file: directories and Hatch TOML spelled out "skip a leading UTF-8 BOM" inline. They now call strip_bom or split_bom (or leave it to a reader that already skips it: top_level_key, the TOML lexer), so one leading BOM is encoding everywhere (#905). yaml_top_level_value skipped it twice and now leaves it to top_level_key. Zero or one leading BOM behaves as before. A file that starts with two BOMs now reads the second as content, the rule every other reader follows since #1160: a .yarnrc's first key, a Bun package.json, a pnpm file: directory manifest, pyproject.toml/hatch.toml and a .yarnrc.yml first key no longer parse past it. PENDING_INLINE_BOMS drops seven files (4 remain, all changed by open PRs); formats/pnpm/lines.rs was a stale entry. Each former caller gets a 0/1/2-BOM test. Assisted-by: Claude Code:claude-opus-5-5 --- .../src/crawlers/npm_crawler.rs | 41 ++++++++++++++++++- .../src/formats/pnpm/workspace.rs | 6 +-- crates/socket-patch-core/src/formats/text.rs | 12 ++---- .../src/hosted/governing_root.rs | 22 ++++++++-- .../src/patch/redirect/npmrc.rs | 33 +++++++++++---- .../src/patch/redirect/upstream/npm.rs | 35 ++++++++++++++-- .../socket-patch-core/src/vex/discover/npm.rs | 40 ++++++++++++++++-- .../src/vex/discover/pypi_other.rs | 35 +++++++++++++--- 8 files changed, 186 insertions(+), 38 deletions(-) diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler.rs b/crates/socket-patch-core/src/crawlers/npm_crawler.rs index 7b796dcff..c6f572cad 100644 --- a/crates/socket-patch-core/src/crawlers/npm_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/npm_crawler.rs @@ -7,6 +7,7 @@ use serde::Deserialize; use super::types::{CrawledPackage, CrawlerOptions}; use super::walk_pool::{par_map, run_walk}; +use crate::formats::text::strip_bom; use crate::patch::path_safety; use crate::utils::fs::{is_dir, is_dir_sync, read_dir_entries_sync}; use crate::utils::purl::{percent_decode_purl_component, strip_purl_qualifiers}; @@ -273,7 +274,7 @@ struct YarnrcModulesFolder { /// each key the last setting wins. fn parse_yarnrc_modules_folder(rc: &str) -> YarnrcModulesFolder { let mut found = YarnrcModulesFolder::default(); - for line in rc.trim_start_matches('\u{feff}').lines() { + for line in strip_bom(rc).lines() { let line = line.trim(); if line.is_empty() || line.starts_with('#') { continue; @@ -796,7 +797,7 @@ 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 = text.strip_prefix('\u{feff}').unwrap_or(&text); + 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(), @@ -7262,6 +7263,42 @@ mod tests { ); } + /// The `.yarnrc` and Bun `package.json` workspace readers skip one + /// leading BOM (`formats::text`); a second one is content. + #[test] + fn yarnrc_and_bun_workspaces_read_past_one_bom_only() { + let rc = "--modules-folder deps\n"; + for bom in ["", "\u{feff}"] { + assert_eq!( + parse_yarnrc_modules_folder(&format!("{bom}{rc}")) + .general + .as_deref(), + Some("deps") + ); + } + assert_eq!( + parse_yarnrc_modules_folder(&format!("\u{feff}\u{feff}{rc}")), + YarnrcModulesFolder::default() + ); + + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir(dir.path().join("a")).unwrap(); + let manifest = r#"{"workspaces":["a"]}"#; + for bom in ["", "\u{feff}"] { + std::fs::write(dir.path().join("package.json"), format!("{bom}{manifest}")).unwrap(); + assert_eq!( + bun_workspace_pattern_members_sync(dir.path()), + Some(vec![dir.path().join("a")]) + ); + } + std::fs::write( + dir.path().join("package.json"), + format!("\u{feff}\u{feff}{manifest}"), + ) + .unwrap(); + assert_eq!(bun_workspace_pattern_members_sync(dir.path()), None); + } + /// `.yarnrc` `--modules-folder` parsing: bare and quoted keys and /// values, an optional `:`, the command-scoped form, comments, a BOM, /// CRLF, a Windows drive path, and last-setting-wins. diff --git a/crates/socket-patch-core/src/formats/pnpm/workspace.rs b/crates/socket-patch-core/src/formats/pnpm/workspace.rs index 66fabf6a8..64f10d450 100644 --- a/crates/socket-patch-core/src/formats/pnpm/workspace.rs +++ b/crates/socket-patch-core/src/formats/pnpm/workspace.rs @@ -57,10 +57,10 @@ pub(crate) fn top_level_key(line: &str) -> Option<(String, &str)> { } /// The value of the last top-level `key` in a YAML settings file -/// (pnpm-workspace.yaml, .yarnrc.yml), quotes removed. +/// (pnpm-workspace.yaml, .yarnrc.yml), quotes removed. [`top_level_key`] +/// skips the first line's BOM. pub(crate) fn yaml_top_level_value(text: &str, key: &str) -> Option { - strip_bom(text) - .lines() + text.lines() .filter_map(top_level_key) .rfind(|(k, _)| k == key) .map(|(_, value)| value.trim_matches(['"', '\'']).to_string()) diff --git a/crates/socket-patch-core/src/formats/text.rs b/crates/socket-patch-core/src/formats/text.rs index bd64b7d77..05f69fba0 100644 --- a/crates/socket-patch-core/src/formats/text.rs +++ b/crates/socket-patch-core/src/formats/text.rs @@ -10,10 +10,13 @@ //! rewrites the file must put it back. Exactly one BOM is encoding; a //! second one is content, as for every tool above. +/// U+FEFF, the character a UTF-8 BOM decodes to. +pub const BOM: char = '\u{feff}'; + /// `(bom, rest)`: a leading UTF-8 BOM split off (`""` when there is none), /// so an edit can read `rest` and restore `bom` byte-exact on write. pub fn split_bom(text: &str) -> (&'static str, &str) { - match text.strip_prefix('\u{feff}') { + match text.strip_prefix(BOM) { Some(rest) => ("\u{feff}", rest), None => ("", text), } @@ -66,16 +69,9 @@ mod tests { /// on #905 step 3 (they are changed by open PRs). Drop a file when you /// move it onto the helpers above. const PENDING_INLINE_BOMS: &[&str] = &[ - "crawlers/npm_crawler.rs", - "formats/pnpm/lines.rs", - "hosted/governing_root.rs", "patch/redirect/mod.rs", - "patch/redirect/npmrc.rs", - "patch/redirect/upstream/npm.rs", "patch/redirect/upstream/pypi.rs", "vendor/yarn_classic_lock.rs", - "vex/discover/npm.rs", - "vex/discover/pypi_other.rs", "vex/discover/yarn.rs", ]; diff --git a/crates/socket-patch-core/src/hosted/governing_root.rs b/crates/socket-patch-core/src/hosted/governing_root.rs index 93f16b02f..f64e55052 100644 --- a/crates/socket-patch-core/src/hosted/governing_root.rs +++ b/crates/socket-patch-core/src/hosted/governing_root.rs @@ -23,6 +23,7 @@ use std::path::{Path, PathBuf}; use crate::constants::npm_family::VLT_LOCK; use crate::formats::governing_locks::{npm_lock_files, NpmLockFamily}; +use crate::formats::text::strip_bom; use crate::patch::redirect::npmrc::npmrc_top_level_value; use crate::utils::fs::{read_regular_to_string, read_regular_to_string_sync}; use crate::utils::pnpm_workspace::governing_workspace_file; @@ -611,7 +612,7 @@ fn vlt_workspace_patterns(vlt_json: &str) -> Option> { _ => {} } } - let text = vlt_json.strip_prefix('\u{feff}').unwrap_or(vlt_json); + let text = strip_bom(vlt_json); let doc: serde_json::Value = serde_json::from_str(text).ok()?; let field = doc.get("workspaces")?; let mut out = Vec::new(); @@ -627,9 +628,7 @@ fn vlt_workspace_patterns(vlt_json: &str) -> Option> { /// `nohoist` shape, Bun's catalogs shape). `None` when the field is absent /// or the manifest does not parse. fn workspace_patterns(package_json: &str) -> Option> { - let text = package_json - .strip_prefix('\u{feff}') - .unwrap_or(package_json); + 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 { @@ -1350,6 +1349,21 @@ mod tests { ); } + /// Both workspace readers skip one leading BOM (`formats::text`); a + /// second one is content, so the JSON does not parse. + #[test] + fn workspace_readers_read_past_one_bom_only() { + let json = r#"{"workspaces":["a/*"]}"#; + for bom in ["", "\u{feff}"] { + let text = format!("{bom}{json}"); + assert_eq!(vlt_workspace_patterns(&text), Some(vec!["a/*".to_string()])); + assert_eq!(workspace_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); + } + /// #942: vlt's `workspaces` in `vlt.json` is a string, an array or an /// object of named groups, each a string or an array. #[test] diff --git a/crates/socket-patch-core/src/patch/redirect/npmrc.rs b/crates/socket-patch-core/src/patch/redirect/npmrc.rs index 6c0f5986a..b700ec639 100644 --- a/crates/socket-patch-core/src/patch/redirect/npmrc.rs +++ b/crates/socket-patch-core/src/patch/redirect/npmrc.rs @@ -33,6 +33,8 @@ //! and — when the project file is silent — the user / global / builtin //! config files ([`resolve_outer_allow_remote`]). +use crate::formats::text::{split_bom, BOM}; + /// Repo-relative path of the project `.npmrc` the auto-config edits. pub const NPMRC_REL: &str = ".npmrc"; @@ -51,12 +53,11 @@ pub const NPMRC_ALLOW_REMOTE_LINE: &str = "allow-remote=all"; /// The exact `.npmrc` the auto-config CREATES when none existed. pub const NPMRC_CREATED: &str = "allow-remote=all\n"; -const BOM: char = '\u{feff}'; - /// ECMAScript whitespace — what npm's `ini` means by `\s` and by /// `String.prototype.trim` (WhiteSpace + LineTerminator). Rust's /// `char::is_whitespace` differs only by U+0085 (NEL: not JS whitespace) -/// and U+FEFF (the BOM: JS whitespace). +/// and U+FEFF (the BOM: JS whitespace, so npm trims it off any key or +/// value, not only at the start of the file). fn is_js_ws(c: char) -> bool { c == BOM || (c.is_whitespace() && c != '\u{85}') } @@ -83,7 +84,7 @@ fn has_lone_cr(text: &str) -> bool { } /// npm `ini`'s section header — `^\[([^\]]*)\]\s*$` matched against the -/// UNTRIMMED line: an indented ` [sec]` or a BOM-prefixed `\u{feff}[sec]` +/// UNTRIMMED line: an indented ` [sec]` or a BOM-prefixed `[sec]` /// is NOT a header to npm (it parses as a top-level key), so it must not /// end the top-level scope here either. fn is_section_header(line: &str) -> bool { @@ -98,7 +99,7 @@ fn is_section_header(line: &str) -> bool { /// with lone `\r`s, several npm lines) hold a real section header? `line0` /// is true for the file's first line, which carries the BOM `bom` the /// callers strip off before splitting (npm does NOT strip it, so a -/// `\u{feff}[sec]` first line is no header). +/// BOM-prefixed `[sec]` first line is no header). fn holds_section_header(bom: &str, line: &str, line0: bool) -> bool { let owned; let line = if line0 && !bom.is_empty() { @@ -732,10 +733,7 @@ pub fn plan_npmrc_allow_remote_with(existing: Option<&str>, outer: &OuterAllowRe .into(), ); } - let (bom, body) = match text.strip_prefix(BOM) { - Some(rest) => (&text[..BOM.len_utf8()], rest), - None => ("", text), - }; + let (bom, body) = split_bom(text); let crlf = crate::utils::line_endings::terminator(body) == "\r\n"; let line = if crlf { format!("{NPMRC_ALLOW_REMOTE_LINE}\r") @@ -903,6 +901,23 @@ mod tests { assert_eq!(text, "\u{feff}[x]\nallow-remote=all\n[sec]\ny=1\n"); } + /// One leading BOM is split off and put back byte-exact + /// (`formats::text::split_bom`); a second one stays in the body, where + /// npm's JS trim drops it from the key. + #[test] + fn npmrc_splice_keeps_one_bom_and_reads_a_second_as_whitespace() { + for bom in ["", "\u{feff}"] { + assert_eq!( + plan_npmrc_allow_remote(Some(&format!("{bom}a=1\n"))), + NpmrcPlan::Append(format!("{bom}a=1\nallow-remote=all\n")) + ); + } + assert_eq!( + plan_npmrc_allow_remote(Some("\u{feff}\u{feff}allow-remote=none\n")), + NpmrcPlan::UserSet("none".into()) + ); + } + /// The spliced line takes `line_endings::terminator`'s style: the /// majority of a mixed file's breaks, LF on a tie. #[test] diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs b/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs index de0b0d04b..c3d0d26e7 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs @@ -517,6 +517,7 @@ fn berry_registry_locator( } use crate::formats::pnpm::workspace::yaml_top_level_value; +use crate::formats::text::strip_bom; /// The registry a berry restore reads `name`'s version document from: /// `.yarnrc.yml`'s `npmRegistryServer`. A scoped package may resolve @@ -524,9 +525,8 @@ use crate::formats::pnpm::workspace::yaml_top_level_value; /// it keeps the default registry's document. fn berry_lookup_registry(yarnrc: Option<&str>, name: &str) -> Option { let text = yarnrc?; + // `top_level_key` skips the first line's BOM (`formats::text`). let has_scopes = text - .strip_prefix('\u{feff}') - .unwrap_or(text) .lines() .filter_map(crate::formats::pnpm::workspace::top_level_key) .any(|(key, _)| key == "npmScopes"); @@ -573,7 +573,7 @@ async fn restore_berry( let mut pkg: Option = pkg_text .as_deref() - .and_then(|t| serde_json::from_str(t.strip_prefix('\u{feff}').unwrap_or(t)).ok()) + .and_then(|t| serde_json::from_str(strip_bom(t)).ok()) .filter(serde_json::Value::is_object); let mut pkg_changed = false; @@ -2893,6 +2893,35 @@ mod tests { } } + /// The `npmScopes` probe and `yaml_top_level_value` skip one leading + /// BOM (`formats::text`, through `top_level_key`); a second one is + /// content, so the first key is not `npmScopes`. + #[test] + fn berry_scopes_probe_reads_past_one_bom_only() { + let rc = "npmScopes:\n s:\n npmRegistryServer: https://s.example\n\ + npmRegistryServer: https://m.example/\n"; + for bom in ["", "\u{feff}"] { + let rc = format!("{bom}{rc}"); + assert_eq!(berry_lookup_registry(Some(&rc), "@s/a"), None); + } + let rc = format!("\u{feff}\u{feff}{rc}"); + assert_eq!( + berry_lookup_registry(Some(&rc), "@s/a").as_deref(), + Some("https://m.example/") + ); + let rc = "npmRegistryServer: https://m.example/\n"; + for bom in ["", "\u{feff}"] { + assert_eq!( + yaml_top_level_value(&format!("{bom}{rc}"), "npmRegistryServer").as_deref(), + Some("https://m.example/") + ); + } + assert_eq!( + yaml_top_level_value(&format!("\u{feff}\u{feff}{rc}"), "npmRegistryServer"), + None + ); + } + #[test] fn berry_reads_the_project_registry_except_for_npm_scopes() { let rc = "npmRegistryServer: \"https://m.example/npm/\"\n"; diff --git a/crates/socket-patch-core/src/vex/discover/npm.rs b/crates/socket-patch-core/src/vex/discover/npm.rs index ae6063888..f044bb889 100644 --- a/crates/socket-patch-core/src/vex/discover/npm.rs +++ b/crates/socket-patch-core/src/vex/discover/npm.rs @@ -61,6 +61,7 @@ use crate::formats::pnpm::{ classify_pnpm_key, entry_bundled, entry_field, pnpm_registry_key, Bundled, PnpmKey, PnpmLock, PnpmPackage, }; +use crate::formats::text::strip_bom; use crate::utils::digest::is_sri_pin; use crate::vendor::lock_inventory::pnpm::rush_lock_rels; use crate::vendor::lock_inventory::{ @@ -812,9 +813,9 @@ async fn record_pnpm_file_copies( } else { format!("{rel}/package.json") }; - ctx.read_advisory_text(&manifest).await.and_then(|t| { - serde_json::from_str(t.trim_start_matches('\u{feff}')).ok() - }) + ctx.read_advisory_text(&manifest) + .await + .and_then(|t| serde_json::from_str(strip_bom(&t)).ok()) } Some(rel) => match ctx.read_advisory_bytes(&rel).await { Some(bytes) => tokio::task::spawn_blocking(move || { @@ -2711,6 +2712,39 @@ mod tests { ); } + /// A `file:` directory's `package.json` is read past one leading BOM + /// (`formats::text`): with one it still contests the ref; with two the + /// manifest does not parse, so the copy is left alone like any + /// unreadable one. + #[tokio::test] + async fn pnpm_file_directory_manifest_reads_past_one_bom_only() { + let url = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz"); + let lock = format!( + "lockfileVersion: '9.0'\n\npackages:\n\n \ + left-pad@1.3.0:\n resolution: {{integrity: {SRI}, tarball: {url}}}\n\n \ + left-pad@file:forks/left-pad:\n \ + resolution: {{directory: forks/left-pad, type: directory}}\n\n" + ); + let manifest = r#"{"name":"left-pad","version":"1.3.0"}"#; + for bom in ["", "\u{feff}"] { + let p = Project::new(); + p.write("pnpm-lock.yaml", lock.clone()); + p.write("forks/left-pad/package.json", format!("{bom}{manifest}")); + let out = run(&p).await; + assert!(out.refs.is_empty(), "{bom:?}: {:#?}", out.refs); + } + let p = Project::new(); + p.write("pnpm-lock.yaml", lock.clone()); + p.write( + "forks/left-pad/package.json", + format!("\u{feff}\u{feff}{manifest}"), + ); + assert_refs( + &run(&p).await, + &[("pkg:npm/left-pad@1.3.0", UUID_A, WiringMode::Hosted)], + ); + } + /// pnpm unpacks a package's `bundledDependencies` from its own tarball /// and never locks them, so a bundled copy of the wired package stays /// unpatched beside the Socket wiring (audit B04; npm, bun and vlt diff --git a/crates/socket-patch-core/src/vex/discover/pypi_other.rs b/crates/socket-patch-core/src/vex/discover/pypi_other.rs index c1a8d3c56..b8ecbc066 100644 --- a/crates/socket-patch-core/src/vex/discover/pypi_other.rs +++ b/crates/socket-patch-core/src/vex/discover/pypi_other.rs @@ -342,12 +342,8 @@ async fn extract_hatch(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { async fn read_toml(ctx: &DiscoverCtx<'_>, file: &str, out: &mut Discovery) -> Option { let text = ctx.read_text(file, out).await?; - toml_or_diag( - file, - text.trim_start_matches('\u{feff}'), - TomlDiag::BomFlattened, - out, - ) + // The TOML lexer skips one leading BOM itself (`formats::text`'s rule). + toml_or_diag(file, &text, TomlDiag::BomFlattened, out) } /// One PEP 508 dependency string. `root_uri_expanded`: whether Hatch @@ -1432,6 +1428,33 @@ mod tests { assert!(out.diagnostics.is_empty(), "{:?}", out.diagnostics); } + /// Hatch's TOML reads skip one leading BOM (the TOML lexer does, as + /// `formats::text` rules): a second one is content, so the file is + /// unparseable. + #[tokio::test] + async fn hatch_toml_reads_past_one_bom_only() { + let wheel = vendored_wheel(UUID_A, WHEEL); + let pyproject = format!( + "[project]\nname = \"app\"\nversion = \"0\"\ndependencies = \ + [\"urllib3 @ {{root:uri}}/{wheel}#sha256={SHA}\"]\n" + ); + for bom in ["", "\u{feff}"] { + let p = Project::new(); + p.write("pyproject.toml", format!("{bom}{pyproject}")); + let out = run(&p).await; + assert_refs( + &out, + &[("pkg:pypi/urllib3@1.26.18", UUID_A, WiringMode::Vendored)], + ); + assert!(out.diagnostics.is_empty(), "{:?}", out.diagnostics); + } + let p = Project::new(); + p.write("pyproject.toml", format!("\u{feff}\u{feff}{pyproject}")); + let out = run(&p).await; + assert_refs(&out, &[]); + assert_eq!(diag_codes(&out), vec![DIAG_LOCKFILE_UNPARSEABLE]); + } + /// Hatch reads hatch.toml's top-level `envs` INSTEAD of pyproject's /// `[tool.hatch.envs]`; `{root:uri}` in PEP 735 groups is never /// expanded; other prefixes and traversal are rejected; registry and