Move BOM handling in 8 more files onto formats::text (#905)

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<Vec<PathBuf>> {
         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.

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<String> {
-    strip_bom(text)
-        .lines()
+    text.lines()
         .filter_map(top_level_key)
         .rfind(|(k, _)| k == key)
         .map(|(_, value)| value.trim_matches(['"', '\'']).to_string())

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",
     ];
 

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<Vec<String>> {
             _ => {}
         }
     }
-    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<Vec<String>> {
 /// `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<Vec<String>> {
-    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]

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]

crates/socket-patch-core/src/patch/redirect/upstream/npm.rs

@@ -517,16 +517,16 @@ 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
 /// against an `npmScopes` registry instead, so with such a block present
 /// it keeps the default registry's document.
 fn berry_lookup_registry(yarnrc: Option<&str>, name: &str) -> Option<String> {
     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<serde_json::Value> = 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";

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