Fix CRLF .npmrc append with no final newline (#1196)

crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs

@@ -379,6 +379,39 @@ async fn package_lock_redirect_writes_npmrc_warns_and_rollback_removes_it() {
     );
 }
 
+/// A CRLF `.npmrc` whose last line has no terminator (a Windows editor
+/// save) gets that line terminated in CRLF before the appended line,
+/// never `…\nallow-remote=all\r`: a bare LF plus a lone CR at end of
+/// file that the planner itself refuses on a later run (#1196).
+#[tokio::test]
+async fn crlf_npmrc_without_final_newline_keeps_crlf_on_append() {
+    let server = MockServer::start().await;
+    mock_api(&server).await;
+
+    let tmp = tempfile::tempdir().unwrap();
+    write_npm_project(tmp.path(), "package-lock.json");
+    std::fs::write(tmp.path().join(".npmrc"), "fund=false\r\naudit=false").unwrap();
+    let (code, doc, _) = scan_hosted(tmp.path(), &server.uri(), &["--json"]);
+    assert_eq!(code, 0, "{doc:#}");
+    let want = "fund=false\r\naudit=false\r\nallow-remote=all";
+    assert_eq!(
+        std::fs::read_to_string(tmp.path().join(".npmrc")).unwrap(),
+        want
+    );
+
+    // A re-run reads the line it wrote: no CR-only refusal, no rewrite.
+    let (code, doc, _) = scan_hosted(tmp.path(), &server.uri(), &["--json"]);
+    assert_eq!(code, 0, "{doc:#}");
+    assert!(
+        !doc.to_string().contains("carriage-return"),
+        "the re-run must not refuse the file it wrote: {doc:#}"
+    );
+    assert_eq!(
+        std::fs::read_to_string(tmp.path().join(".npmrc")).unwrap(),
+        want
+    );
+}
+
 /// An existing `.npmrc` (BOM + CRLF, no allow-remote) gets exactly one
 /// appended line in its own line ending. With no ledger recording the
 /// append, rollback never edits a file the user owns: the `.npmrc` (and a

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

@@ -238,7 +238,8 @@ pub enum NpmrcPlan {
     /// full new text, with exactly one [`NPMRC_ALLOW_REMOTE_LINE`] line
     /// spliced in (after the last non-empty top-level line — before any
     /// `[section]` header — in the file's own line ending); every other byte
-    /// (BOM, CRLF, trailing-newline shape) preserved.
+    /// (BOM, CRLF, trailing-newline shape) preserved, except that an
+    /// unterminated last line it follows gains the file's terminator.
     Append(String),
     /// Already resolves to `allow-remote=all` — nothing to write.
     AlreadyAll,
@@ -720,8 +721,9 @@ pub fn replace_registry_host_rewrites(value: &str, host: &str) -> bool {
 
 /// Decide how to ensure `allow-remote=all` in the project `.npmrc`,
 /// considering only the project file (no outer npm config layers). Line
-/// splices only: untouched lines stay byte-identical, so an unwind can
-/// remove exactly what was added.
+/// splices only: untouched lines stay byte-identical, except that an
+/// unterminated last line the new line follows gains the file's
+/// terminator (#1196).
 pub fn plan_npmrc_allow_remote(existing: Option<&str>) -> NpmrcPlan {
     plan_npmrc_allow_remote_with(existing, &OuterAllowRemote::default())
 }
@@ -766,18 +768,23 @@ pub fn plan_npmrc_allow_remote_with(existing: Option<&str>, outer: &OuterAllowRe
         );
     }
     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")
-    } else {
-        NPMRC_ALLOW_REMOTE_LINE.to_string()
-    };
+    let term = crate::utils::line_endings::terminator(body);
     let mut lines: Vec<&str> = body.split('\n').collect();
     let end = top_level_end(bom, &lines);
     let anchor = lines[..end]
         .iter()
-        .rposition(|l| !l.trim().is_empty())
+        .rposition(|l| !js_trim(l).is_empty())
         .map_or(0, |i| i + 1);
+    if anchor == lines.len() {
+        // The anchor follows an unterminated last line (#1196): terminate
+        // it in the file's style and keep the no-final-newline shape. The
+        // splice below would give it the join's bare LF and leave a CRLF
+        // file ending in a lone CR.
+        return NpmrcPlan::Append(format!("{bom}{body}{term}{NPMRC_ALLOW_REMOTE_LINE}"));
+    }
+    // Every line before the anchor is followed by a `\n` from the join,
+    // so the spliced line carries only the CR half of a CRLF terminator.
+    let line = format!("{NPMRC_ALLOW_REMOTE_LINE}{}", term.trim_end_matches('\n'));
     lines.insert(anchor, &line);
     NpmrcPlan::Append(format!("{bom}{}", lines.join("\n")))
 }
@@ -970,6 +977,40 @@ mod tests {
         }
     }
 
+    /// A last line with no terminator gets the file's terminator before
+    /// the appended line, which keeps the no-final-newline shape (#1196).
+    /// The old splice gave CRLF files `…\nallow-remote=all\r`: a bare LF
+    /// on the old last line and a lone CR at end of file.
+    #[test]
+    fn append_after_an_unterminated_last_line_keeps_its_terminator() {
+        for (existing, want) in [
+            (
+                "fund=false\r\naudit=false",
+                "fund=false\r\naudit=false\r\nallow-remote=all",
+            ),
+            (
+                "\u{feff}fund=false\r\naudit=false",
+                "\u{feff}fund=false\r\naudit=false\r\nallow-remote=all",
+            ),
+            (
+                "fund=false\naudit=false",
+                "fund=false\naudit=false\nallow-remote=all",
+            ),
+            ("a=1", "a=1\nallow-remote=all"),
+            // A BOM-only last line is blank to npm, so the line goes
+            // after `a=1`, not after the stray BOM.
+            ("a=1\r\n\u{feff}", "a=1\r\nallow-remote=all\r\n\u{feff}"),
+        ] {
+            let NpmrcPlan::Append(text) = plan_npmrc_allow_remote(Some(existing)) else {
+                panic!("append expected for {existing:?}");
+            };
+            assert_eq!(text, want, "{existing:?}");
+            assert!(!has_lone_cr(&text), "{text:?}");
+            // The result is stable: a re-plan sees the line it wrote.
+            assert_eq!(plan_npmrc_allow_remote(Some(&text)), NpmrcPlan::AlreadyAll);
+        }
+    }
+
     fn cfg_env(vars: &[(&str, &str)]) -> NpmConfigEnv {
         NpmConfigEnv {
             vars: vars