diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f4f8067ea..cce660612 100644 --- a/.github/workflows/ci.yml +++ b/.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 }} diff --git a/crates/socket-patch-cli/src/commands/scan/hosted/python.rs b/crates/socket-patch-cli/src/commands/scan/hosted/python.rs index cce5c5f1e..55fffd20a 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted/python.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted/python.rs @@ -26,8 +26,9 @@ pub(super) async fn stale_install_warnings( records: &BTreeMap, ) -> StaleInstallOutcome { let mut out = StaleInstallOutcome::default(); - let pipenv_lock: Option = - pipenv_lock.and_then(|text| serde_json::from_str(text.trim_start_matches('\u{feff}')).ok()); + let pipenv_lock: Option = 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/")) diff --git a/crates/socket-patch-core/src/formats/text.rs b/crates/socket-patch-core/src/formats/text.rs index f52bd8df4..88db2369f 100644 --- a/crates/socket-patch-core/src/formats/text.rs +++ b/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) { + 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." + ); + } } diff --git a/crates/socket-patch-core/src/formats/yarn/blocks.rs b/crates/socket-patch-core/src/formats/yarn/blocks.rs index 9529edcf4..a15c1ff2f 100644 --- a/crates/socket-patch-core/src/formats/yarn/blocks.rs +++ b/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 { } 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)); } diff --git a/crates/socket-patch-core/src/formats/yarn/stanzas.rs b/crates/socket-patch-core/src/formats/yarn/stanzas.rs index bbedb8bd4..566c6bacb 100644 --- a/crates/socket-patch-core/src/formats/yarn/stanzas.rs +++ b/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'); diff --git a/crates/socket-patch-core/src/gradle/dsl.rs b/crates/socket-patch-core/src/gradle/dsl.rs index 038fed266..fd1a9c416 100644 --- a/crates/socket-patch-core/src/gradle/dsl.rs +++ b/crates/socket-patch-core/src/gradle/dsl.rs @@ -33,7 +33,7 @@ pub fn dsl_of(rel: &str) -> Option { /// 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 { - 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, 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' { diff --git a/crates/socket-patch-core/src/hosted/npm_manifest.rs b/crates/socket-patch-core/src/hosted/npm_manifest.rs index 7f24a84d4..9087631a7 100644 --- a/crates/socket-patch-core/src/hosted/npm_manifest.rs +++ b/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::(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")); + } } diff --git a/crates/socket-patch-core/src/manifest/operations.rs b/crates/socket-patch-core/src/manifest/operations.rs index 3590adeb9..4e00a7ac9 100644 --- a/crates/socket-patch-core/src/manifest/operations.rs +++ b/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) } diff --git a/crates/socket-patch-core/src/patch/redirect/requirements.rs b/crates/socket-patch-core/src/patch/redirect/requirements.rs index a5581b6a5..47beb2ffc 100644 --- a/crates/socket-patch-core/src/patch/redirect/requirements.rs +++ b/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 { } 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 { 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; } diff --git a/crates/socket-patch-core/src/policy/socket_yml.rs b/crates/socket-patch-core/src/policy/socket_yml.rs index 103fe20bd..a3a16c582 100644 --- a/crates/socket-patch-core/src/policy/socket_yml.rs +++ b/crates/socket-patch-core/src/policy/socket_yml.rs @@ -711,7 +711,7 @@ fn decode(ctx: &Ctx<'_>, bytes: &[u8]) -> Result { 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")); } diff --git a/crates/socket-patch-core/src/utils/requirements.rs b/crates/socket-patch-core/src/utils/requirements.rs index 1b8229e1b..a215e724d 100644 --- a/crates/socket-patch-core/src/utils/requirements.rs +++ b/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 { // 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 { // 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,