From 09ac6dedf3b9b0ac1ff922173f251c2c3249181f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 17:25:19 +0000 Subject: [PATCH 1/4] Start fix for #826 Assisted-by: Claude Code:claude-opus-5-5 From 05fc3261fffaa1bcd80e8f7911763beb89c7f71f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 17:40:12 +0000 Subject: [PATCH 2/4] Keep gem declarations sharing a line with ; A Gemfile line like `gem "a", "1"; gem "b", "2"` had its second declaration deleted when socket-patch redirected or vendored gem "a", because the rewrite replaces the whole line and the safety check did not know that `;` starts a new statement. The next frozen `bundle install` then failed. Such lines are now refused with a warning and left untouched. A declaration ending in a bare `;` (optionally followed by a comment) was refused as "continues on the next line" since #637. It is complete, so it is rewritten again, without the `;`. Fixes #826 Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_redirect_gem_build.rs | 80 +++++++- .../src/patch/redirect/mod.rs | 177 +++++++++++++++++- crates/socket-patch-core/src/vendor/gem.rs | 39 ++++ 3 files changed, 290 insertions(+), 6 deletions(-) diff --git a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs index 6d590d8e4..f2337d8b8 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs @@ -440,6 +440,13 @@ enum Driver { ScanVexHeredocDeclaration, /// A double-quoted interpolation can itself contain a heredoc opener. ScanVexInterpolatedHeredocDeclaration, + /// A second declaration joined to the gem's line by `;` (#826): the + /// line rewrite would delete it. Same contract as + /// [`Driver::ScanVexDuplicateDeclaration`]. + ScanVexSemicolonJoinedDeclaration, + /// [`Driver::ScanVex`] on a declaration ending in a bare `;` and a + /// comment (#826): a complete declaration, so it is redirected. + ScanVexTrailingSemicolonDeclaration, /// [`Driver::ScanVexDualBoot`] with `BUNDLE_GEMFILE=Gemfile` exported to /// socket-patch too (#507): bundler's local app config outranks the /// environment, so bundler still loads `Gemfile.next` and the run must @@ -465,6 +472,12 @@ impl Driver { Driver::ScanVexDualBootEnvGemfile => { "scan --mode hosted (config Gemfile.next, env BUNDLE_GEMFILE=Gemfile)" } + Driver::ScanVexSemicolonJoinedDeclaration => { + "scan --mode hosted (two `;`-joined gem declarations)" + } + Driver::ScanVexTrailingSemicolonDeclaration => { + "scan --mode hosted (gem line ending in `;`)" + } } } } @@ -760,6 +773,14 @@ async fn redirect_scanned_project( "source \"{}/upstream\"\n\ngem \"{DEP}\", require: \"#{{<<~REQUIRE_PATH}}\".chomp\n vuln_gem\nREQUIRE_PATH\n", server.uri() ), + Driver::ScanVexSemicolonJoinedDeclaration => format!( + "source \"{}/upstream\"\n\ngem \"{DEP}\"; gem \"{TRANSITIVE}\"\n", + server.uri() + ), + Driver::ScanVexTrailingSemicolonDeclaration => format!( + "source \"{}/upstream\"\n\ngem \"{DEP}\"; # the vulnerable one\n", + server.uri() + ), _ => format!("source \"{}/upstream\"\n\ngem \"{DEP}\"\n", server.uri()), }; std::fs::write(proj.join(gemfile_name), gemfile_body).unwrap(); @@ -871,7 +892,9 @@ async fn redirect_scanned_project( | Driver::ScanVexConditionalDeclaration | Driver::ScanVexScopedConstantModifier | Driver::ScanVexHeredocDeclaration - | Driver::ScanVexInterpolatedHeredocDeclaration => vec![ + | Driver::ScanVexInterpolatedHeredocDeclaration + | Driver::ScanVexSemicolonJoinedDeclaration + | Driver::ScanVexTrailingSemicolonDeclaration => vec![ "scan", "--mode", "hosted", @@ -932,7 +955,8 @@ async fn redirect_scanned_project( | Driver::ScanVexConditionalDeclaration | Driver::ScanVexScopedConstantModifier | Driver::ScanVexHeredocDeclaration - | Driver::ScanVexInterpolatedHeredocDeclaration => { + | Driver::ScanVexInterpolatedHeredocDeclaration + | Driver::ScanVexSemicolonJoinedDeclaration => { Some("redirect_gem_unrecognized_declaration") } _ => None, @@ -1013,7 +1037,7 @@ async fn redirect_scanned_project( ); } match driver { - Driver::ScanVex => { + Driver::ScanVex | Driver::ScanVexTrailingSemicolonDeclaration => { assert_eq!(env["vex"]["statements"], 1, "vex block: {env}"); assert_eq!( env["vex"]["verified"], false, @@ -1028,7 +1052,8 @@ async fn redirect_scanned_project( | Driver::ScanVexConditionalDeclaration | Driver::ScanVexScopedConstantModifier | Driver::ScanVexHeredocDeclaration - | Driver::ScanVexInterpolatedHeredocDeclaration => { + | Driver::ScanVexInterpolatedHeredocDeclaration + | Driver::ScanVexSemicolonJoinedDeclaration => { unreachable!("asserted and returned above") } Driver::GetUuid => { @@ -1797,6 +1822,53 @@ async fn gem_hosted_multi_line_declaration_is_refused_and_still_installs() { assert!(fx.is_none(), "the multi-line driver asserts in place"); } +/// #826: `gem "x"; gem "y"` must not be rewritten. The line rewrite +/// deleted `gem "y"`, so the next frozen install failed. +#[tokio::test(flavor = "multi_thread")] +#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17); \ + run with a pinned toolchain via --ignored"] +async fn gem_hosted_semicolon_joined_declarations_are_refused_and_still_install() { + let fx = redirect_scanned_project( + "semicolon-joined", + Spelling::Gemfile, + false, + true, + None, + Driver::ScanVexSemicolonJoinedDeclaration, + ) + .await; + assert!(fx.is_none(), "the `;`-joined driver asserts in place"); +} + +/// #826: `gem "x"; # c` is a complete declaration. Since #637 it was +/// refused as continuing on the next line; it must redirect, and a fresh +/// checkout must install the patched bytes. +#[tokio::test(flavor = "multi_thread")] +#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17); \ + run with a pinned toolchain via --ignored"] +async fn gem_hosted_trailing_semicolon_declaration_redirects_and_installs() { + let Some(fx) = redirect_scanned_project( + "trailing-semicolon", + Spelling::Gemfile, + false, + true, + None, + Driver::ScanVexTrailingSemicolonDeclaration, + ) + .await + else { + return; + }; + let (fresh, install) = fresh_checkout_bundle_install(&fx); + assert!( + install.status.success(), + "fresh-checkout `bundle install` must succeed from the patch registry.\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&install.stdout), + String::from_utf8_lossy(&install.stderr), + ); + assert_patched_install(&fx, &fresh); +} + /// #340: a `gem` declaration with an `if` modifier must not be rewritten /// (the rewrite dropped the condition and declared the gem unconditionally). #[tokio::test(flavor = "multi_thread")] diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 8cd5e6e26..139097ee5 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -5200,12 +5200,49 @@ pub(crate) fn gem_line_trailing_options(tail: &str) -> String { Some(end) => rest = arg[1 + end + 1..].trim_start(), None => return String::new(), }, - Some(_) => return arg.trim_end().to_string(), + Some(_) => return without_statement_end(arg.trim_end()), None => return String::new(), } } } +/// `opts` minus a top-level `;` statement terminator (and any extra `;`s), +/// keeping a trailing `#` comment. [`gem_line_tail_blocks_edit`] has already +/// refused a tail where another statement follows the `;`, so only `;`s, +/// whitespace and a comment can follow it here (#826). +fn without_statement_end(opts: &str) -> String { + let mut quote: Option = None; + let mut depth: i64 = 0; + let mut chars = opts.char_indices(); + while let Some((i, c)) = chars.next() { + if let Some(q) = quote { + if c == '\\' { + chars.next(); + } else if c == q { + quote = None; + } + continue; + } + match c { + '#' => break, + '"' | '\'' => quote = Some(c), + '(' | '[' | '{' => depth += 1, + ')' | ']' | '}' => depth -= 1, + ';' if depth == 0 => { + let code = opts[..i].trim_end(); + let rest = opts[i..].trim_start_matches(|c: char| c == ';' || c.is_whitespace()); + return if rest.is_empty() { + code.to_string() + } else { + format!("{code} {rest}") + }; + } + _ => {} + } + } + opts.to_string() +} + /// Why the argument tail of a one-line `gem "name"…` declaration can't be /// rewritten in place (`None` = safe). Both Gemfile rewriters replace the /// declaration's LINE, so the tail must be the whole declaration: a `,`-led @@ -5216,7 +5253,9 @@ pub(crate) fn gem_line_trailing_options(tail: &str) -> String { /// after the rewrite, and bundler refuses the Gemfile; /// - a modifier (`if` / `unless` / `while` / `until` / `rescue` / `and` / /// `or`) or a `do` block would be dropped, silently changing when the gem -/// is declared. +/// is declared; +/// - another statement after a top-level `;` would be deleted with the line +/// (#826). A bare trailing `;` ends the declaration and is fine. /// /// Only code outside ordinary string literals and before a `#` comment /// counts, so a keyword or `,` inside `require: "…"` or a comment is fine. @@ -5249,6 +5288,18 @@ pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option { } match c { '#' => break, + // A top-level `;` ends the declaration's statement. Anything + // after it but more `;`s or a comment is another statement on + // the line the rewrite replaces, so it would be deleted (#826). + ';' if depth == 0 => { + let rest = chars + .as_str() + .trim_start_matches(|c: char| c == ';' || c.is_whitespace()); + if rest.is_empty() || rest.starts_with('#') { + break; + } + return Some("another statement follows the declaration on its line".to_string()); + } '"' | '\'' => quote = Some(c), '(' | '[' | '{' => depth += 1, ')' | ']' | '}' => depth -= 1, @@ -12982,6 +13033,128 @@ mod tests { } } + /// #826: a top-level `;` ends the declaration's statement. Another + /// statement after it (`gem "a", "1"; gem "b", "2"`) shares the line the + /// rewrite replaces, so it would be deleted: refuse. A bare trailing + /// `;` (optionally before a comment) ends nothing else, so it is a + /// complete one-line declaration, not a continuation. + #[test] + fn gem_line_tail_semicolon_statements() { + for tail in [ + ", \"0.8.1\"; gem \"rainbow\", \"3.1.1\"", + ", \"0.8.1\";gem \"rainbow\"", + ", require: false; gem \"rainbow\" # c", + ";gem \"rainbow\"", + ", \"0.8.1\"; ; puts 1", + ] { + let reason = gem_line_tail_blocks_edit(tail); + assert!( + reason + .as_deref() + .is_some_and(|r| r.contains("another statement")), + "{tail:?}: {reason:?}" + ); + } + for tail in [ + ", \"0.8.1\";", + ", \"0.8.1\"; ", + ", \"0.8.1\"; # c", + ", \"0.8.1\";; ", + ", require: false;", + ", require: \"a;b\"", + ", require: \"a\" # x; gem \"b\"", + ";", + ] { + assert_eq!(gem_line_tail_blocks_edit(tail), None, "{tail:?}"); + } + // The kept options drop the terminator but keep a comment and any + // `;` inside a string or the comment. + for (tail, opts) in [ + (", \"0.8.1\";", ""), + (", require: false;", "require: false"), + ( + ", require: false ;; # lazy; ok", + "require: false # lazy; ok", + ), + (", require: \"a;b\";", "require: \"a;b\""), + (", require: \"a;b\"", "require: \"a;b\""), + (", require: false # x;", "require: false # x;"), + ] { + assert_eq!(gem_line_trailing_options(tail), opts, "{tail:?}"); + } + } + + /// #826: the hosted rewrite replaces the whole physical line, so a + /// second `;`-joined declaration on it must refuse instead of being + /// deleted (the next `bundle install` would drop that dependency). + #[test] + fn gemfile_semicolon_joined_declarations_fail_closed() { + let lock = "GEM\n remote: https://rubygems.org/\n specs:\n rainbow (3.1.1)\n \ + vuln-gem (1.0.0)\n\nPLATFORMS\n ruby\n\nDEPENDENCIES\n rainbow (= 3.1.1)\n \ + vuln-gem (= 1.0.0)\n\nBUNDLED WITH\n 4.0.17\n"; + for decl in [ + "gem \"vuln-gem\", \"1.0.0\"; gem \"rainbow\", \"3.1.1\"", + "gem \"vuln-gem\", \"1.0.0\";gem \"rainbow\", \"3.1.1\" # pair", + "gem \"vuln-gem\", require: false; gem \"rainbow\", \"3.1.1\"", + ] { + let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n"); + let files = BTreeMap::from([ + ("Gemfile".to_string(), gemfile), + ("Gemfile.lock".to_string(), lock.to_string()), + ]); + let r = rewrite_registry_redirect(&files, &[gem_override("vuln-gem", "1.0.0")]); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "{decl:?} must not be rewritten: files={:?}", + r.files + ); + assert_eq!( + warning_codes(&r), + vec!["redirect_gem_unrecognized_declaration"], + "{decl:?}: {:?}", + r.warnings + ); + } + } + + /// #826 (the #637 regression): a declaration ending in a bare `;`, + /// with or without a trailing comment, is complete and still rewrites. + #[test] + fn gemfile_trailing_semicolon_declaration_rewrites() { + let lock = "GEM\n remote: https://rubygems.org/\n specs:\n vuln-gem (1.0.0)\n\n\ + PLATFORMS\n ruby\n\nDEPENDENCIES\n vuln-gem\n\n\ + BUNDLED WITH\n 4.0.17\n"; + for (decl, want) in [ + ( + "gem \"vuln-gem\", \"1.0.0\";", + " gem \"vuln-gem\", \"1.0.0\"\nend", + ), + ( + "gem \"vuln-gem\", \"1.0.0\"; # c", + " gem \"vuln-gem\", \"1.0.0\"\nend", + ), + ( + "gem \"vuln-gem\", require: false;", + " gem \"vuln-gem\", \"1.0.0\", require: false\nend", + ), + ] { + let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n"); + let files = BTreeMap::from([ + ("Gemfile".to_string(), gemfile), + ("Gemfile.lock".to_string(), lock.to_string()), + ]); + let r = rewrite_registry_redirect(&files, &[gem_override("vuln-gem", "1.0.0")]); + assert!( + !warning_codes(&r).contains(&"redirect_gem_unrecognized_declaration"), + "{decl:?}: {:?}", + r.warnings + ); + let out = r.files.get("Gemfile").expect("declaration rewritten"); + assert!(out.contains(want), "{decl:?}: {out}"); + assert!(!out.contains(';'), "{decl:?}: {out}"); + } + } + /// Control for #340: single-line declarations whose tails merely look /// like the refused shapes (a keyword inside a string or a comment, a /// symbol or a key named like a keyword, a closed bracket) still diff --git a/crates/socket-patch-core/src/vendor/gem.rs b/crates/socket-patch-core/src/vendor/gem.rs index fb14ceba1..25c39163a 100644 --- a/crates/socket-patch-core/src/vendor/gem.rs +++ b/crates/socket-patch-core/src/vendor/gem.rs @@ -7174,6 +7174,45 @@ mod tests { ); } + /// #826: the vendored rewrite replaces the declaration's whole line, so + /// a second `;`-joined statement on it must refuse rather than vanish; + /// a bare trailing `;` is a complete declaration and still rewrites. + #[test] + fn plan_gemfile_edit_semicolon_statements() { + let rel = copy_rel(); + for gemfile in [ + "gem \"rack\", \"~> 3.1\"; gem \"rainbow\", \"3.1.1\"\n", + "gem \"rack\", require: false;gem \"rainbow\" # pair\n", + ] { + let err = plan_gemfile_edit(gemfile, "rack", "3.2.6", &rel) + .err() + .expect("a second statement on the line must refuse"); + assert!(err.contains("another statement"), "{gemfile:?}: {err}"); + } + for (gemfile, want) in [ + ( + "gem \"rack\", \"~> 3.1\";\n", + format!("gem \"rack\", \"3.2.6\", path: \"{rel}\""), + ), + ( + "gem \"rack\", \"~> 3.1\"; # web\n", + format!("gem \"rack\", \"3.2.6\", path: \"{rel}\""), + ), + ( + "gem \"rack\", require: false;\n", + format!("gem \"rack\", \"3.2.6\", path: \"{rel}\", require: false"), + ), + ] { + match plan_gemfile_edit(gemfile, "rack", "3.2.6", &rel) { + Ok(GemfilePlan::Rewrite { new_line, .. }) => { + assert_eq!(new_line, want, "{gemfile:?}") + } + Ok(_) => panic!("{gemfile:?}: expected an in-place rewrite"), + Err(e) => panic!("{gemfile:?}: {e}"), + } + } + } + /// [`plan_gemfile_edit`]'s refusal grammar, leg by leg — a wrong Gemfile /// rewrite executes on every `bundle`, so each unsafe shape must name /// its refusal (and the `gemspec` keyword must NOT block the Append). From 8bd4c7b04177637c002910c5168492efd8964c90 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 18:03:41 +0000 Subject: [PATCH 3/4] Use the reported line shape in the ; e2e test The `;`-joined fixture had no version argument, so the old check already refused it as "unexpected tokens" and the test passed without the fix. Use `gem "x", "v"; gem "y", "v"` from #826, which the old code rewrote and lost the second gem. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs index f2337d8b8..63f812be1 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs @@ -774,7 +774,7 @@ async fn redirect_scanned_project( server.uri() ), Driver::ScanVexSemicolonJoinedDeclaration => format!( - "source \"{}/upstream\"\n\ngem \"{DEP}\"; gem \"{TRANSITIVE}\"\n", + "source \"{}/upstream\"\n\ngem \"{DEP}\", \"{DEP_VERSION}\"; gem \"{TRANSITIVE}\", \"1.0.0\"\n", server.uri() ), Driver::ScanVexTrailingSemicolonDeclaration => format!( From 5b9953d336d5e7ff2dd9ba5f0813ab5952f7bf67 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 19:00:08 +0000 Subject: [PATCH 4/4] Port #878: route Gradle digests through helpers main is red: #646 added inline sha1/sha256 calls that #865's production_digests_go_through_the_helpers guard rejects. This ports the fix from #878 so this PR's coverage job can go green. It becomes a no-op once #878 lands. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/crawlers/gradle_cache.rs | 9 ++++----- crates/socket-patch-core/src/patch/jvm_jar.rs | 7 ++----- crates/socket-patch-core/src/patch/sidecars/maven.rs | 4 +--- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/crates/socket-patch-core/src/crawlers/gradle_cache.rs b/crates/socket-patch-core/src/crawlers/gradle_cache.rs index ef295ee27..afd7c4fba 100644 --- a/crates/socket-patch-core/src/crawlers/gradle_cache.rs +++ b/crates/socket-patch-core/src/crawlers/gradle_cache.rs @@ -70,8 +70,7 @@ pub fn hash_eq(dir_name: &str, sha1_hex: &str) -> bool { /// Whether `bytes` are the pristine download Gradle stored in the hash /// directory `dir_name` (their sha1 names it). pub fn pristine(dir_name: &str, bytes: &[u8]) -> bool { - use sha1::{Digest, Sha1}; - hash_eq(dir_name, &hex::encode(Sha1::digest(bytes))) + hash_eq(dir_name, &crate::utils::digest::sha1_hex_of(bytes)) } /// Whether `path` is a version directory of a `files-2.1` tree @@ -432,8 +431,6 @@ impl DerivedIndex { /// The [`DerivedCopies`] of the jar `jar_leaf` whose pristine bytes /// hash to `pristine_sha1`. pub fn query(&self, jar_leaf: &str, pristine_sha1: &str) -> DerivedCopies { - use sha1::{Digest, Sha1}; - let instrumented = format!("instrumented-{jar_leaf}"); let mut out = DerivedCopies { incomplete: self.incomplete, @@ -460,7 +457,9 @@ impl DerivedIndex { out.stale.push(path.clone()); } else if name == jar_leaf || name == instrumented { match crate::utils::fs::read_regular_to_bytes_sync(path) { - Ok(bytes) if hash_eq(&hex::encode(Sha1::digest(&bytes)), pristine_sha1) => { + Ok(bytes) + if hash_eq(&crate::utils::digest::sha1_hex_of(&bytes), pristine_sha1) => + { out.stale.push(path.clone()) } Ok(_) => out.unknown.push(path.clone()), diff --git a/crates/socket-patch-core/src/patch/jvm_jar.rs b/crates/socket-patch-core/src/patch/jvm_jar.rs index 82d679406..f38a84403 100644 --- a/crates/socket-patch-core/src/patch/jvm_jar.rs +++ b/crates/socket-patch-core/src/patch/jvm_jar.rs @@ -25,8 +25,6 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use sha1::Digest as _; - use crate::crawlers::gradle_cache; use crate::hash::git_sha256::compute_git_sha256_from_bytes; use crate::manifest::schema::PatchFileInfo; @@ -353,12 +351,11 @@ fn unpatched_members( } fn sha256_hex(bytes: &[u8]) -> String { - use sha2::Digest as _; - hex::encode(sha2::Sha256::digest(bytes)) + crate::utils::digest::sha256_hex_of(bytes) } fn sha1_hex(bytes: &[u8]) -> String { - hex::encode(sha1::Sha1::digest(bytes)) + crate::utils::digest::sha1_hex_of(bytes) } /// `/jvm-originals/.jar`. diff --git a/crates/socket-patch-core/src/patch/sidecars/maven.rs b/crates/socket-patch-core/src/patch/sidecars/maven.rs index f2f5a2466..8798bfce6 100644 --- a/crates/socket-patch-core/src/patch/sidecars/maven.rs +++ b/crates/socket-patch-core/src/patch/sidecars/maven.rs @@ -17,8 +17,6 @@ use std::path::{Path, PathBuf}; -use sha1::Digest as _; - use super::{ SidecarAdvisory, SidecarAdvisoryCode, SidecarError, SidecarFile, SidecarFileAction, SidecarPayload, SidecarSeverity, @@ -44,7 +42,7 @@ impl Algo { fn digest(self, bytes: &[u8]) -> String { match self { - Algo::Sha1 => hex::encode(sha1::Sha1::digest(bytes)), + Algo::Sha1 => crate::utils::digest::sha1_hex_of(bytes), Algo::Md5 => hex::encode(md5(bytes)), } }