Route inline BOM strips through formats::text (#905)

.github/workflows/ci.yml

@@ -1397,7 +1397,7 @@ jobs:
         # The composer capstones shell out to a real composer; `composer:`
         # pins the release line (1, 2.2 LTS, 2) so the composer.lock grammar
         # the edits assert stays stable across runners.
-        uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2
+        uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
         with:
           php-version: '8.2'
           tools: composer:${{ matrix.composer }}

crates/socket-patch-cli/src/commands/scan/hosted/python.rs

@@ -26,8 +26,9 @@ pub(super) async fn stale_install_warnings(
     records: &BTreeMap<String, PatchRecord>,
 ) -> StaleInstallOutcome {
     let mut out = StaleInstallOutcome::default();
-    let pipenv_lock: Option<serde_json::Value> =
-        pipenv_lock.and_then(|text| serde_json::from_str(text.trim_start_matches('\u{feff}')).ok());
+    let pipenv_lock: Option<serde_json::Value> = pipenv_lock.and_then(|text| {
+        serde_json::from_str(socket_patch_core::formats::text::strip_bom(text)).ok()
+    });
     let candidates: Vec<_> = confirmed
         .iter()
         .filter(|(purl, _)| purl.starts_with("pkg:pypi/"))

crates/socket-patch-core/src/formats/text.rs

@@ -12,7 +12,7 @@
 
 /// `(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) -> (&str, &str) {
+pub fn split_bom(text: &str) -> (&'static str, &str) {
     match text.strip_prefix('\u{feff}') {
         Some(rest) => ("\u{feff}", rest),
         None => ("", text),
@@ -24,6 +24,11 @@ pub fn strip_bom(text: &str) -> &str {
     split_bom(text).1
 }
 
+/// [`strip_bom`] for bytes not yet decoded: drops one leading `EF BB BF`.
+pub fn strip_bom_bytes(bytes: &[u8]) -> &[u8] {
+    bytes.strip_prefix(b"\xef\xbb\xbf").unwrap_or(bytes)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -42,4 +47,128 @@ mod tests {
         assert_eq!(strip_bom("x\u{feff}"), "x\u{feff}");
         assert_eq!(strip_bom("\u{feff}\u{feff}x"), "\u{feff}x");
     }
+
+    #[test]
+    fn strip_bom_bytes_drops_one_leading_bom_only() {
+        assert_eq!(strip_bom_bytes(b"\xef\xbb\xbfx"), b"x");
+        assert_eq!(strip_bom_bytes(b"x\xef\xbb\xbf"), b"x\xef\xbb\xbf");
+        assert_eq!(
+            strip_bom_bytes(b"\xef\xbb\xbf\xef\xbb\xbfx"),
+            b"\xef\xbb\xbfx"
+        );
+        assert_eq!(strip_bom_bytes(b"\xef\xbbx"), b"\xef\xbbx");
+        for text in ["", "x", "\u{feff}x", "\u{feff}\u{feff}x"] {
+            assert_eq!(strip_bom_bytes(text.as_bytes()), strip_bom(text).as_bytes());
+        }
+    }
+
+    /// Production files that still spell out BOM handling inline, waiting
+    /// 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/gradle_cache.rs",
+        "crawlers/ivy_cache.rs",
+        "crawlers/npm_crawler.rs",
+        "formats/pnpm/lines.rs",
+        "formats/sbt/owned_file.rs",
+        "formats/yarn/berry_gates.rs",
+        "formats/yarn/mod.rs",
+        "hosted/governing_root.rs",
+        "patch/redirect/gradle.rs",
+        "patch/redirect/mod.rs",
+        "patch/redirect/npmrc.rs",
+        "patch/redirect/upstream/gradle.rs",
+        "patch/redirect/upstream/npm.rs",
+        "patch/redirect/upstream/pypi.rs",
+        "patch/redirect/vlt.rs",
+        "policy/mod.rs",
+        "vendor/common.rs",
+        "vendor/go_mod_edit.rs",
+        "vendor/jvm/gradle.rs",
+        "vendor/lock_inventory/pypi.rs",
+        "vendor/npm_dir.rs",
+        "vendor/pypi_hatch.rs",
+        "vendor/yarn_classic_lock.rs",
+        "vex/discover/npm.rs",
+        "vex/discover/pypi_other.rs",
+        "vex/discover/yarn.rs",
+    ];
+
+    /// Files whose inline BOM handling is deliberate, not a copy of the rule.
+    const OWN_BOM_RULE: &[&str] = &[
+        // `sbt_version` reads a Java properties file line by line and skips
+        // a BOM on any line (its test pins a BOM after a comment line).
+        "formats/sbt/build.rs",
+        // vlt cannot read a BOM lock, so the sniff refuses it unstripped.
+        "vendor/vlt_lock_text.rs",
+    ];
+
+    /// No new production file decides for itself what a leading BOM is: it
+    /// calls `split_bom`, `strip_bom` or `strip_bom_bytes`. The guard is
+    /// one-sided (a pending file that loses its last inline BOM does not
+    /// fail it), so another PR that migrates a file can never turn it red.
+    /// Test modules and test-support files are exempt: fixtures spell BOMs.
+    #[test]
+    fn production_bom_handling_goes_through_the_helpers() {
+        const INLINE: &[&str] = &[
+            "\\u{feff}",
+            "\\u{FEFF}",
+            "0xEF, 0xBB, 0xBF",
+            "0xef, 0xbb, 0xbf",
+            "\\xef\\xbb\\xbf",
+            "\\xEF\\xBB\\xBF",
+        ];
+        fn walk(dir: &std::path::Path, out: &mut Vec<std::path::PathBuf>) {
+            for entry in std::fs::read_dir(dir).unwrap() {
+                let path = entry.unwrap().path();
+                if path.is_dir() {
+                    walk(&path, out);
+                } else if path.extension().is_some_and(|e| e == "rs") {
+                    out.push(path);
+                }
+            }
+        }
+        let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src");
+        let mut files = Vec::new();
+        walk(&src, &mut files);
+        let mut inline = Vec::new();
+        for path in files {
+            let rel = path
+                .strip_prefix(&src)
+                .unwrap()
+                .to_string_lossy()
+                .replace('\\', "/");
+            if rel == "formats/text.rs"
+                || rel.ends_with("tests.rs")
+                || rel.contains("test_support")
+                || PENDING_INLINE_BOMS.contains(&rel.as_str())
+                || OWN_BOM_RULE.contains(&rel.as_str())
+            {
+                continue;
+            }
+            // Windows CI checks out with CRLF.
+            let text = std::fs::read_to_string(&path)
+                .unwrap()
+                .replace("\r\n", "\n");
+            let production = [
+                "#[cfg(test)]\nmod tests",
+                "#[cfg(test)]\npub(crate) mod tests",
+                "#[cfg(test)]\nmod architecture_tests",
+            ]
+            .iter()
+            .filter_map(|marker| text.find(marker))
+            .min()
+            .map_or(text.as_str(), |at| &text[..at]);
+            if INLINE.iter().any(|p| production.contains(p)) {
+                inline.push(rel);
+            }
+        }
+        inline.sort();
+        assert!(
+            inline.is_empty(),
+            "these files handle a UTF-8 BOM inline: {inline:?}. Call \
+             crate::formats::text::{{split_bom, strip_bom, strip_bom_bytes}} \
+             instead (#905). Do not add them to PENDING_INLINE_BOMS."
+        );
+    }
 }

crates/socket-patch-core/src/formats/yarn/blocks.rs

@@ -7,6 +7,7 @@
 //! apart on what a block, a key or a field is.
 
 use super::patterns::{berry_npm_alias_target, split_berry_key_patterns, split_pattern};
+use crate::formats::text::split_bom;
 use crate::vendor::common::detect_eol;
 
 /// One key-line block of a yarn lockfile (classic or berry).
@@ -42,10 +43,9 @@ pub(crate) fn scan_blocks(text: &str) -> Vec<LockBlock> {
         }
         let mut content = content.strip_suffix('\r').unwrap_or(content);
         if start == 0 {
-            if let Some(rest) = content.strip_prefix('\u{feff}') {
-                start = '\u{feff}'.len_utf8();
-                content = rest;
-            }
+            let (bom, rest) = split_bom(content);
+            start = bom.len();
+            content = rest;
         }
         lines.push((start, pos, content, terminated));
     }

crates/socket-patch-core/src/formats/yarn/stanzas.rs

@@ -13,6 +13,7 @@
 
 use std::borrow::Cow;
 
+use crate::formats::text::split_bom;
 use crate::utils::line_endings::{to_lf, LineEndings};
 
 /// A berry lock split into its stanzas (see the module docs).
@@ -31,10 +32,7 @@ impl BerryStanzas {
     /// Split `raw`. A [`LineEndings::Mixed`] lock must be refused by the
     /// caller first; one would be rendered back in a single style.
     pub(crate) fn parse(raw: &str) -> Self {
-        let (bom, body) = match raw.strip_prefix('\u{feff}') {
-            Some(rest) => ("\u{feff}", rest),
-            None => ("", raw),
-        };
+        let (bom, body) = split_bom(raw);
         let eol = LineEndings::of(body);
         let lf = to_lf(body).into_owned();
         let trimmed = lf.trim_end_matches('\n');

crates/socket-patch-core/src/gradle/dsl.rs

@@ -33,7 +33,7 @@ pub fn dsl_of(rel: &str) -> Option<Dsl> {
 /// Script bytes as text: a leading BOM is dropped and anything that is not
 /// UTF-8 is `None` (unparseable), never decoded lossily.
 pub fn decode(bytes: &[u8]) -> Option<String> {
-    let bytes = bytes.strip_prefix(b"\xef\xbb\xbf").unwrap_or(bytes);
+    let bytes = crate::formats::text::strip_bom_bytes(bytes);
     String::from_utf8(bytes.to_vec()).ok()
 }
 
@@ -97,7 +97,7 @@ fn lex(src: &str, dsl: Dsl) -> (Vec<Token>, bool) {
     let b = src.as_bytes();
     let mut out = Vec::new();
     let mut clean = true;
-    let mut i = if src.starts_with('\u{feff}') { 3 } else { 0 };
+    let mut i = crate::formats::text::split_bom(src).0.len();
     // A `#!` first line (shebang) is a comment in both DSLs.
     if b[i..].starts_with(b"#!") {
         while i < b.len() && b[i] != b'\n' {

crates/socket-patch-core/src/hosted/npm_manifest.rs

@@ -23,7 +23,7 @@ pub fn decode_hosted_npm_manifest(bytes: &[u8], sha512: Option<&str>) -> Result<
         .ok_or_else(|| "hosted tarball has no package/package.json".to_string())?;
     let text = std::str::from_utf8(manifest)
         .map_err(|_| "hosted tarball package.json is not UTF-8".to_string())?;
-    let text = text.strip_prefix('\u{feff}').unwrap_or(text);
+    let text = crate::formats::text::strip_bom(text);
     if !serde_json::from_str::<serde_json::Value>(text).is_ok_and(|v| v.is_object()) {
         return Err("hosted tarball package.json is not a JSON object".to_string());
     }
@@ -96,4 +96,26 @@ mod tests {
             .contains("not a JSON object"));
         assert!(decode_hosted_npm_manifest(b"not gzip", None).is_err());
     }
+
+    /// One leading BOM is encoding (npm strips it); a second is content
+    /// and leaves the manifest unparseable, as for every `formats::text`
+    /// reader.
+    #[test]
+    fn reads_past_one_bom_only() {
+        let one = tgz(&[(
+            "package/package.json",
+            "\u{feff}{\"name\":\"uuid\"}".as_bytes(),
+        )]);
+        assert_eq!(
+            decode_hosted_npm_manifest(&one, None).unwrap(),
+            "{\"name\":\"uuid\"}"
+        );
+        let two = tgz(&[(
+            "package/package.json",
+            "\u{feff}\u{feff}{\"name\":\"uuid\"}".as_bytes(),
+        )]);
+        assert!(decode_hosted_npm_manifest(&two, None)
+            .unwrap_err()
+            .contains("not a JSON object"));
+    }
 }

crates/socket-patch-core/src/manifest/operations.rs

@@ -64,7 +64,7 @@ pub async fn read_manifest(
     // Tolerate a UTF-8 byte-order mark (Windows editors add one on save):
     // the file looks fine in an editor, yet serde_json rejects it with an
     // opaque "expected value at line 1 column 1".
-    let json = content.strip_prefix('\u{feff}').unwrap_or(&content);
+    let json = crate::formats::text::strip_bom(&content);
     parse_manifest(json).map(Some)
 }
 

crates/socket-patch-core/src/patch/redirect/requirements.rs

@@ -5,6 +5,7 @@ use serde_json::Value;
 
 use super::{DepOverride, FileEdit, RewriteResult, RewriteWarning};
 use crate::crawlers::python_crawler::canonicalize_pypi_name;
+use crate::formats::text::{split_bom, strip_bom};
 use crate::utils::purl::percent_decode_purl_component;
 use crate::vendor::state::{VendorEntry, WiringAction};
 
@@ -32,11 +33,7 @@ pub(super) fn logical_requirements(content: &str) -> Vec<LogicalRequirement> {
             } else {
                 (physical_line, "")
             };
-            let parsed_body = if index == 0 {
-                body.strip_prefix('\u{feff}').unwrap_or(body)
-            } else {
-                body
-            };
+            let parsed_body = if index == 0 { strip_bom(body) } else { body };
             let continued =
                 !parsed_body.trim_start().starts_with('#') && body.trim_end().ends_with('\\');
             if continued && index + 1 < physical.len() {
@@ -48,7 +45,7 @@ pub(super) fn logical_requirements(content: &str) -> Vec<LogicalRequirement> {
             original.push_str(body);
             text.push_str(body);
             if start == 0 {
-                text = text.strip_prefix('\u{feff}').unwrap_or(&text).to_owned();
+                text = strip_bom(&text).to_owned();
             }
             requirements.push(LogicalRequirement {
                 original,
@@ -297,17 +294,9 @@ pub(super) fn rewrite(
             let extras = captures
                 .get(2)
                 .map_or("", |capture| capture.as_str().trim());
-            let prefix_body = requirement
-                .original
-                .strip_prefix('\u{feff}')
-                .unwrap_or(&requirement.original);
+            let (bom, prefix_body) = split_bom(&requirement.original);
             let indent = &prefix_body
                 [..prefix_body.len() - prefix_body.trim_start_matches([' ', '\t']).len()];
-            let bom = if requirement.original.starts_with('\u{feff}') {
-                "\u{feff}"
-            } else {
-                ""
-            };
             // Unhashed file: the url's `#sha256=` fragment, which pip
             // verifies without turning hash-checking mode on.
             let location = if hashed {
@@ -343,10 +332,7 @@ pub(super) fn rewrite(
                     original: Some(Value::String(requirement.original.clone())),
                     new: Some(Value::String(rewritten.clone())),
                 });
-                requirement.text = rewritten
-                    .strip_prefix('\u{feff}')
-                    .unwrap_or(&rewritten)
-                    .to_owned();
+                requirement.text = strip_bom(&rewritten).to_owned();
                 requirement.original = rewritten;
                 changed = true;
             }

crates/socket-patch-core/src/policy/socket_yml.rs

@@ -711,7 +711,7 @@ fn decode(ctx: &Ctx<'_>, bytes: &[u8]) -> Result<String, PolicyError> {
     if bytes.starts_with(&[0xFE, 0xFF]) || bytes.starts_with(&[0xFF, 0xFE]) {
         return Err(ctx.err("", "file is UTF-16; save it as UTF-8"));
     }
-    let bytes = bytes.strip_prefix(&[0xEF, 0xBB, 0xBF]).unwrap_or(bytes);
+    let bytes = crate::formats::text::strip_bom_bytes(bytes);
     if bytes.contains(&0) {
         return Err(ctx.err("", "file contains NUL bytes; save it as UTF-8 text"));
     }

crates/socket-patch-core/src/utils/requirements.rs

@@ -14,6 +14,8 @@
 //! `--hash=sha256:ab#cd` are data. Exactly one leading BOM is encoding, not
 //! data (pip decodes with utf-8-sig; uv strips it too).
 
+use crate::formats::text::strip_bom;
+
 /// Decode a requirements file the way pip's `auto_decode` does: a UTF-16
 /// or UTF-32 byte-order mark selects that encoding and is dropped; anything
 /// else is UTF-8, its one leading BOM kept for [`logical_lines`] to drop.
@@ -74,7 +76,7 @@ pub(crate) fn logical_lines(content: &str) -> Vec<LogicalLine> {
         // hide that line's comment either.
         let comment = |i: usize| {
             let line = if i == 0 {
-                lines[0].strip_prefix('\u{feff}').unwrap_or(lines[0])
+                strip_bom(lines[0])
             } else {
                 lines[i]
             };
@@ -97,9 +99,7 @@ pub(crate) fn logical_lines(content: &str) -> Vec<LogicalLine> {
         // stays raw, so a rewrite records (and a revert restores) the
         // original bytes.
         if start == 0 {
-            if let Some(stripped) = text.strip_prefix('\u{feff}') {
-                text = stripped.to_string();
-            }
+            text = strip_bom(&text).to_string();
         }
         out.push(LogicalLine {
             start,