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..6d590d8e4 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,22 @@ 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, + /// 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 @@ -439,6 +455,13 @@ 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::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)" } @@ -717,6 +740,26 @@ 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() + ), + 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(); @@ -823,7 +866,12 @@ async fn redirect_scanned_project( | Driver::ScanVexDualBoot | Driver::ScanVexDualBootEnvGemfile | Driver::ScanVexDuplicateDeclaration - | Driver::ScanVexEvalGemfile => vec![ + | Driver::ScanVexEvalGemfile + | Driver::ScanVexMultiLineDeclaration + | Driver::ScanVexConditionalDeclaration + | Driver::ScanVexScopedConstantModifier + | Driver::ScanVexHeredocDeclaration + | Driver::ScanVexInterpolatedHeredocDeclaration => vec![ "scan", "--mode", "hosted", @@ -880,6 +928,13 @@ 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::ScanVexScopedConstantModifier + | Driver::ScanVexHeredocDeclaration + | Driver::ScanVexInterpolatedHeredocDeclaration => { + Some("redirect_gem_unrecognized_declaration") + } _ => None, } { assert_unwirable_declaration_redirects_nothing( @@ -968,7 +1023,14 @@ async fn redirect_scanned_project( Driver::ScanVexDualBoot | Driver::ScanVexDualBootEnvGemfile | Driver::ScanVexDuplicateDeclaration - | Driver::ScanVexEvalGemfile => unreachable!("asserted and returned above"), + | Driver::ScanVexEvalGemfile + | Driver::ScanVexMultiLineDeclaration + | 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; @@ -1717,6 +1779,73 @@ 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"); +} + +#[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 c5b565053..7c67adf16 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -5179,6 +5179,120 @@ 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 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() { + if let Some(q) = quote { + if c == '\\' { + 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. + 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()); + } + // A heredoc body lives on the following lines, past where the rewrite + // 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(); + 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 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 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; + } + 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 +5814,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 +12497,181 @@ 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", + // 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(); + 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 + ); + } + } + + #[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: \"< 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\", require: { if: \"vuln-gem\" }.values", + "require: { if: \"vuln-gem\" }.values", + ), + ( + "gem \"vuln-gem\", require: \"<(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,41 @@ 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"), + ("gem \"rack\", \"~> 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() + .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", @@ -6959,6 +6988,21 @@ mod tests { assert!(err.contains("path:"), "{gemfile:?}: {err}"); } + for options in [ + "require: { if: \"rack\" }.values", + "require: \"<