From 4816a8c1bc8959a20c3d2aa06b4e02083b4d39e8 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Thu, 13 Aug 2026 15:33:53 -0700 Subject: [PATCH 1/2] =?UTF-8?q?fix(gem):=20bundler=20vendored+hosted=20swe?= =?UTF-8?q?ep=20=E2=80=94=2013=20confirmed=20bugs=20fixed,=20tests=20harde?= =?UTF-8?q?ned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ULTRACODE review + full test matrix over the gem/bundler ecosystem (vendored and hosted modes, every configuration). 24 findings survived adversarial verification; the 13 well-scoped ones are fixed here, each pinned by a test that fails without its fix. Hosted redirect (patch/redirect/mod.rs, gem section): - splice the Gemfile edit by regex byte range: a commented-out duplicate of the gem line no longer gets rewritten instead of the live line - grant-agnostic idempotency guard: re-running scan --mode hosted with a rotated grant token refreshes the source URL in place instead of nesting a second source block (new edit kind redirect_gemfile_source_url) - fail closed on gem-level git:/github:/path:/source: options (they override the enclosing source block, making the redirect an attested no-op); warn redirect_gem_source_option and skip the dep - fail closed on platform-suffixed CHECKSUMS siblings (redirect_gem_platform_unsupported) instead of inserting a duplicate bare-coordinate pin - recognize paren/tab/multi-space gem declarations; gate the append-branch on the gem being genuinely undeclared (no more duplicate declarations bundler rejects) - never pin the lock CHECKSUMS when the Gemfile source redirect did not land (mixed state guaranteed a checksum failure) - warn that a redirected pair breaks frozen/deployment installs - record the upstream sha256 line as original on the CHECKSUMS edit so a future revert can restore it (golden fixture updated) Vendored backend (vendor/gem.rs): - insert new PATH sections at bundler's sorted position (identifier order, verified against real bundler 4.0.15 bundle lock) — two or more vendored gems no longer churn the committed lock - fail closed on platform-suffixed GEM-specs siblings on no-CHECKSUMS locks (mirrors the existing CHECKSUMS guard) - re-vendor on a patch UPDATE (new uuid, same purl): recognize our own path: wiring and rewire in place instead of refusing with gemfile_declaration_not_editable — the documented automatic re-vendor contract now actually works for gem Auto-fetch (vendor/registry_fetch.rs): - stage fetched gems into the canonical - leaf instead of a dir literally named "gem", which vendor_gem refused as platform_gem_unsupported — lockfile auto-fetch for gems was dead Crawler (crawlers/ruby_crawler.rs): - parse_dir_name_version prefers the last dotted-version boundary, so http-2-1.0.1 parses as (http-2, 1.0.1) instead of the ghost (http, 2) - vendor/bundle discovery enumerates engine dirs (jruby, truffleruby) instead of hardcoding ruby/ Scan/get plumbing: - run_nested_apply now threads --ecosystems: scan --ecosystems gem --sync no longer applies (or mutates) other ecosystems' patches - scan --vendor --dry-run --vex no longer writes the VEX file nor exits 1 on not-yet-vendored state Test hardening: - e2e_gem lifecycle harness: BUNDLE_PATH replaces bundle install --path (removed in bundler 3+; all 3 lifecycle tests green under 4.0.15) - e2e_hosted_production gem leg now asserts the CHECKSUMS digest CHANGED after redirect (client-verifiable today) and, in the success arm, verifies installed content against the published afterHashes — an inert gem patch can no longer stay green (the npm minimist blindspot) - docker_e2e_gem serves the true git-blob beforeHash so the chain exercises the default non-forced apply path, not just --force - setup_matrix_gem module doc: the with-setup Docker cases ARE still a baseline gap (bootstrap deadlock: plugin registration evaluates plugins.rb before any gems land; exit-semantics twin), doc corrected Verified: core 2460/0, cli lib 350/0, clippy+fmt clean; e2e_gem 11/11 (incl. live lifecycle under bundler 4.0.15), e2e_vendor_gem_build 6/6 (incl. real-bundler capstone), docker_e2e_gem 2/2, docker_e2e_vendor_gem 1/1, hosted production gem leg green (redirect verified; install still blocked by the known depscan#23630 compact-index 404 — server-side). Co-Authored-By: Claude Fable 5 --- crates/socket-patch-cli/src/commands/get.rs | 24 +- .../socket-patch-cli/src/commands/scan/mod.rs | 1 + .../src/commands/scan/vendor_flow.rs | 17 +- .../socket-patch-cli/tests/docker_e2e_gem.rs | 112 ++- crates/socket-patch-cli/tests/e2e_gem.rs | 11 +- .../tests/e2e_hosted_production.rs | 169 ++++ .../tests/in_process_get_manifest_path.rs | 181 +++++ .../tests/in_process_get_update_count.rs | 1 + .../socket-patch-cli/tests/in_process_scan.rs | 107 +++ .../tests/setup_matrix_gem.rs | 16 +- .../src/crawlers/ruby_crawler.rs | 162 +++- .../src/patch/redirect/mod.rs | 563 +++++++++++++- crates/socket-patch-core/src/vendor/gem.rs | 735 +++++++++++++++++- .../src/vendor/registry_fetch.rs | 45 +- .../tests/crawler_ruby_e2e.rs | 41 + .../gem/bundler/basic/expected-edits.json | 1 + 16 files changed, 2095 insertions(+), 91 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/get.rs b/crates/socket-patch-cli/src/commands/get.rs index e9d1bf94..60ffec69 100644 --- a/crates/socket-patch-cli/src/commands/get.rs +++ b/crates/socket-patch-cli/src/commands/get.rs @@ -683,6 +683,11 @@ pub struct DownloadParams { /// `--strict` forwarded to the nested apply (a beforeHash mismatch /// fails instead of warn-and-overwrite). pub strict: bool, + /// `--ecosystems` forwarded to the nested apply. Without this the + /// nested apply ran UNSCOPED over the whole manifest, so + /// `scan --ecosystems gem --sync` could mutate other ecosystems' + /// packages the user had explicitly filtered out. + pub ecosystems: Option>, /// Persist downloaded blob content into `.socket/blobs` (the apply /// flows need it for later hook/rollback runs). Vendor flows pass /// `false`: their patch content is staged in memory and the committed @@ -1098,6 +1103,7 @@ async fn run_nested_apply( download_mode: String, strict: bool, api: socket_patch_core::api::client::ApiClientEnvOverrides, + ecosystems: Option>, ) -> bool { // Apply re-resolves a relative manifest path against ITS `--cwd` // (`resolved_manifest_path`), but ours is already cwd-resolved — @@ -1119,6 +1125,11 @@ async fn run_nested_apply( api_token: api.api_token, org: api.org_slug, proxy_url: api.proxy_url, + // Scope the nested apply like the caller was scoped: leaving + // this at the default `None` made `scan --ecosystems gem --sync` + // apply the WHOLE manifest, mutating other ecosystems' packages + // the user filtered out. + ecosystems, ..crate::args::GlobalArgs::default() }, force: false, @@ -1232,10 +1243,7 @@ pub async fn download_and_apply_patches( // status/exit code degrade and it is never auto-applied. if files.is_empty() { if !params.json && !params.silent { - eprintln!( - " [fail] {} (patch has no applicable files)", - patch.purl - ); + eprintln!(" [fail] {} (patch has no applicable files)", patch.purl); } downloaded_patches.push(serde_json::json!({ "purl": patch.purl, @@ -1393,6 +1401,7 @@ pub async fn download_and_apply_patches( params.download_mode.clone(), params.strict, resolved_api_overrides(params), + params.ecosystems.clone(), ) .await; } @@ -1824,6 +1833,7 @@ pub async fn run(args: GetArgs) -> i32 { api_overrides: args.common.api_client_overrides(), all_releases: args.all_releases, strict: args.common.strict, + ecosystems: args.common.ecosystems.clone(), persist_blobs: true, }; @@ -2034,6 +2044,7 @@ async fn save_and_apply_patch(args: &GetArgs, patch: &PatchResponse) -> i32 { args.common.download_mode.clone(), args.common.strict, args.common.api_client_overrides(), + args.common.ecosystems.clone(), ) .await; } @@ -3106,7 +3117,10 @@ mod tests { // record — the guardrail-triggering condition the download/apply // flows now count as failed rather than applied. let mut broken = HashMap::new(); - broken.insert("src/lib.rs".to_string(), file_resp(Some(&"e".repeat(64)), None)); + broken.insert( + "src/lib.rs".to_string(), + file_resp(Some(&"e".repeat(64)), None), + ); let broken_patch = patch_with_files(broken); assert!( files_for_manifest(&broken_patch).is_empty(), diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index b76700ab..eefed677 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -411,6 +411,7 @@ fn download_params(args: &ScanArgs, save_only: bool, json: bool, silent: bool) - api_overrides: args.common.api_client_overrides(), all_releases: args.all_releases, strict: args.common.strict, + ecosystems: args.common.ecosystems.clone(), persist_blobs: args.mode != Some(ScanMode::Vendored), } } diff --git a/crates/socket-patch-cli/src/commands/scan/vendor_flow.rs b/crates/socket-patch-cli/src/commands/scan/vendor_flow.rs index 4851dad4..5927ed20 100644 --- a/crates/socket-patch-cli/src/commands/scan/vendor_flow.rs +++ b/crates/socket-patch-cli/src/commands/scan/vendor_flow.rs @@ -229,7 +229,9 @@ async fn run_scan_vendor_step( } /// The `scan --vendor` JSON path: discovery → (dry-run preview | download -/// → GC → vendor engine) → embedded VEX → print `result` → exit code. +/// → GC → vendor engine → embedded VEX) → print `result` → exit code. +/// The dry-run arm skips the VEX embed (emitting a `vex.skipped` marker +/// instead): a dry run vendors nothing, so there is no state to attest. /// /// Extracted from `run` (and called through `Box::pin`) so its sizeable /// temporaries get their own poll frame, entered only when `--vendor` is @@ -285,10 +287,17 @@ async fn run_vendor_json_path( ) .await; } - let final_code = - embed_vex_into_json(&args.common, &args.vex, manifest_path, 0, result).await; + // Embedded VEX is skipped on a dry run (apply.rs's precedent): + // nothing was vendored, so there is no just-vendored state to + // attest — generating here would verify the deliberately untouched + // tree (failing outright on a not-yet-vendored project) and write + // an attestation file during --dry-run. The marker keeps the + // request visible to JSON consumers instead of silently dropping it. + if args.vex.vex.is_some() { + result["vex"] = serde_json::json!({ "skipped": true, "reason": "dry_run" }); + } println!("{}", serde_json::to_string_pretty(&result).unwrap()); - return final_code; + return 0; } // 1) Download phase. Manifest mode reuses the `--apply` diff --git a/crates/socket-patch-cli/tests/docker_e2e_gem.rs b/crates/socket-patch-cli/tests/docker_e2e_gem.rs index c85450f9..e906f10d 100644 --- a/crates/socket-patch-cli/tests/docker_e2e_gem.rs +++ b/crates/socket-patch-cli/tests/docker_e2e_gem.rs @@ -9,6 +9,15 @@ //! - `gem_global_install_full_apply_chain` — `gem install` without //! --install-dir, installs to the system gem directory; socket-patch //! scans + applies with `--global`. +//! +//! The fixture serves the TRUE git-blob sha256 of the installed +//! `lib/colorize.rb` as `beforeHash` (computed once by a probe container +//! from the real upstream artifact — see [`upstream_before_hash`]), so +//! both apply paths run gated, without `--force`: `scan --sync`'s own +//! nested apply must patch the file in the same run, and the explicit +//! `apply` (against restored pristine bytes) must pass the variant gate. +//! With the old all-zeros placeholder the nested apply failed invisibly +//! and only the `--force` escape hatch was ever exercised. #![cfg(feature = "docker-e2e")] @@ -69,6 +78,52 @@ fn plain_sha256(content: &[u8]) -> String { hex::encode(hasher.finalize()) } +/// Probe: install colorize 1.1.0 from the real registry and emit the +/// git-blob sha256 of the exact `lib/colorize.rb` bytes `gem install` +/// lays down — the value the fixture must serve as `beforeHash` for the +/// default (no `--force`) apply path to pass the variant gate. +const BEFORE_HASH_PROBE_SCRIPT: &str = r#"#!/usr/bin/env bash +set -uo pipefail +gem install --no-document --install-dir /tmp/probe colorize -v 1.1.0 > /tmp/install.log 2>&1 || { + cat /tmp/install.log >&2; exit 1 +} +F=/tmp/probe/gems/colorize-1.1.0/lib/colorize.rb +[ -f "$F" ] || { echo "FAIL: $F missing" >&2; exit 1; } +{ printf 'blob %d\0' "$(wc -c < "$F")"; cat "$F"; } | sha256sum | cut -d' ' -f1 +"#; + +/// True git-blob sha256 of the colorize-1.1.0 `lib/colorize.rb` that +/// `gem install` produces, computed once per process by +/// [`BEFORE_HASH_PROBE_SCRIPT`] in a probe container. Serving this as the +/// fixture's `beforeHash` — instead of an all-zeros placeholder — is what +/// lets the gated apply paths run: with the placeholder, `scan --sync`'s +/// nested apply hit the variant gate and failed invisibly, so the chain +/// only ever proved the `--force` path. +fn upstream_before_hash() -> String { + static HASH: std::sync::OnceLock = std::sync::OnceLock::new(); + HASH.get_or_init(|| { + let out = run_container(BEFORE_HASH_PROBE_SCRIPT); + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + out.status.success(), + "beforeHash probe container failed:\nstdout=\n{stdout}\nstderr=\n{stderr}" + ); + stdout + .lines() + .rev() + .map(str::trim) + .find(|l| l.len() == 64 && l.bytes().all(|b| b.is_ascii_hexdigit())) + .unwrap_or_else(|| { + panic!( + "beforeHash probe emitted no 64-hex git-blob sha256:\nstdout=\n{stdout}\nstderr=\n{stderr}" + ) + }) + .to_string() + }) + .clone() +} + /// Shared verification block for both scripts. Expects `GEM_FILE`, /// `EXPECTED_SHA`, and `APPLY_EXIT` to be set, plus the JSON captured in /// `/tmp/scan.json` and `/tmp/apply.json`. @@ -123,7 +178,7 @@ exit 0 "# } -async fn make_mock_server(after_hash: &str) -> MockServer { +async fn make_mock_server(before_hash: &str, after_hash: &str) -> MockServer { let listener = std::net::TcpListener::bind("0.0.0.0:0").expect("bind wiremock"); let server = MockServer::builder().listener(listener).start().await; @@ -169,9 +224,12 @@ async fn make_mock_server(after_hash: &str) -> MockServer { "publishedAt": "2024-01-01T00:00:00Z", "files": { // gem uses `package/` (npm-style) — apply strips - // the prefix and joins with the gem dir. + // the prefix and joins with the gem dir. beforeHash is + // the TRUE git-blob sha256 of the installed upstream + // file so the default (gated, no --force) apply path is + // what the chain exercises. "package/lib/colorize.rb": { - "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", + "beforeHash": before_hash, "afterHash": after_hash, "blobContent": blob_b64, } @@ -213,6 +271,9 @@ gem install --no-document --install-dir "$INSTALL_DIR" colorize -v 1.1.0 > /tmp/ GEM_FILE="$INSTALL_DIR/gems/colorize-1.1.0/lib/colorize.rb" [ -f "$GEM_FILE" ] || {{ echo "FAIL: $GEM_FILE missing" >&2; exit 1; }} echo "Installed to: $GEM_FILE" >&2 +# Keep a pristine copy: the explicit apply below is exercised against it +# after scan --sync's own nested apply has already patched the live file. +cp "$GEM_FILE" /tmp/pristine.rb # Pre-seed setup.manual so the agent-mode VEX leg keeps the gem patch through # property 7 (this project isn't `socket-patch setup`-configured; agent patches @@ -229,7 +290,19 @@ socket-patch scan --json --sync --yes \ --ecosystems gem > /tmp/scan.json 2>/tmp/sync.err cat /tmp/sync.err >&2 -socket-patch apply --json --force --offline --ecosystems gem > /tmp/apply.json 2>/tmp/apply.err +# The fixture serves the TRUE beforeHash, so scan --sync's own nested apply +# (which never uses --force) must pass the variant gate and patch the file +# in the same run — with an all-zeros placeholder this failed invisibly. +grep -q 'SOCKET-PATCH-E2E-MARKER' "$GEM_FILE" || {{ + echo "FAIL: scan --sync's nested apply left $GEM_FILE unpatched" >&2 + cat /tmp/scan.json >&2; head -3 "$GEM_FILE" >&2; exit 1; }} + +# Restore the pristine file so the explicit apply below exercises the +# default (gated, no --force) path end to end instead of short-circuiting +# on an already-patched file. +cp /tmp/pristine.rb "$GEM_FILE" + +socket-patch apply --json --offline --ecosystems gem > /tmp/apply.json 2>/tmp/apply.err APPLY_EXIT=$? cat /tmp/apply.err >&2 @@ -276,6 +349,9 @@ GEM_DIR=$(gem env gemdir) GEM_FILE="$GEM_DIR/gems/colorize-1.1.0/lib/colorize.rb" [ -f "$GEM_FILE" ] || {{ echo "FAIL: $GEM_FILE missing" >&2; exit 1; }} echo "Global-installed at: $GEM_FILE" >&2 +# Keep a pristine copy: the explicit apply below is exercised against it +# after scan --sync's own nested apply has already patched the live file. +cp "$GEM_FILE" /tmp/pristine.rb mkdir -p /workspace/proj && cd /workspace/proj @@ -285,7 +361,19 @@ socket-patch scan --json --sync --yes --global \ --ecosystems gem > /tmp/scan.json 2>/tmp/sync.err cat /tmp/sync.err >&2 -socket-patch apply --json --force --offline --global --ecosystems gem > /tmp/apply.json 2>/tmp/apply.err +# The fixture serves the TRUE beforeHash, so scan --sync's own nested apply +# (which never uses --force) must pass the variant gate and patch the file +# in the same run — with an all-zeros placeholder this failed invisibly. +grep -q 'SOCKET-PATCH-E2E-MARKER' "$GEM_FILE" || {{ + echo "FAIL: scan --sync's nested apply left $GEM_FILE unpatched" >&2 + cat /tmp/scan.json >&2; head -3 "$GEM_FILE" >&2; exit 1; }} + +# Restore the pristine file so the explicit apply below exercises the +# default (gated, no --force) path end to end instead of short-circuiting +# on an already-patched file. +cp /tmp/pristine.rb "$GEM_FILE" + +socket-patch apply --json --offline --global --ecosystems gem > /tmp/apply.json 2>/tmp/apply.err APPLY_EXIT=$? cat /tmp/apply.err >&2 {verify}"# @@ -390,12 +478,13 @@ async fn assert_api_path_exercised(server: &MockServer) { #[tokio::test] async fn gem_local_install_full_apply_chain() { - let after_hash = git_sha256(PATCHED_RB); - let server = make_mock_server(&after_hash).await; - let api_url = format!("http://host.docker.internal:{}", server.address().port()); if skip_if_no_image() { return; } + let before_hash = upstream_before_hash(); + let after_hash = git_sha256(PATCHED_RB); + let server = make_mock_server(&before_hash, &after_hash).await; + let api_url = format!("http://host.docker.internal:{}", server.address().port()); let expected_sha = plain_sha256(PATCHED_RB); let out = run_container(&local_script(&api_url, &expected_sha)); let stdout = String::from_utf8_lossy(&out.stdout); @@ -418,12 +507,13 @@ async fn gem_local_install_full_apply_chain() { #[tokio::test] async fn gem_global_install_full_apply_chain() { - let after_hash = git_sha256(PATCHED_RB); - let server = make_mock_server(&after_hash).await; - let api_url = format!("http://host.docker.internal:{}", server.address().port()); if skip_if_no_image() { return; } + let before_hash = upstream_before_hash(); + let after_hash = git_sha256(PATCHED_RB); + let server = make_mock_server(&before_hash, &after_hash).await; + let api_url = format!("http://host.docker.internal:{}", server.address().port()); let expected_sha = plain_sha256(PATCHED_RB); let out = run_container(&global_script(&api_url, &expected_sha)); let stdout = String::from_utf8_lossy(&out.stdout); diff --git a/crates/socket-patch-cli/tests/e2e_gem.rs b/crates/socket-patch-cli/tests/e2e_gem.rs index f9dc8120..9fb92a72 100644 --- a/crates/socket-patch-cli/tests/e2e_gem.rs +++ b/crates/socket-patch-cli/tests/e2e_gem.rs @@ -94,6 +94,11 @@ fn assert_run_ok(cwd: &Path, args: &[&str], context: &str) -> (String, String) { fn bundle_run(cwd: &Path, args: &[&str]) { let mut cmd = Command::new("bundle"); cmd.args(args).current_dir(cwd); + // Bundler 4 removed `bundle install --path`; BUNDLE_PATH is honored by + // bundler 2 through 4 and keeps the vendor/bundle/ruby/*/gems layout + // `find_gem_dir` expects. It also upholds cache_env's hermeticity + // invariant that every `bundle install` pins its gem tree to the fixture. + cmd.env("BUNDLE_PATH", "vendor/bundle"); cache_env::isolate(&mut cmd); let out = cmd.output().expect("failed to run bundle"); assert!( @@ -449,7 +454,7 @@ fn test_gem_full_lifecycle() { // -- Setup: create project and install activestorage@5.2.0 ---------------- write_gemfile(cwd); - bundle_run(cwd, &["install", "--path", "vendor/bundle"]); + bundle_run(cwd, &["install"]); let gem_dir = find_gem_dir(cwd); @@ -540,7 +545,7 @@ fn test_gem_dry_run() { let cwd = dir.path(); write_gemfile(cwd); - bundle_run(cwd, &["install", "--path", "vendor/bundle"]); + bundle_run(cwd, &["install"]); let gem_dir = find_gem_dir(cwd); @@ -579,7 +584,7 @@ fn test_gem_save_only() { let cwd = dir.path(); write_gemfile(cwd); - bundle_run(cwd, &["install", "--path", "vendor/bundle"]); + bundle_run(cwd, &["install"]); let gem_dir = find_gem_dir(cwd); diff --git a/crates/socket-patch-cli/tests/e2e_hosted_production.rs b/crates/socket-patch-cli/tests/e2e_hosted_production.rs index 1356509e..437b3092 100644 --- a/crates/socket-patch-cli/tests/e2e_hosted_production.rs +++ b/crates/socket-patch-cli/tests/e2e_hosted_production.rs @@ -558,6 +558,82 @@ async fn published_patch_dates(purl: &str) -> Result, Stri .unwrap_or_default()) } +/// `GET /patch/view/` against the real proxy — the same route the +/// CLI's free-proxy client fetches patch content from. Returns, per file the +/// patch touches, the `(path, beforeHash, afterHash)` triple (hashes are +/// git-blob sha256, `None` for pure additions/deletions respectively). +async fn published_patch_files( + uuid: &str, +) -> Result, Option)>, String> { + let url = format!("{PROXY}/patch/view/{uuid}"); + let resp = reqwest::Client::new() + .get(&url) + .header("Accept", "application/json") + .send() + .await + .map_err(|e| format!("GET {url}: {e}"))?; + let status = resp.status(); + let body = resp + .text() + .await + .map_err(|e| format!("GET {url}: reading body: {e}"))?; + if !status.is_success() { + return Err(format!("GET {url}: HTTP {status}\n{body}")); + } + let v: serde_json::Value = + serde_json::from_str(&body).map_err(|e| format!("GET {url}: bad JSON ({e}):\n{body}"))?; + Ok(v["files"] + .as_object() + .map(|m| { + m.iter() + .map(|(path, f)| { + ( + path.clone(), + f["beforeHash"].as_str().map(str::to_string), + f["afterHash"].as_str().map(str::to_string), + ) + }) + .collect() + }) + .unwrap_or_default()) +} + +/// The `sha256=` hex value the lock's CHECKSUMS section pins for +/// ` ()`, or `None` when the entry is absent. Bundler >= 2.6 +/// writes one two-space-indented ` name (version) sha256=` line per +/// resolved gem. +fn gem_lock_checksum(lock: &str, name: &str, version: &str) -> Option { + let prefix = format!(" {name} ({version}) sha256="); + lock.lines() + .find_map(|l| l.strip_prefix(&prefix).map(|h| h.trim().to_string())) +} + +/// Locate the bundler-installed `gems/-` directory under a +/// `BUNDLE_PATH` root. The `ruby/` segment in between varies by host +/// interpreter, so walk for it (depth-bounded — the layout is only a few +/// levels deep) instead of hardcoding the version. +fn installed_gem_dir(root: &Path, dir_name: &str, depth: usize) -> Option { + for entry in std::fs::read_dir(root).ok()?.flatten() { + let p = entry.path(); + if !p.is_dir() { + continue; + } + if p.file_name().is_some_and(|n| n == dir_name) + && p.parent() + .and_then(|d| d.file_name()) + .is_some_and(|n| n == "gems") + { + return Some(p); + } + if depth > 0 { + if let Some(found) = installed_gem_dir(&p, dir_name, depth - 1) { + return Some(found); + } + } + } + None +} + /// The Socket patch-registry base URL the gem rewriter pinned into `Gemfile` /// as `source "" do`, or `None` when no Socket source block is present. /// @@ -1621,6 +1697,19 @@ async fn gem_bundler_hosted_redirect_and_known_install_defect() { CHECKSUMS section)" ); } + // Capture the UPSTREAM checksum pin before any redirect. Bundler installs + // whatever matches this pin, so the redirect must replace it with the + // patched artifact's digest — the assertion after the redirect below is + // what makes an inert rewrite (URL repointed, upstream digest kept, so + // bundler verify-and-installs the UNPATCHED gem) go red instead of green. + let pristine_lock = read(&proj.join("Gemfile.lock")); + let upstream_sha = + gem_lock_checksum(&pristine_lock, GEM_NAME, GEM_VERSION).unwrap_or_else(|| { + panic!( + "{LEG}: `bundle lock --add-checksums` wrote no sha256 CHECKSUMS \ + entry for {GEM_NAME} ({GEM_VERSION}):\n{pristine_lock}" + ) + }); let install = tool(&proj, "bundle", &["install", "--quiet"], &env); if !ok(&install) { soft_skip!(LEG, "upstream `bundle install` failed:\n{}", dump(&install)); @@ -1642,6 +1731,19 @@ async fn gem_bundler_hosted_redirect_and_known_install_defect() { lock.contains("CHECKSUMS"), "{LEG}: Gemfile.lock lost its CHECKSUMS section:\n{lock}" ); + let redirected_sha = gem_lock_checksum(&lock, GEM_NAME, GEM_VERSION).unwrap_or_else(|| { + panic!( + "{LEG}: redirected Gemfile.lock carries no sha256 CHECKSUMS entry \ + for {GEM_NAME} ({GEM_VERSION}):\n{lock}" + ) + }); + assert_ne!( + redirected_sha, upstream_sha, + "{LEG}: the redirect left {GEM_NAME}'s CHECKSUMS pin at the UPSTREAM \ + sha256 — bundler would verify and install the unpatched artifact, so \ + the hosted patch is inert (the same blindspot that let an inert npm \ + patch stay green).\nGemfile.lock:\n{lock}" + ); // Known-broken leg: reinstall from the redirected Gemfile. std::fs::remove_dir_all(&bundle_path).ok(); @@ -1658,6 +1760,73 @@ async fn gem_bundler_hosted_redirect_and_known_install_defect() { appears to be FIXED — delete the tolerance branch in this test and \ assert unconditionally." ); + // Exit 0 proves only that bundler fetched an artifact matching the + // CHECKSUMS pin. Close the loop on CONTENT: fetch the patch's file + // manifest from the proxy and assert every file it rewrites landed + // on disk byte-exact (afterHash is the git-blob sha256 the patch + // service publishes — the same digest the CLI's apply verifies). + let patch_files = published_patch_files(GEM_UUID).await.unwrap_or_else(|e| { + panic!( + "{LEG}: `bundle install` from the redirected Gemfile succeeded \ + but the patch file manifest could not be fetched to verify \ + the installed content: {e}" + ) + }); + let gem_dir = installed_gem_dir( + Path::new(&bundle_path), + &format!("{GEM_NAME}-{GEM_VERSION}"), + 4, + ) + .unwrap_or_else(|| { + panic!( + "{LEG}: `bundle install` succeeded but no \ + gems/{GEM_NAME}-{GEM_VERSION} directory exists under \ + {bundle_path}" + ) + }); + use socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes; + let mut verified = 0usize; + let mut rewritten = 0usize; + for (path, before, after) in &patch_files { + // No afterHash = the patch deletes the file; nothing to hash. + let Some(after) = after else { continue }; + let rel = path.strip_prefix("package/").unwrap_or(path.as_str()); + let installed = gem_dir.join(rel); + let bytes = std::fs::read(&installed).unwrap_or_else(|e| { + panic!( + "{LEG}: patch {GEM_UUID} rewrites `{path}` but the \ + installed gem has no readable {}: {e}", + installed.display() + ) + }); + assert_eq!( + compute_git_sha256_from_bytes(&bytes), + *after, + "{LEG}: installed {} does not hash to the patch's afterHash — \ + bundler fetched an artifact whose content is NOT the \ + published patch", + installed.display() + ); + verified += 1; + if before.as_deref() != Some(after.as_str()) { + rewritten += 1; + } + } + assert!( + verified >= 1, + "{LEG}: patch {GEM_UUID} names no files with an afterHash, so \ + nothing was content-verified — the install success is vacuous" + ); + assert!( + rewritten >= 1, + "{LEG}: every file in patch {GEM_UUID} has afterHash == \ + beforeHash — the published patch is inert and this install \ + proved nothing" + ); + println!( + "{LEG}: verified {verified} patched file(s) on disk against the \ + published afterHash ({rewritten} differ from upstream)" + ); return; } let detail = dump(&reinstall); diff --git a/crates/socket-patch-cli/tests/in_process_get_manifest_path.rs b/crates/socket-patch-cli/tests/in_process_get_manifest_path.rs index e8c888cf..00fac6f9 100644 --- a/crates/socket-patch-cli/tests/in_process_get_manifest_path.rs +++ b/crates/socket-patch-cli/tests/in_process_get_manifest_path.rs @@ -18,6 +18,14 @@ //! `get --cwd proj ` made the nested apply look for //! `proj/proj/.socket/manifest.json`, hit the no-manifest clean no-op, //! and report success (`applied: 1`, exit 0) without patching anything. +//! +//! 3. `run_nested_apply` threaded cwd/global/silent/download-mode/strict +//! and the four API flags into the nested `ApplyArgs` but left +//! `ecosystems` at `GlobalArgs::default()` (`None`), so a download +//! scoped with `--ecosystems ` (`scan --ecosystems gem --sync`, or +//! `get --ecosystems npm `) ran its apply step UNSCOPED over the +//! whole manifest — mutating other ecosystems' packages the user had +//! explicitly filtered out. use std::path::Path; @@ -251,3 +259,176 @@ async fn get_with_relative_cwd_actually_applies() { against --cwd twice and apply no-op'd on a missing manifest" ); } + +// --------------------------------------------------------------------------- +// 4. --ecosystems must scope the nested apply, not just the download +// --------------------------------------------------------------------------- + +const COMPOSER_PURL: &str = "pkg:composer/acme/lib@1.0.0"; +const COMPOSER_UUID: &str = "44444444-4444-4444-8444-444444444444"; +const COMPOSER_ORIGINAL: &[u8] = b" (std::path::PathBuf, std::path::PathBuf) { + // The npm target package, installed with pre-patch content. + std::fs::write( + root.join("package.json"), + r#"{"name":"r","version":"0.0.0"}"#, + ) + .unwrap(); + let npm_pkg = root.join("node_modules/manifest-path-test"); + std::fs::create_dir_all(&npm_pkg).unwrap(); + std::fs::write( + npm_pkg.join("package.json"), + r#"{"name":"manifest-path-test","version":"1.0.0"}"#, + ) + .unwrap(); + std::fs::write(npm_pkg.join("index.js"), ORIGINAL).unwrap(); + + // A composer package installed side by side (the composer crawler + // probes `/vendor/composer/installed.json` plus a composer.json + // marker — fully deterministic, no tooling or env vars involved). + std::fs::write(root.join("composer.json"), "{}").unwrap(); + let composer_dir = root.join("vendor/composer"); + std::fs::create_dir_all(&composer_dir).unwrap(); + std::fs::write( + composer_dir.join("installed.json"), + r#"{"packages": [{"name": "acme/lib", "version": "1.0.0"}]}"#, + ) + .unwrap(); + let composer_pkg = root.join("vendor/acme/lib"); + std::fs::create_dir_all(&composer_pkg).unwrap(); + std::fs::write(composer_pkg.join("index.php"), COMPOSER_ORIGINAL).unwrap(); + + // Manifest pre-seeded with the PENDING composer patch record and its + // cached after-blob (no network fetch needed to apply it). + let composer_before_hash = git_sha256(COMPOSER_ORIGINAL); + let composer_after_hash = git_sha256(COMPOSER_PATCHED); + let socket = root.join(".socket"); + std::fs::create_dir_all(socket.join("blobs")).unwrap(); + std::fs::write( + socket.join("blobs").join(&composer_after_hash), + COMPOSER_PATCHED, + ) + .unwrap(); + std::fs::write( + socket.join("manifest.json"), + format!( + r#"{{ "patches": {{ + "{COMPOSER_PURL}": {{ + "uuid": "{COMPOSER_UUID}", + "exportedAt": "2024-01-01T00:00:00Z", + "files": {{ + "package/index.php": {{ + "beforeHash": "{composer_before_hash}", + "afterHash": "{composer_after_hash}" + }} + }}, + "vulnerabilities": {{}}, + "description": "pending composer patch", "license": "MIT", "tier": "free" + }} + }}}}"# + ), + ) + .unwrap(); + + (npm_pkg, composer_pkg) +} + +/// A download restricted with `--ecosystems npm` must run its nested +/// apply under the SAME scope: the pending composer patch staged by +/// [`stage_npm_and_composer_project`] must stay unapplied while the npm +/// package is patched. (The control test below proves the fixture bites — +/// an UNSCOPED run does patch the composer package.) +#[tokio::test] +#[serial] +async fn get_ecosystems_flag_scopes_nested_apply() { + let server = MockServer::start().await; + mount_view_mock(&server).await; + mount_search_mock(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + let (npm_pkg, composer_pkg) = stage_npm_and_composer_project(tmp.path()); + + let mut args = get_args(PURL, tmp.path(), server.uri()); + args.save_only = false; // exercise the nested apply step + args.common.ecosystems = Some(vec!["npm".to_string()]); + + let code = run(args).await; + assert_eq!(code, 0, "scoped get + apply must succeed"); + + // The in-scope npm package was patched... + assert_eq!( + std::fs::read(npm_pkg.join("index.js")).unwrap(), + PATCHED, + "the npm package selected by --ecosystems npm must be patched" + ); + // ...and the out-of-scope composer package was NOT: patching it means + // `ecosystems` was dropped when building the nested ApplyArgs and the + // apply step ran unscoped over the whole manifest. + assert_eq!( + std::fs::read(composer_pkg.join("index.php")).unwrap(), + COMPOSER_ORIGINAL, + "--ecosystems npm must scope the nested apply: the pending \ + composer patch must not be applied" + ); +} + +/// Negative control for the scoped test above: WITHOUT `--ecosystems`, +/// the very same fixture's nested apply covers the whole manifest and +/// patches the pending composer package too. This pins that the fixture +/// genuinely can reach the composer package — if this stopped holding, +/// the scoped test would pass vacuously. +#[tokio::test] +#[serial] +async fn get_without_ecosystems_applies_whole_manifest() { + let server = MockServer::start().await; + mount_view_mock(&server).await; + mount_search_mock(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + let (npm_pkg, composer_pkg) = stage_npm_and_composer_project(tmp.path()); + + let mut args = get_args(PURL, tmp.path(), server.uri()); + args.save_only = false; // exercise the nested apply step + + let code = run(args).await; + assert_eq!(code, 0, "unscoped get + apply must succeed"); + + assert_eq!( + std::fs::read(npm_pkg.join("index.js")).unwrap(), + PATCHED, + "the npm package must be patched" + ); + assert_eq!( + std::fs::read(composer_pkg.join("index.php")).unwrap(), + COMPOSER_PATCHED, + "without --ecosystems the nested apply covers the whole manifest, \ + including the pending composer patch" + ); +} diff --git a/crates/socket-patch-cli/tests/in_process_get_update_count.rs b/crates/socket-patch-cli/tests/in_process_get_update_count.rs index 04bb8ccf..6756f29a 100644 --- a/crates/socket-patch-cli/tests/in_process_get_update_count.rs +++ b/crates/socket-patch-cli/tests/in_process_get_update_count.rs @@ -73,6 +73,7 @@ fn params(root: &Path, server: &MockServer) -> DownloadParams { proxy_url: None, }, strict: false, + ecosystems: None, persist_blobs: true, // Skip release-narrowing; npm has no variants anyway. all_releases: true, diff --git a/crates/socket-patch-cli/tests/in_process_scan.rs b/crates/socket-patch-cli/tests/in_process_scan.rs index 622d2cb4..96cb0111 100644 --- a/crates/socket-patch-cli/tests/in_process_scan.rs +++ b/crates/socket-patch-cli/tests/in_process_scan.rs @@ -1433,3 +1433,110 @@ async fn scan_discovers_maven_and_nuget_in_every_mode() { } std::env::remove_var("MAVEN_REPO_LOCAL"); } + +// --------------------------------------------------------------------------- +// Regression: `scan --vendor --dry-run --vex` must skip the embedded VEX. +// +// The vendor JSON dry-run arm handed base_code 0 straight to +// `embed_vex_into_json`, which generated the document for real: on a +// not-yet-vendored project (no manifest) generation failed and flipped the +// whole dry run to exit 1; with attestable state it wrote the VEX file to +// disk — a mutation during --dry-run. Both must be skipped instead +// (apply.rs's precedent: a dry run has no just-vendored state to attest). +// --------------------------------------------------------------------------- + +#[tokio::test] +#[serial] +async fn scan_vendor_dry_run_with_vex_does_not_fail_on_not_yet_vendored() { + let server = MockServer::start().await; + mock_batch_one(&server).await; + mock_by_package(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "in-proc-scan", "1.0.0"); + + let vex_path = tmp.path().join("vendor-dry.vex.json"); + let mut args = default_args(tmp.path()); + args.common.api_url = Some(server.uri()); + args.vendor = true; + args.common.dry_run = true; + args.vex.vex = Some(vex_path.clone()); + + let code = run_scrubbed(args).await; + // Nothing is vendored yet (no manifest, no ledger) — that is the normal + // state a vendor dry run previews from, not an error. The old behavior + // generated the VEX for real, hit `manifest_not_found`, and exited 1. + assert_eq!( + code, 0, + "a vendor dry run must not fail for not-yet-vendored state" + ); + assert!(!vex_path.exists(), "dry run must not write the VEX file"); + // Prove the vendor dry-run path was actually entered (not + // short-circuited before discovery): batch + patch details were queried. + let reqs = recorded(&server).await; + assert!( + batch_posts(&reqs).len() == 1 && by_package_gets(&reqs) >= 1, + "vendor dry run must query batch + patch details; batch={}, by_package={}", + batch_posts(&reqs).len(), + by_package_gets(&reqs), + ); +} + +#[tokio::test] +#[serial] +async fn scan_vendor_dry_run_with_vex_does_not_write_attestation_file() { + let server = MockServer::start().await; + mock_batch_one(&server).await; + mock_by_package(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "in-proc-scan", "1.0.0"); + + // A manifest whose sole record WOULD attest successfully: vulnerability + // metadata for the statement, `setup.manual: ["npm"]` to pass the + // property-7 ecosystem filter, and `--vex-no-verify` below to skip the + // on-disk hash check. Under the old behavior the dry run generated the + // document for real and wrote it to disk. + let socket = tmp.path().join(".socket"); + std::fs::create_dir_all(&socket).unwrap(); + std::fs::write( + socket.join("manifest.json"), + r#"{ "patches": { + "pkg:npm/in-proc-scan@1.0.0": { + "uuid": "11111111-1111-4111-8111-111111111111", + "exportedAt": "2024-01-01T00:00:00Z", + "files": {}, + "vulnerabilities": { "GHSA-aaaa-bbbb-cccc": { + "cves": ["CVE-2024-0001"], "summary": "s", + "severity": "HIGH", "description": "d" + }}, + "description": "x", "license": "MIT", "tier": "free" + } + }, "setup": { "manual": ["npm"] } }"#, + ) + .unwrap(); + let before = std::fs::read_to_string(socket.join("manifest.json")).unwrap(); + + let vex_path = tmp.path().join("vendor-dry.vex.json"); + let mut args = default_args(tmp.path()); + args.common.api_url = Some(server.uri()); + args.vendor = true; + args.common.dry_run = true; + args.vex.vex = Some(vex_path.clone()); + args.vex.vex_no_verify = true; + + let code = run_scrubbed(args).await; + assert_eq!(code, 0, "vendor dry run with attestable state must exit 0"); + assert!( + !vex_path.exists(), + "dry run must not write the VEX file even when generation would succeed" + ); + // The dry run's non-mutation contract extends to the manifest. + let after = std::fs::read_to_string(socket.join("manifest.json")).unwrap(); + assert_eq!( + after, before, + "vendor dry run must leave the manifest byte-for-byte unchanged" + ); +} diff --git a/crates/socket-patch-cli/tests/setup_matrix_gem.rs b/crates/socket-patch-cli/tests/setup_matrix_gem.rs index 06711f6a..c8d68164 100644 --- a/crates/socket-patch-cli/tests/setup_matrix_gem.rs +++ b/crates/socket-patch-cli/tests/setup_matrix_gem.rs @@ -2,8 +2,20 @@ //! — it appends a managed `plugin "socket-patch"` block to the Gemfile and //! generates a committed in-tree Bundler plugin under `.socket/bundler-plugin/` //! whose `plugins.rb` re-runs `socket-patch apply --ecosystems gem` on every -//! `bundle install` (load-time digest gate + `after-install-all` hook). So the -//! with-setup cases are no longer a baseline gap. +//! `bundle install` (load-time digest gate + `after-install-all` hook). +//! +//! The with-setup Docker cases (`baseline_with_setup`, `alt_content_patchset`) +//! are still a [BASELINE GAP], for two structural reasons (verified +//! 2026-08-13): (a) installing the plugin evaluates `plugins.rb` BEFORE any +//! project gems land, and the load-time `SocketPatch.apply!` treats apply's +//! exit 1 ("No packages found that match available patches") as a genuine +//! failure and raises `Bundler::BundlerError`, so the FIRST `bundle install` +//! after `setup` on a never-installed project always dies — this is the gem +//! twin of the documented apply exit-semantics issue; (b) the matrix fixture's +//! synthetic beforeHashes only pass hash-gated ecosystems via npm's +//! mismatch-warn-and-apply path, which gem apply does not have. The +//! `after-install-all` re-apply flow itself works (asserted in-container with +//! realistic hashes). //! //! IMPORTANT — why this file carries a real assertion of its own: //! `smc::run_pm("gem", "bundler")` routes gem through the shared Docker diff --git a/crates/socket-patch-core/src/crawlers/ruby_crawler.rs b/crates/socket-patch-core/src/crawlers/ruby_crawler.rs index 8a6fed15..3d5e4892 100644 --- a/crates/socket-patch-core/src/crawlers/ruby_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/ruby_crawler.rs @@ -22,7 +22,7 @@ impl RubyCrawler { /// Get gem installation paths based on options. /// - /// In local mode, checks `vendor/bundle/ruby/*/gems/` first (Bundler + /// In local mode, checks `vendor/bundle//*/gems/` first (Bundler /// deployment layout), then — only if the cwd holds a Bundler manifest /// or lockfile — falls back to the gem homes `gem env` reports. /// @@ -178,18 +178,31 @@ impl RubyCrawler { paths } - /// Find `vendor/bundle/ruby/*/gems/` directories. + /// Find `vendor/bundle//*/gems/` directories. + /// + /// Bundler's deployment scope is `#{Gem.ruby_engine}/#{ruby_version}` + /// (`Bundler.ruby_scope`) — `ruby` under MRI, but `jruby`/`truffleruby` + /// under the alternative engines. Hardcoding `ruby` made JRuby and + /// TruffleRuby deployments discover zero gems, so enumerate every + /// engine dir that holds `/gems/` children; non-engine + /// clutter is filtered by that shape. async fn get_vendor_bundle_paths(cwd: &Path) -> Vec { - let vendor_ruby = cwd.join("vendor").join("bundle").join("ruby"); + let vendor_bundle = cwd.join("vendor").join("bundle"); let mut paths = Vec::new(); - for entry in list_dir_entries(&vendor_ruby).await { - if !entry_is_dir(&entry).await { + for engine_entry in list_dir_entries(&vendor_bundle).await { + if !entry_is_dir(&engine_entry).await { continue; } - let gems_dir = vendor_ruby.join(entry.file_name()).join("gems"); - if is_dir(&gems_dir).await { - paths.push(gems_dir); + let engine_dir = vendor_bundle.join(engine_entry.file_name()); + for entry in list_dir_entries(&engine_dir).await { + if !entry_is_dir(&entry).await { + continue; + } + let gems_dir = engine_dir.join(entry.file_name()).join("gems"); + if is_dir(&gems_dir).await { + paths.push(gems_dir); + } } } paths @@ -338,22 +351,41 @@ impl RubyCrawler { /// /// Gem directories follow `-` (ruby-platform gems) or /// `--` (platform gems, e.g. - /// `nokogiri-1.16.5-x86_64-linux`). The name/version boundary is the - /// **first** `-` followed by a digit. A RubyGems version is dash-free - /// (prerelease dashes render as `.pre.`), so the version is the run up - /// to the next `-`; anything after that is the platform suffix, which - /// we drop — the installed platform is resolved later by hashing the - /// gem's files (the same model as PyPI's `artifact_id`). The qualified - /// `?platform=` PURL is only ever carried in the manifest/API. + /// `nokogiri-1.16.5-x86_64-linux`). A RubyGems version is dash-free + /// (prerelease dashes render as `.pre.`), so every `-` followed by a + /// digit is a candidate name/version boundary and the version is the + /// dash-free token after it; anything past that is the platform + /// suffix, which we drop — the installed platform is resolved later by + /// hashing the gem's files (the same model as PyPI's `artifact_id`). + /// The qualified `?platform=` PURL is only ever carried in the + /// manifest/API. + /// + /// Names may themselves contain `-` runs (`http-2`, + /// `http-2-next`), so the first candidate boundary is not always + /// right: `http-2-1.0.1` must parse as `("http-2", "1.0.1")`, not the + /// ghost `("http", "2")`. Real versions are almost always dotted while + /// digit runs embedded in names (`-2-`) and trailing platform OS + /// revisions (`-darwin-21`) are not, so prefer the LAST boundary whose + /// version token contains a `.`; fall back to the first dash-digit + /// boundary only when no dotted candidate exists (a bare + /// single-segment version like `g-1` is legal but vanishingly rare). fn parse_dir_name_version(dir_name: &str) -> Option<(String, String)> { - let idx = dir_name + let candidates: Vec = dir_name .match_indices('-') - .find(|(i, _)| dir_name[i + 1..].starts_with(|c: char| c.is_ascii_digit())) - .map(|(i, _)| i)?; - let name = &dir_name[..idx]; - let rest = &dir_name[idx + 1..]; + .filter(|(i, _)| dir_name[i + 1..].starts_with(|c: char| c.is_ascii_digit())) + .map(|(i, _)| i) + .collect(); // Version is the leading dash-free token; drop any `-`. - let version = rest.split('-').next().unwrap_or(rest); + let version_token = |i: usize| { + let rest = &dir_name[i + 1..]; + rest.split('-').next().unwrap_or(rest) + }; + let idx = *candidates + .iter() + .rfind(|&&i| version_token(i).contains('.')) + .or_else(|| candidates.first())?; + let name = &dir_name[..idx]; + let version = version_token(idx); if name.is_empty() || version.is_empty() { return None; } @@ -556,6 +588,34 @@ mod tests { assert_eq!(paths[0], vendor_gems); } + /// Bundler's deployment scope is `/` — `jruby` and + /// `truffleruby` deployments live beside `ruby` under `vendor/bundle` + /// and must be discovered too (hardcoding `ruby` found zero gems + /// there). Non-engine clutter — files, and dirs whose children hold no + /// `gems/` — must not produce paths. + #[tokio::test] + async fn test_get_vendor_bundle_paths_alternative_engines() { + let dir = tempfile::tempdir().unwrap(); + let bundle = dir.path().join("vendor").join("bundle"); + let ruby_gems = bundle.join("ruby").join("3.2.0").join("gems"); + let jruby_gems = bundle.join("jruby").join("3.1.4.0").join("gems"); + let truffle_gems = bundle.join("truffleruby").join("3.2.2").join("gems"); + for gems in [&ruby_gems, &jruby_gems, &truffle_gems] { + tokio::fs::create_dir_all(gems).await.unwrap(); + } + tokio::fs::write(bundle.join("install.log"), b"x") + .await + .unwrap(); + tokio::fs::create_dir_all(bundle.join("cache").join("3.2.0")) + .await + .unwrap(); + + let paths = RubyCrawler::get_vendor_bundle_paths(dir.path()).await; + assert_eq!(paths.len(), 3, "one gems dir per engine; got {paths:?}"); + let found: HashSet = paths.into_iter().collect(); + assert_eq!(found, HashSet::from([ruby_gems, jruby_gems, truffle_gems])); + } + #[tokio::test] async fn test_deduplication() { let dir = tempfile::tempdir().unwrap(); @@ -980,9 +1040,65 @@ mod tests { assert!(!is_safe_gem_coordinate("rails", "C:1.0.0")); } + /// Names with embedded `-` runs (`http-2`, `http-2-next`) must + /// keep the digits in the name: the boundary is the LAST dash-digit + /// whose version token is dotted, not the first dash-digit. Without + /// that preference `http-2-1.0.1` parsed as `("http", "2")` — a ghost + /// PURL — and the real gem was never discovered. + #[test] + fn parse_dir_name_version_prefers_last_dotted_boundary() { + assert_eq!( + RubyCrawler::parse_dir_name_version("http-2-1.0.1"), + Some(("http-2".to_string(), "1.0.1".to_string())) + ); + assert_eq!( + RubyCrawler::parse_dir_name_version("http-2-next-1.0.3"), + Some(("http-2-next".to_string(), "1.0.3".to_string())) + ); + // A platform suffix after the real version still drops. + assert_eq!( + RubyCrawler::parse_dir_name_version("http-2-1.0.1-java"), + Some(("http-2".to_string(), "1.0.1".to_string())) + ); + } + + /// The dotted-boundary preference must not regress the plain shapes: + /// dotted versions, prereleases, platform dirs, and — via the + /// first-boundary fallback — bare single-segment versions (legal per + /// RubyGems, just vanishingly rare). + #[test] + fn parse_dir_name_version_boundary_shapes() { + assert_eq!( + RubyCrawler::parse_dir_name_version("rack-3.1.0"), + Some(("rack".to_string(), "3.1.0".to_string())) + ); + assert_eq!( + RubyCrawler::parse_dir_name_version("aws-sdk-s3-1.140.0"), + Some(("aws-sdk-s3".to_string(), "1.140.0".to_string())) + ); + assert_eq!( + RubyCrawler::parse_dir_name_version("gem2-1.0"), + Some(("gem2".to_string(), "1.0".to_string())) + ); + // No dotted candidate → first dash-digit boundary fallback. + assert_eq!( + RubyCrawler::parse_dir_name_version("g-1"), + Some(("g".to_string(), "1".to_string())) + ); + // Prerelease dashes render as dots, so the token stays dotted. + assert_eq!( + RubyCrawler::parse_dir_name_version("rails-7.1.0.beta1"), + Some(("rails".to_string(), "7.1.0.beta1".to_string())) + ); + assert_eq!( + RubyCrawler::parse_dir_name_version("nokogiri-1.16.0-arm64-darwin"), + Some(("nokogiri".to_string(), "1.16.0".to_string())) + ); + } + /// Gem names with embedded underscores/digits and multi-dash names - /// must keep their full name; the version starts at the first - /// dash-then-digit boundary. + /// must keep their full name; the version starts at the dash-then-digit + /// boundary that opens the dotted version token. #[test] fn parse_dir_name_version_name_shapes() { assert_eq!( diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 0ff40eb5..87740fcf 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -1673,6 +1673,31 @@ pub(crate) fn gem_line_trailing_options(tail: &str) -> String { } } +/// 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 +/// OVERRIDES the block and the redirect becomes a silent no-op. Mirrors the +/// token list `vendor::gem::rest_blocks_edit` refuses for the same reason. +fn gem_tail_source_option(tail: &str) -> Option<&'static str> { + let code = tail.split('#').next().unwrap_or(""); + [ + "path:", + ":path", + "git:", + ":git", + "github:", + ":github", + "source:", + ":source", + "gist:", + ":gist", + "bitbucket:", + ":bitbucket", + ] + .into_iter() + .find(|tok| code.contains(tok)) +} + fn rewrite_gem( files: &BTreeMap, overrides: &[DepOverride], @@ -1713,35 +1738,159 @@ fn rewrite_gem( continue; }; + // Platform-suffixed CHECKSUMS siblings (`name (version-arm64-darwin) + // sha256=`) mean bundler resolves platform-specific gems the patch + // registry does not serve — redirecting would pin the bare-platform + // sha while installs keep fetching the upstream platform gem + // (guaranteed mismatch or a silently unpatched install). Fail closed: + // skip the dep entirely. + if let Some(lk) = lock.as_deref() { + let platform_re = Regex::new( + &(String::from(r"(?m)^ ") + + ®ex::escape(&dep.name) + + r" \(" + + ®ex::escape(&dep.version) + + r"-[^)]+\) sha256="), + ) + .unwrap(); + if platform_re.is_match(lk) { + result.warnings.push(RewriteWarning { + code: "redirect_gem_platform_unsupported".into(), + detail: format!( + "Gemfile.lock CHECKSUMS carries platform-specific entries for {} {} — \ + the patch registry serves only the ruby platform gem; redirect skipped", + dep.name, dep.version + ), + }); + continue; + } + } + + // Whether THIS dep's Gemfile source redirect is in place (just + // written or already present) — the lock pin below is gated on it. + let mut source_placed = false; if let Some(gf) = gemfile.as_mut() { - if !gf.contains(&format!("source \"{}\"", ov.index_url)) { + // Grant-agnostic idempotency guard: the grant-token (and patch + // uuid) segments of the index URL rotate per request, so an + // exact-URL check misses the block a previous run wrote and this + // run would wrap the gem line inside it — nesting source blocks. + // Wildcard the rotating segments instead (mirrors the CHECKSUMS + // at-target guard below). + let mut url_pat = regex::escape(&ov.index_url); + for rotating in [&dep.token, &dep.patch_uuid] { + if !rotating.is_empty() { + url_pat = + url_pat.replace(®ex::escape(&format!("/{rotating}/")), "/[^/\"]+/"); + } + } + let block_re = Regex::new( + &(String::from(r#"(?m)^source "("#) + + &url_pat + + r#")" do\n gem ["']"# + + ®ex::escape(&dep.name) + + r#"["']"#), + ) + .unwrap(); + if let Some(m) = block_re.captures(gf) { + let url = m.get(1).unwrap(); + if url.as_str() == ov.index_url { + source_placed = true; + } else { + // Rotated grant: refresh the URL in place — never nest. + let (range, old_url) = (url.range(), url.as_str().to_string()); + gf.replace_range(range, &ov.index_url); + gemfile_changed = true; + result.edits.push(FileEdit { + path: "Gemfile".into(), + kind: "redirect_gemfile_source_url".into(), + action: "rewritten".into(), + key: Some(dep.name.clone()), + original: Some(Value::String(old_url)), + new: Some(Value::String(ov.index_url.clone())), + }); + source_placed = true; + } + } else { + // Tolerate the legal spellings of a declaration: tab / extra + // spaces after `gem`, and the parenthesized call form. let gem_line_re = Regex::new( - &(String::from(r#"(?m)^\s*gem ["']"#) + &(String::from(r#"(?m)^\s*gem(?:[ \t]*(\()[ \t]*|[ \t]+)["']"#) + ®ex::escape(&dep.name) + r#"["']([^\n]*)$"#), ) .unwrap(); - let block = format!( - "source \"{}\" do\n gem \"{}\", \"{}\"\nend", - ov.index_url, dep.name, dep.version - ); + // Looser "declared at all?" probe: gates the append branch — + // appending next to a declaration the recognizer above cannot + // parse would leave the gem declared twice (bundler + // hard-fails on the duplicate). + let declared_re = Regex::new( + &(String::from(r#"(?m)^[ \t]*gem\b[^\n]*["']"#) + + ®ex::escape(&dep.name) + + r#"["']"#), + ) + .unwrap(); if let Some(m) = gem_line_re.captures(gf) { + let range = m.get(0).unwrap().range(); let original = m.get(0).unwrap().as_str().to_string(); + let paren = m.get(1).is_some(); + let raw_tail = m.get(2).unwrap().as_str().to_string(); + // A parenthesized call keeps its closing `)` in the tail: + // strip it (dropping any comment with it), or fail closed + // when it is absent (the call continues past this line). + let tail = if paren { + let code = raw_tail.split('#').next().unwrap_or("").trim_end(); + match code.strip_suffix(')') { + Some(t) => t.to_string(), + None => { + result.warnings.push(RewriteWarning { + code: "redirect_gem_unrecognized_declaration".into(), + detail: format!( + "the `gem \"{}\"` declaration is in a form the \ + rewriter cannot safely edit; redirect skipped", + dep.name + ), + }); + continue; + } + } + } else { + raw_tail + }; + // A source-selecting option would move into the block and + // OVERRIDE it in bundler's DSL, leaving the redirect a + // silent no-op that still gets attested. Fail closed. + if let Some(tok) = gem_tail_source_option(&tail) { + result.warnings.push(RewriteWarning { + code: "redirect_gem_source_option".into(), + detail: format!( + "the `gem \"{}\"` declaration carries `{tok}`, which would \ + override the Socket source block; 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. - let opts = gem_line_trailing_options(m.get(1).unwrap().as_str()); + let opts = gem_line_trailing_options(&tail); let block = if opts.is_empty() { - block + format!( + "source \"{}\" do\n gem \"{}\", \"{}\"\nend", + ov.index_url, dep.name, dep.version + ) } else { format!( "source \"{}\" do\n gem \"{}\", \"{}\", {opts}\nend", ov.index_url, dep.name, dep.version ) }; - // Plain replacen: the block may carry user text (`opts`), - // which a regex replacement would `$`-expand. - *gf = gf.replacen(&original, &block, 1); + // Splice by the match's byte range: a substring replace of + // the line's TEXT would hit an identical commented-out + // duplicate earlier in the file and corrupt it (and the + // block may carry user text a regex replacement would + // `$`-expand). + gf.replace_range(range, &block); gemfile_changed = true; result.edits.push(FileEdit { path: "Gemfile".into(), @@ -1751,7 +1900,23 @@ fn rewrite_gem( original: Some(Value::String(original)), new: Some(Value::String(block)), }); + source_placed = true; + } else if declared_re.is_match(gf) { + result.warnings.push(RewriteWarning { + code: "redirect_gem_unrecognized_declaration".into(), + detail: format!( + "the `gem \"{}\"` declaration is in a form the rewriter \ + cannot safely edit; redirect skipped", + dep.name + ), + }); + continue; } else { + // Genuinely undeclared (a transitive dep): append a block. + let block = format!( + "source \"{}\" do\n gem \"{}\", \"{}\"\nend", + ov.index_url, dep.name, dep.version + ); let sep = if gf.ends_with('\n') { "" } else { "\n" }; *gf = format!("{gf}{sep}{block}\n"); gemfile_changed = true; @@ -1763,17 +1928,32 @@ fn rewrite_gem( original: None, new: Some(Value::String(block)), }); + source_placed = true; } } } if let Some(lk) = lock.as_mut() { + // The pin only makes sense once the source redirect is in place + // (just written or already present): pinning the patched sha + // while the gem still resolves upstream guarantees a checksum + // failure on the next install. + if !source_placed { + result.warnings.push(RewriteWarning { + code: "redirect_gem_lock_without_source".into(), + detail: format!( + "no Gemfile source redirect is in place for {} — CHECKSUMS pin skipped", + dep.name + ), + }); + continue; + } let sum_line_re = Regex::new( &(String::from(r"(?m)^( ") + ®ex::escape(&dep.name) + r" \(" + ®ex::escape(&dep.version) - + r"\)) sha256=[0-9a-f]+$"), + + r"\)) sha256=([0-9a-f]+)$"), ) .unwrap(); let new_val = format!("{} ({}) sha256={sha256}", dep.name, dep.version); @@ -1781,7 +1961,15 @@ fn rewrite_gem( // target value; recording an edit would grow the ledger forever. if lk.contains(&format!("\n {new_val}\n")) || lk.ends_with(&format!("\n {new_val}")) { // no-op - } else if sum_line_re.is_match(lk) { + } else if let Some(m) = sum_line_re.captures(lk) { + // The pre-edit line goes into the ledger as `original` so a + // future `--revert` can restore the upstream sha. + let old_val = format!( + "{} ({}) sha256={}", + dep.name, + dep.version, + m.get(2).unwrap().as_str() + ); *lk = sum_line_re .replace(lk, format!("${{1}} sha256={sha256}").as_str()) .to_string(); @@ -1791,7 +1979,7 @@ fn rewrite_gem( kind: "redirect_gemfile_lock_checksum".into(), action: "rewritten".into(), key: Some(dep.name.clone()), - original: None, + original: Some(Value::String(old_val)), new: Some(Value::String(new_val)), }); } else if checksums_re.is_match(lk) { @@ -1826,6 +2014,21 @@ fn rewrite_gem( } } + // The rewritten pair breaks bundler's frozen/deployment mode: the lock's + // GEM section still records the upstream source, so `bundle install` with + // `frozen`/`--deployment` set rejects the Gemfile's new source block. + // Mirror of the CLI's pnpm trust-lockfile warning. + if gemfile_changed || lock_changed { + result.warnings.push(RewriteWarning { + code: "redirect_gem_frozen_install".into(), + detail: "Gemfile was repointed at the Socket patch registry but Gemfile.lock's \ + GEM section still records the upstream source; bundler rejects the pair \ + under frozen/deployment mode — run `bundle install` (unfrozen) once to \ + record the new source in Gemfile.lock" + .into(), + }); + } + if gemfile_changed { if let Some(gf) = gemfile { result.files.insert("Gemfile".into(), gf); @@ -3586,6 +3789,338 @@ mod tests { ); } + /// A minimal Gemfile.lock with the given CHECKSUMS lines (rails 7.0.0). + fn gem_lock(checksums: &str) -> String { + format!( + "GEM\n remote: https://rubygems.org/\n specs:\n rails (7.0.0)\n\n\ + PLATFORMS\n ruby\n\nDEPENDENCIES\n rails (= 7.0.0)\n\n\ + CHECKSUMS\n{checksums}\n\nBUNDLED WITH\n 2.6.2\n" + ) + } + + /// The edit must splice by the regex match's byte range: a substring + /// replace of the matched line's TEXT finds an identical commented-out + /// duplicate earlier in the file first and corrupts the comment while the + /// live line keeps resolving upstream. + #[test] + fn gemfile_rewrite_ignores_commented_duplicate() { + let mut files = BTreeMap::new(); + files.insert( + "Gemfile".to_string(), + "source \"https://rubygems.org\"\n\ + # gem \"rails\", \"7.0.0\" pinned during the 6.x upgrade\n\ + gem \"rails\", \"7.0.0\"\n" + .to_string(), + ); + let r = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]); + let out = r.files.get("Gemfile").expect("Gemfile rewritten"); + assert!( + out.contains("\n# gem \"rails\", \"7.0.0\" pinned during the 6.x upgrade\n"), + "commented-out duplicate left untouched: {out}" + ); + assert!( + out.contains( + "\nsource \"https://patch.test/gem/tok/uuid/\" do\n gem \"rails\", \"7.0.0\"\nend\n" + ), + "live line replaced by the source block: {out}" + ); + } + + /// The grant token in the index URL rotates per request, so a re-run must + /// recognize the source block a previous run wrote (token-wildcard match, + /// not exact URL) and refresh its URL in place — never wrap the block's + /// gem line inside a new nested block. + #[test] + fn gemfile_rerun_with_rotated_grant_updates_url_never_nests() { + fn ov(token: &str) -> DepOverride { + let mut o = gem_override("rails", "7.0.0"); + o.token = token.into(); + if let Some(r) = o.registry_override.as_mut() { + r.index_url = format!("https://patch.test/gem/{token}/uuid/"); + } + o + } + let mut files = BTreeMap::new(); + files.insert( + "Gemfile".to_string(), + "source \"https://rubygems.org\"\n\ngem \"rails\", \"7.0.0\"\n".to_string(), + ); + let first = rewrite_registry_redirect(&files, &[ov("tok-one")]); + let redirected = first.files.get("Gemfile").expect("first run rewrites"); + files.insert("Gemfile".to_string(), redirected.clone()); + + let second = rewrite_registry_redirect(&files, &[ov("tok-two")]); + let out = second + .files + .get("Gemfile") + .expect("rotated grant refreshes the URL"); + assert_eq!( + out.matches("source \"https://patch.test/gem/").count(), + 1, + "exactly one Socket source block, never nested: {out}" + ); + assert!( + out.contains( + "source \"https://patch.test/gem/tok-two/uuid/\" do\n gem \"rails\", \"7.0.0\"\nend" + ), + "URL refreshed in place: {out}" + ); + assert!(!out.contains("tok-one"), "old grant token gone: {out}"); + assert!( + second + .edits + .iter() + .any(|e| e.kind == "redirect_gemfile_source_url" + && e.original + == Some(Value::String("https://patch.test/gem/tok-one/uuid/".into()))), + "URL refresh recorded with the old URL as original: {:?}", + second.edits + ); + + // Same grant again: a true no-op. + files.insert("Gemfile".to_string(), out.clone()); + let third = rewrite_registry_redirect(&files, &[ov("tok-two")]); + assert!( + third.files.is_empty() && third.edits.is_empty(), + "same-grant re-run must be a no-op: files={:?} edits={:?}", + third.files.keys(), + third.edits + ); + } + + /// A gem-level source option (`git:` / `path:` / `github:` / `source:`) + /// preserved into the Socket source block OVERRIDES it in bundler's DSL, + /// leaving the redirect a silent no-op that still gets attested. Fail + /// closed: warn and leave both files untouched. + #[test] + fn gemfile_gem_with_source_option_fails_closed() { + let mut files = BTreeMap::new(); + files.insert( + "Gemfile".to_string(), + "source \"https://rubygems.org\"\n\n\ + gem \"rails\", \"7.0.0\", git: \"https://github.com/rails/rails\"\n" + .to_string(), + ); + files.insert( + "Gemfile.lock".to_string(), + gem_lock(&format!(" rails (7.0.0) sha256={}", "2".repeat(64))), + ); + let r = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "source-selecting option must skip the redirect: files={:?} edits={:?}", + r.files.keys(), + r.edits + ); + assert!( + r.warnings + .iter() + .any(|w| w.code == "redirect_gem_source_option"), + "skip must warn: {:?}", + r.warnings + ); + } + + /// Platform-specific CHECKSUMS siblings (`rails (7.0.0-arm64-darwin)`) + /// mean bundler resolves a platform gem the patch registry does not + /// serve — the bare-platform pin would leave the platform line at the + /// upstream sha (or duplicate the bare line). Fail closed: skip the dep. + #[test] + fn gem_platform_checksums_fail_closed() { + let mut files = BTreeMap::new(); + files.insert( + "Gemfile".to_string(), + "source \"https://rubygems.org\"\n\ngem \"rails\", \"7.0.0\"\n".to_string(), + ); + files.insert( + "Gemfile.lock".to_string(), + gem_lock(&format!( + " rails (7.0.0) sha256={}\n rails (7.0.0-arm64-darwin) sha256={}", + "2".repeat(64), + "3".repeat(64) + )), + ); + let r = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "platform gems must skip the whole dep: files={:?} edits={:?}", + r.files.keys(), + r.edits + ); + assert!( + r.warnings + .iter() + .any(|w| w.code == "redirect_gem_platform_unsupported"), + "skip must warn: {:?}", + r.warnings + ); + } + + /// Legal-but-non-canonical declarations (parenthesized call, tab / double + /// space after `gem`) must be recognized and rewritten in place — falling + /// through to the append branch declares the gem twice, which bundler + /// rejects. + #[test] + fn gemfile_paren_and_whitespace_declarations_are_rewritten_not_duplicated() { + let mut files = BTreeMap::new(); + files.insert( + "Gemfile".to_string(), + "source \"https://rubygems.org\"\n\n\ + gem(\"rails\", \"7.0.0\", require: false)\n\ + gem\t\"puma\", \"6.0.0\"\n" + .to_string(), + ); + let r = rewrite_registry_redirect( + &files, + &[ + gem_override("rails", "7.0.0"), + gem_override("puma", "6.0.0"), + ], + ); + let out = r.files.get("Gemfile").expect("Gemfile rewritten"); + assert!( + out.contains( + "source \"https://patch.test/gem/tok/uuid/\" do\n \ + gem \"rails\", \"7.0.0\", require: false\nend" + ), + "paren declaration rewritten with options kept, `)` stripped: {out}" + ); + assert!( + !out.contains("gem(\"rails\"") && !out.contains("gem\t\"puma\""), + "original declarations replaced, not duplicated: {out}" + ); + assert!( + out.contains( + "source \"https://patch.test/gem/tok/uuid/\" do\n gem \"puma\", \"6.0.0\"\nend" + ), + "tab-separated declaration rewritten: {out}" + ); + } + + /// A declaration the recognizer cannot parse (`gem\"rails\"` — legal ruby, + /// no separator) must NOT fall through to the append branch: warn and skip + /// instead of declaring the gem twice. + #[test] + fn gemfile_unrecognizable_declaration_fails_closed_no_append() { + let mut files = BTreeMap::new(); + files.insert( + "Gemfile".to_string(), + "source \"https://rubygems.org\"\n\ngem\"rails\", \"7.0.0\"\n".to_string(), + ); + let r = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "unrecognizable declaration must not append a duplicate: files={:?} edits={:?}", + r.files.keys(), + r.edits + ); + assert!( + r.warnings + .iter() + .any(|w| w.code == "redirect_gem_unrecognized_declaration"), + "skip must warn: {:?}", + r.warnings + ); + } + + /// The CHECKSUMS pin is gated on the Gemfile source redirect being in + /// place: with no Gemfile in the candidate map, pinning the patched sha + /// while the gem still resolves upstream guarantees a checksum failure. + #[test] + fn gem_lock_pin_gated_on_source_redirect() { + let mut files = BTreeMap::new(); + files.insert( + "Gemfile.lock".to_string(), + gem_lock(&format!(" rails (7.0.0) sha256={}", "2".repeat(64))), + ); + let r = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "lock pin without a source redirect must be skipped: files={:?} edits={:?}", + r.files.keys(), + r.edits + ); + assert!( + r.warnings + .iter() + .any(|w| w.code == "redirect_gem_lock_without_source"), + "skip must warn: {:?}", + r.warnings + ); + } + + /// A landed gem redirect breaks bundler frozen/deployment installs (the + /// lock's GEM section still records the upstream source), so the rewrite + /// must say so — and only when it actually changed something. + #[test] + fn gem_redirect_warns_about_frozen_installs() { + let mut files = BTreeMap::new(); + files.insert( + "Gemfile".to_string(), + "source \"https://rubygems.org\"\n\ngem \"rails\", \"7.0.0\"\n".to_string(), + ); + files.insert( + "Gemfile.lock".to_string(), + gem_lock(&format!(" rails (7.0.0) sha256={}", "2".repeat(64))), + ); + let ovr = gem_override("rails", "7.0.0"); + let first = rewrite_registry_redirect(&files, std::slice::from_ref(&ovr)); + assert!( + first + .warnings + .iter() + .any(|w| w.code == "redirect_gem_frozen_install"), + "landed redirect must warn about frozen installs: {:?}", + first.warnings + ); + + // No-op re-run: nothing landed, so no frozen-install warning. + for (name, content) in first.files { + files.insert(name, content); + } + let second = rewrite_registry_redirect(&files, std::slice::from_ref(&ovr)); + assert!( + second.files.is_empty() + && !second + .warnings + .iter() + .any(|w| w.code == "redirect_gem_frozen_install"), + "a no-op re-run must not warn: files={:?} warnings={:?}", + second.files.keys(), + second.warnings + ); + } + + /// The rewritten CHECKSUMS edit must carry the pre-edit line as + /// `original` — with `None` the ledger cannot restore the upstream sha on + /// a future revert. + #[test] + fn gem_lock_rewrite_records_original_checksum_line() { + let mut files = BTreeMap::new(); + files.insert( + "Gemfile".to_string(), + "source \"https://rubygems.org\"\n\ngem \"rails\", \"7.0.0\"\n".to_string(), + ); + files.insert( + "Gemfile.lock".to_string(), + gem_lock(&format!(" rails (7.0.0) sha256={}", "2".repeat(64))), + ); + let r = rewrite_registry_redirect(&files, &[gem_override("rails", "7.0.0")]); + let edit = r + .edits + .iter() + .find(|e| e.kind == "redirect_gemfile_lock_checksum" && e.action == "rewritten") + .expect("lock checksum edit recorded"); + assert_eq!( + edit.original, + Some(Value::String(format!( + "rails (7.0.0) sha256={}", + "2".repeat(64) + ))), + "pre-edit CHECKSUMS line captured for revert" + ); + } + /// An unparseable package-lock.json must surface a warning, not silently /// skip the npm redirect entirely (missing-lockfile already warns; a /// corrupt lockfile is strictly worse and was silent). diff --git a/crates/socket-patch-core/src/vendor/gem.rs b/crates/socket-patch-core/src/vendor/gem.rs index 3f3ee42b..2e5c47a2 100644 --- a/crates/socket-patch-core/src/vendor/gem.rs +++ b/crates/socket-patch-core/src/vendor/gem.rs @@ -38,7 +38,11 @@ //! trailing options like `require: false` preserved) or, for a transitive //! dependency, a managed block appended at EOF. Anything //! the conservative line grammar cannot prove safe to rewrite is REFUSED — -//! never guessed at. +//! never guessed at. The one exception is OUR OWN previous wiring: a patch +//! update moves the manifest to a new uuid (same purl), and a `path:` that +//! parses as the socket vendor dir for exactly this gem is repointed in +//! place (the older patch uuid is re-vendored automatically, like the +//! npm/cargo/golang backends — no revert-first). //! //! The stub gemspec from `/specifications/` is copied into the //! vendored dir as `.gemspec` (a path source needs one; the spike showed @@ -63,7 +67,7 @@ use super::common::{ already_patched_result, copy_matches_after_hashes, done, refused, service_offline_conflict, synthesized_result, }; -use super::path::vendor_uuid_dir_rel; +use super::path::{parse_vendor_path, vendor_uuid_dir_rel}; use super::registry_fetch::extract_gem_data; use super::service_fetch::{ fetch_verified_archive, fetch_verified_secondary, SecondaryArtifactResult, ServiceArtifact, @@ -459,15 +463,50 @@ pub async fn vendor_gem( original: None, new: Some(Value::String(block.clone())), }, + // Re-vendor over our own wiring (see `GemfilePlan::RewireOurs`): + // `original: None`, carried forward by the caller. The managed-fence + // form stays `Added` with the whole updated block so revert deletes + // the fence too. + GemfilePlan::RewireOurs { + new_line, + managed_block, + .. + } => match managed_block { + Some(block) => WiringRecord { + file: GEMFILE.to_string(), + kind: GEMFILE_WIRING_KIND.to_string(), + action: WiringAction::Added, + key: Some(name.to_string()), + original: None, + new: Some(Value::String(block.clone())), + }, + None => WiringRecord { + file: GEMFILE.to_string(), + kind: GEMFILE_WIRING_KIND.to_string(), + action: WiringAction::Rewritten, + key: Some(name.to_string()), + original: None, + new: Some(Value::String(new_line.clone())), + }, + }, + }; + // A rewire lifted OUR OWN previous PATH section, not pre-vendor + // fragments: record `original: None` — the true originals live in the + // ledger entry being replaced, which the caller carries forward by + // wiring identity (`persist_vendor_entry`). + let lock_original = if lock_edit.rewired_ours { + None + } else { + let mut original_lines: Vec = lock_edit + .removed_spec_block + .iter() + .map(|l| Value::String(l.clone())) + .collect(); + if let Some(dep) = &lock_edit.old_dep_line { + original_lines.push(Value::String(dep.clone())); + } + Some(Value::Array(original_lines)) }; - let mut original_lines: Vec = lock_edit - .removed_spec_block - .iter() - .map(|l| Value::String(l.clone())) - .collect(); - if let Some(dep) = &lock_edit.old_dep_line { - original_lines.push(Value::String(dep.clone())); - } let mut new_lines: Vec = lock_edit .path_section .iter() @@ -479,7 +518,7 @@ pub async fn vendor_gem( kind: LOCK_WIRING_KIND.to_string(), action: WiringAction::Rewritten, key: Some(name.to_string()), - original: Some(Value::Array(original_lines)), + original: lock_original, new: Some(Value::Array(new_lines)), }; let mut wiring = vec![gemfile_record, lock_record]; @@ -496,6 +535,23 @@ pub async fn vendor_gem( original: Some(Value::String(orig_line.clone())), new: Some(Value::String(new_line.clone())), }); + } else if lock_edit.rewired_ours { + // Re-vendor with the bare path-form line already in place (our + // previous run stripped the registry token): the record must ride + // again with `original: None` — dropped, the first run's registry + // `sha256=` line would vanish from the ledger with the entry being + // replaced, and a later --revert could no longer restore it (a bare + // leftover on a registry gem hard-fails frozen installs, exit 16). + if let Some(bare) = &lock_edit.checksum_bare { + wiring.push(WiringRecord { + file: GEMFILE_LOCK.to_string(), + kind: LOCK_CHECKSUM_WIRING_KIND.to_string(), + action: WiringAction::Rewritten, + key: Some(name.to_string()), + original: None, + new: Some(Value::String(bare.clone())), + }); + } } let entry = VendorEntry { @@ -882,6 +938,22 @@ enum GemfilePlan { original_line: String, new_line: String, }, + /// The declaration already carries OUR OWN `path:` wiring from an older + /// patch uuid (a patch update changes the uuid, never the purl): + /// repoint it at the new copy in place, everything else on the line + /// byte-preserved. The wiring record carries `original: None` — the true + /// pre-vendor line lives in the ledger entry being replaced and the + /// caller carries it forward by wiring identity (`persist_vendor_entry`); + /// recording the old-uuid line would make a later revert "restore" a + /// dangling vendor pointer. `managed_block` is `Some(updated block)` when + /// the line sits inside our managed fence (the transitive-gem form): the + /// record then stays `Added` with the whole block, so revert still + /// deletes the fence. + RewireOurs { + original_line: String, + new_line: String, + managed_block: Option, + }, /// The gem is transitive (not declared): append a fenced managed block. Append { block: String }, } @@ -893,7 +965,9 @@ enum GemfilePlan { /// (inside a `group`/`platforms`/conditional block), parenthesized, /// continued onto the next line, conditional, or already carrying a /// `path:`/`git:`/`github:` source — is refused rather than guessed at: a -/// wrong Gemfile rewrite executes on every `bundle` invocation. +/// wrong Gemfile rewrite executes on every `bundle` invocation. The one +/// `path:` exception is our own vendored dir for this gem (an older patch +/// uuid), which is repointed in place — see [`GemfilePlan::RewireOurs`]. fn plan_gemfile_edit( text: &str, name: &str, @@ -935,6 +1009,30 @@ fn plan_gemfile_edit( "the `gem \"{name}\"` declaration uses a parenthesized call" )); } + // Our own wiring from an older patch uuid: the `path:` value parses as + // the socket vendor dir for exactly this gem. Repoint it in place — + // refusing here (the source-option blocklist below) would make every + // patch update demand a manual `vendor --revert` first. A path that + // parses as anything else (a user fork, another gem's dir) still refuses. + if let Some(prev_rel) = gem_line_path_value(&rest) { + if is_our_vendor_rel(prev_rel, name, version) { + let original_line = lines[idx].to_string(); + // The rel appears exactly once (its charset excludes quotes and + // `#`, and the code before `path:` cannot contain a `/`-bearing + // token); swapping just the value preserves quote style and + // trailing options verbatim. + let new_line = original_line.replacen(prev_rel, rel, 1); + let managed_block = (idx > 0 + && lines[idx - 1] == MANAGED_OPEN + && lines.get(idx + 1).is_some_and(|l| *l == MANAGED_CLOSE)) + .then(|| format!("{MANAGED_OPEN}\n{new_line}\n{MANAGED_CLOSE}\n")); + return Ok(GemfilePlan::RewireOurs { + original_line, + new_line, + managed_block, + }); + } + } if let Some(reason) = rest_blocks_edit(&rest) { return Err(format!( "the `gem \"{name}\"` declaration is not editable: {reason}" @@ -1025,11 +1123,45 @@ fn rest_blocks_edit(rest: &str) -> Option { None } +/// The quoted `path:` option value on a gem line's argument tail (only the +/// code before any `#` comment counts) — the form our own rewrite emits. +/// `None` for anything else (`:path =>`, interpolation, no `path:` at all): +/// those fall through to [`rest_blocks_edit`]'s refusal, fail-closed. +fn gem_line_path_value(rest: &str) -> Option<&str> { + let code = rest.split('#').next().unwrap_or(""); + let idx = code.find("path:")?; + if idx > 0 && !matches!(code.as_bytes()[idx - 1], b' ' | b'\t' | b',') { + return None; + } + let after = code[idx + "path:".len()..].trim_start(); + let q = after.chars().next()?; + if q != '"' && q != '\'' { + return None; + } + let value = &after[1..]; + let end = value.find(q)?; + Some(&value[..end]) +} + +/// True when a `path:`/`remote:` value is OUR vendored dir for exactly this +/// gem (`.socket/vendor/gem//-`) — the shape +/// [`vendor_gem`] wires, and the only wiring a patch UPDATE (new uuid, same +/// purl) may rewire. +fn is_our_vendor_rel(value: &str, name: &str, version: &str) -> bool { + parse_vendor_path(value) + .is_some_and(|p| p.eco == "gem" && p.leaf == format!("{name}-{version}")) +} + fn apply_gemfile_plan(text: &str, plan: &GemfilePlan) -> String { match plan { GemfilePlan::Rewrite { original_line, new_line, + } + | GemfilePlan::RewireOurs { + original_line, + new_line, + .. } => { let mut lines: Vec<&str> = text.split('\n').collect(); if let Some(i) = lines.iter().position(|l| *l == original_line) { @@ -1068,6 +1200,17 @@ struct LockEdit { /// "original" — reverting it onto a registry-sourced lock would break /// frozen installs). checksum_rewrite: Option<(String, String)>, + /// The spec block was lifted from OUR OWN previous PATH section (a + /// re-vendor to a newer patch uuid), not from GEM/specs: the lifted + /// fragments are this backend's own prior wiring, so the caller records + /// `original: None` and the true pre-vendor originals ride forward from + /// the ledger entry being replaced (`persist_vendor_entry`). + rewired_ours: bool, + /// The already-bare CHECKSUMS line for the gem, when one is present. + /// Only consulted on a re-vendor (`rewired_ours`): the checksum record + /// must ride again or the first run's registry `sha256=` restore line + /// drops out of the ledger with the entry being replaced. + checksum_bare: Option, } /// Produce the pair-edited lock text (see the module doc for the canonical @@ -1077,21 +1220,86 @@ struct LockEdit { fn edit_lock(text: &str, name: &str, version: &str, rel: &str) -> Result { let mut lines: Vec = text.split('\n').map(str::to_string).collect(); - // 1. Lift the gem's spec block out of GEM/specs. + // 1. Lift the gem's spec block out of GEM/specs — or, on a re-vendor to + // a newer patch uuid (same purl), out of the PATH section our previous + // run emitted. let (gem_start, gem_end) = section_span(&lines, "GEM").ok_or_else(|| "Gemfile.lock has no GEM section".to_string())?; if !(gem_start..gem_end).any(|i| lines[i] == " specs:") { return Err("Gemfile.lock GEM section has no specs: stanza".to_string()); } - let target = format!(" {name} ({version})"); - let block_start = (gem_start..gem_end) - .find(|&i| lines[i] == target) - .ok_or_else(|| format!("Gemfile.lock GEM specs has no entry `{name} ({version})`"))?; - let mut block_end = block_start + 1; - while block_end < gem_end && lines[block_end].starts_with(" ") { - block_end += 1; + // SECURITY/fail-closed: platform-suffixed installs were refused + // (`platform_gem_unsupported`) before this point, so a platform-suffixed + // GEM spec sibling means the lock disagrees with the installed tree — + // and lifting only the plain entry would leave the sibling behind as a + // stale registry spec. The CHECKSUMS branch below refuses the same + // shape, but only bundler ≥ 2.6 locks have a CHECKSUMS section to catch + // it in. + let platform_prefix = format!("{version}-"); + for line in lines.iter().take(gem_end).skip(gem_start + 1) { + if let Some((n, v)) = spec_entry(line) { + if n == name && v.starts_with(&platform_prefix) { + return Err(format!( + "Gemfile.lock GEM specs has a platform-suffixed entry `{n} ({v})` but the installed gem is not platform-specific; the lock disagrees with the install (re-resolve it before vendoring)" + )); + } + } } - let removed_spec_block: Vec = lines.drain(block_start..block_end).collect(); + let target = format!(" {name} ({version})"); + let mut rewired_ours = false; + let removed_spec_block: Vec = match (gem_start..gem_end).find(|&i| lines[i] == target) { + Some(block_start) => { + let mut block_end = block_start + 1; + while block_end < gem_end && lines[block_end].starts_with(" ") { + block_end += 1; + } + lines.drain(block_start..block_end).collect() + } + None => { + // Re-vendor: the entry lives in the PATH section our previous + // run emitted (remote parses as our vendored dir for exactly + // this gem). Lift the block and drop the old section — step 3 + // re-emits it at the NEW uuid's sorted position. The lifted + // lines are our own wiring, not pre-vendor originals: flagged + // via `rewired_ours` (see the `LockEdit` field docs). + let Some((ps, pe)) = find_our_path_section(&lines, name, version) else { + return Err(format!( + "Gemfile.lock GEM specs has no entry `{name} ({version})`" + )); + }; + let block_start = (ps..pe).find(|&i| lines[i] == target).ok_or_else(|| { + format!( + "Gemfile.lock PATH section for `{name}` lost its `{name} ({version})` spec entry" + ) + })?; + let mut block_end = block_start + 1; + while block_end < pe && lines[block_end].starts_with(" ") { + block_end += 1; + } + // Grammar-strict: besides the block, the section must be exactly + // what vendor wrote (header, one remote, specs:, blank + // separators). Anything extra — a hand edit, a merged-in second + // spec — would be destroyed by the drain below; never guess. + let non_block: Vec<&str> = (ps..pe) + .filter(|i| !(block_start..block_end).contains(i)) + .map(|i| lines[i].as_str()) + .filter(|l| !l.is_empty()) + .collect(); + if non_block.len() != 3 + || non_block[0] != "PATH" + || !non_block[1].starts_with(" remote: ") + || non_block[2] != " specs:" + { + return Err(format!( + "Gemfile.lock PATH section for `{name} ({version})` is not the shape vendor wrote; refusing to rewire it" + )); + } + let block: Vec = lines[block_start..block_end].to_vec(); + lines.drain(ps..pe); + rewired_ours = true; + block + } + }; // 2. DEPENDENCIES: exact pin + `!` path-source marker. A transitive gem // (absent pre-vendor) is inserted at bundler's sorted position — it is a @@ -1122,8 +1330,16 @@ fn edit_lock(text: &str, name: &str, version: &str, rel: &str) -> Result lines.insert(insert_at, new_dep_line.clone()), } - // 3. PATH section directly above the GEM section (bundler's canonical - // placement; spike claim 2). `remote:` is the bare relative path. + // 3. PATH section above the GEM section, at bundler's SORTED position + // among any existing PATH sections: bundler emits path/git/plugin + // sources sorted by identifier (source_list.rb `lock_other_sources`, + // verified against bundler 4.0.15) — `source at ``` for a path + // source, so PATH sections order by their remote path and all sit in one + // contiguous run (no other source's identifier can start with that + // prefix). Splicing at invocation order instead churns the committed + // lock on the next `bundle lock`. Non-PATH leading sections keep the + // legacy insert-before-GEM fallback. `remote:` is the bare relative + // path (spike claim 2). let mut path_section = vec![ "PATH".to_string(), format!(" remote: {rel}"), @@ -1134,9 +1350,29 @@ fn edit_lock(text: &str, name: &str, version: &str, rel: &str) -> Result our_ident => { + at = i; + break; + } + // Ours sorts after this section (a remote-less section is + // grammar-degenerate; keep the legacy after-everything spot). + _ => at = end.min(gem_hdr), + } + i = end; + } else { + i += 1; + } + } let mut insert = path_section.clone(); - insert.push(String::new()); // blank separator before GEM - lines.splice(gem_hdr..gem_hdr, insert); + insert.push(String::new()); // blank separator before the next section + lines.splice(at..at, insert); // 4. CHECKSUMS (bundler ≥ 2.6 `lockfile_checksums`): a path-sourced gem // keeps a BARE ` ()` entry — bundler's own re-lock emits @@ -1147,9 +1383,9 @@ fn edit_lock(text: &str, name: &str, version: &str, rel: &str) -> Result = None; + let mut checksum_bare: Option = None; if let Some((ck_start, ck_end)) = section_span(&lines, "CHECKSUMS") { let bare = format!(" {name} ({version})"); - let platform_prefix = format!("{version}-"); let mut plain_at: Option = None; for (i, line) in lines.iter().enumerate().take(ck_end).skip(ck_start + 1) { match checksum_entry(line) { @@ -1190,6 +1426,8 @@ fn edit_lock(text: &str, name: &str, version: &str, rel: &str) -> Result Result Result Option<(usize, usize)> { let start = lines.iter().position(|l| l.as_str() == header)?; + Some((start, section_end(lines, start))) +} + +/// End (exclusive) of the section whose column-0 header sits at `start` — +/// the [`section_span`] rule for a known header position. +fn section_end(lines: &[String], start: usize) -> usize { let mut end = start + 1; while end < lines.len() { let l = &lines[end]; @@ -1217,7 +1463,39 @@ fn section_span(lines: &[String], header: &str) -> Option<(usize, usize)> { } end += 1; } - Some((start, end)) + end +} + +/// Bundler's lock-sort identifier for a path source — `source at ``` +/// (`Source::Path#to_s`, aliased as `identifier`); sections order by a +/// byte-wise comparison of these, which Rust's `str` ordering matches. +fn path_source_identifier(path: &str) -> String { + format!("source at `{path}`") +} + +/// The ` remote: ` value of the section slice starting at its header line. +fn path_section_remote(section: &[String]) -> Option<&str> { + section.iter().find_map(|l| l.strip_prefix(" remote: ")) +} + +/// Find the PATH section whose `remote:` is OUR vendored dir for this gem — +/// any patch uuid (the previous run's wiring, sought during a re-vendor). +fn find_our_path_section(lines: &[String], name: &str, version: &str) -> Option<(usize, usize)> { + let mut i = 0; + while i < lines.len() { + if lines[i].as_str() == "PATH" { + let end = section_end(lines, i); + if path_section_remote(&lines[i..end]) + .is_some_and(|p| is_our_vendor_rel(p, name, version)) + { + return Some((i, end)); + } + i = end; + } else { + i += 1; + } + } + None } /// Name of a 2-space DEPENDENCIES entry (` rack (~> 3.1)` / ` rack!`). @@ -1239,6 +1517,25 @@ fn spec_entry_name(line: &str) -> Option<&str> { Some(rest.split(' ').next().unwrap_or(rest)) } +/// Parse a 4-space specs entry line: ` ()`, nothing after +/// the closing paren. Returns `(name, parenthesized token)` — the platform +/// suffix stays inside the token, mirroring [`checksum_entry`]'s grammar at +/// specs indentation (` ffi (1.17.2-aarch64-linux-gnu)`). +fn spec_entry(line: &str) -> Option<(&str, &str)> { + let rest = line.strip_prefix(" ")?; + if rest.is_empty() || rest.starts_with(' ') { + return None; + } + let open = rest.find(" (")?; + let after = &rest[open + 2..]; + let close = after.find(')')?; + let (name, ver, tail) = (&rest[..open], &after[..close], &after[close + 1..]); + if name.is_empty() || ver.is_empty() || !tail.is_empty() { + return None; + } + Some((name, ver)) +} + /// Parse a CHECKSUMS entry line: two-space indent, ` ()` or /// ` (-)`, then optional space-separated tokens /// (`sha256=` on registry entries, nothing on path entries). Returns @@ -1665,10 +1962,21 @@ mod tests { installed: &Path, record: &PatchRecord, dry_run: bool, + ) -> VendorOutcome { + run_vendor_purl(PURL, root, blobs, installed, record, dry_run).await + } + + async fn run_vendor_purl( + purl: &str, + root: &Path, + blobs: &Path, + installed: &Path, + record: &PatchRecord, + dry_run: bool, ) -> VendorOutcome { let sources = PatchSources::blobs_only(blobs); vendor_gem( - PURL, + purl, installed, root, record, @@ -1681,6 +1989,23 @@ mod tests { .await } + /// Simulate the CLI caller's `persist_vendor_entry` carry-forward: fill + /// the replacement entry's `original: None` holes from the entry being + /// replaced, by wiring identity (file, kind, key). + fn carry_forward_originals(prev: &VendorEntry, next: &mut VendorEntry) { + for rec in &mut next.wiring { + if rec.action == WiringAction::Rewritten && rec.original.is_none() { + if let Some(p) = prev + .wiring + .iter() + .find(|p| p.file == rec.file && p.kind == rec.kind && p.key == rec.key) + { + rec.original = p.original.clone(); + } + } + } + } + fn expected_lock_direct() -> String { format!( "PATH\n remote: {rel}\n specs:\n rack (3.2.6)\n base64 (>= 0.1.0)\n\nGEM\n remote: https://rubygems.org/\n specs:\n puma (6.4.2)\n nio4r (~> 2.0)\n\nPLATFORMS\n arm64-darwin-23\n ruby\n\nDEPENDENCIES\n puma\n rack (= 3.2.6)!\n\nBUNDLED WITH\n 2.5.22\n", @@ -2777,6 +3102,360 @@ mod tests { ); } + // ── multiple vendored gems: PATH sections sort like bundler's ──────────── + + /// Second gem for multi-PATH tests. Its uuid sorts BEFORE rack's + /// (`1a…` < `9f…`), so vendoring rack first is the order a naive + /// insert-before-GEM splice would leave unsorted. + const UUID_PUMA: &str = "1a2b3c4d-5e6f-4a1b-8c2d-3e4f5a6b7c8d"; + const PURL_PUMA: &str = "pkg:gem/puma@6.4.2"; + const PRISTINE_PUMA: &[u8] = b"module Puma\n VERSION = \"6.4.2\"\nend\n"; + const PATCHED_PUMA: &[u8] = + b"module Puma\n SOCKET_PATCHED = true\n VERSION = \"6.4.2\"\nend\n"; + const GEMSPEC_PUMA: &str = "Gem::Specification.new do |s|\n s.name = \"puma\"\n s.version = \"6.4.2\"\n s.require_paths = [\"lib\"]\nend\n"; + + fn puma_rel() -> String { + format!(".socket/vendor/gem/{UUID_PUMA}/puma-6.4.2") + } + + /// Add a puma install + blob + record alongside [`fixture`]'s rack, so a + /// test can vendor TWO gems into one project. + async fn add_puma_fixture(installed_rack: &Path, blobs: &Path) -> (PathBuf, PatchRecord) { + let gems = installed_rack.parent().unwrap(); + let installed = gems.join("puma-6.4.2"); + tokio::fs::create_dir_all(installed.join("lib")) + .await + .unwrap(); + tokio::fs::write(installed.join("lib/puma.rb"), PRISTINE_PUMA) + .await + .unwrap(); + let specs = gems.parent().unwrap().join("specifications"); + tokio::fs::write(specs.join("puma-6.4.2.gemspec"), GEMSPEC_PUMA) + .await + .unwrap(); + let before = compute_git_sha256_from_bytes(PRISTINE_PUMA); + let after = compute_git_sha256_from_bytes(PATCHED_PUMA); + tokio::fs::write(blobs.join(&after), PATCHED_PUMA) + .await + .unwrap(); + let mut files = HashMap::new(); + files.insert( + "lib/puma.rb".to_string(), + PatchFileInfo { + before_hash: before, + after_hash: after, + }, + ); + let record = PatchRecord { + uuid: UUID_PUMA.to_string(), + exported_at: "2026-06-09T00:00:00Z".to_string(), + files, + vulnerabilities: HashMap::new(), + description: String::new(), + license: String::new(), + tier: String::new(), + }; + (installed, record) + } + + fn expected_lock_two_path() -> String { + format!( + "PATH\n remote: {puma}\n specs:\n puma (6.4.2)\n nio4r (~> 2.0)\n\nPATH\n remote: {rack}\n specs:\n rack (3.2.6)\n base64 (>= 0.1.0)\n\nGEM\n remote: https://rubygems.org/\n specs:\n\nPLATFORMS\n arm64-darwin-23\n ruby\n\nDEPENDENCIES\n puma (= 6.4.2)!\n rack (= 3.2.6)!\n\nBUNDLED WITH\n 2.5.22\n", + puma = puma_rel(), + rack = copy_rel() + ) + } + + /// Bundler regenerates PATH sections sorted by source identifier — by + /// remote path, the uuid level deciding here (verified against a real + /// bundler 4.0.15 `bundle lock` over this exact two-PATH shape). The + /// splice must land each new section at that sorted position no matter + /// the vendor invocation order, or the committed lock churns on the + /// next `bundle lock`/`bundle install`. + #[tokio::test] + async fn test_two_path_sections_sorted_regardless_of_vendor_order() { + for rack_first in [true, false] { + let (_tmp, root, installed_rack, blobs, record_rack) = + fixture(GEMFILE_DIRECT, LOCK_DIRECT).await; + let (installed_puma, record_puma) = add_puma_fixture(&installed_rack, &blobs).await; + let runs: [(&str, &Path, &PatchRecord); 2] = if rack_first { + [ + (PURL, &installed_rack, &record_rack), + (PURL_PUMA, &installed_puma, &record_puma), + ] + } else { + [ + (PURL_PUMA, &installed_puma, &record_puma), + (PURL, &installed_rack, &record_rack), + ] + }; + for (purl, installed, record) in runs { + let (result, _e, _w) = unwrap_done( + run_vendor_purl(purl, &root, &blobs, installed, record, false).await, + ); + assert!(result.success, "vendor {purl} failed: {:?}", result.error); + } + let lock = tokio::fs::read_to_string(root.join(GEMFILE_LOCK)) + .await + .unwrap(); + assert_eq!(lock, expected_lock_two_path(), "rack_first={rack_first}"); + } + } + + // ── re-vendor: a patch update (new uuid, same purl) ────────────────────── + + /// Re-vendor uuid; sorts BEFORE `UUID_PUMA`'s (`0e…` < `1a…`). + const UUID2: &str = "0e1f2a3b-4c5d-4e6f-8a7b-9c0d1e2f3a4b"; + + /// A patch update moves the manifest to a NEW uuid for the same gem. The + /// CLI re-vendors straight over the first run's live wiring (originals + /// carried forward and the old uuid dir swept by the caller — no + /// revert-first; the cargo backend pins the same design). Both pair + /// files must be repointed in place, with `original: None` on the + /// rewired records. + #[tokio::test] + async fn test_revendor_new_uuid_direct_rewires_in_place() { + let (_tmp, root, installed, blobs, record) = fixture(GEMFILE_DIRECT, LOCK_DIRECT).await; + let (r1, e1, _) = unwrap_done(run_vendor(&root, &blobs, &installed, &record, false).await); + assert!(r1.success); + let entry1 = e1.unwrap(); + + let mut record2 = record.clone(); + record2.uuid = UUID2.to_string(); + let (r2, e2, _) = unwrap_done(run_vendor(&root, &blobs, &installed, &record2, false).await); + assert!(r2.success, "re-vendor must succeed: {:?}", r2.error); + + let new_rel = format!(".socket/vendor/gem/{UUID2}/rack-3.2.6"); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE)).await.unwrap(), + format!( + "source \"https://rubygems.org\"\n\ngem \"puma\"\ngem \"rack\", \"3.2.6\", path: \"{new_rel}\"\n" + ), + "Gemfile repointed in place" + ); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE_LOCK)) + .await + .unwrap(), + expected_lock_direct().replace(UUID, UUID2), + "lock repointed in place" + ); + // New copy built; the old uuid dir is left for the caller's + // stale-artifact sweep (the caller owns the ledger). + assert_eq!( + tokio::fs::read(root.join(&new_rel).join("lib/rack.rb")) + .await + .unwrap(), + PATCHED + ); + assert!(root.join(format!(".socket/vendor/gem/{UUID}")).exists()); + + // The rewired records carry `original: None` — never the old-uuid + // lines (reverting those would "restore" a dangling vendor pointer). + let mut entry2 = e2.expect("re-vendor emits the new ledger entry"); + assert_eq!(entry2.uuid, UUID2); + assert_eq!(entry2.wiring.len(), 2); + for rec in &entry2.wiring { + assert_eq!(rec.action, WiringAction::Rewritten); + assert!(rec.original.is_none(), "{rec:?}"); + } + + // With the caller's carry-forward applied, revert restores the + // PRE-VENDOR files byte-exactly. + carry_forward_originals(&entry1, &mut entry2); + let outcome = revert_gem(&entry2, &root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!( + !outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_drifted"), + "clean revert must not report drift: {:?}", + outcome.warnings + ); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE)).await.unwrap(), + GEMFILE_DIRECT + ); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE_LOCK)) + .await + .unwrap(), + LOCK_DIRECT + ); + } + + /// Transitive form: the managed block is repointed in place (never + /// duplicated) and the record stays `Added` with the WHOLE updated block, + /// so a later revert deletes the fence. + #[tokio::test] + async fn test_revendor_new_uuid_transitive_updates_managed_block() { + let (_tmp, root, installed, blobs, record) = + fixture(GEMFILE_TRANSITIVE, LOCK_TRANSITIVE).await; + let (r1, e1, _) = unwrap_done(run_vendor(&root, &blobs, &installed, &record, false).await); + assert!(r1.success); + let entry1 = e1.unwrap(); + + let mut record2 = record.clone(); + record2.uuid = UUID2.to_string(); + let (r2, e2, _) = unwrap_done(run_vendor(&root, &blobs, &installed, &record2, false).await); + assert!(r2.success, "re-vendor must succeed: {:?}", r2.error); + + let new_rel = format!(".socket/vendor/gem/{UUID2}/rack-3.2.6"); + let new_block = format!( + "{MANAGED_OPEN}\ngem \"rack\", \"3.2.6\", path: \"{new_rel}\"\n{MANAGED_CLOSE}\n" + ); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE)).await.unwrap(), + format!("source \"https://rubygems.org\"\n\ngem \"puma\"\n{new_block}"), + "ONE managed block, repointed — never a duplicate declaration" + ); + + let mut entry2 = e2.unwrap(); + assert_eq!(entry2.wiring[0].action, WiringAction::Added); + assert!(entry2.wiring[0].original.is_none()); + assert_eq!( + entry2.wiring[0].new.as_ref().unwrap(), + &Value::String(new_block) + ); + + carry_forward_originals(&entry1, &mut entry2); + let outcome = revert_gem(&entry2, &root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!( + !outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_drifted"), + "clean revert must not report drift: {:?}", + outcome.warnings + ); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE)).await.unwrap(), + GEMFILE_TRANSITIVE + ); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE_LOCK)) + .await + .unwrap(), + LOCK_TRANSITIVE + ); + } + + /// A re-vendor must RE-SORT: the replacement PATH section lands wherever + /// the NEW uuid sorts among the other vendored gems' sections, not where + /// the old one sat. + #[tokio::test] + async fn test_revendor_new_uuid_resorts_path_sections() { + let (_tmp, root, installed_rack, blobs, record_rack) = + fixture(GEMFILE_DIRECT, LOCK_DIRECT).await; + let (installed_puma, record_puma) = add_puma_fixture(&installed_rack, &blobs).await; + for (purl, installed, record) in [ + (PURL, &installed_rack, &record_rack), + (PURL_PUMA, &installed_puma, &record_puma), + ] { + let (result, _e, _w) = + unwrap_done(run_vendor_purl(purl, &root, &blobs, installed, record, false).await); + assert!(result.success, "vendor {purl} failed: {:?}", result.error); + } + + // The patch update moves rack to a uuid sorting BEFORE puma's. + let mut rack2 = record_rack.clone(); + rack2.uuid = UUID2.to_string(); + let (result, _e, _w) = + unwrap_done(run_vendor(&root, &blobs, &installed_rack, &rack2, false).await); + assert!(result.success, "re-vendor must succeed: {:?}", result.error); + + let lock = tokio::fs::read_to_string(root.join(GEMFILE_LOCK)) + .await + .unwrap(); + assert_eq!( + lock, + expected_lock_two_path() + .replace( + &format!("PATH\n remote: {puma}\n specs:\n puma (6.4.2)\n nio4r (~> 2.0)\n\nPATH\n remote: {rack}\n specs:\n rack (3.2.6)\n base64 (>= 0.1.0)\n\n", puma = puma_rel(), rack = copy_rel()), + &format!("PATH\n remote: {rack}\n specs:\n rack (3.2.6)\n base64 (>= 0.1.0)\n\nPATH\n remote: {puma}\n specs:\n puma (6.4.2)\n nio4r (~> 2.0)\n\n", puma = puma_rel(), rack = copy_rel().replace(UUID, UUID2)), + ), + "rack's section moved to the new uuid's sorted position" + ); + } + + /// On a re-vendor over a CHECKSUMS lock the checksum record must ride + /// AGAIN with `original: None`: dropped, the first run's registry + /// `sha256=` line would vanish from the ledger with the replaced entry, + /// and a post-update revert would leave a bare CHECKSUMS entry on a + /// registry gem (frozen installs exit 16). + #[tokio::test] + async fn test_revendor_new_uuid_checksums_keeps_restore_data() { + let (_tmp, root, installed, blobs, record) = + fixture_318(SPIKE_GEMFILE_CHECKSUMS, SPIKE_LOCK_CHECKSUMS_BEFORE).await; + let (r1, e1, _) = + unwrap_done(run_vendor_318(&root, &blobs, &installed, &record, false).await); + assert!(r1.success); + let entry1 = e1.unwrap(); + + let mut record2 = record.clone(); + record2.uuid = UUID2.to_string(); + let (r2, e2, _) = + unwrap_done(run_vendor_318(&root, &blobs, &installed, &record2, false).await); + assert!(r2.success, "re-vendor must succeed: {:?}", r2.error); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE_LOCK)) + .await + .unwrap(), + expected_lock_checksums().replace(UUID, UUID2) + ); + + let mut entry2 = e2.unwrap(); + assert_eq!(entry2.wiring.len(), 3, "{:?}", entry2.wiring); + let ck = &entry2.wiring[2]; + assert_eq!(ck.kind, LOCK_CHECKSUM_WIRING_KIND); + assert!(ck.original.is_none(), "{:?}", ck.original); + assert_eq!( + ck.new.as_ref().unwrap(), + &Value::String(" rack (3.1.8)".to_string()) + ); + + carry_forward_originals(&entry1, &mut entry2); + let outcome = revert_gem(&entry2, &root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!( + !outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_drifted"), + "clean revert must not report drift: {:?}", + outcome.warnings + ); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE_LOCK)) + .await + .unwrap(), + SPIKE_LOCK_CHECKSUMS_BEFORE, + "registry sha256 line restored" + ); + assert_eq!( + tokio::fs::read_to_string(root.join(GEMFILE)).await.unwrap(), + SPIKE_GEMFILE_CHECKSUMS + ); + } + + /// The GEM-specs twin of `test_checksums_platform_sibling_fails_closed`: + /// on a bundler < 2.6 lock (no CHECKSUMS section to catch it in) a + /// platform-suffixed sibling spec must fail the lift closed — lifting + /// only the plain entry would leave the sibling behind as a stale + /// registry spec. + #[test] + fn test_gem_specs_platform_sibling_fails_closed() { + let lock = "GEM\n remote: https://rubygems.org/\n specs:\n nokogiri (1.16.0)\n racc (~> 1.4)\n nokogiri (1.16.0-arm64-darwin)\n racc (~> 1.4)\n\nPLATFORMS\n arm64-darwin\n ruby\n\nDEPENDENCIES\n nokogiri\n\nBUNDLED WITH\n 2.5.22\n"; + let rel = format!(".socket/vendor/gem/{UUID}/nokogiri-1.16.0"); + let err = match edit_lock(lock, "nokogiri", "1.16.0", &rel) { + Err(e) => e, + Ok(_) => panic!("a platform-suffixed GEM specs sibling must fail closed"), + }; + assert!(err.contains("platform-suffixed"), "{err}"); + } + /// Trailing options on the declaration (`require: false`, `group: :test`, /// …) must survive the rewrite: dropping `require: false` auto-requires /// the gem at boot, changing app behavior while vendored (the redirect diff --git a/crates/socket-patch-core/src/vendor/registry_fetch.rs b/crates/socket-patch-core/src/vendor/registry_fetch.rs index 0f0c92a5..532f02f2 100644 --- a/crates/socket-patch-core/src/vendor/registry_fetch.rs +++ b/crates/socket-patch-core/src/vendor/registry_fetch.rs @@ -26,6 +26,7 @@ use sha2::{Digest, Sha256, Sha384, Sha512}; use crate::constants::USER_AGENT; use crate::crawlers::go_crawler::encode_module_path; use crate::patch::apply::is_safe_relative_subpath; +use crate::patch::path_safety::is_safe_single_segment; use super::lock_inventory::{LockIntegrity, LockfileEntry}; @@ -237,6 +238,18 @@ async fn fetch_gem( entry: &LockfileEntry, client: &reqwest::Client, ) -> Result { + // The staged leaf must be the canonical `{name}-{version}`: the gem + // vendor backend refuses any other leaf as a platform-suffixed install + // (`platform_gem_unsupported`), so a generic name would kill the whole + // auto-fetch path. The coordinates thereby become a tempdir path + // component — `inventory_gemfile_lock` already filters both, but + // re-assert locally (defense in depth), before any network I/O. + if !is_safe_single_segment(&entry.name) || !is_safe_single_segment(&entry.version) { + return Err(FetchError::Failed(format!( + "unsafe gem coordinates `{}` @ `{}` — refusing to stage", + entry.name, entry.version + ))); + } let Some(url) = entry.resolved.clone() else { return Err(FetchError::Unverifiable(format!( "no download URL for {}@{}", @@ -248,7 +261,7 @@ async fn fetch_gem( let tmp = tempfile::tempdir() .map_err(|e| FetchError::Failed(format!("cannot create fetch tempdir: {e}")))?; - let dir = tmp.path().join("gem"); + let dir = tmp.path().join(format!("{}-{}", entry.name, entry.version)); extract_gem_data(&bytes, &dir).map_err(FetchError::Failed)?; Ok(FetchedPackage { dir, @@ -1482,6 +1495,36 @@ mod tests { "data.tar.gz content extracts at the root (no strip)" ); assert!(fetched.dir().join("README.md").is_file()); + // The staged leaf must be the canonical `{name}-{version}`: + // vendor_gem's platform-suffix guard refuses any other leaf + // (`platform_gem_unsupported`), which killed lockfile auto-fetch + // when this dir was named `gem`. + assert_eq!( + fetched.dir().file_name().unwrap().to_string_lossy(), + "rails-7.1.0", + "staged dir leaf must satisfy vendor_gem's `{{name}}-{{version}}` check" + ); + } + + #[tokio::test] + async fn gem_fetch_refuses_unsafe_coordinates_without_network() { + // The coordinates become the staged-dir leaf, so a separator-bearing + // name must refuse — and BEFORE any I/O (the URL would hard-fail if + // contacted). + let entry = LockfileEntry { + ecosystem: "gem", + name: "ra/ils".into(), + version: "7.1.0".into(), + purl: "pkg:gem/ra/ils@7.1.0".into(), + resolved: Some("http://127.0.0.1:1/nope.gem".into()), + integrity: LockIntegrity::Sha256Hex("0".repeat(64)), + }; + match fetch_and_stage(&entry, &build_registry_client()).await { + Err(FetchError::Failed(msg)) => { + assert!(msg.contains("unsafe gem coordinates"), "{msg}") + } + other => panic!("expected coordinate refusal, got {other:?}"), + } } #[tokio::test] diff --git a/crates/socket-patch-core/tests/crawler_ruby_e2e.rs b/crates/socket-patch-core/tests/crawler_ruby_e2e.rs index ac137863..d611dded 100644 --- a/crates/socket-patch-core/tests/crawler_ruby_e2e.rs +++ b/crates/socket-patch-core/tests/crawler_ruby_e2e.rs @@ -290,6 +290,47 @@ async fn get_gem_paths_vendor_bundle_takes_precedence_over_global() { ); } +/// A JRuby deployment (`bundle install --deployment` under JRuby) puts +/// gems in `vendor/bundle/jruby//gems` — Bundler scopes the path +/// by `Gem.ruby_engine`, not the literal `ruby`. The crawler must discover +/// that layout end-to-end; hardcoding the `ruby` engine dir made JRuby and +/// TruffleRuby deployments silently yield zero gems. No Gemfile is staged, +/// so a regression cannot green via the `gem env` fallback. +#[tokio::test] +#[serial_test::parallel] +async fn get_gem_paths_vendor_bundle_jruby_engine_layout() { + let tmp = tempfile::tempdir().unwrap(); + let jruby_gems = tmp + .path() + .join("vendor") + .join("bundle") + .join("jruby") + .join("3.1.4.0") + .join("gems"); + tokio::fs::create_dir_all(jruby_gems.join("rails-7.1.0").join("lib")) + .await + .unwrap(); + + let crawler = RubyCrawler; + let paths = crawler + .get_gem_paths(&options_at(tmp.path())) + .await + .unwrap(); + assert_eq!( + paths, + vec![jruby_gems.clone()], + "vendor/bundle/jruby/*/gems must be discovered; got {paths:?}" + ); + + // End-to-end: the gem inside the jruby scope is crawled. + let crawled = crawler.crawl_all(&options_at(tmp.path())).await; + let purls: Vec<&str> = crawled.iter().map(|p| p.purl.as_str()).collect(); + assert!( + purls.contains(&"pkg:gem/rails@7.1.0"), + "gem in a jruby vendor scope must be crawled; got {purls:?}" + ); +} + #[tokio::test] #[serial_test::parallel] async fn get_gem_paths_no_gemfile_returns_empty() { diff --git a/crates/socket-patch-core/tests/fixtures/redirect/gem/bundler/basic/expected-edits.json b/crates/socket-patch-core/tests/fixtures/redirect/gem/bundler/basic/expected-edits.json index 9467dc3d..314c0958 100644 --- a/crates/socket-patch-core/tests/fixtures/redirect/gem/bundler/basic/expected-edits.json +++ b/crates/socket-patch-core/tests/fixtures/redirect/gem/bundler/basic/expected-edits.json @@ -12,6 +12,7 @@ "kind": "redirect_gemfile_lock_checksum", "action": "rewritten", "key": "rails", + "original": "rails (7.0.0) sha256=2222222222222222222222222222222222222222222222222222222222222222", "new": "rails (7.0.0) sha256=deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef" } ] From 99f9cd0cd476de62eec33a4de55b3752d9cd2d35 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Thu, 13 Aug 2026 17:15:32 -0700 Subject: [PATCH 2/2] fix(review): CRLF-tolerant source-block guard; dry-run VEX skip for all scan arms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both Bugbot findings on #175: - The grant-agnostic Gemfile idempotency guard required LF (`do\n`), so a core.autocrlf checkout of a previously-redirected Gemfile was not recognized and the indented gem line inside the block got wrapped in a second, nested source block on re-run. The recognizer now accepts `do\r?\n`; pinned by gemfile_rerun_on_crlf_checkout_never_nests (verified red without the fix). - The dry-run VEX skip only covered the vendor JSON arm; the interactive `scan --vendor --dry-run --vex` path (embed_vex_human) and the JSON `scan --apply --dry-run --vex` path (embed_vex_into_json at the apply fall-through) still generated the document — exiting 1 on a not-yet-vendored project or writing the attestation during --dry-run. The guard now lives at the top of both embed helpers, covering every scan arm; pinned by scan_vendor_dry_run_with_vex_interactive_* and scan_apply_json_dry_run_with_vex_* (both verified red without it). Co-Authored-By: Claude Fable 5 --- .../socket-patch-cli/src/commands/scan/mod.rs | 16 ++++ .../socket-patch-cli/tests/in_process_scan.rs | 86 +++++++++++++++++++ .../src/patch/redirect/mod.rs | 53 +++++++++++- 3 files changed, 154 insertions(+), 1 deletion(-) diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index eefed677..e4a5320a 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -280,6 +280,15 @@ async fn embed_vex_into_json( if vex_args.vex.is_none() || base_code != 0 { return base_code; } + // A dry run is a non-mutating preview: generating here would verify the + // deliberately untouched tree (failing outright on a not-yet-vendored + // project) and write an attestation file to disk. The marker keeps the + // request visible to JSON consumers instead of silently dropping it + // (same shape as the vendor JSON arm's early return). + if common.dry_run { + result["vex"] = serde_json::json!({ "skipped": true, "reason": "dry_run" }); + return base_code; + } let params = vex_args.to_build_params(); match generate_vex_from_manifest_path(common, ¶ms, manifest_path).await { Ok(summary) => { @@ -314,6 +323,13 @@ async fn embed_vex_human( if vex_args.vex.is_none() || base_code != 0 { return base_code; } + // Dry-run twin of the JSON guard above: no generation, no file write. + if common.dry_run { + if !common.silent { + println!("[dry-run] VEX generation skipped. No attestation written."); + } + return base_code; + } let params = vex_args.to_build_params(); match generate_vex_from_manifest_path(common, ¶ms, manifest_path).await { Ok(summary) => { diff --git a/crates/socket-patch-cli/tests/in_process_scan.rs b/crates/socket-patch-cli/tests/in_process_scan.rs index 96cb0111..c0ea588f 100644 --- a/crates/socket-patch-cli/tests/in_process_scan.rs +++ b/crates/socket-patch-cli/tests/in_process_scan.rs @@ -1540,3 +1540,89 @@ async fn scan_vendor_dry_run_with_vex_does_not_write_attestation_file() { "vendor dry run must leave the manifest byte-for-byte unchanged" ); } + +/// The INTERACTIVE arm's twin: `scan --vendor --dry-run --vex` without +/// `--json` returns through `embed_vex_human`, which generated (and wrote) +/// the document for real — exit 1 on a not-yet-vendored project, an +/// attestation file on disk otherwise. The dry-run guard lives in the embed +/// helpers, so both output modes must skip. +#[tokio::test] +#[serial] +async fn scan_vendor_dry_run_with_vex_interactive_does_not_fail_or_write() { + let server = MockServer::start().await; + mock_batch_one(&server).await; + mock_by_package(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "in-proc-scan", "1.0.0"); + + let vex_path = tmp.path().join("vendor-dry-interactive.vex.json"); + let mut args = default_args(tmp.path()); + args.common.api_url = Some(server.uri()); + args.common.json = false; + args.vendor = true; + args.common.dry_run = true; + args.vex.vex = Some(vex_path.clone()); + + let code = run_scrubbed(args).await; + assert_eq!( + code, 0, + "an interactive vendor dry run must not fail for not-yet-vendored state" + ); + assert!( + !vex_path.exists(), + "interactive dry run must not write the VEX file" + ); +} + +/// `scan --apply --json --dry-run --vex`: the JSON apply arm synthesizes its +/// preview and falls through to `embed_vex_into_json` with apply_code 0, so +/// without the guard the dry run generated and wrote the attestation. +#[tokio::test] +#[serial] +async fn scan_apply_json_dry_run_with_vex_does_not_write_attestation() { + let server = MockServer::start().await; + mock_batch_one(&server).await; + mock_by_package(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "in-proc-scan", "1.0.0"); + + // Attestable manifest (same fixture as the vendor twin above): metadata + // for the statement, `setup.manual: ["npm"]`, `--vex-no-verify` below. + let socket = tmp.path().join(".socket"); + std::fs::create_dir_all(&socket).unwrap(); + std::fs::write( + socket.join("manifest.json"), + r#"{ "patches": { + "pkg:npm/in-proc-scan@1.0.0": { + "uuid": "11111111-1111-4111-8111-111111111111", + "exportedAt": "2024-01-01T00:00:00Z", + "files": {}, + "vulnerabilities": { "GHSA-aaaa-bbbb-cccc": { + "cves": ["CVE-2024-0001"], "summary": "s", + "severity": "HIGH", "description": "d" + }}, + "description": "x", "license": "MIT", "tier": "free" + } + }, "setup": { "manual": ["npm"] } }"#, + ) + .unwrap(); + + let vex_path = tmp.path().join("apply-dry.vex.json"); + let mut args = default_args(tmp.path()); + args.common.api_url = Some(server.uri()); + args.apply = true; + args.common.dry_run = true; + args.vex.vex = Some(vex_path.clone()); + args.vex.vex_no_verify = true; + + let code = run_scrubbed(args).await; + assert_eq!(code, 0, "apply dry run with attestable state must exit 0"); + assert!( + !vex_path.exists(), + "apply dry run must not write the VEX file even when generation would succeed" + ); +} diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 87740fcf..3f337eed 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -1783,10 +1783,14 @@ fn rewrite_gem( url_pat.replace(®ex::escape(&format!("/{rotating}/")), "/[^/\"]+/"); } } + // `\r?\n`: the rewriter emits LF, but a `core.autocrlf` checkout + // rewrites the working tree to CRLF — the guard must still + // recognize the block there, or the indented `gem` line inside + // it falls through to `gem_line_re` and gets wrapped again. let block_re = Regex::new( &(String::from(r#"(?m)^source "("#) + &url_pat - + r#")" do\n gem ["']"# + + r#")" do\r?\n gem ["']"# + ®ex::escape(&dep.name) + r#"["']"#), ) @@ -3888,6 +3892,53 @@ mod tests { ); } + /// A `core.autocrlf` checkout rewrites a previously-redirected Gemfile to + /// CRLF. The block recognizer must still see the Socket source block + /// there: if it misses, the indented `gem` line inside the block matches + /// `gem_line_re` and gets wrapped in a second, nested source block. + #[test] + fn gemfile_rerun_on_crlf_checkout_never_nests() { + fn ov(token: &str) -> DepOverride { + let mut o = gem_override("rails", "7.0.0"); + o.token = token.into(); + if let Some(r) = o.registry_override.as_mut() { + r.index_url = format!("https://patch.test/gem/{token}/uuid/"); + } + o + } + // The block exactly as run 1 writes it, after a CRLF checkout. + let crlf_gemfile = "source \"https://rubygems.org\"\r\n\r\n\ + source \"https://patch.test/gem/tok-one/uuid/\" do\r\n \ + gem \"rails\", \"7.0.0\"\r\nend\r\n"; + let mut files = BTreeMap::new(); + files.insert("Gemfile".to_string(), crlf_gemfile.to_string()); + + // Same grant: recognized in place, a true no-op. + let same = rewrite_registry_redirect(&files, &[ov("tok-one")]); + assert!( + !same.files.contains_key("Gemfile"), + "same-grant re-run on a CRLF checkout must not rewrite the Gemfile: {:?}", + same.files.get("Gemfile") + ); + + // Rotated grant: URL refreshed inside the existing block, never nested. + let rotated = rewrite_registry_redirect(&files, &[ov("tok-two")]); + let out = rotated + .files + .get("Gemfile") + .expect("rotated grant refreshes the URL on a CRLF checkout"); + assert_eq!( + out.matches("source \"https://patch.test/gem/").count(), + 1, + "exactly one Socket source block, never nested: {out}" + ); + assert!(!out.contains("tok-one"), "old grant token gone: {out}"); + assert!( + out.contains("source \"https://patch.test/gem/tok-two/uuid/\" do\r\n"), + "existing CRLF block body left intact: {out}" + ); + } + /// A gem-level source option (`git:` / `path:` / `github:` / `source:`) /// preserved into the Socket source block OVERRIDES it in bundler's DSL, /// leaving the redirect a silent no-op that still gets attested. Fail