From 651df1cf952e1e212fcc7fe825479673bbe0f75f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 02:23:19 +0000 Subject: [PATCH 1/4] Start fix for #340 Assisted-by: Claude Code:claude-opus-5-5 From 369daa52639ff831fb41e6f0cd564f90be416e54 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 02:55:46 +0000 Subject: [PATCH 2/4] Refuse multi-line and conditional gem lines Hosted scan rewrote a `gem` declaration that continued on the next line, leaving the continuation orphaned after the new source block so bundler refused the Gemfile. It also dropped `if`/`unless` modifiers, declaring the gem unconditionally. Both now skip the redirect with a redirect_gem_unrecognized_declaration warning and leave the Gemfile untouched. The check is shared with vendored mode, which also now catches continuations after `=>`, a key, `\` or an open bracket, and modifiers separated by tabs. Fixes #340 Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_redirect_gem_build.rs | 66 +++++- .../src/patch/redirect/mod.rs | 207 ++++++++++++++++++ crates/socket-patch-core/src/vendor/gem.rs | 31 ++- 3 files changed, 292 insertions(+), 12 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 b69a7c3f3..e58e34e31 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs @@ -424,6 +424,15 @@ enum Driver { /// appending a source block would declare it twice. Same contract as /// [`Driver::ScanVexDuplicateDeclaration`]. ScanVexEvalGemfile, + /// [`Driver::ScanVex`] on a Gemfile whose declaration continues on the + /// next line (`gem "x",` ↵ `require: false`, #340): rewriting the first + /// line would orphan the continuation after the source block. Same + /// contract as [`Driver::ScanVexDuplicateDeclaration`]. + ScanVexMultiLineDeclaration, + /// [`Driver::ScanVex`] on a Gemfile whose declaration carries an `if` + /// modifier (#340): rewriting it would drop the condition. Same contract + /// as [`Driver::ScanVexDuplicateDeclaration`]. + ScanVexConditionalDeclaration, /// [`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 @@ -439,6 +448,8 @@ impl Driver { Driver::ScanVexDualBoot => "scan --mode hosted (BUNDLE_GEMFILE=Gemfile.next)", Driver::ScanVexDuplicateDeclaration => "scan --mode hosted (gem in two groups)", Driver::ScanVexEvalGemfile => "scan --mode hosted (gem via eval_gemfile)", + Driver::ScanVexMultiLineDeclaration => "scan --mode hosted (multi-line gem line)", + Driver::ScanVexConditionalDeclaration => "scan --mode hosted (gem line with `if`)", Driver::ScanVexDualBootEnvGemfile => { "scan --mode hosted (config Gemfile.next, env BUNDLE_GEMFILE=Gemfile)" } @@ -717,6 +728,14 @@ async fn redirect_scanned_project( server.uri() ) } + Driver::ScanVexMultiLineDeclaration => format!( + "source \"{}/upstream\"\n\ngem \"{DEP}\",\n require: false\n", + server.uri() + ), + Driver::ScanVexConditionalDeclaration => format!( + "source \"{}/upstream\"\n\ngem \"{DEP}\" if ENV[\"WITH_VULN\"] != \"0\"\n", + server.uri() + ), _ => format!("source \"{}/upstream\"\n\ngem \"{DEP}\"\n", server.uri()), }; std::fs::write(proj.join(gemfile_name), gemfile_body).unwrap(); @@ -823,7 +842,9 @@ async fn redirect_scanned_project( | Driver::ScanVexDualBoot | Driver::ScanVexDualBootEnvGemfile | Driver::ScanVexDuplicateDeclaration - | Driver::ScanVexEvalGemfile => vec![ + | Driver::ScanVexEvalGemfile + | Driver::ScanVexMultiLineDeclaration + | Driver::ScanVexConditionalDeclaration => vec![ "scan", "--mode", "hosted", @@ -880,6 +901,9 @@ async fn redirect_scanned_project( if let Some(warning) = match driver { Driver::ScanVexDuplicateDeclaration => Some("redirect_gem_declared_more_than_once"), Driver::ScanVexEvalGemfile => Some("redirect_gem_declaration_not_visible"), + Driver::ScanVexMultiLineDeclaration | Driver::ScanVexConditionalDeclaration => { + Some("redirect_gem_unrecognized_declaration") + } _ => None, } { assert_unwirable_declaration_redirects_nothing( @@ -968,7 +992,9 @@ async fn redirect_scanned_project( Driver::ScanVexDualBoot | Driver::ScanVexDualBootEnvGemfile | Driver::ScanVexDuplicateDeclaration - | Driver::ScanVexEvalGemfile => unreachable!("asserted and returned above"), + | Driver::ScanVexEvalGemfile + | Driver::ScanVexMultiLineDeclaration + | Driver::ScanVexConditionalDeclaration => unreachable!("asserted and returned above"), Driver::GetUuid => { // get's hosted envelope (CLI_CONTRACT.md "get --mode and // installed narrowing"): `found` counts the resolved patch; @@ -1717,6 +1743,42 @@ async fn gem_hosted_eval_gemfile_direct_dep_is_refused_and_still_installs() { assert!(fx.is_none(), "the eval_gemfile driver asserts in place"); } +/// #340: a `gem` declaration that continues on the next line must not be +/// rewritten (the orphaned `require: false` made bundler refuse the Gemfile). +#[tokio::test(flavor = "multi_thread")] +#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17; CHECKSUMS arm >= 2.6); \ + the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"] +async fn gem_hosted_multi_line_declaration_is_refused_and_still_installs() { + let fx = redirect_scanned_project( + "multi-line", + Spelling::Gemfile, + false, + true, + None, + Driver::ScanVexMultiLineDeclaration, + ) + .await; + assert!(fx.is_none(), "the multi-line driver asserts in place"); +} + +/// #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")] +#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17; CHECKSUMS arm >= 2.6); \ + the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"] +async fn gem_hosted_conditional_declaration_is_refused_and_still_installs() { + let fx = redirect_scanned_project( + "conditional", + Spelling::Gemfile, + false, + true, + None, + Driver::ScanVexConditionalDeclaration, + ) + .await; + assert!(fx.is_none(), "the conditional driver asserts in place"); +} + /// #507: the same dual boot with `BUNDLE_GEMFILE=Gemfile` exported. Bundler /// ranks the committed `.bundle/config` above the environment (it still /// loads `Gemfile.next`), so socket-patch must not follow the env value and diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index c5b565053..dfa6e0c5c 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -5179,6 +5179,97 @@ pub(crate) fn gem_line_trailing_options(tail: &str) -> 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 +/// option list that ends on this line and carries no modifier. Anything else +/// is refused fail-closed (#340): +/// - a tail that continues on the next line (a dangling `,`, `=>`, key, `\`, +/// an unclosed bracket or string) would leave the continuation orphaned +/// 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. +/// +/// Only code outside string literals and before a `#` comment counts, so a +/// keyword or `,` inside `require: "…"` or a trailing comment is fine. +/// Shared with the vendor backend's Gemfile rewrite (`vendor::gem`). +pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option { + const CONTINUES: &str = "the declaration continues on the next line"; + let mut code = String::new(); + let mut quote: Option = None; + let mut depth: i64 = 0; + let mut chars = tail.chars(); + while let Some(c) = chars.next() { + if let Some(q) = quote { + if c == '\\' { + chars.next(); + } else if c == q { + quote = None; + } + // String contents never count as code: keep a placeholder so + // word boundaries and the final character stay meaningful. + code.push(if c == q && quote.is_none() { c } else { 'x' }); + continue; + } + match c { + '#' => break, + '"' | '\'' => quote = Some(c), + '(' | '[' | '{' => depth += 1, + ')' | ']' | '}' => depth -= 1, + _ => {} + } + code.push(c); + } + if quote.is_some() || depth > 0 { + return Some(CONTINUES.to_string()); + } + let code = code.trim(); + if code.is_empty() { + return None; + } + if depth < 0 || !code.starts_with(',') { + return Some("unexpected tokens after the gem name".to_string()); + } + let last = code.chars().next_back().unwrap_or(','); + if !(last.is_alphanumeric() || matches!(last, '_' | '"' | '\'' | ')' | ']' | '}' | '?' | '!')) { + return Some(CONTINUES.to_string()); + } + let bytes = code.as_bytes(); + let is_ident = |b: u8| b.is_ascii_alphanumeric() || b == b'_'; + let mut i = 0; + while i < bytes.len() { + if !is_ident(bytes[i]) { + i += 1; + continue; + } + let start = i; + while i < bytes.len() && is_ident(bytes[i]) { + i += 1; + } + let word = &code[start..i]; + // A symbol (`:if`), method call (`.if`) or variable sigil is a name, + // not a keyword; so is a hash key (`if:`) or predicate (`if?`). + let prev_ok = start == 0 || !matches!(bytes[start - 1], b':' | b'.' | b'@' | b'$'); + let next_ok = bytes + .get(i) + .is_none_or(|b| !matches!(b, b':' | b'?' | b'!')); + if !(prev_ok && next_ok) { + continue; + } + match word { + "if" | "unless" | "while" | "until" => { + return Some(format!("conditional declaration (`{word}` modifier)")); + } + "rescue" | "and" | "or" | "do" => { + return Some(format!("a trailing `{word}` after the declaration")); + } + _ => {} + } + } + None +} + /// The source-selecting option a `gem` line's argument tail carries, if any /// (only the code before any `#` comment counts). Bundler allows ONE source /// per gem, so an option like `git:` preserved into the Socket source block @@ -5700,6 +5791,20 @@ fn rewrite_gem( }); continue; } + // Only a whole one-line declaration can move into the + // block: a continuation would be orphaned after `end` + // and a modifier silently dropped (#340). + if let Some(reason) = gem_line_tail_blocks_edit(&tail) { + result.warnings.push(RewriteWarning { + code: "redirect_gem_unrecognized_declaration".into(), + detail: format!( + "the `gem \"{}\"` declaration is in a form the \ + rewriter cannot safely edit ({reason}); redirect skipped", + dep.name + ), + }); + continue; + } // Trailing options (`require: false`, `group: …`) must // survive the move into the source block — dropping // `require: false` auto-requires the gem at boot. @@ -12369,6 +12474,108 @@ mod tests { } } + /// #340: a declaration whose tail continues on the next line, or that + /// carries a modifier (`if` / `unless` / …), is not a single-line + /// declaration the rewriter can move into a source block. Rewriting it + /// orphans the continuation after `end` (bundler refuses the Gemfile) or + /// silently drops the condition. Fail closed and leave both files alone. + #[test] + fn gemfile_multi_line_or_conditional_declaration_fails_closed() { + 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 in [ + // Continuations: a dangling `,`, `=>`, key, backslash, open bracket. + "gem \"vuln-gem\",\n require: false", + "gem \"vuln-gem\", # keep it lazy\n require: false", + "gem \"vuln-gem\",\r\n require: false", + "gem \"vuln-gem\", :require =>\n false", + "gem \"vuln-gem\", require:\n false", + "gem \"vuln-gem\", \"1.0.0\", \\\n require: false", + "gem \"vuln-gem\", platforms: [:mri,\n :mingw]", + "gem \"vuln-gem\", platforms: [\n :mri]", + "gem \"vuln-gem\", require: \"vuln\n/gem\"", + "gem(\"vuln-gem\",\n require: false)", + // Modifiers and other non-option tails. + "gem \"vuln-gem\" if true", + "gem \"vuln-gem\" if ENV[\"WITH_VULN\"] != \"0\"", + "gem \"vuln-gem\", require: false if ENV[\"CI\"]", + "gem \"vuln-gem\", \"1.0.0\"\tunless RUBY_VERSION < \"3\"", + "gem \"vuln-gem\", \"1.0.0\" if(ENV[\"CI\"])", + "gem \"vuln-gem\", require: false rescue nil", + "gem(\"vuln-gem\") if true", + "gem \"vuln-gem\" do", + ] { + let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n"); + let mut files = BTreeMap::new(); + files.insert("Gemfile".to_string(), gemfile.clone()); + files.insert("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={:?} edits={:?}", + r.files, + r.edits + ); + assert_eq!( + warning_codes(&r), + vec!["redirect_gem_unrecognized_declaration"], + "{decl:?}: {:?}", + r.warnings + ); + } + } + + /// 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 + /// rewrite, keeping their options. + #[test] + fn gemfile_single_line_declaration_lookalikes_still_rewrite() { + 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, opts) in [ + ("gem \"vuln-gem\"", ""), + ("gem \"vuln-gem\" # only if needed,", ""), + ( + "gem \"vuln-gem\", \"~> 1.0\" # pinned, unless told otherwise", + "", + ), + ( + "gem \"vuln-gem\", require: \"if/unless\"", + "require: \"if/unless\"", + ), + ("gem \"vuln-gem\", require: 'a,'", "require: 'a,'"), + ("gem \"vuln-gem\", group: :unless", "group: :unless"), + ( + "gem \"vuln-gem\", platforms: [:mri, :mingw]", + "platforms: [:mri, :mingw]", + ), + ("gem \"vuln-gem\", require: \"a#b\"", "require: \"a#b\""), + ("gem \"vuln-gem\", require: false\r", "require: false"), + ("gem(\"vuln-gem\", require: false)", "require: false"), + ] { + let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n"); + let mut files = BTreeMap::new(); + files.insert("Gemfile".to_string(), gemfile.clone()); + files.insert("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"); + let want = if opts.is_empty() { + " gem \"vuln-gem\", \"1.0.0\"\nend".to_string() + } else { + format!(" gem \"vuln-gem\", \"1.0.0\", {opts}\nend") + }; + assert!(out.contains(&want), "{decl:?}: {out}"); + } + } + /// #482: a DIRECT dependency the root Gemfile declares out of the /// rewriter's sight (`eval_gemfile`, a loop) is listed under the lock's /// DEPENDENCIES. Appending a source block for it declares it twice and diff --git a/crates/socket-patch-core/src/vendor/gem.rs b/crates/socket-patch-core/src/vendor/gem.rs index 99fc41219..b7eaed8b7 100644 --- a/crates/socket-patch-core/src/vendor/gem.rs +++ b/crates/socket-patch-core/src/vendor/gem.rs @@ -60,7 +60,7 @@ use crate::manifest::schema::PatchRecord; use crate::patch::apply::{ApplyResult, PatchSources}; use crate::patch::copy_tree::remove_tree; use crate::patch::path_safety::is_safe_single_segment; -use crate::patch::redirect::gem_line_trailing_options; +use crate::patch::redirect::{gem_line_tail_blocks_edit, gem_line_trailing_options}; use crate::utils::fs::{atomic_write_bytes_preserving_mode, read_regular_to_string}; use crate::utils::purl::{build_gem_purl, parse_gem_purl, purl_qualifier}; use crate::utils::socket_dir::remove_tree_and_prune; @@ -1530,16 +1530,13 @@ fn gem_declaration<'a>(trimmed: &'a str, name: &str) -> Option> { /// preserved `source:` (etc.) alongside the `path:` we add would fail every /// `bundle` invocation. fn rest_blocks_edit(rest: &str) -> Option { + if let Some(reason) = gem_line_tail_blocks_edit(rest) { + return Some(reason); + } let code = rest.split('#').next().unwrap_or("").trim(); if code.is_empty() { return None; } - if !code.starts_with(',') { - return Some("unexpected tokens after the gem name".to_string()); - } - if code.ends_with(',') { - return Some("the declaration continues on the next line".to_string()); - } for tok in [ "path:", ":path", @@ -1560,9 +1557,6 @@ fn rest_blocks_edit(rest: &str) -> Option { )); } } - if code.contains(" if ") || code.contains(" unless ") { - return Some("conditional declaration".to_string()); - } None } @@ -6949,6 +6943,23 @@ mod tests { .expect("a conditional declaration must refuse"); assert!(err.contains("conditional"), "{err}"); + // #340: the shared tail guard also catches continuations and + // modifiers the old substring checks missed. + for (gemfile, want) in [ + ("gem \"rack\", platforms: [\n :mri]\n", "continues"), + ("gem \"rack\", :require =>\n false\n", "continues"), + ( + "gem \"rack\", \"~> 3.1\"\tunless ENV[\"CI\"]\n", + "conditional", + ), + ("gem \"rack\", require: false rescue nil\n", "rescue"), + ] { + let err = plan_gemfile_edit(gemfile, "rack", "3.2.6", &rel) + .err() + .expect("a multi-line or modified declaration must refuse"); + assert!(err.contains(want), "{gemfile:?}: {err}"); + } + for gemfile in [ "gem \"rack\", mypath: \"y\"\n", "gem \"rack\", path: File.expand_path(\"x\")\n", From 84a43d49edf0debfc940670585c642ddb75ad8d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 04:28:18 +0000 Subject: [PATCH 3/4] Refuse gem modifiers written as labels, and heredocs `gem "x", "1.0.0" if::FEATURE` and `if:flag == ...` are `if` modifiers, not hash keys, because nothing separates them from the preceding value. The guard now counts `word:` as a key only where an argument can start (after `,`, `(` or `{`) and never as `word::`. A heredoc opener is also refused: its body follows on later lines, past the inserted `end`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015ZK1CuReoar7pKs2eZwepT --- .../src/patch/redirect/mod.rs | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index dfa6e0c5c..15521223c 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -5235,6 +5235,11 @@ pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option { if !(last.is_alphanumeric() || matches!(last, '_' | '"' | '\'' | ')' | ']' | '}' | '?' | '!')) { return Some(CONTINUES.to_string()); } + // A heredoc body lives on the following lines, past where the rewrite + // would insert its closing `end`. + if code.contains("<<") { + return Some(CONTINUES.to_string()); + } let bytes = code.as_bytes(); let is_ident = |b: u8| b.is_ascii_alphanumeric() || b == b'_'; let mut i = 0; @@ -5249,11 +5254,19 @@ pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option { } let word = &code[start..i]; // A symbol (`:if`), method call (`.if`) or variable sigil is a name, - // not a keyword; so is a hash key (`if:`) or predicate (`if?`). + // not a keyword; so is a predicate (`if?`). `if:` is a hash key only + // where an argument can start (after `,`, `(` or `{`) and not `if::`; + // straight after a value, `if:FLAG` is a modifier on symbol `:FLAG`. let prev_ok = start == 0 || !matches!(bytes[start - 1], b':' | b'.' | b'@' | b'$'); - let next_ok = bytes - .get(i) - .is_none_or(|b| !matches!(b, b':' | b'?' | b'!')); + let arg_start = matches!( + code[..start].trim_end().as_bytes().last(), + Some(b',' | b'(' | b'{') + ); + let next_ok = match bytes.get(i) { + Some(b'?' | b'!') => false, + Some(b':') => !(arg_start && bytes.get(i + 1) != Some(&b':')), + _ => true, + }; if !(prev_ok && next_ok) { continue; } @@ -12505,6 +12518,11 @@ mod tests { "gem \"vuln-gem\", require: false rescue nil", "gem(\"vuln-gem\") if true", "gem \"vuln-gem\" do", + // A label-looking modifier straight after a value, and a heredoc. + "gem \"vuln-gem\", \"1.0.0\" if::FEATURE", + "gem \"vuln-gem\", \"1.0.0\" unless::FEATURE", + "gem \"vuln-gem\", \"1.0.0\" if:enabled == ENV[\"MODE\"].to_sym", + "gem \"vuln-gem\", require: <<~REQ.strip\n vuln\nREQ", ] { let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n"); let mut files = BTreeMap::new(); From 464896d0d404d69f6f7fb213b9e6de0d21256e8e Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Sat, 3 Oct 2026 00:38:09 -0400 Subject: [PATCH 4/4] fix(gem): refuse interpolated heredoc declarations --- .../tests/e2e_redirect_gem_build.rs | 73 +++++++++++++++- .../src/patch/redirect/mod.rs | 86 ++++++++++++++++++- crates/socket-patch-core/src/vendor/gem.rs | 33 +++++++ 3 files changed, 185 insertions(+), 7 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 e58e34e31..6d590d8e4 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs @@ -433,6 +433,13 @@ enum Driver { /// modifier (#340): rewriting it would drop the condition. Same contract /// as [`Driver::ScanVexDuplicateDeclaration`]. ScanVexConditionalDeclaration, + /// A modifier adjacent to a top-level constant is still a modifier, + /// not a hash label (`if::ENV`, #340). + ScanVexScopedConstantModifier, + /// A heredoc option continues beyond the declaration's physical line. + ScanVexHeredocDeclaration, + /// A double-quoted interpolation can itself contain a heredoc opener. + ScanVexInterpolatedHeredocDeclaration, /// [`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 @@ -450,6 +457,11 @@ impl Driver { Driver::ScanVexEvalGemfile => "scan --mode hosted (gem via eval_gemfile)", Driver::ScanVexMultiLineDeclaration => "scan --mode hosted (multi-line gem line)", Driver::ScanVexConditionalDeclaration => "scan --mode hosted (gem line with `if`)", + Driver::ScanVexScopedConstantModifier => "scan --mode hosted (gem line with `if::ENV`)", + Driver::ScanVexHeredocDeclaration => "scan --mode hosted (heredoc gem option)", + Driver::ScanVexInterpolatedHeredocDeclaration => { + "scan --mode hosted (interpolated heredoc gem option)" + } Driver::ScanVexDualBootEnvGemfile => { "scan --mode hosted (config Gemfile.next, env BUNDLE_GEMFILE=Gemfile)" } @@ -736,6 +748,18 @@ async fn redirect_scanned_project( "source \"{}/upstream\"\n\ngem \"{DEP}\" if ENV[\"WITH_VULN\"] != \"0\"\n", server.uri() ), + Driver::ScanVexScopedConstantModifier => format!( + "source \"{}/upstream\"\n\ngem \"{DEP}\", \"{DEP_VERSION}\" if::ENV[\"WITH_VULN\"] != \"0\"\n", + server.uri() + ), + Driver::ScanVexHeredocDeclaration => format!( + "source \"{}/upstream\"\n\ngem \"{DEP}\", require: <<~REQUIRE_PATH.chomp\n vuln_gem\nREQUIRE_PATH\n", + server.uri() + ), + Driver::ScanVexInterpolatedHeredocDeclaration => format!( + "source \"{}/upstream\"\n\ngem \"{DEP}\", require: \"#{{<<~REQUIRE_PATH}}\".chomp\n vuln_gem\nREQUIRE_PATH\n", + server.uri() + ), _ => format!("source \"{}/upstream\"\n\ngem \"{DEP}\"\n", server.uri()), }; std::fs::write(proj.join(gemfile_name), gemfile_body).unwrap(); @@ -844,7 +868,10 @@ async fn redirect_scanned_project( | Driver::ScanVexDuplicateDeclaration | Driver::ScanVexEvalGemfile | Driver::ScanVexMultiLineDeclaration - | Driver::ScanVexConditionalDeclaration => vec![ + | Driver::ScanVexConditionalDeclaration + | Driver::ScanVexScopedConstantModifier + | Driver::ScanVexHeredocDeclaration + | Driver::ScanVexInterpolatedHeredocDeclaration => vec![ "scan", "--mode", "hosted", @@ -901,7 +928,11 @@ async fn redirect_scanned_project( if let Some(warning) = match driver { Driver::ScanVexDuplicateDeclaration => Some("redirect_gem_declared_more_than_once"), Driver::ScanVexEvalGemfile => Some("redirect_gem_declaration_not_visible"), - Driver::ScanVexMultiLineDeclaration | Driver::ScanVexConditionalDeclaration => { + Driver::ScanVexMultiLineDeclaration + | Driver::ScanVexConditionalDeclaration + | Driver::ScanVexScopedConstantModifier + | Driver::ScanVexHeredocDeclaration + | Driver::ScanVexInterpolatedHeredocDeclaration => { Some("redirect_gem_unrecognized_declaration") } _ => None, @@ -994,7 +1025,12 @@ async fn redirect_scanned_project( | Driver::ScanVexDuplicateDeclaration | Driver::ScanVexEvalGemfile | Driver::ScanVexMultiLineDeclaration - | Driver::ScanVexConditionalDeclaration => unreachable!("asserted and returned above"), + | Driver::ScanVexConditionalDeclaration + | Driver::ScanVexScopedConstantModifier + | Driver::ScanVexHeredocDeclaration + | Driver::ScanVexInterpolatedHeredocDeclaration => { + unreachable!("asserted and returned above") + } Driver::GetUuid => { // get's hosted envelope (CLI_CONTRACT.md "get --mode and // installed narrowing"): `found` counts the resolved patch; @@ -1779,6 +1815,37 @@ async fn gem_hosted_conditional_declaration_is_refused_and_still_installs() { assert!(fx.is_none(), "the conditional driver asserts in place"); } +#[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_scoped_constant_modifier_is_refused_and_still_installs() { + let fx = redirect_scanned_project( + "scoped-constant-modifier", + Spelling::Gemfile, + false, + true, + None, + Driver::ScanVexScopedConstantModifier, + ) + .await; + assert!(fx.is_none(), "the scoped modifier driver asserts in place"); +} + +#[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_heredoc_declaration_is_refused_and_still_installs() { + for driver in [ + Driver::ScanVexHeredocDeclaration, + Driver::ScanVexInterpolatedHeredocDeclaration, + ] { + let fx = + redirect_scanned_project(driver.label(), Spelling::Gemfile, false, true, None, driver) + .await; + assert!(fx.is_none(), "the heredoc driver asserts in place"); + } +} + /// #507: the same dual boot with `BUNDLE_GEMFILE=Gemfile` exported. Bundler /// ranks the committed `.bundle/config` above the environment (it still /// loads `Gemfile.next`), so socket-patch must not follow the env value and diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 15521223c..7c67adf16 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -5191,13 +5191,17 @@ pub(crate) fn gem_line_trailing_options(tail: &str) -> String { /// `or`) or a `do` block would be dropped, silently changing when the gem /// is declared. /// -/// Only code outside string literals and before a `#` comment counts, so a -/// keyword or `,` inside `require: "…"` or a trailing comment is fine. +/// Only code outside ordinary string literals and before a `#` comment +/// counts, so a keyword or `,` inside `require: "…"` or a comment is fine. +/// Double-quoted interpolation can execute a heredoc, so its presence with +/// a possible `<<` opener is refused conservatively too. /// Shared with the vendor backend's Gemfile rewrite (`vendor::gem`). pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option { const CONTINUES: &str = "the declaration continues on the next line"; let mut code = String::new(); let mut quote: Option = None; + let mut interpolated = false; + let mut quoted_operator = false; let mut depth: i64 = 0; let mut chars = tail.chars(); while let Some(c) = chars.next() { @@ -5206,6 +5210,10 @@ pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option { chars.next(); } else if c == q { quote = None; + } else if q == '"' && c == '#' && chars.as_str().starts_with('{') { + interpolated = true; + } else if c == '<' && chars.as_str().starts_with('<') { + quoted_operator = true; } // String contents never count as code: keep a placeholder so // word boundaries and the final character stay meaningful. @@ -5236,8 +5244,10 @@ pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option { return Some(CONTINUES.to_string()); } // A heredoc body lives on the following lines, past where the rewrite - // would insert its closing `end`. - if code.contains("<<") { + // would insert its closing `end`. Interpolation is executable Ruby too + // (`"#{<<~NAME}"`), so do not let quote masking hide its opener. Literal + // and escaped-interpolation lookalikes remain masked. + if code.contains("<<") || (interpolated && quoted_operator) { return Some(CONTINUES.to_string()); } let bytes = code.as_bytes(); @@ -12544,6 +12554,66 @@ mod tests { } } + #[test] + fn gemfile_quoted_and_interpolated_heredocs_fail_closed() { + 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 in [ + "gem \"vuln-gem\", require: <<'REQUIRE_PATH'\nvuln-gem\nREQUIRE_PATH", + "gem(\"vuln-gem\", require: <<\"REQUIRE_PATH\")\nvuln-gem\nREQUIRE_PATH", + "gem \"vuln-gem\", require: \"#{<<~REQUIRE_PATH}\".chomp\n vuln_gem\nREQUIRE_PATH", + ] { + let gemfile = format!("source \"https://rubygems.org\"\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: {:?}", + r.files + ); + assert_eq!( + warning_codes(&r), + vec!["redirect_gem_unrecognized_declaration"], + "{decl:?}: {:?}", + r.warnings + ); + } + } + + #[test] + fn gem_line_tail_colon_and_heredoc_syntax_is_not_confused_with_literals() { + for tail in [ + ", \"1.0.0\" if::FEATURE", + ", \"1.0.0\" unless::FEATURE", + ", \"1.0.0\" if:enabled == ENV[\"MODE\"].to_sym", + ", require: <<~REQUIRE_PATH.chomp", + ", require: <<'REQUIRE_PATH'", + ", require: <<\"REQUIRE_PATH\"", + ", require: \"#{<<~REQUIRE_PATH}\".chomp", + ] { + assert!(gem_line_tail_blocks_edit(tail).is_some(), "{tail:?}"); + } + for tail in [ + ", require: \"< 3.1\" if::FEATURE\n", "conditional"), + ("gem \"rack\", \"~> 3.1\" unless::FEATURE\n", "conditional"), + ( + "gem \"rack\", \"~> 3.1\" if:enabled == ENV[\"MODE\"].to_sym\n", + "conditional", + ), + ( + "gem \"rack\", require: <<~REQUIRE_PATH.chomp\n rack\nREQUIRE_PATH\n", + "continues", + ), + ( + "gem \"rack\", require: <<'REQUIRE_PATH'\nrack\nREQUIRE_PATH\n", + "continues", + ), + ( + "gem \"rack\", require: \"#{<<~REQUIRE_PATH}\".chomp\n rack\nREQUIRE_PATH\n", + "continues", + ), ] { let err = plan_gemfile_edit(gemfile, "rack", "3.2.6", &rel) .err() @@ -6970,6 +6988,21 @@ mod tests { assert!(err.contains("path:"), "{gemfile:?}: {err}"); } + for options in [ + "require: { if: \"rack\" }.values", + "require: \"<