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 bd737a2ca..50fe32f06 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 @@ -471,6 +478,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 `;`)" + } Driver::ScanVexCustomGitSource => "scan --mode hosted (gem from a custom git_source)", } } @@ -809,6 +822,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}\", \"{DEP_VERSION}\"; gem \"{TRANSITIVE}\", \"1.0.0\"\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(); @@ -930,7 +951,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", @@ -992,7 +1015,8 @@ async fn redirect_scanned_project( | Driver::ScanVexConditionalDeclaration | Driver::ScanVexScopedConstantModifier | Driver::ScanVexHeredocDeclaration - | Driver::ScanVexInterpolatedHeredocDeclaration => { + | Driver::ScanVexInterpolatedHeredocDeclaration + | Driver::ScanVexSemicolonJoinedDeclaration => { Some("redirect_gem_unrecognized_declaration") } _ => None, @@ -1073,7 +1097,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, @@ -1089,7 +1113,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 => { @@ -1877,6 +1902,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/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/formats/gem/gemfile.rs b/crates/socket-patch-core/src/formats/gem/gemfile.rs index 8203c8a0a..ca97e022e 100644 --- a/crates/socket-patch-core/src/formats/gem/gemfile.rs +++ b/crates/socket-patch-core/src/formats/gem/gemfile.rs @@ -243,6 +243,7 @@ pub(crate) struct SourceOption { /// it may carry one. Positional arguments (`*V`, constants, method calls) /// are version constraints and never do. pub(crate) fn source_option(tail: &str) -> Option { + let tail = &without_statement_end(tail); let Some(args) = args(tail) else { return Some(SourceOption { key: tail.trim().to_string(), @@ -273,6 +274,7 @@ pub(crate) fn source_option(tail: &str) -> Option { /// `path:`. Empty when the line carries none; bails to empty on an /// unparseable tail (unbalanced quote or bracket). pub(crate) fn trailing_options(tail: &str) -> String { + let tail = &without_statement_end(tail); let Some(args) = args(tail) else { return String::new(); }; @@ -284,6 +286,43 @@ pub(crate) fn trailing_options(tail: &str) -> String { .unwrap_or_default() } +/// `opts` minus a top-level `;` statement terminator (and any extra `;`s), +/// keeping a trailing `#` comment. Both rewriters first refuse a tail where +/// another statement follows the `;` (`gem_line_tail_blocks_edit`), 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() +} + #[cfg(test)] mod tests { use super::*; @@ -397,4 +436,30 @@ mod tests { "require: \"a,b\", group: [:x, :y]" ); } + + /// #826: a bare `;` ending the statement is not part of the options + /// (it would otherwise read as a positional `"7.0";` and be kept). + #[test] + fn trailing_options_drop_the_statement_terminator() { + // Nor is it an unreadable tail, which would fail closed as a + // source-selecting option. + for tail in [";", "; # c", ", \"0.8.1\";", ", require: false; # c"] { + assert_eq!(key(tail), None, "{tail:?}"); + } + assert_eq!(key(", git: \"x\";"), Some("git:".into())); + for (tail, opts) in [ + (", \"0.8.1\";", ""), + (", \"0.8.1\"; # c", ""), + (", 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!(trailing_options(tail), opts, "{tail:?}"); + } + } } 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/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 72c8b2967..547f58666 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -5428,7 +5428,9 @@ fn gem_source_option_detail(dep: &DepOverride, what: &str, socket_vendored: bool /// 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. @@ -5461,6 +5463,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, @@ -13498,6 +13512,113 @@ 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:?}"); + } + } + + /// #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/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)), } } diff --git a/crates/socket-patch-core/src/vendor/gem.rs b/crates/socket-patch-core/src/vendor/gem.rs index a9e7d65ff..3867ddc98 100644 --- a/crates/socket-patch-core/src/vendor/gem.rs +++ b/crates/socket-patch-core/src/vendor/gem.rs @@ -7164,6 +7164,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).