Fix hosted gem block mixing CRLF Gemfile endings (#1831)

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

@@ -4769,3 +4769,93 @@ async fn gem_vendored_unwind_names_a_patched_archive_in_vendor_cache() {
         vendored_unwind_names_the_patched_cached_archive(&fx, command);
     }
 }
+
+/// #1831: a hosted redirect over a CRLF Gemfile (a Windows checkout, or
+/// `core.autocrlf=true`) keeps it CRLF throughout, for a plain top-level
+/// declaration and for one inside the user's own `source … do` block; a
+/// real bundler converges the result, a re-scan is a no-op, and `rollback`
+/// hands back a CRLF Gemfile that still declares the gem.
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17); \
+            the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"]
+async fn gem_hosted_redirect_keeps_a_crlf_gemfile_crlf() {
+    fn all_crlf(text: &str) -> bool {
+        let crlf = text.matches("\r\n").count();
+        crlf > 0 && crlf == text.matches('\n').count() && crlf == text.matches('\r').count()
+    }
+    for (tag, driver) in [
+        ("crlf-gemfile", Driver::ScanVex),
+        ("crlf-gemfile-source-block", Driver::ScanVexSourceBlock),
+    ] {
+        let Some(fx) =
+            redirect_scanned_project(tag, Spelling::Gemfile, false, true, None, driver).await
+        else {
+            return;
+        };
+        let api = fx._server.uri();
+        let gemfile_path = fx.proj.join(fx.gemfile_name);
+        let lock_path = fx.proj.join(fx.lock_name);
+        // Start over from the pristine pair, its Gemfile in CRLF.
+        let pristine = String::from_utf8(fx.pristine_gemfile.clone()).unwrap();
+        let crlf = pristine.replace("\r\n", "\n").replace('\n', "\r\n");
+        let _ = std::fs::remove_dir_all(fx.proj.join(".socket"));
+        std::fs::write(&gemfile_path, &crlf).unwrap();
+        std::fs::write(&lock_path, &fx.pristine_lock).unwrap();
+
+        let (code, out, err) = run_hosted_scan(&fx.proj, &api);
+        assert_eq!(code, 0, "{tag}: hosted scan: {out}\n{err}");
+        let gemfile = std::fs::read_to_string(&gemfile_path).unwrap();
+        assert!(
+            gemfile.contains(&fx.index_url),
+            "{tag}: the Gemfile is redirected: {gemfile:?}"
+        );
+        assert!(
+            all_crlf(&gemfile),
+            "{tag}: the redirected Gemfile mixes line endings: {gemfile:?}"
+        );
+
+        let install = bundle(&fx.proj, &["install"]);
+        assert!(
+            install.status.success(),
+            "{tag}: converging install of the CRLF Gemfile:\n{}",
+            String::from_utf8_lossy(&install.stderr)
+        );
+        let (code, out, err) = run_hosted_scan(&fx.proj, &api);
+        assert_eq!(code, 0, "{tag}: re-scan: {out}\n{err}");
+        assert_eq!(
+            std::fs::read_to_string(&gemfile_path).unwrap(),
+            gemfile,
+            "{tag}: a re-scan leaves the Gemfile byte-identical"
+        );
+
+        let upstream = format!("{api}/upstream");
+        let cwd = fx.proj.to_str().expect("utf8 tmp path");
+        let (code, stdout, stderr) = run_socket_env(
+            &fx.proj,
+            &[
+                "rollback",
+                PURL,
+                "--json",
+                "--cwd",
+                cwd,
+                "--api-url",
+                &api,
+                "--org",
+                ORG,
+                "--api-token",
+                "fake",
+                "--patch-server-url",
+                &api,
+            ],
+            &[("SOCKET_RUBYGEMS_URL", &upstream)],
+        );
+        assert_eq!(code, 0, "{tag}: rollback: {stdout}\nstderr:\n{stderr}");
+        let restored = std::fs::read_to_string(&gemfile_path).unwrap();
+        assert!(
+            !restored.contains("patch-registry")
+                && restored.contains(&format!("gem \"{DEP}\""))
+                && all_crlf(&restored),
+            "{tag}: rollback restores a CRLF Gemfile declaring the gem: {restored:?}"
+        );
+    }
+}

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

@@ -25,6 +25,7 @@ use serde_json::{json, Value};
 
 use crate::formats::yarn::berry_gates::{self, Yarnrc};
 use crate::utils::digest::is_hex64_lower;
+use crate::utils::line_endings;
 #[cfg(test)]
 use crate::utils::line_endings::LineEndings;
 use crate::vendor::common::{parse_json_text, JsonLayout};
@@ -7649,7 +7650,8 @@ pub(crate) fn gem_index_url_pattern(dep: &DepOverride, index_url: &str) -> Strin
 /// divergence the rewriter itself created. Trailing whitespace is trimmed (a
 /// block appended to a newline-less file adds a final newline the other
 /// spelling never had). `\r?` mirrors the block recognizer in `rewrite_gem`:
-/// a `core.autocrlf` checkout rewrites run 1's LF block to CRLF, and a block
+/// a CRLF manifest gets a CRLF block (#1831) and a `core.autocrlf` checkout
+/// rewrites run 1's LF block to CRLF, and a block
 /// the recognizer accepts must also be erased here or the re-run is trapped
 /// behind the divergence warning before it can reach the recognizer.
 fn gem_spelling_residue(content: &str, deps: &[&DepOverride]) -> String {
@@ -7950,10 +7952,11 @@ fn rewrite_gem(
             // Wildcard the rotating segments instead (mirrors the CHECKSUMS
             // at-target guard below).
             let url_pat = gem_index_url_pattern(dep, &ov.index_url);
-            // `\r?\n`: the rewriter emits LF, but a `core.autocrlf` checkout
-            // rewrites the working tree to CRLF — the guard must still
-            // recognize the block there, or the indented `gem` line inside
-            // it falls through to `gem_line_re` and gets wrapped again.
+            // `\r?\n`: the rewriter writes a CRLF Gemfile's block in CRLF
+            // (#1831), and a `core.autocrlf` checkout rewrites an LF one to
+            // CRLF — the guard must recognize the block either way, or the
+            // indented `gem` line inside it falls through to `gem_line_re`
+            // and gets wrapped again.
             let block_re = Regex::new(
                 &(String::from(r#"(?m)^source "("#)
                     + &url_pat
@@ -8004,8 +8007,24 @@ fn rewrite_gem(
                     let raw_tail = m
                         .get(2)
                         .expect("gem_line_re always captures group 2 (tail)")
-                        .as_str()
-                        .to_string();
+                        .as_str();
+                    // The CR of a CRLF line is its ending, not part of the
+                    // declaration: the block's own lines take that ending
+                    // and the CR goes back at its end (#1831). An
+                    // unterminated last line (a lone trailing CR included)
+                    // takes the file's.
+                    let terminated = range.end < gf.len();
+                    let cr = if terminated && raw_tail.ends_with('\r') {
+                        "\r"
+                    } else {
+                        ""
+                    };
+                    let raw_tail = raw_tail.strip_suffix('\r').unwrap_or(raw_tail).to_string();
+                    let nl = match (cr, terminated) {
+                        ("\r", _) => "\r\n",
+                        (_, true) => "\n",
+                        _ => line_endings::terminator(gf),
+                    };
                     // A parenthesized call keeps its closing `)` in the tail:
                     // strip it (dropping any comment with it), or fail closed
                     // when it is absent (the call continues past this line).
@@ -8075,12 +8094,12 @@ fn rewrite_gem(
                     let opts = gemfile::trailing_options(&tail);
                     let block = if opts.is_empty() {
                         format!(
-                            "source \"{}\" do\n  gem \"{}\", \"{}\"\nend",
+                            "source \"{}\" do{nl}  gem \"{}\", \"{}\"{nl}end",
                             ov.index_url, dep.name, dep.version
                         )
                     } else {
                         format!(
-                            "source \"{}\" do\n  gem \"{}\", \"{}\", {opts}\nend",
+                            "source \"{}\" do{nl}  gem \"{}\", \"{}\", {opts}{nl}end",
                             ov.index_url, dep.name, dep.version
                         )
                     };
@@ -8089,6 +8108,7 @@ fn rewrite_gem(
                     // duplicate earlier in the file and corrupt it (and the
                     // block may carry user text a regex replacement would
                     // `$`-expand).
+                    let block = format!("{block}{cr}");
                     gf.replace_range(range, &block);
                     gemfile_changed = true;
                     result.edits.push(FileEdit {
@@ -8131,23 +8151,24 @@ fn rewrite_gem(
                     });
                     continue;
                 } else {
-                    // Genuinely undeclared (a transitive dep): append a block.
+                    // Genuinely undeclared (a transitive dep): append a block
+                    // in the file's line endings (#1831).
+                    let eol = line_endings::terminator(gf);
                     let block = format!(
-                        "source \"{}\" do\n  gem \"{}\", \"{}\"\nend",
+                        "source \"{}\" do{eol}  gem \"{}\", \"{}\"{eol}end",
                         ov.index_url, dep.name, dep.version
                     );
                     // Always one blank line before the block: the in-place
                     // rewrite above swallows every blank line before a
                     // declaration, so this is what lets `rollback` /
                     // `remove` prove the block is this append and drop it
                     // instead of restoring a direct exact pin (#457).
-                    let eol = if gf.contains("\r\n") { "\r\n" } else { "\n" };
                     let sep = if gf.is_empty() || gf.ends_with('\n') {
                         eol.to_string()
                     } else {
                         eol.repeat(2)
                     };
-                    *gf = format!("{gf}{sep}{block}\n");
+                    *gf = format!("{gf}{sep}{block}{eol}");
                     gemfile_changed = true;
                     result.edits.push(FileEdit {
                         path: gemfile_name.into(),
@@ -18019,6 +18040,124 @@ mod tests {
         );
     }
 
+    /// #1831: the Socket source block takes the CRLF endings of the Gemfile
+    /// it is written into, for a rewritten direct declaration (plain, with
+    /// options, parenthesized, inside a user `source … do` block, on an
+    /// unterminated last line) and for the block appended for a transitive
+    /// gem. The re-run over the result is a no-op.
+    #[test]
+    fn gemfile_source_block_keeps_crlf_line_endings() {
+        let direct = gem_lock_resolving(&[("rails", "7.0.0", &[])], &["rails"]);
+        let transitive = gem_lock_resolving(
+            &[
+                ("puma", "6.0.0", &["rails (>= 0)"]),
+                ("rails", "7.0.0", &[]),
+            ],
+            &["puma"],
+        );
+        let head = "source \"https://rubygems.org\"\r\n\r\n";
+        let block = "source \"https://patch.test/gem/tok/uuid/\" do\r\n  gem \"rails\", \"7.0.0\"";
+        for (gemfile, lock, want) in [
+            (
+                format!("{head}gem \"rails\"\r\n"),
+                &direct,
+                format!("{block}\r\nend\r\n"),
+            ),
+            (
+                format!("{head}gem \"rails\", require: false # boot\r\n"),
+                &direct,
+                format!("{block}, require: false # boot\r\nend\r\n"),
+            ),
+            (
+                format!("{head}gem(\"rails\")\r\n"),
+                &direct,
+                format!("{block}\r\nend\r\n"),
+            ),
+            (
+                format!("{head}source \"https://gems.example\" do\r\n  gem \"rails\"\r\nend\r\n"),
+                &direct,
+                format!("do\r\n{block}\r\nend\r\nend\r\n"),
+            ),
+            (
+                format!("{head}gem \"rails\""),
+                &direct,
+                format!("{block}\r\nend"),
+            ),
+            // A lone CR at the end of the file is not a line ending.
+            (
+                format!("{head}gem \"rails\"\r"),
+                &direct,
+                format!("{block}\r\nend"),
+            ),
+            (
+                format!("{head}gem \"puma\"\r\n"),
+                &transitive,
+                format!("gem \"puma\"\r\n\r\n{block}\r\nend\r\n"),
+            ),
+            (
+                format!("{head}gem \"puma\""),
+                &transitive,
+                format!("gem \"puma\"\r\n\r\n{block}\r\nend\r\n"),
+            ),
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert("Gemfile".to_string(), gemfile.clone());
+            files.insert("Gemfile.lock".to_string(), lock.to_string());
+            let res = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]);
+            let out = res
+                .files
+                .get("Gemfile")
+                .unwrap_or_else(|| panic!("{gemfile:?} is redirected: {:?}", res.warnings));
+            assert_eq!(
+                LineEndings::of(out),
+                LineEndings::Crlf,
+                "{gemfile:?}: mixed line endings: {out:?}"
+            );
+            assert!(out.ends_with(&want), "{gemfile:?}: {out:?}");
+
+            files.insert("Gemfile".to_string(), out.clone());
+            files.insert(
+                "Gemfile.lock".to_string(),
+                res.files
+                    .get("Gemfile.lock")
+                    .cloned()
+                    .unwrap_or_else(|| lock.to_string()),
+            );
+            let again = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]);
+            assert!(
+                !again.files.contains_key("Gemfile"),
+                "{gemfile:?}: re-run rewrote the Gemfile: {:?}",
+                again.files.get("Gemfile")
+            );
+        }
+    }
+
+    /// #1831: an LF Gemfile still gets an LF block, and a line ending in LF
+    /// inside a mostly-CRLF file keeps its own LF (the block takes the
+    /// ending of the line it replaces).
+    #[test]
+    fn gemfile_source_block_keeps_lf_line_endings() {
+        let direct = gem_lock_resolving(&[("rails", "7.0.0", &[])], &["rails"]);
+        for (gemfile, want) in [
+            (
+                "source \"https://rubygems.org\"\n\ngem \"rails\"\n",
+                "source \"https://patch.test/gem/tok/uuid/\" do\n  gem \"rails\", \"7.0.0\"\nend\n",
+            ),
+            (
+                "source \"https://rubygems.org\"\r\n\r\ngem \"rails\"\n",
+                "source \"https://patch.test/gem/tok/uuid/\" do\n  gem \"rails\", \"7.0.0\"\nend\n",
+            ),
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert("Gemfile".to_string(), gemfile.to_string());
+            files.insert("Gemfile.lock".to_string(), direct.clone());
+            let res = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]);
+            let out = res.files.get("Gemfile").expect("redirected");
+            assert!(out.ends_with(want), "{gemfile:?}: {out:?}");
+            assert!(!out.contains("\r\r"), "{out:?}");
+        }
+    }
+
     /// A gem-level source option (`git:` / `path:` / `github:` / `source:`)
     /// preserved into the Socket source block OVERRIDES it in bundler's DSL,
     /// leaving the redirect a silent no-op that still gets attested. Fail
@@ -18762,10 +18901,12 @@ mod tests {
                 r.warnings
             );
             let out = r.files.get("Gemfile").expect("declaration rewritten");
+            // A CRLF declaration's block takes its CRLF (#1831).
+            let nl = if decl.ends_with('\r') { "\r\n" } else { "\n" };
             let want = if opts.is_empty() {
-                "  gem \"vuln-gem\", \"1.0.0\"\nend".to_string()
+                format!("  gem \"vuln-gem\", \"1.0.0\"{nl}end")
             } else {
-                format!("  gem \"vuln-gem\", \"1.0.0\", {opts}\nend")
+                format!("  gem \"vuln-gem\", \"1.0.0\", {opts}{nl}end")
             };
             assert!(out.contains(&want), "{decl:?}: {out}");
         }

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

@@ -62,7 +62,7 @@ use super::{Ctx, FormatResult, HostedPin, UpstreamClient, View};
 use crate::formats::gem::{
     bundler_manifest_for, parse_spec, same_remote, split_checksum_entry, BUNDLER_LOCKS,
 };
-use crate::utils::line_endings::{to_lf, LineEndings};
+use crate::utils::line_endings::{self, to_lf, LineEndings};
 use crate::vendor::gem::{gem_declaration_any, quoted_literal};
 
 /// The default upstream `GEM` remote.
@@ -1178,7 +1178,14 @@ fn line_before_start(text: &str, at: usize) -> usize {
 }
 
 fn restore_manifest(text: &str, block: &Block, gem: &Gem<'_>, decl: &Decl) -> String {
-    let eol = if text.contains("\r\n") { "\r\n" } else { "\n" };
+    // The restored line takes the block's ending: the rewriter writes it in
+    // the ending of the line it replaced (#1831). A block in LF, the only
+    // spelling before #1831, takes the ending of the lines around it.
+    let eol = if text[block.start..block.end].contains("\r\n") {
+        "\r\n"
+    } else {
+        line_endings::terminator(&format!("{}{}", &text[..block.start], &text[block.end..]))
+    };
     let mut start = block.start;
     let replacement = match decl {
         // The append's own blank separator goes with it.
@@ -1785,7 +1792,7 @@ mod tests {
             run(&t, &direct),
             "group :test do\n  gem \"rails\", \"7.0.0\", require: false\nend\n"
         );
-        // CRLF manifest (the rewriter wrote its block in LF).
+        // CRLF manifest, its block in LF (written before #1831).
         let t = format!("source \"https://rubygems.org\"\r\ngem \"puma\"\r\n{block}\n");
         assert_eq!(
             run(&t, &direct),