From 63148f8dd31d468f14e5241484013eb377cf3384 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 04:30:05 +0000 Subject: [PATCH 1/3] Start fix for #1196 Assisted-by: Claude Code:claude-opus-5-5 From b9c4d8537170474534ff0401d7dd567a72a493ab Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 04:36:32 +0000 Subject: [PATCH 2/3] Keep CRLF when .npmrc has no final newline A hosted npm scan that appended allow-remote=all to a CRLF .npmrc whose last line had no terminator wrote ...\nallow-remote=all\r: the old last line lost its CR and the file ended in a lone CR, a shape socket-patch itself refuses to edit later. The old last line now gets the file's terminator and the new line keeps the no-final-newline shape. LF files are unchanged. Fixes #1196 Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/vendor/redirect_npm_allow_remote.rs | 20 ++++++++++ .../src/patch/redirect/npmrc.rs | 39 +++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs b/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs index 86daddc96..f760629c1 100644 --- a/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs +++ b/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs @@ -379,6 +379,26 @@ 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:#}"); + assert_eq!( + std::fs::read_to_string(tmp.path().join(".npmrc")).unwrap(), + "fund=false\r\naudit=false\r\nallow-remote=all" + ); +} + /// 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 diff --git a/crates/socket-patch-core/src/patch/redirect/npmrc.rs b/crates/socket-patch-core/src/patch/redirect/npmrc.rs index abf9a024c..a8e9d3f91 100644 --- a/crates/socket-patch-core/src/patch/redirect/npmrc.rs +++ b/crates/socket-patch-core/src/patch/redirect/npmrc.rs @@ -778,6 +778,14 @@ pub fn plan_npmrc_allow_remote_with(existing: Option<&str>, outer: &OuterAllowRe .iter() .rposition(|l| !l.trim().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, + // rather than letting the join give it a bare LF and leave the + // inserted line's CR dangling at end of file. + let term = if crlf { "\r\n" } else { "\n" }; + return NpmrcPlan::Append(format!("{bom}{body}{term}{NPMRC_ALLOW_REMOTE_LINE}")); + } lines.insert(anchor, &line); NpmrcPlan::Append(format!("{bom}{}", lines.join("\n"))) } @@ -970,6 +978,37 @@ 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"), + ] { + 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 From 758f53aa545c62419f2c61e6f610e3fd5be3585f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 04:59:18 +0000 Subject: [PATCH 3/3] Tidy .npmrc splice and check the re-run Derive the spliced line's ending from one terminator, skip BOM-only lines when choosing where the line goes (npm reads them as blank), update the docs for the terminated last line, and have the CLI test re-run scan to show the written file is read back, not refused. Refs #1196 Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/vendor/redirect_npm_allow_remote.rs | 15 +++++++++- .../src/patch/redirect/npmrc.rs | 30 ++++++++++--------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs b/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs index f760629c1..88d461484 100644 --- a/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs +++ b/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs @@ -393,9 +393,22 @@ async fn crlf_npmrc_without_final_newline_keeps_crlf_on_append() { 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(), - "fund=false\r\naudit=false\r\nallow-remote=all" + 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 ); } diff --git a/crates/socket-patch-core/src/patch/redirect/npmrc.rs b/crates/socket-patch-core/src/patch/redirect/npmrc.rs index a8e9d3f91..e082375f7 100644 --- a/crates/socket-patch-core/src/patch/redirect/npmrc.rs +++ b/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,26 +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, - // rather than letting the join give it a bare LF and leave the - // inserted line's CR dangling at end of file. - let term = if crlf { "\r\n" } else { "\n" }; + // 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"))) } @@ -998,6 +997,9 @@ mod tests { "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:?}");