From 3a8a5abf34a55beb4445e63b385921e99eca67e5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 11:29:26 +0000 Subject: [PATCH 1/3] Start fix for #457 Assisted-by: Claude Code:claude-opus-5-5 From 89dfc9edfa9709118fc190527c190ad564f03f07 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 11:39:18 +0000 Subject: [PATCH 2/3] Fix gem rollback re-pinning transitive gems When hosted mode redirected a gem the Gemfile does not declare (a transitive dependency), it appended the patch-registry source block with no blank line before it. Rollback and remove only recognize the block as that append when a blank line precedes it, so they restored the gem as a new top-level `gem "", ""` plus an exact DEPENDENCIES pin, freezing the vulnerable version against `bundle update` (#457). The append now always leaves one blank line before the block, and the transitive restore removes that separator with the block, so the Gemfile and lock come back byte for byte. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- .../src/patch/redirect/mod.rs | 12 +- .../src/patch/redirect/upstream/client.rs | 12 + .../src/patch/redirect/upstream/gem.rs | 219 +++++++++++++++++- 4 files changed, 233 insertions(+), 12 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index bcbf0fa68..305becbfd 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/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-"` pin (the shorthand the rewriter produced collapses back); the unreferenced `[registries.socket-patch-]` 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 "" 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 "", ""`. 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 "" 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 "", ""`. 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.` version suffix, the added `` / `` 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-` 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`). diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 59d148cdf..6724dd40e 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/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 { diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/client.rs b/crates/socket-patch-core/src/patch/redirect/upstream/client.rs index bf8e07787..69702e2f2 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/client.rs +++ b/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::*; diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs b/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs index e42e183c8..a20a3cb3c 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs +++ b/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 ""` 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"); From 3aaf1dce89737fdd2dab37a259f3439d84623f4a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 12:01:35 +0000 Subject: [PATCH 3/3] Test the transitive gem append round trip The upstream-restore golden pinned the old behavior: the rewriter's own append for a transitive gem was kept as a direct exact pin on rollback. It now round-trips byte for byte; a legacy append with no blank line before it still comes back as the exact pin. Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/upstream_restore_golden.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/crates/socket-patch-core/tests/upstream_restore_golden.rs b/crates/socket-patch-core/tests/upstream_restore_golden.rs index 373e0bc04..92d37d4f7 100644 --- a/crates/socket-patch-core/tests/upstream_restore_golden.rs +++ b/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!(