Skip to content

Commit 369daa5

Browse files
committed
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
1 parent 651df1c commit 369daa5

3 files changed

Lines changed: 292 additions & 12 deletions

File tree

‎crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs‎

Lines changed: 64 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,15 @@ enum Driver {
424424
/// appending a source block would declare it twice. Same contract as
425425
/// [`Driver::ScanVexDuplicateDeclaration`].
426426
ScanVexEvalGemfile,
427+
/// [`Driver::ScanVex`] on a Gemfile whose declaration continues on the
428+
/// next line (`gem "x",` ↵ `require: false`, #340): rewriting the first
429+
/// line would orphan the continuation after the source block. Same
430+
/// contract as [`Driver::ScanVexDuplicateDeclaration`].
431+
ScanVexMultiLineDeclaration,
432+
/// [`Driver::ScanVex`] on a Gemfile whose declaration carries an `if`
433+
/// modifier (#340): rewriting it would drop the condition. Same contract
434+
/// as [`Driver::ScanVexDuplicateDeclaration`].
435+
ScanVexConditionalDeclaration,
427436
/// [`Driver::ScanVexDualBoot`] with `BUNDLE_GEMFILE=Gemfile` exported to
428437
/// socket-patch too (#507): bundler's local app config outranks the
429438
/// environment, so bundler still loads `Gemfile.next` and the run must
@@ -439,6 +448,8 @@ impl Driver {
439448
Driver::ScanVexDualBoot => "scan --mode hosted (BUNDLE_GEMFILE=Gemfile.next)",
440449
Driver::ScanVexDuplicateDeclaration => "scan --mode hosted (gem in two groups)",
441450
Driver::ScanVexEvalGemfile => "scan --mode hosted (gem via eval_gemfile)",
451+
Driver::ScanVexMultiLineDeclaration => "scan --mode hosted (multi-line gem line)",
452+
Driver::ScanVexConditionalDeclaration => "scan --mode hosted (gem line with `if`)",
442453
Driver::ScanVexDualBootEnvGemfile => {
443454
"scan --mode hosted (config Gemfile.next, env BUNDLE_GEMFILE=Gemfile)"
444455
}
@@ -717,6 +728,14 @@ async fn redirect_scanned_project(
717728
server.uri()
718729
)
719730
}
731+
Driver::ScanVexMultiLineDeclaration => format!(
732+
"source \"{}/upstream\"\n\ngem \"{DEP}\",\n require: false\n",
733+
server.uri()
734+
),
735+
Driver::ScanVexConditionalDeclaration => format!(
736+
"source \"{}/upstream\"\n\ngem \"{DEP}\" if ENV[\"WITH_VULN\"] != \"0\"\n",
737+
server.uri()
738+
),
720739
_ => format!("source \"{}/upstream\"\n\ngem \"{DEP}\"\n", server.uri()),
721740
};
722741
std::fs::write(proj.join(gemfile_name), gemfile_body).unwrap();
@@ -823,7 +842,9 @@ async fn redirect_scanned_project(
823842
| Driver::ScanVexDualBoot
824843
| Driver::ScanVexDualBootEnvGemfile
825844
| Driver::ScanVexDuplicateDeclaration
826-
| Driver::ScanVexEvalGemfile => vec![
845+
| Driver::ScanVexEvalGemfile
846+
| Driver::ScanVexMultiLineDeclaration
847+
| Driver::ScanVexConditionalDeclaration => vec![
827848
"scan",
828849
"--mode",
829850
"hosted",
@@ -880,6 +901,9 @@ async fn redirect_scanned_project(
880901
if let Some(warning) = match driver {
881902
Driver::ScanVexDuplicateDeclaration => Some("redirect_gem_declared_more_than_once"),
882903
Driver::ScanVexEvalGemfile => Some("redirect_gem_declaration_not_visible"),
904+
Driver::ScanVexMultiLineDeclaration | Driver::ScanVexConditionalDeclaration => {
905+
Some("redirect_gem_unrecognized_declaration")
906+
}
883907
_ => None,
884908
} {
885909
assert_unwirable_declaration_redirects_nothing(
@@ -968,7 +992,9 @@ async fn redirect_scanned_project(
968992
Driver::ScanVexDualBoot
969993
| Driver::ScanVexDualBootEnvGemfile
970994
| Driver::ScanVexDuplicateDeclaration
971-
| Driver::ScanVexEvalGemfile => unreachable!("asserted and returned above"),
995+
| Driver::ScanVexEvalGemfile
996+
| Driver::ScanVexMultiLineDeclaration
997+
| Driver::ScanVexConditionalDeclaration => unreachable!("asserted and returned above"),
972998
Driver::GetUuid => {
973999
// get's hosted envelope (CLI_CONTRACT.md "get --mode and
9741000
// installed narrowing"): `found` counts the resolved patch;
@@ -1717,6 +1743,42 @@ async fn gem_hosted_eval_gemfile_direct_dep_is_refused_and_still_installs() {
17171743
assert!(fx.is_none(), "the eval_gemfile driver asserts in place");
17181744
}
17191745

1746+
/// #340: a `gem` declaration that continues on the next line must not be
1747+
/// rewritten (the orphaned `require: false` made bundler refuse the Gemfile).
1748+
#[tokio::test(flavor = "multi_thread")]
1749+
#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17; CHECKSUMS arm >= 2.6); \
1750+
the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"]
1751+
async fn gem_hosted_multi_line_declaration_is_refused_and_still_installs() {
1752+
let fx = redirect_scanned_project(
1753+
"multi-line",
1754+
Spelling::Gemfile,
1755+
false,
1756+
true,
1757+
None,
1758+
Driver::ScanVexMultiLineDeclaration,
1759+
)
1760+
.await;
1761+
assert!(fx.is_none(), "the multi-line driver asserts in place");
1762+
}
1763+
1764+
/// #340: a `gem` declaration with an `if` modifier must not be rewritten
1765+
/// (the rewrite dropped the condition and declared the gem unconditionally).
1766+
#[tokio::test(flavor = "multi_thread")]
1767+
#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17; CHECKSUMS arm >= 2.6); \
1768+
the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"]
1769+
async fn gem_hosted_conditional_declaration_is_refused_and_still_installs() {
1770+
let fx = redirect_scanned_project(
1771+
"conditional",
1772+
Spelling::Gemfile,
1773+
false,
1774+
true,
1775+
None,
1776+
Driver::ScanVexConditionalDeclaration,
1777+
)
1778+
.await;
1779+
assert!(fx.is_none(), "the conditional driver asserts in place");
1780+
}
1781+
17201782
/// #507: the same dual boot with `BUNDLE_GEMFILE=Gemfile` exported. Bundler
17211783
/// ranks the committed `.bundle/config` above the environment (it still
17221784
/// loads `Gemfile.next`), so socket-patch must not follow the env value and

‎crates/socket-patch-core/src/patch/redirect/mod.rs‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5179,6 +5179,97 @@ pub(crate) fn gem_line_trailing_options(tail: &str) -> String {
51795179
}
51805180
}
51815181

5182+
/// Why the argument tail of a one-line `gem "name"…` declaration can't be
5183+
/// rewritten in place (`None` = safe). Both Gemfile rewriters replace the
5184+
/// declaration's LINE, so the tail must be the whole declaration: a `,`-led
5185+
/// option list that ends on this line and carries no modifier. Anything else
5186+
/// is refused fail-closed (#340):
5187+
/// - a tail that continues on the next line (a dangling `,`, `=>`, key, `\`,
5188+
/// an unclosed bracket or string) would leave the continuation orphaned
5189+
/// after the rewrite, and bundler refuses the Gemfile;
5190+
/// - a modifier (`if` / `unless` / `while` / `until` / `rescue` / `and` /
5191+
/// `or`) or a `do` block would be dropped, silently changing when the gem
5192+
/// is declared.
5193+
///
5194+
/// Only code outside string literals and before a `#` comment counts, so a
5195+
/// keyword or `,` inside `require: "…"` or a trailing comment is fine.
5196+
/// Shared with the vendor backend's Gemfile rewrite (`vendor::gem`).
5197+
pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option<String> {
5198+
const CONTINUES: &str = "the declaration continues on the next line";
5199+
let mut code = String::new();
5200+
let mut quote: Option<char> = None;
5201+
let mut depth: i64 = 0;
5202+
let mut chars = tail.chars();
5203+
while let Some(c) = chars.next() {
5204+
if let Some(q) = quote {
5205+
if c == '\\' {
5206+
chars.next();
5207+
} else if c == q {
5208+
quote = None;
5209+
}
5210+
// String contents never count as code: keep a placeholder so
5211+
// word boundaries and the final character stay meaningful.
5212+
code.push(if c == q && quote.is_none() { c } else { 'x' });
5213+
continue;
5214+
}
5215+
match c {
5216+
'#' => break,
5217+
'"' | '\'' => quote = Some(c),
5218+
'(' | '[' | '{' => depth += 1,
5219+
')' | ']' | '}' => depth -= 1,
5220+
_ => {}
5221+
}
5222+
code.push(c);
5223+
}
5224+
if quote.is_some() || depth > 0 {
5225+
return Some(CONTINUES.to_string());
5226+
}
5227+
let code = code.trim();
5228+
if code.is_empty() {
5229+
return None;
5230+
}
5231+
if depth < 0 || !code.starts_with(',') {
5232+
return Some("unexpected tokens after the gem name".to_string());
5233+
}
5234+
let last = code.chars().next_back().unwrap_or(',');
5235+
if !(last.is_alphanumeric() || matches!(last, '_' | '"' | '\'' | ')' | ']' | '}' | '?' | '!')) {
5236+
return Some(CONTINUES.to_string());
5237+
}
5238+
let bytes = code.as_bytes();
5239+
let is_ident = |b: u8| b.is_ascii_alphanumeric() || b == b'_';
5240+
let mut i = 0;
5241+
while i < bytes.len() {
5242+
if !is_ident(bytes[i]) {
5243+
i += 1;
5244+
continue;
5245+
}
5246+
let start = i;
5247+
while i < bytes.len() && is_ident(bytes[i]) {
5248+
i += 1;
5249+
}
5250+
let word = &code[start..i];
5251+
// A symbol (`:if`), method call (`.if`) or variable sigil is a name,
5252+
// not a keyword; so is a hash key (`if:`) or predicate (`if?`).
5253+
let prev_ok = start == 0 || !matches!(bytes[start - 1], b':' | b'.' | b'@' | b'$');
5254+
let next_ok = bytes
5255+
.get(i)
5256+
.is_none_or(|b| !matches!(b, b':' | b'?' | b'!'));
5257+
if !(prev_ok && next_ok) {
5258+
continue;
5259+
}
5260+
match word {
5261+
"if" | "unless" | "while" | "until" => {
5262+
return Some(format!("conditional declaration (`{word}` modifier)"));
5263+
}
5264+
"rescue" | "and" | "or" | "do" => {
5265+
return Some(format!("a trailing `{word}` after the declaration"));
5266+
}
5267+
_ => {}
5268+
}
5269+
}
5270+
None
5271+
}
5272+
51825273
/// The source-selecting option a `gem` line's argument tail carries, if any
51835274
/// (only the code before any `#` comment counts). Bundler allows ONE source
51845275
/// per gem, so an option like `git:` preserved into the Socket source block
@@ -5700,6 +5791,20 @@ fn rewrite_gem(
57005791
});
57015792
continue;
57025793
}
5794+
// Only a whole one-line declaration can move into the
5795+
// block: a continuation would be orphaned after `end`
5796+
// and a modifier silently dropped (#340).
5797+
if let Some(reason) = gem_line_tail_blocks_edit(&tail) {
5798+
result.warnings.push(RewriteWarning {
5799+
code: "redirect_gem_unrecognized_declaration".into(),
5800+
detail: format!(
5801+
"the `gem \"{}\"` declaration is in a form the \
5802+
rewriter cannot safely edit ({reason}); redirect skipped",
5803+
dep.name
5804+
),
5805+
});
5806+
continue;
5807+
}
57035808
// Trailing options (`require: false`, `group: …`) must
57045809
// survive the move into the source block — dropping
57055810
// `require: false` auto-requires the gem at boot.
@@ -12369,6 +12474,108 @@ mod tests {
1236912474
}
1237012475
}
1237112476

12477+
/// #340: a declaration whose tail continues on the next line, or that
12478+
/// carries a modifier (`if` / `unless` / …), is not a single-line
12479+
/// declaration the rewriter can move into a source block. Rewriting it
12480+
/// orphans the continuation after `end` (bundler refuses the Gemfile) or
12481+
/// silently drops the condition. Fail closed and leave both files alone.
12482+
#[test]
12483+
fn gemfile_multi_line_or_conditional_declaration_fails_closed() {
12484+
let lock = "GEM\n remote: https://rubygems.org/\n specs:\n vuln-gem (1.0.0)\n\n\
12485+
PLATFORMS\n ruby\n\nDEPENDENCIES\n vuln-gem\n\n\
12486+
BUNDLED WITH\n 4.0.17\n";
12487+
for decl in [
12488+
// Continuations: a dangling `,`, `=>`, key, backslash, open bracket.
12489+
"gem \"vuln-gem\",\n require: false",
12490+
"gem \"vuln-gem\", # keep it lazy\n require: false",
12491+
"gem \"vuln-gem\",\r\n require: false",
12492+
"gem \"vuln-gem\", :require =>\n false",
12493+
"gem \"vuln-gem\", require:\n false",
12494+
"gem \"vuln-gem\", \"1.0.0\", \\\n require: false",
12495+
"gem \"vuln-gem\", platforms: [:mri,\n :mingw]",
12496+
"gem \"vuln-gem\", platforms: [\n :mri]",
12497+
"gem \"vuln-gem\", require: \"vuln\n/gem\"",
12498+
"gem(\"vuln-gem\",\n require: false)",
12499+
// Modifiers and other non-option tails.
12500+
"gem \"vuln-gem\" if true",
12501+
"gem \"vuln-gem\" if ENV[\"WITH_VULN\"] != \"0\"",
12502+
"gem \"vuln-gem\", require: false if ENV[\"CI\"]",
12503+
"gem \"vuln-gem\", \"1.0.0\"\tunless RUBY_VERSION < \"3\"",
12504+
"gem \"vuln-gem\", \"1.0.0\" if(ENV[\"CI\"])",
12505+
"gem \"vuln-gem\", require: false rescue nil",
12506+
"gem(\"vuln-gem\") if true",
12507+
"gem \"vuln-gem\" do",
12508+
] {
12509+
let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n");
12510+
let mut files = BTreeMap::new();
12511+
files.insert("Gemfile".to_string(), gemfile.clone());
12512+
files.insert("Gemfile.lock".to_string(), lock.to_string());
12513+
let r = rewrite_registry_redirect(&files, &[gem_override("vuln-gem", "1.0.0")]);
12514+
assert!(
12515+
r.files.is_empty() && r.edits.is_empty(),
12516+
"{decl:?} must not be rewritten: files={:?} edits={:?}",
12517+
r.files,
12518+
r.edits
12519+
);
12520+
assert_eq!(
12521+
warning_codes(&r),
12522+
vec!["redirect_gem_unrecognized_declaration"],
12523+
"{decl:?}: {:?}",
12524+
r.warnings
12525+
);
12526+
}
12527+
}
12528+
12529+
/// Control for #340: single-line declarations whose tails merely look
12530+
/// like the refused shapes (a keyword inside a string or a comment, a
12531+
/// symbol or a key named like a keyword, a closed bracket) still
12532+
/// rewrite, keeping their options.
12533+
#[test]
12534+
fn gemfile_single_line_declaration_lookalikes_still_rewrite() {
12535+
let lock = "GEM\n remote: https://rubygems.org/\n specs:\n vuln-gem (1.0.0)\n\n\
12536+
PLATFORMS\n ruby\n\nDEPENDENCIES\n vuln-gem\n\n\
12537+
BUNDLED WITH\n 4.0.17\n";
12538+
for (decl, opts) in [
12539+
("gem \"vuln-gem\"", ""),
12540+
("gem \"vuln-gem\" # only if needed,", ""),
12541+
(
12542+
"gem \"vuln-gem\", \"~> 1.0\" # pinned, unless told otherwise",
12543+
"",
12544+
),
12545+
(
12546+
"gem \"vuln-gem\", require: \"if/unless\"",
12547+
"require: \"if/unless\"",
12548+
),
12549+
("gem \"vuln-gem\", require: 'a,'", "require: 'a,'"),
12550+
("gem \"vuln-gem\", group: :unless", "group: :unless"),
12551+
(
12552+
"gem \"vuln-gem\", platforms: [:mri, :mingw]",
12553+
"platforms: [:mri, :mingw]",
12554+
),
12555+
("gem \"vuln-gem\", require: \"a#b\"", "require: \"a#b\""),
12556+
("gem \"vuln-gem\", require: false\r", "require: false"),
12557+
("gem(\"vuln-gem\", require: false)", "require: false"),
12558+
] {
12559+
let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n");
12560+
let mut files = BTreeMap::new();
12561+
files.insert("Gemfile".to_string(), gemfile.clone());
12562+
files.insert("Gemfile.lock".to_string(), lock.to_string());
12563+
let r = rewrite_registry_redirect(&files, &[gem_override("vuln-gem", "1.0.0")]);
12564+
assert!(
12565+
!warning_codes(&r).contains(&"redirect_gem_unrecognized_declaration"),
12566+
"{decl:?}: {:?}",
12567+
r.warnings
12568+
);
12569+
let out = r.files.get("Gemfile").expect("declaration rewritten");
12570+
let want = if opts.is_empty() {
12571+
" gem \"vuln-gem\", \"1.0.0\"\nend".to_string()
12572+
} else {
12573+
format!(" gem \"vuln-gem\", \"1.0.0\", {opts}\nend")
12574+
};
12575+
assert!(out.contains(&want), "{decl:?}: {out}");
12576+
}
12577+
}
12578+
1237212579
/// #482: a DIRECT dependency the root Gemfile declares out of the
1237312580
/// rewriter's sight (`eval_gemfile`, a loop) is listed under the lock's
1237412581
/// DEPENDENCIES. Appending a source block for it declares it twice and

0 commit comments

Comments
 (0)