diff --git a/crates/socket-patch-cli/tests/e2e_redirect_gem_stale_install.rs b/crates/socket-patch-cli/tests/e2e_redirect_gem_stale_install.rs index 24817325f..f94a760cb 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_gem_stale_install.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_gem_stale_install.rs @@ -630,8 +630,11 @@ async fn gem_hosted_stale_archive_at_configured_cache_path_warns_and_is_not_atte let server = MockServer::start().await; mount_api(&server, None).await; let moved = "---\nBUNDLE_CACHE_PATH: \"vendor/gems\"\n"; + // #951: Bundler drops a trailing `# comment` from the value. + let commented = "---\nBUNDLE_CACHE_PATH: vendor/gems # committed gem cache\n"; for (label, config, env, cache_dir) in [ ("app-config", Some(moved), &[][..], "gems"), + ("app-config-commented", Some(commented), &[][..], "gems"), ( "env", None, diff --git a/crates/socket-patch-cli/tests/in_process_alternate_installers.rs b/crates/socket-patch-cli/tests/in_process_alternate_installers.rs index 6d5553413..0b7d2ebe5 100644 --- a/crates/socket-patch-cli/tests/in_process_alternate_installers.rs +++ b/crates/socket-patch-cli/tests/in_process_alternate_installers.rs @@ -5,7 +5,7 @@ //! supports venv, pyenv, conda, system installs. This file exercises //! the layout variants the crawlers must handle in production. -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::Command; use serial_test::serial; @@ -872,6 +872,99 @@ gem 'colorize', '1.1.0' assert_patched(&lib_file, &patched, &before_hash, &after_hash); } +/// #951: a hand-commented `.bundle/config` path +/// (`BUNDLE_PATH: .gems # project-local gems`). Bundler 2.5.6+ drops the +/// comment and installs into `.gems`; older Bundler keeps it in the +/// directory name. Either way apply must patch the copy Bundler loads +/// (asked from Bundler itself), not fall back to the system gem home. +#[tokio::test] +#[serial] +async fn bundler_commented_config_path_apply_patches_loaded_gem() { + if !has("bundle") || !has("gem") { + println!("SKIP: bundle/gem not on PATH"); + return; + } + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write( + tmp.path().join("Gemfile"), + "source 'https://rubygems.org'\ngem 'colorize', '1.1.0'\n", + ) + .unwrap(); + std::fs::create_dir_all(tmp.path().join(".bundle")).unwrap(); + std::fs::write( + tmp.path().join(".bundle/config"), + "---\nBUNDLE_PATH: .gems # project-local gems\n", + ) + .unwrap(); + let status = pm_command("bundle", &["BUNDLE_"]) + .args(["install", "--quiet"]) + .current_dir(tmp.path()) + .output() + .expect("bundle install"); + if !status.status.success() { + println!( + "SKIP: bundle install failed: {}", + String::from_utf8_lossy(&status.stderr) + ); + return; + } + let loaded = pm_command("bundle", &["BUNDLE_"]) + .args([ + "exec", + "ruby", + "-e", + "print Gem.loaded_specs.fetch('colorize').full_gem_path", + ]) + .current_dir(tmp.path()) + .output() + .expect("bundle exec"); + assert!( + loaded.status.success(), + "bundle exec failed: {}", + String::from_utf8_lossy(&loaded.stderr) + ); + let lib_file = PathBuf::from(String::from_utf8(loaded.stdout).unwrap()).join("lib/colorize.rb"); + assert!( + lib_file.starts_with(tmp.path().canonicalize().unwrap()) + || lib_file.starts_with(tmp.path()), + "bundler must load the project copy: {lib_file:?}" + ); + + let original = std::fs::read(&lib_file).expect("read"); + let before_hash = git_sha256(&original); + let mut patched = original.clone(); + patched.extend_from_slice(b"\n# SOCKET-PATCH-BUNDLER-MARKER\n"); + let after_hash = git_sha256(&patched); + + let socket = tmp.path().join(".socket"); + std::fs::create_dir_all(socket.join("blobs")).unwrap(); + std::fs::write( + socket.join("manifest.json"), + format!( + r#"{{ "patches": {{ + "pkg:gem/colorize@1.1.0": {{ + "uuid": "bundler-uuid-0951", + "exportedAt": "2024-01-01T00:00:00Z", + "files": {{ "package/lib/colorize.rb": {{ + "beforeHash": "{before_hash}", "afterHash": "{after_hash}" + }}}}, + "vulnerabilities": {{}}, "description": "x", + "license": "MIT", "tier": "free" + }} + }}}}"# + ), + ) + .unwrap(); + std::fs::write(socket.join("blobs").join(&after_hash), &patched).unwrap(); + + let mut args = default_apply(tmp.path()); + args.common.ecosystems = Some(vec!["gem".to_string()]); + let code = apply_run(args).await; + assert_eq!(code, 0, "the Bundler-loaded gem must be patchable"); + assert_patched(&lib_file, &patched, &before_hash, &after_hash); +} + // --------------------------------------------------------------------------- // bun install layout // --------------------------------------------------------------------------- diff --git a/crates/socket-patch-core/src/crawlers/ruby_crawler.rs b/crates/socket-patch-core/src/crawlers/ruby_crawler.rs index 61db2695f..fe49c4315 100644 --- a/crates/socket-patch-core/src/crawlers/ruby_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/ruby_crawler.rs @@ -393,7 +393,7 @@ impl RubyCrawler { let mut skipped_config_root = None; if Self::has_bundler_manifest(cwd).await { if let Some(value) = - Self::app_config_bundle_path(cwd, app_config_env, ignore_config).await + Self::app_config_bundle_path(cwd, app_config_env, ignore_config, home).await { match resolve_config_bundle_path(cwd, &value, home) { Some(root) => roots.push(root), @@ -431,10 +431,19 @@ impl RubyCrawler { let shadowed = bundle_path_env.is_some() || (!ignore_config && Self::app_config_sets_path(cwd, app_config_env).await); if !shadowed { - if let Some(value) = read_global_config(global_config, false) - .await - .and_then(|text| parse_bundle_config_path(&text)) - { + let text = read_global_config(global_config, false).await; + let value = match text { + Some(text) => { + bundle_config_dir_reading( + parse_bundle_config_path(&text), + parse_legacy_bundle_config_path(&text), + |value| resolve_bundle_path(cwd, Path::new(value), home), + ) + .await + } + None => None, + }; + if let Some(value) = value { roots.push(resolve_bundle_path(cwd, Path::new(&value), home)); } } @@ -709,6 +718,7 @@ impl RubyCrawler { cwd: &Path, app_config_env: Option<&OsStr>, ignore_config: bool, + home: Option<&Path>, ) -> Option { if ignore_config { return None; @@ -723,7 +733,12 @@ impl RubyCrawler { let contents = crate::utils::fs::read_regular_to_string(&config) .await .ok()?; - parse_bundle_config_path(&contents) + bundle_config_dir_reading( + parse_bundle_config_path(&contents), + parse_legacy_bundle_config_path(&contents), + |value| resolve_bundle_path(cwd, Path::new(value), home), + ) + .await } /// Get global gem paths by querying `gem env` and checking well-known locations. @@ -1513,20 +1528,26 @@ pub async fn bundler_app_cache_dir_with_env( ignore_config: bool, global_config: Option<&Path>, ) -> PathBuf { - let mut configured = read_app_config(root, app_config_env, ignore_config) + // Component-wise, so `vendor/gems` uses the native separator. + let resolve = |value: PathBuf| root.join(normalize_lexically(&value).unwrap_or(value)); + let cache_path = |text: Option| async { + let text = text?; + bundle_config_dir_reading( + bundle_config_setting(&text, "BUNDLE_CACHE_PATH"), + legacy_bundle_config_setting(&text, "BUNDLE_CACHE_PATH"), + |value| resolve(PathBuf::from(value)), + ) .await - .and_then(|text| bundle_config_setting(&text, "BUNDLE_CACHE_PATH")) .map(PathBuf::from) + }; + let mut configured = cache_path(read_app_config(root, app_config_env, ignore_config).await) + .await .or_else(|| cache_env.filter(|v| !v.is_empty()).map(PathBuf::from)); if configured.is_none() { - configured = read_global_config(global_config, ignore_config) - .await - .and_then(|text| bundle_config_setting(&text, "BUNDLE_CACHE_PATH")) - .map(PathBuf::from); + configured = cache_path(read_global_config(global_config, ignore_config).await).await; } match configured { - // Component-wise, so `vendor/gems` uses the native separator. - Some(value) => root.join(normalize_lexically(&value).unwrap_or(value)), + Some(value) => resolve(value), None => root.join("vendor").join("cache"), } } @@ -1618,16 +1639,26 @@ fn resolve_config_bundle_path( /// goes through Bundler's own coercion ([`bundler_truthy`]), so `"1"` or /// `"yes"` count too, while `"false"` leaves the recorded path in effect. fn parse_bundle_config_path(contents: &str) -> Option { + parse_bundle_config_path_with(contents, unquote_bundle_config_value) +} + +/// [`parse_bundle_config_path`] as a Bundler before 2.5.6 reads it (see +/// [`unquote_legacy_bundle_config_value`]). +fn parse_legacy_bundle_config_path(contents: &str) -> Option { + parse_bundle_config_path_with(contents, unquote_legacy_bundle_config_value) +} + +fn parse_bundle_config_path_with(contents: &str, unquote: fn(&str) -> &str) -> Option { let mut path: Option = None; let mut path_system = false; for line in contents.lines() { if let Some(rest) = line.strip_prefix("BUNDLE_PATH:") { - let v = unquote_bundle_config_value(rest); + let v = unquote(rest); if !v.is_empty() { path = Some(v.to_string()); } } else if let Some(rest) = line.strip_prefix("BUNDLE_PATH__SYSTEM:") { - path_system = bundler_truthy(unquote_bundle_config_value(rest)); + path_system = bundler_truthy(unquote(rest)); } } if path_system { @@ -1668,24 +1699,97 @@ pub(crate) fn bundle_config_setting(contents: &str, key: &str) -> Option /// path. Callers that decide whether a lower tier applies need presence, /// not just a non-empty value. pub(crate) fn bundle_config_setting_including_empty(contents: &str, key: &str) -> Option { + bundle_config_setting_with(contents, key, unquote_bundle_config_value) +} + +/// [`bundle_config_setting`] as a Bundler before 2.5.6 reads it (see +/// [`unquote_legacy_bundle_config_value`]). +fn legacy_bundle_config_setting(contents: &str, key: &str) -> Option { + bundle_config_setting_with(contents, key, unquote_legacy_bundle_config_value) + .filter(|value| !value.is_empty()) +} + +fn bundle_config_setting_with( + contents: &str, + key: &str, + unquote: fn(&str) -> &str, +) -> Option { let mut found = None; for line in contents.lines() { if let Some(rest) = line.strip_prefix(key).and_then(|r| r.strip_prefix(':')) { - let v = unquote_bundle_config_value(rest); - found = Some(v.to_string()); + found = Some(unquote(rest).to_string()); } } found } -/// Unwrap one bundler app-config scalar: trim, then strip one matching -/// pair of double or single quotes (bundler double-quotes what it writes). +/// Unwrap one bundler app-config scalar the way Bundler's config loader +/// does (`Gem::YAMLSerializer`, RubyGems 3.5.6+, which Bundler 2.4+ uses +/// when present; Bundler's own copy from 2.5.6): trim, strip one matching +/// pair of double or single quotes (bundler double-quotes what it writes), +/// then `strip_comment` — cut the value at its first `#` and trim, unless +/// it starts with `#` (#951). The quote pair must close the line, so +/// `"vendor/bundle" # note` keeps its quotes, as it does for Bundler. pub(crate) fn unquote_bundle_config_value(rest: &str) -> &str { let v = rest.trim(); + strip_bundle_config_comment(unquote_matching_pair(v).unwrap_or(v)) +} + +/// [`unquote_bundle_config_value`] for the loader Bundler used before +/// `strip_comment` (Bundler < 2.4, or 2.4–2.5.5 on RubyGems < 3.5.6): the +/// `# comment` stays part of the value. +fn unquote_legacy_bundle_config_value(rest: &str) -> &str { + let v = rest.trim(); + unquote_matching_pair(v).unwrap_or(v) +} + +fn unquote_matching_pair(v: &str) -> Option<&str> { v.strip_prefix('"') .and_then(|s| s.strip_suffix('"')) .or_else(|| v.strip_prefix('\'').and_then(|s| s.strip_suffix('\''))) - .unwrap_or(v) +} + +/// Bundler's `YAMLSerializer#strip_comment`. +fn strip_bundle_config_comment(v: &str) -> &str { + match v.split_once('#') { + Some((value, _)) if !v.starts_with('#') => value.trim(), + _ => v, + } +} + +/// A directory setting whose value carries a `# comment` reads differently +/// in the two Bundler config-loader eras (see +/// [`unquote_legacy_bundle_config_value`]), and the installed Bundler's era +/// is not known here. Bundler creates the directory it uses, so take the +/// current reading unless only the legacy reading's directory exists. +/// Values without a comment read the same in both eras. +/// +/// Only two directory readings are weighed against each other. When the +/// current reading is unset — e.g. a commented `path.system: true` that +/// only the current loader honours — the legacy path's directory is no +/// evidence of the era (it may be a leftover install, the #915 shape), so +/// the current reading stands. +async fn bundle_config_dir_reading( + current: Option, + legacy: Option, + resolve: impl Fn(&str) -> PathBuf, +) -> Option { + let (Some(value), Some(legacy)) = (current.as_deref(), legacy) else { + return current; + }; + if value == legacy { + return current; + } + let is_dir = |path: PathBuf| async move { + tokio::fs::metadata(path) + .await + .is_ok_and(|meta| meta.is_dir()) + }; + if !is_dir(resolve(value)).await && is_dir(resolve(&legacy)).await { + Some(legacy) + } else { + current + } } /// Whether a PURL-derived gem coordinate is safe to join onto the gem root. @@ -4558,4 +4662,149 @@ mod tests { } assert!(found > 50, "vacuous fixtures: {found}"); } + + /// #951: Bundler's config loader (`Gem::YAMLSerializer#strip_comment`, + /// RubyGems / Bundler 2.5.6+) cuts a `.bundle/config` value at its + /// first `#` unless the value starts with one, and applies that to the + /// unquoted value. Pinned against the real loader's output. + #[test] + fn bundle_config_values_drop_a_trailing_comment_like_bundler() { + let text = "---\nBUNDLE_PATH: .gems # project-local gems\n\ + BUNDLE_A: \"a#b\"\nBUNDLE_B: x#y\nBUNDLE_C: #z\n\ + BUNDLE_D: \"vendor/bundle\" # quoted then commented\n\ + BUNDLE_E: \"a # b\"\nBUNDLE_F: ' q ' \n"; + let get = |key| bundle_config_setting_including_empty(text, key); + assert_eq!(get("BUNDLE_PATH").as_deref(), Some(".gems")); + assert_eq!(get("BUNDLE_A").as_deref(), Some("a")); + assert_eq!(get("BUNDLE_B").as_deref(), Some("x")); + // A value that STARTS with `#` is kept whole. + assert_eq!(get("BUNDLE_C").as_deref(), Some("#z")); + // The closing quote is not at the end of the line, so the loader + // matches no quote pair: the quotes stay part of the value. + assert_eq!(get("BUNDLE_D").as_deref(), Some("\"vendor/bundle\"")); + assert_eq!(get("BUNDLE_E").as_deref(), Some("a")); + // No `#`: the unquoted value is kept as is. + assert_eq!(get("BUNDLE_F").as_deref(), Some(" q ")); + } + + /// #951: a commented `path`, `path.system`, `cache_path` and `gemfile` + /// read the way Bundler reads them. + #[tokio::test] + async fn commented_bundle_config_settings_follow_bundler() { + assert_eq!( + parse_bundle_config_path("---\nBUNDLE_PATH: .gems # project-local gems\n"), + Some(".gems".to_string()) + ); + assert_eq!( + parse_bundle_config_path( + "---\nBUNDLE_PATH: vendor/bundle\nBUNDLE_PATH__SYSTEM: true # use system gems\n" + ), + None + ); + assert_eq!( + crate::formats::gem::manifest::config_gemfile("---\nBUNDLE_GEMFILE: gems.rb # twin\n") + .as_deref(), + Some("gems.rb") + ); + + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join(".bundle")).unwrap(); + std::fs::write( + root.join(".bundle").join("config"), + "---\nBUNDLE_CACHE_PATH: vendor/gems # committed gem cache\n", + ) + .unwrap(); + assert_eq!( + bundler_app_cache_dir_with_env(root, None, None, false, None).await, + root.join("vendor").join("gems") + ); + } + + /// #951 repro: `BUNDLE_PATH: .gems # comment` must discover the + /// `.gems` store Bundler installs into, not a directory named after + /// the whole line. + #[tokio::test] + async fn commented_app_config_bundle_path_discovers_the_bundler_store() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("Gemfile"), b"gem \"colorize\"\n").unwrap(); + std::fs::create_dir_all(dir.path().join(".bundle")).unwrap(); + std::fs::write( + dir.path().join(".bundle").join("config"), + "---\nBUNDLE_PATH: .gems # project-local gems\n", + ) + .unwrap(); + let gems = dir + .path() + .join(".gems") + .join("ruby") + .join("3.3.0") + .join("gems"); + std::fs::create_dir_all(gems.join("colorize-0.8.1").join("lib")).unwrap(); + std::fs::create_dir_all(dir.path().join(".gems/ruby/3.3.0/specifications")).unwrap(); + + let paths = RubyCrawler::get_vendor_bundle_paths_with_env(dir.path(), None, None).await; + assert_eq!(paths, vec![gems]); + } + + /// Bundler before 2.5.6 (or 2.4/2.5 on RubyGems before 3.5.6) keeps the + /// comment in the value and installs into a directory named after it. + /// When only that legacy-era directory exists, discovery must still + /// find the store (the pre-#951 behaviour), not fall back to the + /// system gem homes. + #[tokio::test] + async fn commented_bundle_path_keeps_the_legacy_bundler_store() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("Gemfile"), b"gem \"colorize\"\n").unwrap(); + std::fs::create_dir_all(dir.path().join(".bundle")).unwrap(); + std::fs::write( + dir.path().join(".bundle").join("config"), + "---\nBUNDLE_PATH: .gems # note\n", + ) + .unwrap(); + let root = dir.path().join(".gems # note"); + let gems = root.join("ruby").join("2.7.0").join("gems"); + std::fs::create_dir_all(gems.join("colorize-0.8.1").join("lib")).unwrap(); + std::fs::create_dir_all(root.join("ruby/2.7.0/specifications")).unwrap(); + + let paths = RubyCrawler::get_vendor_bundle_paths_with_env(dir.path(), None, None).await; + assert_eq!(paths, vec![gems]); + + // Same for the cache path: only the legacy-era dir exists. + std::fs::write( + dir.path().join(".bundle").join("config"), + "---\nBUNDLE_CACHE_PATH: vendor/gems # c\n", + ) + .unwrap(); + let legacy_cache = dir.path().join("vendor").join("gems # c"); + std::fs::create_dir_all(&legacy_cache).unwrap(); + assert_eq!( + bundler_app_cache_dir_with_env(dir.path(), None, None, false, None).await, + legacy_cache + ); + } + + /// Bugbot on #953: a commented `path.system: true` drops the recorded + /// path under the current loader. A leftover directory at that + /// recorded path must not bring it back through the legacy reading. + #[tokio::test] + async fn commented_path_system_true_ignores_a_leftover_recorded_path() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("Gemfile"), b"gem \"foo\"\n").unwrap(); + std::fs::create_dir_all(dir.path().join(".bundle")).unwrap(); + std::fs::write( + dir.path().join(".bundle").join("config"), + "---\nBUNDLE_PATH: vendor/mygems\nBUNDLE_PATH__SYSTEM: true # use system gems\n", + ) + .unwrap(); + let root = dir.path().join("vendor").join("mygems"); + std::fs::create_dir_all(root.join("gems").join("foo-1.0.0").join("lib")).unwrap(); + std::fs::create_dir_all(root.join("specifications")).unwrap(); + + let paths = RubyCrawler::get_vendor_bundle_paths_with_env(dir.path(), None, None).await; + assert!( + paths.is_empty(), + "a commented path.system=true must still drop the config root: {paths:?}" + ); + } }