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..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 @@ -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 diff --git a/crates/socket-patch-core/src/patch/redirect/npmrc.rs b/crates/socket-patch-core/src/patch/redirect/npmrc.rs index abf9a024c..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,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