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

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()?;