Fix gem rollback pinning redirected transitive gems (#457)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -845,7 +845,7 @@ v5.0 replaces v4's per-purl reverts and whole-ledger reverse replay (`revert_rem
   * **cargo** — `Cargo.lock` back on crates.io (source + the sparse index's checksum, `SOCKET_CRATES_INDEX`); every `Cargo.toml` declaration loses its `registry = "socket-patch-<uuid>"` pin (the shorthand the rewriter produced collapses back); the unreferenced `[registries.socket-patch-<uuid>]` block leaves the project cargo config. A declaration it cannot unpin refuses.
   * **golang** — the hosted `replace` and the socket module's go.sum lines go; the upstream module's two go.sum lines come back, hashed from the module proxy (`SOCKET_GOPROXY`, else `GOPROXY` / `GONOPROXY` / `GOPRIVATE` as go reads them) and cross-checked against the checksum database (`SOCKET_GOSUMDB_URL`, else `sum.golang.org` unless `GOSUMDB=off` / `GONOSUMDB` / `GOPRIVATE` say go would not ask it). A `replace` the user had before the hosted run is not recorded anywhere, so the restore lands on the plain upstream module.
   * **pypi** — `Pipfile.lock`, `requirements.txt` (+ in-root `-r` includes), Hatch PEP 508 direct references (`pyproject.toml` / `hatch.toml`), `poetry.lock`, `pdm.lock`, `uv.lock`, PEP 723 script locks and PEP 751 `pylock*.toml` (+ the paired `pyproject.toml` / script metadata): hashes re-derived from PyPI's JSON API (`SOCKET_PYPI_JSON_API`). Refused: a `pdm.lock` without `cross_platform`, or a uv / script / pylock lock, whose release has a wheel that is not pure Python 3 (which files the lock keeps is not re-derivable); a uv lock whose options filter files (`exclude-newer`, `no-binary`, `no-build`), or whose other registry packages name no registry, several, or one other than PyPI's simple index; uv 0.2 `[[distribution]]` locks. A transitive `override-dependencies` entry hosted mode added is removed (`upstream_uv_override_removed`).
-  * **gem** — `Gemfile.lock` / `gems.locked` + `Gemfile` / `gems.rb`: the spec moves back into the upstream `GEM` section (or the Socket remote leaves a merged section), the `source "<patch registry>" do … end` block is undone, the `CHECKSUMS` entry is re-pinned from the rubygems.org compact index (`SOCKET_RUBYGEMS_URL`) and the `DEPENDENCIES` pin loses its `!`. The declaration's original constraint is not recorded, so it comes back as the exact pin `gem "<name>", "<version>"`. Refused: an ambiguous upstream section, an upstream remote other than rubygems.org.
+  * **gem** — `Gemfile.lock` / `gems.locked` + `Gemfile` / `gems.rb`: the spec moves back into the upstream `GEM` section (or the Socket remote leaves a merged section), the `source "<patch registry>" do … end` block is undone, the `CHECKSUMS` entry is re-pinned from the rubygems.org compact index (`SOCKET_RUBYGEMS_URL`) and the `DEPENDENCIES` pin loses its `!`. The declaration's original constraint is not recorded, so it comes back as the exact pin `gem "<name>", "<version>"`. A transitive gem (one the manifest never declared) gets an appended block with a blank line before it; the restore removes that block, its blank line and the `DEPENDENCIES` entry, so the pair comes back byte for byte. An appended block with no blank line before it (written by a release before this one) can't be told apart from an in-place rewrite, so it still comes back as the exact pin. Refused: an ambiguous upstream section, an upstream remote other than rubygems.org.
   * **composer** — `composer.lock`: `dist` and the deleted `source` block from packagist's composer v2 metadata (`SOCKET_PACKAGIST_URL`). Refused unless the entry is packagist-sourced and packagist still serves the lock's `dist.reference` for the version.
   * **maven** — `pom.xml` (the `-socket.<hex8>` version suffix, the added `<repository>` / `<dependencyManagement>` entry) and the `.mvn/maven.config` / `.mvn/checksums/checksums.sha256` lines hosted mode writes: **no network**, so it restores under `--offline` too. `.mvn` files holding anything else keep the resolver lines (`maven_trusted_checksums_left`).
   * **nuget** — `nuget.config` loses the `socket-patch-<uuid>` source and its exact-id mapping; every `packages.lock.json` entry of the id gets nuget.org's `contentHash` back (`SOCKET_NUGET_URL`). Refused when the restored config would not resolve the id from nuget.org alone. A config hosted mode created from scratch is kept (`nuget_default_config_left`).

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

@@ -5067,7 +5067,17 @@ fn rewrite_gem(
                         "source \"{}\" do\n  gem \"{}\", \"{}\"\nend",
                         ov.index_url, dep.name, dep.version
                     );
-                    let sep = if gf.ends_with('\n') { "" } else { "\n" };
+                    // 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");
                     gemfile_changed = true;
                     result.edits.push(FileEdit {

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

@@ -667,6 +667,18 @@ pub(crate) fn go_mod_h1(go_mod: &[u8]) -> String {
     )
 }
 
+#[cfg(test)]
+impl UpstreamClient {
+    /// Answer `rubygems_sha256(name, version)` with `sha` without a request
+    /// (an offline client otherwise refuses every lookup).
+    pub(crate) async fn seed_rubygems_sha256(&self, name: &str, version: &str, sha: &str) {
+        self.rubygems
+            .lock()
+            .await
+            .insert((name.to_string(), version.to_string()), Ok(sha.to_string()));
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

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

@@ -43,8 +43,9 @@
 //!   the manifest's last line produces the same bytes. The block and entry
 //!   are removed only when that is provable: an option-less block the
 //!   rewriter could not have written in place (a blank line before it —
-//!   the in-place match swallows every blank line before the declaration)
-//!   that another locked spec depends on. Otherwise the gem is kept as a
+//!   the in-place match swallows every blank line before the declaration,
+//!   and the append always writes one; it goes with the block) that
+//!   another locked spec depends on. Otherwise the gem is kept as a
 //!   direct pin: a stray exact pin installs the same bytes, while dropping
 //!   a real declaration would stop `Bundler.require` loading the gem.
 //! * a `CHECKSUMS` entry the rewriter ADDED is only recognizable next to
@@ -521,10 +522,26 @@ enum Decl {
     Direct(String),
 }
 
+/// Byte offset of the start of the line before byte `at` (`at` itself at
+/// the start of the text).
+fn line_before_start(text: &str, at: usize) -> usize {
+    match text[..at].strip_suffix('\n') {
+        Some(before) => before.rfind('\n').map_or(0, |i| i + 1),
+        None => at,
+    }
+}
+
 fn restore_manifest(text: &str, block: &Block, gem: &Gem<'_>, decl: &Decl) -> String {
     let eol = if text.contains("\r\n") { "\r\n" } else { "\n" };
+    let mut start = block.start;
     let replacement = match decl {
-        Decl::Transitive => String::new(),
+        // The append's own blank separator goes with it.
+        Decl::Transitive => {
+            if provably_appended(text, block) {
+                start = line_before_start(text, block.start);
+            }
+            String::new()
+        }
         Decl::Direct(args) => {
             let opts = block
                 .opts
@@ -543,11 +560,7 @@ fn restore_manifest(text: &str, block: &Block, gem: &Gem<'_>, decl: &Decl) -> St
             )
         }
     };
-    format!(
-        "{}{replacement}{}",
-        &text[..block.start],
-        &text[block.end..]
-    )
+    format!("{}{replacement}{}", &text[..start], &text[block.end..])
 }
 
 /// The manifest's global `source "<url>"` declarations (no block).
@@ -898,12 +911,17 @@ mod tests {
             run(&t, &direct),
             "source \"https://rubygems.org\"\r\ngem \"puma\"\r\ngem \"rails\", \"7.0.0\"\r\n"
         );
-        // Transitive append removed; mixed-state constraint kept.
+        // Transitive append removed with the blank line the rewriter puts
+        // before it; mixed-state constraint kept.
         let t = format!("source \"https://rubygems.org\"\n\ngem \"puma\"\n\n{block}\n");
         assert_eq!(
             run(&t, &Decl::Transitive),
-            "source \"https://rubygems.org\"\n\ngem \"puma\"\n\n"
+            "source \"https://rubygems.org\"\n\ngem \"puma\"\n"
         );
+        // An append from before that separator existed (no blank line):
+        // only the block goes.
+        let t = format!("gem \"puma\"\n{block}\n");
+        assert_eq!(run(&t, &Decl::Transitive), "gem \"puma\"\n");
         let t = format!("gem \"puma\"\n{block}\n");
         assert_eq!(
             run(&t, &Decl::Direct(constraint_args("rails (>= 6, ~> 7.0)"))),
@@ -946,6 +964,187 @@ mod tests {
         assert!(!is_subdependency(&["    rails (7.0.0)"], "rails"));
     }
 
+    /// #457: what the hosted rewriter itself writes for a TRANSITIVE gem
+    /// (an appended block, converged CHECKSUMS lock) must be recognized as
+    /// its append, and undoing it must give the manifest back byte for
+    /// byte, whatever its trailing newlines. A hand-built fixture is not
+    /// enough: the rewriter used to append with no blank line before the
+    /// block, so rollback re-added the gem as a direct exact pin.
+    #[test]
+    fn rewriter_transitive_append_restores_byte_identically() {
+        use crate::patch::redirect::{
+            rewrite_registry_redirect, DepOverride, Integrity, RegistryOverride,
+            RegistryOverrideIdentifiers,
+        };
+        let client = super::super::UpstreamClient::new(true);
+        let ctx = ctx_with(&client);
+        let ov = DepOverride {
+            ecosystem: "gem".into(),
+            name: "rails".into(),
+            namespace: None,
+            version: "7.0.0".into(),
+            token: "tok".into(),
+            patch_uuid: UUID.into(),
+            artifact_url: "https://patch.test/rails-7.0.0.gem".into(),
+            registry_override: Some(RegistryOverride {
+                kind: "rubygems-compact-index".into(),
+                index_url: IDX.into(),
+                identifiers: RegistryOverrideIdentifiers {
+                    name: "rails".into(),
+                    version: "7.0.0".into(),
+                    gem_checksum_sha256: Some("f".repeat(64)),
+                    ..Default::default()
+                },
+            }),
+            integrity: Integrity::default(),
+        };
+        let lock = format!(
+            "GEM\n  remote: https://rubygems.org/\n  specs:\n    puma (6.0.0)\n      rails (>= 7)\n    \
+             rails (7.0.0)\n\nPLATFORMS\n  ruby\n\nDEPENDENCIES\n  puma\n\nCHECKSUMS\n  puma (6.0.0) \
+             sha256={}\n  rails (7.0.0) sha256={}\n\nBUNDLED WITH\n   4.0.17\n",
+            "a".repeat(64),
+            "2".repeat(64)
+        );
+        let lf = "source \"https://rubygems.org\"\n\ngem \"puma\"\n";
+        let crlf = lf.replace('\n', "\r\n");
+        let unterminated = lf.trim_end_matches('\n');
+        for (original, restored) in [
+            // The issue's shape: the manifest ends with a declaration + LF.
+            (lf.to_string(), lf.to_string()),
+            // A trailing blank line (the issue's passing control).
+            (format!("{lf}\n"), format!("{lf}\n")),
+            (crlf.clone(), crlf.clone()),
+            // No final line break: it comes back with one.
+            (unterminated.to_string(), lf.to_string()),
+        ] {
+            let files = BTreeMap::from([
+                ("Gemfile".to_string(), original.clone()),
+                ("Gemfile.lock".to_string(), lock.clone()),
+            ]);
+            let r = rewrite_registry_redirect(&files, std::slice::from_ref(&ov));
+            let gemfile = &r.files["Gemfile"];
+            let lock_out = &r.files["Gemfile.lock"];
+            assert!(
+                lock_out.contains("  rails (= 7.0.0)!"),
+                "converged lock: {lock_out}"
+            );
+            let b = find_block(gemfile, &gem(), "Gemfile", &ctx)
+                .unwrap()
+                .expect("the appended block is found");
+            assert!(b.opts.is_none());
+            assert!(
+                provably_appended(gemfile, &b),
+                "the rewriter's append must be provably an append: {gemfile:?}"
+            );
+            let lines: Vec<&str> = lock_out.split('\n').collect();
+            assert!(is_subdependency(&lines, "rails"));
+            assert_eq!(
+                restore_manifest(gemfile, &b, &gem(), &Decl::Transitive),
+                restored,
+                "from {gemfile:?}"
+            );
+        }
+
+        // Two transitive appends (two patches) unwind in either order.
+        const RACK_UUID: &str = "88888888-8888-8888-8888-888888888888";
+        let rack_ov = DepOverride {
+            name: "rack".into(),
+            version: "3.0.0".into(),
+            patch_uuid: RACK_UUID.into(),
+            registry_override: ov.registry_override.clone().map(|mut ro| {
+                ro.index_url = IDX.replace(UUID, RACK_UUID);
+                ro.identifiers.name = "rack".into();
+                ro.identifiers.version = "3.0.0".into();
+                ro
+            }),
+            ..ov.clone()
+        };
+        let files = BTreeMap::from([("Gemfile".to_string(), lf.to_string())]);
+        let gemfile =
+            rewrite_registry_redirect(&files, &[ov.clone(), rack_ov]).files["Gemfile"].clone();
+        let rack = || Gem {
+            uuid: RACK_UUID,
+            name: "rack".into(),
+            version: "3.0.0".into(),
+        };
+        for (first, second) in [(gem(), rack()), (rack(), gem())] {
+            let b = find_block(&gemfile, &first, "Gemfile", &ctx)
+                .unwrap()
+                .unwrap();
+            assert!(provably_appended(&gemfile, &b), "{gemfile:?}");
+            let half = restore_manifest(&gemfile, &b, &first, &Decl::Transitive);
+            let b = find_block(&half, &second, "Gemfile", &ctx)
+                .unwrap()
+                .unwrap();
+            assert!(provably_appended(&half, &b), "{half:?}");
+            assert_eq!(restore_manifest(&half, &b, &second, &Decl::Transitive), lf);
+        }
+    }
+
+    /// #457, the whole restore: `scan --mode hosted` on a converged
+    /// (CHECKSUMS) lock redirects a transitive gem, and `rollback` /
+    /// `remove` must give the pair back byte for byte: no new `gem` line in
+    /// the manifest, no `(= version)` DEPENDENCIES pin in the lock (which
+    /// would freeze the vulnerable version against `bundle update`).
+    #[tokio::test]
+    async fn transitive_redirect_round_trips_through_restore() {
+        let client = super::super::UpstreamClient::new(true);
+        let upstream_sha = "2".repeat(64);
+        client
+            .seed_rubygems_sha256("rails", "7.0.0", &upstream_sha)
+            .await;
+        let ctx = ctx_with(&client);
+        let manifest = "source \"https://rubygems.org\"\n\ngem \"puma\", \"6.0.0\"\n";
+        let lock = format!(
+            "GEM\n  remote: https://rubygems.org/\n  specs:\n    puma (6.0.0)\n      rails (>= 7)\n    \
+             rails (7.0.0)\n\nPLATFORMS\n  ruby\n\nDEPENDENCIES\n  puma (= 6.0.0)\n\nCHECKSUMS\n  \
+             puma (6.0.0) sha256={}\n  rails (7.0.0) sha256={upstream_sha}\n\nBUNDLED WITH\n   4.0.17\n",
+            "a".repeat(64)
+        );
+        let files = BTreeMap::from([
+            ("Gemfile".to_string(), manifest.to_string()),
+            ("Gemfile.lock".to_string(), lock.clone()),
+        ]);
+        let r = crate::patch::redirect::rewrite_registry_redirect(
+            &files,
+            &[crate::patch::redirect::DepOverride {
+                ecosystem: "gem".into(),
+                name: "rails".into(),
+                namespace: None,
+                version: "7.0.0".into(),
+                token: "tok".into(),
+                patch_uuid: UUID.into(),
+                artifact_url: "https://patch.test/rails-7.0.0.gem".into(),
+                registry_override: Some(crate::patch::redirect::RegistryOverride {
+                    kind: "rubygems-compact-index".into(),
+                    index_url: IDX.into(),
+                    identifiers: crate::patch::redirect::RegistryOverrideIdentifiers {
+                        name: "rails".into(),
+                        version: "7.0.0".into(),
+                        gem_checksum_sha256: Some("f".repeat(64)),
+                        ..Default::default()
+                    },
+                }),
+                integrity: Default::default(),
+            }],
+        );
+        let (hosted_manifest, hosted_lock) = (&r.files["Gemfile"], &r.files["Gemfile.lock"]);
+        assert!(hosted_lock.contains("  rails (= 7.0.0)!"), "{hosted_lock}");
+        let (next_lock, next_manifest) = restore_one(
+            &gem(),
+            Some(hosted_lock),
+            Some(hosted_manifest),
+            "Gemfile",
+            &global_sources(hosted_manifest),
+            &ctx,
+        )
+        .await
+        .unwrap()
+        .expect("the redirect is found");
+        assert_eq!(next_manifest.as_deref(), Some(manifest));
+        assert_eq!(next_lock.as_deref(), Some(lock.as_str()));
+    }
+
     #[test]
     fn global_sources_skip_blocks() {
         let m = format!("source 'https://rubygems.org'\nsource(\"https://b.example\")\nsource \"{IDX}\" do\nend\n");

crates/socket-patch-core/tests/upstream_restore_golden.rs

@@ -620,17 +620,28 @@ async fn gem_pre_checksums_states() {
 
 #[tokio::test]
 #[serial]
-async fn gem_transitive_without_proof_stays_declared() {
-    // The rewriter's append for a transitive gem is indistinguishable from a
-    // direct last-line declaration, so it is kept as a direct exact pin.
+async fn gem_transitive_append_round_trips_unless_unprovable() {
+    // #457: on a Gemfile ending in a declaration line, the rewriter's own
+    // append for a transitive gem is undone byte for byte: no new `gem`
+    // line, no `(= version)` DEPENDENCIES pin freezing the version.
     let gemfile = "source \"https://rubygems.org\"\n\ngem \"puma\"\n";
     let lock = transitive_lock().replace("  rails (= 7.0.0)\n", "");
     let case = synthetic(
-        "transitive-ambiguous",
+        "transitive-appended",
         &[("Gemfile", gemfile), ("Gemfile.lock", &lock)],
         gem_override("zeitwerk", "2.6.0"),
     );
     let (after, statuses) = gem_run(&case).await;
+    assert_round_trip(&case, &after, &statuses);
+
+    // An append with no blank line before it (what releases before #457
+    // wrote) is indistinguishable from a direct last-line declaration, so
+    // it is kept as a direct exact pin.
+    let mut case = case.clone_with("transitive-ambiguous");
+    let legacy = case.expected["Gemfile"].replace("\n\nsource", "\nsource");
+    assert_ne!(legacy, case.expected["Gemfile"]);
+    case.expected.insert("Gemfile".into(), legacy);
+    let (after, statuses) = gem_run(&case).await;
     assert_eq!(statuses[0].1, PinStatus::Restored);
     assert_eq!(after["Gemfile"], format!("{gemfile}gem \"zeitwerk\", \"2.6.0\"\n"));
     assert_eq!(