Fix JSON writers dropping CRLF/BOM/indent (#324, #351) (#357)

* Start fix for #324, #351

Assisted-by: Claude Code:claude-opus-5-5

* Add tests for JSON writers losing file layout

Regression tests for #324 (npm lock CRLF, tab indent and BOM through
hosted rewrite, rollback, vendor and vendor --revert) and #351
(composer.json CRLF and string escapes through setup and
setup --remove). The composer tests pass with the splice-based
editor added in this commit; the npm ones fail until the next.

Assisted-by: Claude Code:claude-opus-5-5

* Keep npm lock CRLF, tabs and BOM on rewrite

The hosted and vendored npm lock rewriters, rollback and
vendor --revert now render the lock in its own layout (BOM, indent,
line ending, trailer) through JsonLayout, so a CRLF or tab-indented
lock gets a two-line diff and undo restores the original bytes.
A UTF-8 BOM lock, which npm installs from, is now read past its BOM
instead of being skipped (hosted) or refused with the wrong code
(vendored), and VEX discovery reads it too.

Fixes #324

Assisted-by: Claude Code:claude-opus-5-5

* Note the layout-keeping JSON edits in CHANGELOG

Assisted-by: Claude Code:claude-opus-5-5

* Add real-npm e2e for a CRLF + BOM package-lock

Vendors a CRLF lock with a UTF-8 BOM, proves a fresh npm ci installs
the patched bytes from it, and that vendor --revert restores the
lock byte for byte (#324).

Assisted-by: Claude Code:claude-opus-5-5

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Mikola Lysenko <mik@socket.dev>

Author: 3 people

CHANGELOG.md

@@ -162,6 +162,15 @@ limits, and required install commands.
 - Transient apply locks are removed on normal command exit; no-op scans and full
   reversal avoid leaving unused `.socket/` state. Terminal output, telemetry
   timeouts, and update-check handling are more consistent.
+- npm locks keep their own layout when edited. `scan --mode hosted`,
+  `scan --mode vendored`, `rollback` and `vendor --revert`
+  re-serialized `package-lock.json` / `npm-shrinkwrap.json` with LF line
+  endings (and, in hosted mode, a fixed 2-space indent), so a CRLF or
+  tab-indented lock got a whole-file diff and the undo did not restore its
+  bytes. A lock with a UTF-8 BOM, which npm installs from, was skipped as
+  unparseable (hosted) or refused as `vendor_lockfile_version_unsupported`
+  (vendored). The lock now keeps its BOM, indent and line endings, and the
+  undo is byte-exact (#324).
 
 ### Maintenance
 

crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs

@@ -495,6 +495,115 @@ fn npm_vendor_fresh_checkout_npm_ci_and_revert() {
     );
 }
 
+/// #324 with the real npm: a CRLF lock with a UTF-8 BOM (npm installs from
+/// both) is vendored in its own layout, a fresh `npm ci` installs the
+/// patched bytes from it, and `vendor --revert` restores its exact bytes.
+#[test]
+fn npm_vendor_keeps_a_crlf_bom_lock_and_reverts_it_byte_for_byte() {
+    let Some(major) = npm_major_or_skip("e2e_vendor_npm_build") else {
+        return;
+    };
+    let tmp = tempfile::tempdir().unwrap();
+    let proj = tmp.path().join("proj");
+    std::fs::create_dir_all(&proj).unwrap();
+    std::fs::write(
+        proj.join("package.json"),
+        r#"{"name":"vendor-crlf-bom","version":"0.0.0","private":true}"#,
+    )
+    .unwrap();
+    let cache = tmp.path().join("npm-cache");
+    if !npm_e2e_common::install_fixture(
+        "e2e_vendor_npm_build",
+        &proj,
+        &cache,
+        &format!("{DEP}@{DEP_VERSION}"),
+    ) {
+        return;
+    }
+    let orig = std::fs::read(proj.join("node_modules").join(DEP).join("index.js")).unwrap();
+    let patched: Vec<u8> = [MARKER.as_bytes(), orig.as_slice()].concat();
+    let purl = format!("pkg:npm/{DEP}@{DEP_VERSION}");
+    stage_patch_with_vuln(&proj, &purl, "package/index.js", &orig, &patched, TAIL_GHSA);
+    if v1_lock_is_refused(&proj, major) {
+        return;
+    }
+
+    let lock_path = proj.join("package-lock.json");
+    let lf = std::fs::read_to_string(&lock_path).unwrap();
+    let pristine = format!("\u{feff}{}", lf.replace('\n', "\r\n"));
+    std::fs::write(&lock_path, &pristine).unwrap();
+
+    let (code, stdout, stderr) = run_socket(
+        &proj,
+        &[
+            "vendor",
+            "--json",
+            "--offline",
+            "--cwd",
+            proj.to_str().unwrap(),
+        ],
+    );
+    assert_eq!(
+        code, 0,
+        "vendor failed.\nstdout:\n{stdout}\nstderr:\n{stderr}"
+    );
+    let env = parse_envelope(&stdout);
+    assert_eq!(env["summary"]["applied"], 1, "one package vendored: {env}");
+    let wired = std::fs::read_to_string(&lock_path).unwrap();
+    assert!(wired.starts_with('\u{feff}'), "the BOM is kept");
+    assert!(
+        !wired.replace("\r\n", "").contains('\n'),
+        "every line stays CRLF:\n{wired:?}"
+    );
+
+    let fresh = tmp.path().join("fresh");
+    std::fs::create_dir_all(&fresh).unwrap();
+    std::fs::copy(proj.join("package.json"), fresh.join("package.json")).unwrap();
+    std::fs::copy(&lock_path, fresh.join("package-lock.json")).unwrap();
+    copy_dir_recursive(&proj.join(".socket"), &fresh.join(".socket"));
+    let fresh_cache = tmp.path().join("fresh-npm-cache");
+    let ci = npm(
+        &fresh,
+        &[
+            "ci",
+            "--cache",
+            fresh_cache.to_str().unwrap(),
+            "--no-audit",
+            "--no-fund",
+        ],
+    );
+    assert!(
+        ci.status.success(),
+        "`npm ci` must install from the CRLF/BOM lock.\nstderr:\n{}",
+        String::from_utf8_lossy(&ci.stderr),
+    );
+    assert_eq!(
+        std::fs::read(fresh.join("node_modules").join(DEP).join("index.js")).unwrap(),
+        patched,
+        "npm ci installs the PATCHED bytes"
+    );
+
+    let (code, stdout, stderr) = run_socket(
+        &proj,
+        &[
+            "vendor",
+            "--revert",
+            "--json",
+            "--cwd",
+            proj.to_str().unwrap(),
+        ],
+    );
+    assert_eq!(
+        code, 0,
+        "revert failed.\nstdout:\n{stdout}\nstderr:\n{stderr}"
+    );
+    assert_eq!(
+        std::fs::read_to_string(&lock_path).unwrap(),
+        pristine,
+        "revert restores the CRLF/BOM lock byte for byte"
+    );
+}
+
 /// Real-toolchain VEX capstone for npm: after a REAL install + `vendor`, the
 /// vendored `.tgz` is the on-disk evidence. `socket-patch vex` must attest the
 /// patch against that vendored tarball with the `(vendored)` marker — proving

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

@@ -25,6 +25,7 @@ use serde_json::{json, Value};
 
 use crate::utils::digest::is_hex64_lower;
 use crate::utils::line_endings::{to_lf, LineEndings};
+use crate::vendor::common::{parse_json_text, JsonLayout};
 use crate::vendor::npm_origin::{legacy_packages_key, npm_non_registry_entries};
 use crate::vendor::yarn_berry_lock::yarnrc_compression_level;
 
@@ -280,6 +281,17 @@ fn serialize_json(value: &Value) -> String {
     )
 }
 
+/// `value` pretty-printed in the layout of `original`, the text it replaces
+/// (BOM, indent, line ending and trailer; see [`JsonLayout`]), so a rewrite
+/// and its revert change nothing but the edited values. npm keeps a lock's
+/// CRLF and tab indent on its own rewrites, and so must we.
+fn serialize_json_like(value: &Value, original: &str) -> String {
+    let bytes = JsonLayout::of(original)
+        .render(value)
+        .expect("serde_json::Value serializes infallibly");
+    String::from_utf8(bytes).expect("rendered JSON is UTF-8")
+}
+
 /// The dep's registry override when it is of `kind`. `None` for an absent
 /// AND for a foreign-kind override alike — neither can drive this
 /// ecosystem's rewrite, so every rewriter warns its missing-override code
@@ -821,7 +833,8 @@ fn rewrite_one_npm_lock(
     npm: &[&DepOverride],
     result: &mut RewriteResult,
 ) {
-    let Ok(mut lock) = serde_json::from_str::<Value>(content) else {
+    // npm reads past a leading UTF-8 BOM; so do we.
+    let Ok(mut lock) = parse_json_text(content) else {
         // A corrupt lockfile is strictly worse than a missing one (which
         // warns in the caller) — never skip the whole npm redirect silently.
         result.warnings.push(RewriteWarning {
@@ -988,7 +1001,9 @@ fn rewrite_one_npm_lock(
                 ),
             });
         }
-        result.files.insert(lockfile.into(), serialize_json(&lock));
+        result
+            .files
+            .insert(lockfile.into(), serialize_json_like(&lock, content));
     }
 }
 
@@ -12103,6 +12118,43 @@ mod tests {
         );
     }
 
+    /// #324: the hosted npm rewrite changes only the rewired values and keeps
+    /// the lock's layout: CRLF stays CRLF, a tab indent stays tabs, and a
+    /// UTF-8 BOM lock (npm strips the BOM and installs from it) is rewritten
+    /// with its BOM rather than skipped as unparseable.
+    #[test]
+    fn npm_lock_rewrite_keeps_crlf_tabs_and_bom() {
+        let ovr = npm_override(
+            "left-pad",
+            "1.3.0",
+            "http://patch.test/left-pad-1.3.0.tgz",
+            "sha512-PATCHED==",
+        );
+        let lf = "{\n  \"name\": \"app\",\n  \"lockfileVersion\": 3,\n  \"packages\": {\n    \"\": {\n      \"name\": \"app\"\n    },\n    \"node_modules/left-pad\": {\n      \"version\": \"1.3.0\",\n      \"resolved\": \"https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz\",\n      \"integrity\": \"sha512-UPSTREAM==\"\n    }\n  }\n}\n";
+        let shapes = [
+            ("crlf", lf.replace('\n', "\r\n")),
+            ("tabs", lf.replace("  ", "\t")),
+            ("bom", format!("\u{feff}{lf}")),
+            ("bom+crlf+tabs", format!("\u{feff}{}", lf.replace("  ", "\t").replace('\n', "\r\n"))),
+        ];
+        for (shape, pristine) in shapes {
+            let mut files = BTreeMap::new();
+            files.insert("package-lock.json".to_string(), pristine.clone());
+            let r = rewrite_registry_redirect(&files, std::slice::from_ref(&ovr));
+            let out = r
+                .files
+                .get("package-lock.json")
+                .unwrap_or_else(|| panic!("{shape}: lock must be rewritten: {:?}", r.warnings));
+            let expected = pristine
+                .replace(
+                    "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz",
+                    "http://patch.test/left-pad-1.3.0.tgz",
+                )
+                .replace("sha512-UPSTREAM==", "sha512-PATCHED==");
+            assert_eq!(out, &expected, "{shape}: only the rewired values may change");
+        }
+    }
+
     /// An unparseable package-lock.json must surface a warning, not silently
     /// skip the npm redirect entirely (missing-lockfile already warns; a
     /// corrupt lockfile is strictly worse).

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

@@ -149,7 +149,8 @@ pub(crate) async fn restore_npm_locks(
         let Some(text) = read_or_refuse(view, rel, &pins, &mut result).await else {
             continue;
         };
-        let Ok(mut lock) = serde_json::from_str::<Value>(&text) else {
+        // npm reads past a leading UTF-8 BOM; so do we.
+        let Ok(mut lock) = crate::vendor::common::parse_json_text(&text) else {
             refuse_all_in(&pins, rel, &mut result, format!("{rel} is not valid JSON"));
             continue;
         };
@@ -192,7 +193,8 @@ pub(crate) async fn restore_npm_locks(
             changed = true;
         }
         if changed {
-            view.write(rel, super::super::serialize_json(&lock));
+            // The lock's own BOM, indent and line endings (#324).
+            view.write(rel, super::super::serialize_json_like(&lock, &text));
         }
     }
     result

crates/socket-patch-core/src/vendor/common.rs

@@ -182,6 +182,11 @@ pub(crate) fn parse_json_manifest(bytes: &[u8]) -> serde_json::Result<Value> {
     serde_json::from_slice(bytes.strip_prefix(b"\xef\xbb\xbf").unwrap_or(bytes))
 }
 
+/// [`parse_json_manifest`] for text already decoded as UTF-8.
+pub(crate) fn parse_json_text(text: &str) -> serde_json::Result<Value> {
+    serde_json::from_str(text.strip_prefix('\u{feff}').unwrap_or(text))
+}
+
 /// The byte layout a re-serialized JSON manifest keeps from the text it
 /// replaces, so a vendor edit and its revert change nothing but the edited
 /// keys: the leading UTF-8 BOM, the indent unit ([`detect_indent`]), the

crates/socket-patch-core/src/vendor/lock_inventory/npm.rs

@@ -164,7 +164,7 @@ pub(super) async fn inventory_package_lock_in(
             break;
         }
     }
-    let doc: Value = serde_json::from_slice(&bytes?).ok()?;
+    let doc: Value = crate::vendor::common::parse_json_manifest(&bytes?).ok()?;
     // v1 legacy locks have no `packages` map — no inventory (documented).
     doc.get("packages")?.as_object()?;