From dc39f60ecaa0348b0951c045563068c29325ff45 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 4 Nov 2026 11:40:25 -0600 Subject: [PATCH 1/3] Read BOM composer locks and pin the no-lock-download guard Composer: one leading UTF-8 BOM is now read past by every composer.lock reader that parsed it strictly (the lock inventory, the vendored backend's plan, revert and stranded scan, and the CLI's reinstall hints), through the shared `parse_json_manifest` / `parse_json_text` / `strip_bom` helpers lockfile discovery already uses. The vendored splice and its whole-document fallback keep the BOM on write, so vendor then `--revert` is byte-identical. Before, the stranded scan read a BOM lock as "nothing wired". A wet revert of an entry with no pre-vendor fragment (a `repair`-rebuilt entry) then drift-skipped, and in a lock that spells slashes `\/` the drift-keep's raw-text probe missed the path, so the revert deleted the artifact composer.lock still installs from. The new test reproduces it. Pristine fetch: verified that no vendored or repair path downloads what a lock entry pins. Vendored bytes come from the patch service by patch uuid; `recover_lock_entry` only feeds a yes/no refusal gate; registry downloads are the takeover's upstream restore (registry metadata) and the JVM backend's coordinate-built Maven Central fetches. A new architecture test pins those call sites, and the stale docs that said vendor fetches the lock's resolution are corrected. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/composer_hints.rs | 11 +- .../socket-patch-cli/src/commands/vendor.rs | 7 + .../src/vendor/composer_lock.rs | 176 ++++++++++++++++-- .../src/vendor/lock_inventory/composer.rs | 5 +- .../src/vendor/lock_inventory/mod.rs | 19 +- .../src/vendor/lock_inventory/recover.rs | 14 +- .../src/vendor/lock_inventory/tests.rs | 112 +++++++++++ 7 files changed, 318 insertions(+), 26 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/composer_hints.rs b/crates/socket-patch-cli/src/commands/composer_hints.rs index cbc59f33b..bb587147e 100644 --- a/crates/socket-patch-cli/src/commands/composer_hints.rs +++ b/crates/socket-patch-cli/src/commands/composer_hints.rs @@ -46,8 +46,10 @@ const VENDOR_PREFIX: &str = ".socket/vendor/composer/"; /// The `/` of every composer.lock package (`packages` and /// `packages-dev`) whose dist is a vendored `path` copy, sorted and -/// de-duplicated. An unparseable lock yields nothing. +/// de-duplicated. An unparseable lock yields nothing; one leading BOM is +/// read past, as the vendored backend reads the lock. pub(crate) fn vendored_composer_packages(lock_text: &str) -> Vec { + let lock_text = socket_patch_core::formats::text::strip_bom(lock_text); let Ok(lock) = serde_json::from_str::(lock_text) else { return Vec::new(); }; @@ -182,6 +184,13 @@ mod tests { ); assert!(vendored_composer_packages("not json").is_empty()); assert!(vendored_composer_packages("{}").is_empty()); + // One leading BOM is encoding (the backend reads past it); a second + // is content. + assert_eq!( + vendored_composer_packages(&format!("\u{feff}{}", lock())), + vendored_composer_packages(&lock()) + ); + assert!(vendored_composer_packages(&format!("\u{feff}\u{feff}{}", lock())).is_empty()); } #[test] diff --git a/crates/socket-patch-cli/src/commands/vendor.rs b/crates/socket-patch-cli/src/commands/vendor.rs index 873e394fd..c2cd66fde 100644 --- a/crates/socket-patch-cli/src/commands/vendor.rs +++ b/crates/socket-patch-cli/src/commands/vendor.rs @@ -2732,6 +2732,13 @@ async fn sweep_stale_artifact( ); } +/// Whether the lock (or, for a vendored package, the ledger's pre-vendor +/// fragment) resolves `purl` with a content verifier: the yes/no gate +/// [`lock_refusals_reaching_backend`] reads. It downloads nothing — the +/// vendored backends take their bytes from the patch service by patch +/// uuid — so a Socket-hosted entry's patched URL and hash are never used +/// as a pristine source here (the callers leave hosted-pinned purls to the +/// takeover, which restores the upstream entry from the registry). pub(crate) async fn pristine_fetch_is_verifiable( project_root: &Path, inventory: &[lock_inventory::LockfileEntry], diff --git a/crates/socket-patch-core/src/vendor/composer_lock.rs b/crates/socket-patch-core/src/vendor/composer_lock.rs index 5a23aa60d..d65968f61 100644 --- a/crates/socket-patch-core/src/vendor/composer_lock.rs +++ b/crates/socket-patch-core/src/vendor/composer_lock.rs @@ -44,6 +44,7 @@ use std::sync::Arc; use serde_json::{json, Map, Value}; +use crate::formats::text::split_bom; use crate::manifest::schema::PatchRecord; use crate::patch::apply::PatchSources; use crate::patch::copy_tree::remove_tree; @@ -54,7 +55,7 @@ use crate::utils::line_endings::LineEndings; use crate::utils::purl::{build_composer_purl, parse_composer_purl}; use super::common::{ - already_patched_result, copy_matches_after_hashes, done, inventory_or_warn, + already_patched_result, copy_matches_after_hashes, done, inventory_or_warn, parse_json_text, prune_empty_vendor_levels, refused, serialize_json, service_offline_conflict, stage_dir_for, swap_stage_into_place, synthesized_result, }; @@ -183,9 +184,7 @@ async fn composer_prelude( } }; // An unparseable lock is as unusable as a missing one — same refusal code. - let lock = match LOCK_MEMO.parse(lock_text.as_bytes(), || { - serde_json::from_str::(&lock_text) - }) { + let lock = match LOCK_MEMO.parse(lock_text.as_bytes(), || parse_json_text(&lock_text)) { Ok(v) => v, Err(e) => { return Err(refused( @@ -825,17 +824,20 @@ fn reseed_memo(memo: &ParseMemo, bytes: Vec, lock: Value) { /// The lock bytes after `lock[section][idx]` (already set in `lock`) /// replaced the entry at that position of `current`, the text on disk: a /// splice of that entry alone, or the whole document when the entry cannot -/// be located on its own lines. +/// be located on its own lines. A leading BOM on `current` is kept on +/// either path (the readers parse past it). fn lock_bytes_with_entry( lock: &Value, current: &str, section: &str, idx: usize, ) -> std::io::Result> { - match lock_text::replace_entry(current, section, idx, &lock[section][idx]) { - Some(text) => Ok(text.into_bytes()), - None => composer_lock_bytes(lock, current), - } + let (bom, body) = split_bom(current); + let body = match lock_text::replace_entry(body, section, idx, &lock[section][idx]) { + Some(text) => text.into_bytes(), + None => composer_lock_bytes(lock, body)?, + }; + Ok([bom.as_bytes(), &body].concat()) } /// The `
:` keys this entry can actually put back: @@ -859,7 +861,10 @@ fn restorable_keys(entry: &VendorEntry) -> HashSet { /// /// A missing or unparseable composer.lock yields none: no install can be /// consuming a lock nothing can read, and the restore loop already degrades -/// to `vendor_lock_entry_drifted` for it. +/// to `vendor_lock_entry_drifted` for it. A lock with one leading BOM is +/// parseable (read past it like every other reader of the lock): treating +/// it as unreadable here would skip the refusal and let the revert delete +/// artifacts the lock still names. async fn stranded_wired_packages( lock_path: &Path, uuid: &str, @@ -868,7 +873,7 @@ async fn stranded_wired_packages( let Ok(text) = read_regular_to_string(lock_path).await else { return Vec::new(); }; - let Ok(lock) = LOCK_MEMO.parse(text.as_bytes(), || serde_json::from_str::(&text)) else { + let Ok(lock) = LOCK_MEMO.parse(text.as_bytes(), || parse_json_text(&text)) else { return Vec::new(); }; stranded_in(&lock, uuid, restorable) @@ -920,9 +925,7 @@ async fn restore_lock_entry( Err(e) => return Err(format!("unreadable composer.lock: {e}")), }; let lock = LOCK_MEMO - .parse(lock_text.as_bytes(), || { - serde_json::from_str::(&lock_text) - }) + .parse(lock_text.as_bytes(), || parse_json_text(&lock_text)) .map_err(|e| format!("unparseable composer.lock: {e}"))?; let Some(idx) = wired_entry_index(&lock, section, pkg, uuid) else { @@ -4297,4 +4300,149 @@ mod tests { b"" ); } + + // ───────────────────── a lock saved with a UTF-8 BOM ────────────────────── + + /// `text` with one leading UTF-8 BOM, as a Windows editor saving + /// "UTF-8 with signature" writes it. + fn with_bom(text: &[u8]) -> Vec { + let mut bytes = crate::formats::text::BOM.to_string().into_bytes(); + bytes.extend_from_slice(text); + bytes + } + + /// A BOM lock is read past its BOM (the way lockfile discovery reads + /// it): vendoring plans and rewires it instead of refusing it as + /// `vendor_lockfile_missing`, the BOM survives the splice and an in-sync + /// re-run, and `--revert` restores the lock byte for byte. + #[tokio::test] + async fn bom_lock_vendors_keeps_its_bom_and_reverts_byte_identical() { + let lock = lock_value("psr/log", "3.0.2", false); + let (dir, blobs, installed, record) = fixture(&lock).await; + let root = dir.path(); + let bom_bytes = with_bom(&composer_json_bytes(&lock).unwrap()); + tokio::fs::write(root.join(COMPOSER_LOCK), &bom_bytes) + .await + .unwrap(); + + assert!( + service_preflight(PURL, root, &record).await.is_some(), + "the download plan must ask for the BOM lock's package" + ); + let (result, entry, _w) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, PURL, false).await); + assert!(result.success, "{:?}", result.error); + let entry = entry.unwrap(); + let vendored = tokio::fs::read(root.join(COMPOSER_LOCK)).await.unwrap(); + let body = crate::formats::text::strip_bom_bytes(&vendored); + assert_eq!(body.len() + 3, vendored.len(), "the BOM is kept"); + assert_eq!( + crate::formats::text::strip_bom_bytes(body).len(), + body.len(), + "exactly one BOM" + ); + let wired: Value = serde_json::from_slice(body).unwrap(); + assert_eq!(wired["packages"][0]["dist"]["url"], copy_rel()); + + let (rerun, _, _) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, PURL, false).await); + assert!(rerun.success, "{:?}", rerun.error); + assert_eq!( + tokio::fs::read(root.join(COMPOSER_LOCK)).await.unwrap(), + vendored, + "an in-sync re-vendor leaves the BOM lock untouched" + ); + + let outcome = revert_composer(&entry, root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert_eq!( + tokio::fs::read(root.join(COMPOSER_LOCK)).await.unwrap(), + bom_bytes, + "BOM lock restored byte-identically" + ); + assert!(!root.join(copy_rel()).exists(), "uuid dir removed"); + } + + /// The whole-document fallback (an entry not on lines of its own, here + /// a minified lock) keeps the BOM too. + #[test] + fn whole_document_rewrite_keeps_the_bom() { + let mut lock = lock_value("psr/log", "3.0.2", false); + let minified = with_bom(serde_json::to_string(&lock).unwrap().as_bytes()); + let current = String::from_utf8(minified).unwrap(); + lock["packages"][0]["version"] = json!("3.0.3"); + let bytes = lock_bytes_with_entry(&lock, ¤t, "packages", 0).unwrap(); + let body = crate::formats::text::strip_bom_bytes(&bytes); + assert_eq!(body.len() + 3, bytes.len(), "one BOM kept"); + let parsed: Value = serde_json::from_slice(body).unwrap(); + assert_eq!(parsed, lock); + } + + /// The stranded scan reads a BOM lock like any other: an unrestorable + /// wired entry is still reported (a second BOM is content, so that lock + /// stays unparseable). + #[tokio::test] + async fn stranded_scan_reads_a_bom_lock() { + let dir = tempfile::tempdir().unwrap(); + let lock_path = dir.path().join(COMPOSER_LOCK); + let none: HashSet = HashSet::new(); + let lock = json!({ + "packages": [ + { "name": "psr/log", "dist": { "type": "path", "url": copy_rel() } } + ] + }); + let bytes = with_bom(&composer_json_bytes(&lock).unwrap()); + tokio::fs::write(&lock_path, &bytes).await.unwrap(); + assert_eq!( + stranded_wired_packages(&lock_path, UUID, &none).await, + vec!["psr/log".to_string()] + ); + tokio::fs::write(&lock_path, with_bom(&bytes)) + .await + .unwrap(); + assert!(stranded_wired_packages(&lock_path, UUID, &none) + .await + .is_empty()); + } + + /// The stranded refusal guards a BOM lock too. Before, the BOM made the + /// scan read "nothing wired", the no-original record drift-skipped, and + /// the drift-keep's raw-text probe missed the `\/`-escaped path: the + /// revert DELETED the artifact composer.lock still installs from. + #[tokio::test] + async fn revert_refuses_to_strand_a_bom_lock_entry() { + let lock = lock_value("psr/log", "3.0.2", false); + let (dir, blobs, installed, record) = fixture(&lock).await; + let root = dir.path(); + let text = String::from_utf8(composer_json_bytes(&lock).unwrap()).unwrap(); + tokio::fs::write(root.join(COMPOSER_LOCK), text.replace('/', "\\/")) + .await + .unwrap(); + let (result, entry, _w) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, PURL, false).await); + assert!(result.success, "{:?}", result.error); + // A `repair`-reconstructed entry: the artifact, no pre-vendor lock + // fragment to put back. + let mut entry = entry.unwrap(); + entry.wiring[0].original = None; + // The wired lock re-saved with a BOM. + let wired = with_bom(&tokio::fs::read(root.join(COMPOSER_LOCK)).await.unwrap()); + tokio::fs::write(root.join(COMPOSER_LOCK), &wired) + .await + .unwrap(); + + let outcome = revert_composer(&entry, root, false).await; + assert!(!outcome.success, "{:?}", outcome.warnings); + let error = outcome.error.unwrap_or_default(); + assert!( + error.contains("refusing revert") && error.contains("psr/log"), + "{error}" + ); + assert!(root.join(copy_rel()).exists(), "the wired artifact is kept"); + assert_eq!( + tokio::fs::read(root.join(COMPOSER_LOCK)).await.unwrap(), + wired, + "lock untouched" + ); + } } diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/composer.rs b/crates/socket-patch-core/src/vendor/lock_inventory/composer.rs index 207f5088d..c6f855aa8 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/composer.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/composer.rs @@ -7,6 +7,7 @@ use std::path::Path; use serde_json::Value; use crate::formats::composer::ComposerLock; +use crate::vendor::common::parse_json_manifest; use super::view::ProjectView; use super::{dedup_prefer_integrity, LockfileEntry}; @@ -38,6 +39,8 @@ pub(super) async fn inventory_composer_lock_raw_in( view: &ProjectView<'_>, ) -> Option> { let bytes = view.read_bytes("composer.lock").await.ok()?; - let doc: Value = serde_json::from_slice(&bytes).ok()?; + // Past one leading BOM, as lockfile discovery and the vendored backend + // read it: a BOM lock is otherwise inventoried as no lock at all. + let doc: Value = parse_json_manifest(&bytes).ok()?; Some(ComposerLock::from_doc(&doc).entries()) } diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs b/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs index 3d4bc58fb..7ddd5edbf 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs @@ -6,11 +6,20 @@ //! * `scan` / `get` supplement their installed-tree crawl with lockfile-only //! entries (discovery on fresh clones and partial installs), warning that //! those packages are not yet installed; -//! * `vendor` fetches the pristine artifact for a lockfile-resolved package -//! with no installed copy ([`super::registry_fetch`]), verifying the bytes -//! against the integrity the lock records — FAIL-CLOSED: an entry whose -//! lock carries no content verifier is never fetched; -//! * `repair` recovers ledger entries ([`recover_lock_entry`]); +//! * `vendor` asks whether a package with no installed copy is resolved by +//! the lock with a content verifier (the CLI's lock-refusal gate), and +//! names the lock-resolvable packages an offline run could not vendor; +//! * [`recover_lock_entry`] answers the same question from a ledger +//! entry's pre-vendor fragments. +//! +//! None of these DOWNLOADS what an entry pins. A Socket-hosted entry is +//! listed as the package it replaces, with the hosted artifact's URL and +//! the PATCHED artifact's hash for some formats (composer, gem, npm-family +//! tarball pins), so its `resolved`/`integrity` must never be taken for a +//! pristine source: vendored bytes come from the patch service by patch +//! uuid, and a hosted → vendored takeover re-resolves the upstream entry +//! from registry metadata. `tests::lock_pinned_locations_never_drive_a_download` +//! pins this; //! * ledger liveness (`vex::discover`) reads EVERY lock's instance //! ([`inventory_project_every_lock`]) as evidence that a package resolves //! from somewhere other than its patch. diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/recover.rs b/crates/socket-patch-core/src/vendor/lock_inventory/recover.rs index d6b98962c..7b9279a24 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/recover.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/recover.rs @@ -21,11 +21,15 @@ use super::{http_url, LockIntegrity, LockfileEntry, SourceKind}; /// ledger entry's wiring `original` fragments (and `entry.lock` for cargo), /// as a fetchable [`LockfileEntry`]. /// -/// This is the rebuild path for artifacts that are referenced by the rewired -/// lockfile but missing on disk: the live lockfile no longer carries the -/// registry resolution (it points at `.socket/vendor/...`), but `--revert`'s -/// restore data does. golang is deliberately absent — go.sum is never -/// rewired, so the standard [`super::inventory_project`]/[`super::lookup`] path covers it. +/// The live lockfile no longer carries the registry resolution of a +/// vendored package (it points at `.socket/vendor/...`), but `--revert`'s +/// restore data does. The one consumer is the CLI's lock-refusal gate, +/// which only asks whether a verifiable resolution exists: nothing +/// downloads the returned `resolved` (vendored bytes come from the patch +/// service), and a fragment recorded over a hosted pin would carry the +/// patched artifact's URL and hash, never a pristine source. golang is +/// deliberately absent — go.sum is never rewired, so the standard +/// [`super::inventory_project`]/[`super::lookup`] path covers it. /// /// SECURITY: state.json is committed and tamper-able. Recovered URLs go /// through the same http(s)-only gate as inventoried ones, recovered hashes diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/tests.rs b/crates/socket-patch-core/src/vendor/lock_inventory/tests.rs index bd14058d0..1c7960531 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/tests.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/tests.rs @@ -2739,6 +2739,26 @@ async fn bun_binary_hosted_pins_inventory_as_their_registry_package() { assert_eq!(minimist.integrity, LockIntegrity::None); } +/// A composer.lock saved with a UTF-8 BOM inventories past it, as lockfile +/// discovery and the vendored backend read it; before, the whole lock +/// vanished from the inventory. A second BOM is content (unparseable). +#[tokio::test] +async fn composer_lock_with_a_bom_inventories_its_entries() { + let lock = r#"{"packages": [{"name": "good/pkg", "version": "1.0.0", + "dist": {"type": "zip", "url": "https://example.com/p.zip", + "shasum": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"}}]}"#; + let bom = crate::formats::text::BOM; + let tmp = tempfile::tempdir().unwrap(); + write(tmp.path(), "composer.lock", &format!("{bom}{lock}")).await; + let entries = inventory_composer_lock(tmp.path()).await.unwrap(); + assert_eq!(entries.len(), 1, "{entries:?}"); + assert_eq!(entries[0].purl, "pkg:composer/good/pkg@1.0.0"); + assert!(matches!(entries[0].integrity, LockIntegrity::Sha1Hex(_))); + + write(tmp.path(), "composer.lock", &format!("{bom}{bom}{lock}")).await; + assert!(inventory_composer_lock(tmp.path()).await.is_none()); +} + /// composer.lock packages missing a name or version are skipped, and /// names that are unsafe or not `vendor/pkg`-shaped are dropped /// fail-closed (SECURITY: they feed paths and download URLs). @@ -3734,3 +3754,95 @@ fn unsupported_layout_warnings_rewrites_known_codes_and_forwards_unknown_ones() assert_ne!(warnings[1].1, "probe detail text"); assert_eq!(warnings[2].1, "probe detail text"); } + +/// The guard that keeps Socket's PATCHED bytes from ever standing in for a +/// pristine upstream artifact. This inventory (and [`recover_lock_entry`]) +/// lists a Socket-hosted entry as the package it replaces, with the hosted +/// artifact's URL as `resolved` and the patched artifact's hash as +/// `integrity` (composer's dist sha1, a gem's CHECKSUMS sha256, an npm +/// hosted tarball's SRI). That is safe only because no code downloads +/// what a lock pins: +/// +/// * the vendored backends and `repair` take their bytes from the patch +/// service by patch uuid (`vendor::service_fetch::fetch_verified_archive`), +/// verified against the service's own digest; +/// * `recover_lock_entry`'s one production caller is the CLI's +/// `pristine_fetch_is_verifiable`, a yes/no gate on which lock-text +/// refusals to report; it fetches nothing; +/// * the only registry downloads are the hosted → vendored takeover's +/// upstream restore (`patch::redirect::upstream::client`), which +/// re-resolves the entry from the registry's own metadata and never +/// reads the hosted pin's URL or hash, and the JVM backend's Maven +/// Central fetches (`maven_repo`, `jvm_jar`), whose URLs are built from +/// coordinates and checked against the upstream checksum files. +/// +/// A new call site of any of these must keep that property: read the +/// pristine source from the registry, never from a lock entry that might +/// be Socket-hosted. +#[test] +fn lock_pinned_locations_never_drive_a_download() { + fn walk(dir: &Path, out: &mut Vec) { + for entry in std::fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + walk(&path, out); + } else if path.extension().is_some_and(|e| e == "rs") { + out.push(path); + } + } + } + let core = Path::new(env!("CARGO_MANIFEST_DIR")); + let crates = core.parent().unwrap(); + let mut files = Vec::new(); + for krate in ["socket-patch-core", "socket-patch-cli"] { + walk(&crates.join(krate).join("src"), &mut files); + } + // Test modules kept in files of their own (this one included). + files.retain(|path| { + let path = path.to_string_lossy(); + !path.ends_with("tests.rs") && !path.contains("test_support") + }); + let allowed: &[(&str, &[&str])] = &[ + ( + "registry_fetch::download(", + &["socket-patch-core/src/patch/redirect/upstream/client.rs"], + ), + ( + "fetch_registry_bytes(", + &[ + "socket-patch-core/src/patch/jvm_jar.rs", + "socket-patch-core/src/vendor/maven_repo.rs", + ], + ), + ( + "recover_lock_entry(", + &[ + "socket-patch-cli/src/commands/vendor.rs", + "socket-patch-core/src/vendor/lock_inventory/recover.rs", + ], + ), + ]; + for (needle, expected) in allowed { + let mut found: Vec = files + .iter() + .filter(|path| { + let text = std::fs::read_to_string(path).unwrap(); + let production = text.split("#[cfg(test)]").next().unwrap_or_default(); + production.contains(needle) + }) + .map(|path| { + path.strip_prefix(crates) + .unwrap() + .to_string_lossy() + .replace('\\', "/") + }) + .collect(); + found.sort(); + assert_eq!( + found, *expected, + "`{needle}` gained or lost a production call site. A download must never take \ + its URL or hash from a lock entry, which may be Socket-hosted (patched bytes); \ + re-check the new site against this test's docs, then update the list" + ); + } +} From 75b6ca30e64aa15b6a779fcf3017ddb92af2794d Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Thu, 5 Nov 2026 21:57:50 -0600 Subject: [PATCH 2/3] Fail closed on unparseable composer locks; tighten the download guard Review findings on #2017: - Revert now refuses when composer.lock exists but cannot be read or parsed (merge conflict, trailing comma, a second BOM, a FIFO), in dry and wet runs alike. Before, the stranded scan read such a lock as "nothing wired", a record with no pre-vendor fragment drift-skipped, and the `\/`-blind drift-keep let the revert delete the artifact the lock still installs from. - composer.json readers (discovery's install preference, the upstream restore's repository gates, the crawler's installer-paths and vendor-dir) read past one leading BOM via `parse_json_text`. - The vendored lock write copies nothing extra when the lock has no BOM, and the parse memo keeps a BOM lock (it compares the render to the bytes past the BOM). - New `vendor::test_support::production_sources`: each production file read once, CRLF normalized, `#[cfg(test)]` / `cfg(all(test, ..))` items blanked in place (no truncation at the first one), plus a comment/literal-blind identifier scan. The BOM-helper guard and the no-lock-download guard share it. - The download guard pins every production reference to `registry_fetch::download`, `fetch_registry_bytes` and `recover_lock_entry` by file and line text, matched by name (any path, method call, `use`/alias, re-export, function value). Core checks only its own crate; the CLI's `recover_lock_entry` reference is pinned by a twin test in the CLI crate (the two downloads are `pub(crate)`). - Module doc: the "no download" paragraph follows the consumer list. Co-Authored-By: Claude Opus 5.5 --- .../socket-patch-cli/src/commands/vendor.rs | 50 +++ .../src/crawlers/composer_crawler.rs | 12 +- crates/socket-patch-core/src/formats/text.rs | 44 +- .../src/patch/redirect/upstream/composer.rs | 19 +- .../src/vendor/composer_lock.rs | 142 +++++-- .../src/vendor/lock_inventory/mod.rs | 10 +- .../src/vendor/lock_inventory/tests.rs | 183 +++++--- .../src/vendor/test_support.rs | 1 + .../vendor/test_support/production_sources.rs | 398 ++++++++++++++++++ .../src/vex/discover/composer.rs | 21 +- 10 files changed, 738 insertions(+), 142 deletions(-) create mode 100644 crates/socket-patch-core/src/vendor/test_support/production_sources.rs diff --git a/crates/socket-patch-cli/src/commands/vendor.rs b/crates/socket-patch-cli/src/commands/vendor.rs index c2cd66fde..1a4aac73f 100644 --- a/crates/socket-patch-cli/src/commands/vendor.rs +++ b/crates/socket-patch-cli/src/commands/vendor.rs @@ -8160,3 +8160,53 @@ mod unused_vendored_manifest_keys_tests { ); } } + +/// The CLI side of core's `lock_pinned_locations_never_drive_a_download` +/// (see its docs for what the check does and does not catch). Core's +/// registry downloads (`registry_fetch::download`, +/// `maven_repo::fetch_registry_bytes`) are `pub(crate)`, so the compiler +/// already keeps the CLI from calling them (a by-name scan here would +/// only meet the CLI's own `download` locals). What is pinned is the +/// public `recover_lock_entry`: its one CLI reference is +/// [`pristine_fetch_is_verifiable`]'s yes/no gate. +#[cfg(test)] +mod lock_download_guard_tests { + use socket_patch_core::vendor::test_support::production_sources::{ + production_sources, references, + }; + + const PINNED_REFERENCES: &[(&str, &str, &str)] = &[( + "commands/vendor.rs", + "recover_lock_entry", + "Some(le) => lock_inventory::recover_lock_entry(project_root, le)", + )]; + + #[test] + fn lock_pinned_locations_never_drive_a_download() { + let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut found: Vec<(String, &str, String)> = Vec::new(); + for file in production_sources(&src) { + let code = file.code(); + let lines: Vec<&str> = file.text.lines().collect(); + for (line, _) in references(&code, "recover_lock_entry") { + found.push(( + file.rel.clone(), + "recover_lock_entry", + lines[line - 1].trim().to_string(), + )); + } + } + found.sort(); + let expected: Vec<(String, &str, String)> = PINNED_REFERENCES + .iter() + .map(|(rel, name, line)| (rel.to_string(), *name, line.to_string())) + .collect(); + assert_eq!( + found, expected, + "a production reference to `recover_lock_entry` was added, removed or changed. \ + Its recovered `resolved`/`integrity` may be a Socket-hosted (patched) artifact's: \ + never download them as a pristine source; re-check the site, then update \ + PINNED_REFERENCES" + ); + } +} diff --git a/crates/socket-patch-core/src/crawlers/composer_crawler.rs b/crates/socket-patch-core/src/crawlers/composer_crawler.rs index 500874cba..6a694766b 100644 --- a/crates/socket-patch-core/src/crawlers/composer_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/composer_crawler.rs @@ -102,7 +102,7 @@ impl InstallerLayout { let root = crate::utils::fs::read_regular_to_string(&project_root.join("composer.json")) .await .ok() - .and_then(|content| serde_json::from_str::(&content).ok()); + .and_then(|content| crate::vendor::common::parse_json_text(&content).ok()); let extra = root.as_ref().and_then(|r| r.get("extra")); Self { active: true, @@ -630,7 +630,7 @@ const PROJECT_ROOT_SEARCH_DEPTH: usize = 3; /// value counts as unset. Placeholder expansion is the caller's call, see /// [`resolve_vendor_dir`]. fn parse_config_vendor_dir(composer_json: &str) -> Option { - let doc: serde_json::Value = serde_json::from_str(composer_json).ok()?; + let doc: serde_json::Value = crate::vendor::common::parse_json_text(composer_json).ok()?; let raw = doc.get("config")?.get("vendor-dir")?.as_str()?; let trimmed = raw.trim_end_matches(['/', '\\']); (!trimmed.is_empty()).then(|| trimmed.to_string()) @@ -1964,6 +1964,14 @@ mod tests { None ); assert_eq!(parse_config_vendor_dir("{ not json"), None); + // One leading BOM is encoding; a second is content. + let bom = crate::formats::text::BOM; + let json = r#"{"config":{"vendor-dir":"lib/deps"}}"#; + assert_eq!( + parse_config_vendor_dir(&format!("{bom}{json}")).as_deref(), + Some("lib/deps") + ); + assert_eq!(parse_config_vendor_dir(&format!("{bom}{bom}{json}")), None); } /// Composer's `Platform::expandPath`: only a LEADING `~/`, `$VAR` or diff --git a/crates/socket-patch-core/src/formats/text.rs b/crates/socket-patch-core/src/formats/text.rs index 05f69fba0..b33b9509f 100644 --- a/crates/socket-patch-core/src/formats/text.rs +++ b/crates/socket-patch-core/src/formats/text.rs @@ -105,49 +105,19 @@ mod tests { "\\xef\\xbb\\xbf", "\\xEF\\xBB\\xBF", ]; - fn walk(dir: &std::path::Path, out: &mut Vec) { - for entry in std::fs::read_dir(dir).unwrap() { - let path = entry.unwrap().path(); - if path.is_dir() { - walk(&path, out); - } else if path.extension().is_some_and(|e| e == "rs") { - out.push(path); - } - } - } let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); - let mut files = Vec::new(); - walk(&src, &mut files); let mut inline = Vec::new(); - for path in files { - let rel = path - .strip_prefix(&src) - .unwrap() - .to_string_lossy() - .replace('\\', "/"); + // Test-only files and items are left out by the shared reader. + for file in crate::vendor::test_support::production_sources::production_sources(&src) { + let rel = file.rel.as_str(); if rel == "formats/text.rs" - || rel.ends_with("tests.rs") - || rel.contains("test_support") - || PENDING_INLINE_BOMS.contains(&rel.as_str()) - || OWN_BOM_RULE.contains(&rel.as_str()) + || PENDING_INLINE_BOMS.contains(&rel) + || OWN_BOM_RULE.contains(&rel) { continue; } - // Windows CI checks out with CRLF. - let text = std::fs::read_to_string(&path) - .unwrap() - .replace("\r\n", "\n"); - let production = [ - "#[cfg(test)]\nmod tests", - "#[cfg(test)]\npub(crate) mod tests", - "#[cfg(test)]\nmod architecture_tests", - ] - .iter() - .filter_map(|marker| text.find(marker)) - .min() - .map_or(text.as_str(), |at| &text[..at]); - if INLINE.iter().any(|p| production.contains(p)) { - inline.push(rel); + if INLINE.iter().any(|p| file.text.contains(p)) { + inline.push(file.rel); } } inline.sort(); diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/composer.rs b/crates/socket-patch-core/src/patch/redirect/upstream/composer.rs index c2ed53faf..a0b971d0e 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/composer.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/composer.rs @@ -33,6 +33,7 @@ use crate::formats::composer::hosted::{ find_composer_entry, find_composer_member, json_string_field, ComposerEntry, }; use crate::utils::redact::url_hostname; +use crate::vendor::common::parse_json_text; const COMPOSER_LOCK: &str = "composer.lock"; /// The `notification-url` composer records for packagist packages. @@ -119,7 +120,7 @@ fn declares_custom_repositories(composer_json: Option<&str>) -> Result false, Some(Value::Array(a)) => !a.is_empty(), @@ -137,7 +138,7 @@ fn declares_custom_repositories(composer_json: Option<&str>) -> Result) -> bool { - let Some(doc) = composer_json.and_then(|t| serde_json::from_str::(t).ok()) else { + let Some(doc) = composer_json.and_then(|t| parse_json_text(t).ok()) else { return false; }; let packagist_with_options = |r: &Value| { @@ -495,6 +496,14 @@ mod tests { Ok(true) ); assert!(declares_custom_repositories(Some("{")).is_err()); + // One leading BOM is encoding, as composer.lock's readers treat it. + let bom = crate::formats::text::BOM; + assert_eq!( + declares_custom_repositories(Some(&format!( + "{bom}{{\"repositories\": [{{\"type\": \"vcs\", \"url\": \"https://g.example/x\"}}]}}" + ))), + Ok(true) + ); } #[test] @@ -522,6 +531,12 @@ mod tests { r#"{"repositories": [{"type": "composer", "url": "https://packagist.org.evil.example", "options": {"http": {"header": ["X-Token: t"]}}}]}"# ))); + let bom = crate::formats::text::BOM; + assert!(packagist_declares_options(Some(&format!( + "{bom}{}", + r#"{"repositories": [{"type": "composer", "url": "https://repo.packagist.org", + "options": {"http": {"proxy": "http://proxy:3128"}}}]}"# + )))); } #[test] diff --git a/crates/socket-patch-core/src/vendor/composer_lock.rs b/crates/socket-patch-core/src/vendor/composer_lock.rs index d65968f61..b76d7cf35 100644 --- a/crates/socket-patch-core/src/vendor/composer_lock.rs +++ b/crates/socket-patch-core/src/vendor/composer_lock.rs @@ -44,7 +44,7 @@ use std::sync::Arc; use serde_json::{json, Map, Value}; -use crate::formats::text::split_bom; +use crate::formats::text::{split_bom, strip_bom_bytes}; use crate::manifest::schema::PatchRecord; use crate::patch::apply::PatchSources; use crate::patch::copy_tree::remove_tree; @@ -524,7 +524,17 @@ pub async fn revert_composer_opts( // preserve-state revert never performs. if !keep_artifact { let stranded = - stranded_wired_packages(&lock_path, &entry.uuid, &restorable_keys(entry)).await; + match stranded_wired_packages(&lock_path, &entry.uuid, &restorable_keys(entry)).await { + Ok(stranded) => stranded, + Err(why) => { + return RevertOutcome::failed(format!( + "refusing revert: {why}, so whether it still points packages at \ + {uuid_dir_rel} cannot be told. The vendored artifacts were LEFT IN \ + PLACE. Fix composer.lock (resolve any merge conflict, or restore it \ + from version control), then re-run `socket-patch vendor --revert`" + )); + } + }; if !stranded.is_empty() { let listed = stranded.join(", "); let args = stranded.join(" "); @@ -806,7 +816,9 @@ fn composer_lock_bytes(value: &Value, current: &str) -> std::io::Result> /// `bytes` are exactly `lock`'s canonical render: the memo then answers the /// next package's read with the very document those bytes parse to. Any /// other bytes (a CRLF lock written back CRLF, a spliced entry) leave the -/// memo empty, so the next read parses what is actually on disk. +/// memo empty, so the next read parses what is actually on disk. A leading +/// BOM is not compared (the readers parse past it), so a BOM lock keeps the +/// memo too; the memo stays keyed by the full bytes on disk. fn reseed_lock_memo(bytes: Vec, lock: Value) { reseed_memo(&LOCK_MEMO, bytes, lock); } @@ -814,7 +826,7 @@ fn reseed_lock_memo(bytes: Vec, lock: Value) { /// [`reseed_lock_memo`] over any memo (the tests use their own). fn reseed_memo(memo: &ParseMemo, bytes: Vec, lock: Value) { match composer_json_bytes(&lock) { - Ok(render) if render == bytes => { + Ok(render) if render == strip_bom_bytes(&bytes) => { memo.store(bytes, lock); } _ => memo.invalidate(), @@ -833,11 +845,14 @@ fn lock_bytes_with_entry( idx: usize, ) -> std::io::Result> { let (bom, body) = split_bom(current); - let body = match lock_text::replace_entry(body, section, idx, &lock[section][idx]) { - Some(text) => text.into_bytes(), - None => composer_lock_bytes(lock, body)?, - }; - Ok([bom.as_bytes(), &body].concat()) + let mut bytes = bom.as_bytes().to_vec(); + match lock_text::replace_entry(body, section, idx, &lock[section][idx]) { + Some(text) if bom.is_empty() => return Ok(text.into_bytes()), + Some(text) => bytes.extend_from_slice(text.as_bytes()), + None if bom.is_empty() => return composer_lock_bytes(lock, body), + None => bytes.extend_from_slice(&composer_lock_bytes(lock, body)?), + } + Ok(bytes) } /// The `
:` keys this entry can actually put back: @@ -859,24 +874,26 @@ fn restorable_keys(entry: &VendorEntry) -> HashSet { /// returned lowercase and deduped (composer package names are canonically /// lowercase; the lock's own casing is display-only). /// -/// A missing or unparseable composer.lock yields none: no install can be -/// consuming a lock nothing can read, and the restore loop already degrades -/// to `vendor_lock_entry_drifted` for it. A lock with one leading BOM is -/// parseable (read past it like every other reader of the lock): treating -/// it as unreadable here would skip the refusal and let the revert delete -/// artifacts the lock still names. +/// A missing composer.lock yields none: nothing can be consuming it. One +/// that exists but cannot be read or parsed (a merge conflict, a trailing +/// comma, a second BOM, a FIFO) is `Err`, FAIL-CLOSED: the revert cannot +/// tell what it still installs from, and a record the restore loop then +/// drift-skips must not let the artifact be deleted from under it. One +/// leading BOM is encoding, read past like every other reader of the lock. async fn stranded_wired_packages( lock_path: &Path, uuid: &str, restorable: &HashSet, -) -> Vec { - let Ok(text) = read_regular_to_string(lock_path).await else { - return Vec::new(); - }; - let Ok(lock) = LOCK_MEMO.parse(text.as_bytes(), || parse_json_text(&text)) else { - return Vec::new(); +) -> Result, String> { + let text = match read_regular_to_string(lock_path).await { + Ok(text) => text, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), + Err(e) => return Err(format!("composer.lock is unreadable ({e})")), }; - stranded_in(&lock, uuid, restorable) + let lock = LOCK_MEMO + .parse(text.as_bytes(), || parse_json_text(&text)) + .map_err(|e| format!("composer.lock is not valid JSON ({e})"))?; + Ok(stranded_in(&lock, uuid, restorable)) } /// [`stranded_wired_packages`] over a parsed lock. @@ -1886,9 +1903,16 @@ mod tests { .replace('/', "\\/") .into_bytes(); reseed_memo(&memo, render.clone(), lock.clone()); - reseed_memo(&memo, escaped.clone(), lock); + reseed_memo(&memo, escaped.clone(), lock.clone()); assert!(memo.get(&escaped).is_none()); assert!(memo.get(&render).is_none()); + + // A BOM lock is the render behind its BOM: the memo keeps it, keyed + // by the bytes on disk. + let bom = with_bom(&render); + reseed_memo(&memo, bom.clone(), lock.clone()); + assert_eq!(memo.get(&bom).as_deref(), Some(&lock)); + assert!(memo.get(&with_bom(&bom)).is_none()); } /// Vendor then `--revert` on `lock_bytes`, asserting the revert @@ -3269,12 +3293,14 @@ mod tests { // Missing lock: nothing can be consuming it. assert!(stranded_wired_packages(&lock_path, UUID, &none) .await + .unwrap() .is_empty()); - // Unparseable lock: same degrade (the restore loop reports drift). + // Unparseable lock: fail closed (what it wires cannot be read). tokio::fs::write(&lock_path, b"not json").await.unwrap(); - assert!(stranded_wired_packages(&lock_path, UUID, &none) + let err = stranded_wired_packages(&lock_path, UUID, &none) .await - .is_empty()); + .unwrap_err(); + assert!(err.contains("not valid JSON"), "{err}"); // No package sections at all. tokio::fs::write( &lock_path, @@ -3284,6 +3310,7 @@ mod tests { .unwrap(); assert!(stranded_wired_packages(&lock_path, UUID, &none) .await + .unwrap() .is_empty()); // A wired entry without a name is skipped; the named one (pretty @@ -3299,12 +3326,13 @@ mod tests { .unwrap(); assert_eq!( stranded_wired_packages(&lock_path, UUID, &none).await, - vec!["psr/log".to_string()] + Ok(vec!["psr/log".to_string()]) ); // The same entry is no longer stranded once a record can restore it. let restorable: HashSet = std::iter::once("packages:psr/log".to_string()).collect(); assert!(stranded_wired_packages(&lock_path, UUID, &restorable) .await + .unwrap() .is_empty()); } @@ -4380,7 +4408,7 @@ mod tests { /// The stranded scan reads a BOM lock like any other: an unrestorable /// wired entry is still reported (a second BOM is content, so that lock - /// stays unparseable). + /// is unparseable and the scan fails closed). #[tokio::test] async fn stranded_scan_reads_a_bom_lock() { let dir = tempfile::tempdir().unwrap(); @@ -4395,14 +4423,14 @@ mod tests { tokio::fs::write(&lock_path, &bytes).await.unwrap(); assert_eq!( stranded_wired_packages(&lock_path, UUID, &none).await, - vec!["psr/log".to_string()] + Ok(vec!["psr/log".to_string()]) ); tokio::fs::write(&lock_path, with_bom(&bytes)) .await .unwrap(); assert!(stranded_wired_packages(&lock_path, UUID, &none) .await - .is_empty()); + .is_err()); } /// The stranded refusal guards a BOM lock too. Before, the BOM made the @@ -4445,4 +4473,58 @@ mod tests { "lock untouched" ); } + + /// A wired composer.lock that cannot be parsed for any reason (here a + /// git merge conflict) refuses a delete-the-artifacts revert, FAIL-CLOSED. + /// Before, the stranded scan read it as "nothing wired", the + /// no-original record drift-skipped, and the drift-keep's raw-text probe + /// missed the `\/`-escaped path: the revert deleted the artifact both + /// sides of the conflict still install from. + #[tokio::test] + async fn revert_refuses_over_an_unparseable_lock() { + let lock = lock_value("psr/log", "3.0.2", false); + let (dir, blobs, installed, record) = fixture(&lock).await; + let root = dir.path(); + let text = String::from_utf8(composer_json_bytes(&lock).unwrap()).unwrap(); + tokio::fs::write(root.join(COMPOSER_LOCK), text.replace('/', "\\/")) + .await + .unwrap(); + let (result, entry, _w) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, PURL, false).await); + assert!(result.success, "{:?}", result.error); + let mut entry = entry.unwrap(); + entry.wiring[0].original = None; + let wired = tokio::fs::read_to_string(root.join(COMPOSER_LOCK)) + .await + .unwrap(); + let conflicted = format!( + "<<<<<<< HEAD\n{wired}=======\n{}>>>>>>> theirs\n", + text.replace('/', "\\/") + ); + tokio::fs::write(root.join(COMPOSER_LOCK), &conflicted) + .await + .unwrap(); + + for dry_run in [true, false] { + let outcome = revert_composer(&entry, root, dry_run).await; + assert!( + !outcome.success, + "dry_run={dry_run}: {:?}", + outcome.warnings + ); + let error = outcome.error.unwrap_or_default(); + assert!( + error.contains("refusing revert") && error.contains("not valid JSON"), + "{error}" + ); + } + assert!(root.join(copy_rel()).exists(), "the wired artifact is kept"); + assert_eq!( + tokio::fs::read_to_string(root.join(COMPOSER_LOCK)) + .await + .unwrap(), + conflicted, + "lock untouched" + ); + } } diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs b/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs index 7ddd5edbf..3cf0e88eb 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs @@ -10,7 +10,10 @@ //! the lock with a content verifier (the CLI's lock-refusal gate), and //! names the lock-resolvable packages an offline run could not vendor; //! * [`recover_lock_entry`] answers the same question from a ledger -//! entry's pre-vendor fragments. +//! entry's pre-vendor fragments; +//! * ledger liveness (`vex::discover`) reads EVERY lock's instance +//! ([`inventory_project_every_lock`]) as evidence that a package resolves +//! from somewhere other than its patch. //! //! None of these DOWNLOADS what an entry pins. A Socket-hosted entry is //! listed as the package it replaces, with the hosted artifact's URL and @@ -19,10 +22,7 @@ //! pristine source: vendored bytes come from the patch service by patch //! uuid, and a hosted → vendored takeover re-resolves the upstream entry //! from registry metadata. `tests::lock_pinned_locations_never_drive_a_download` -//! pins this; -//! * ledger liveness (`vex::discover`) reads EVERY lock's instance -//! ([`inventory_project_every_lock`]) as evidence that a package resolves -//! from somewhere other than its patch. +//! (and the CLI's twin) pins this. //! //! # Layout //! diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/tests.rs b/crates/socket-patch-core/src/vendor/lock_inventory/tests.rs index 1c7960531..2ff5d8447 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/tests.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/tests.rs @@ -3768,7 +3768,7 @@ fn unsupported_layout_warnings_rewrites_known_codes_and_forwards_unknown_ones() /// verified against the service's own digest; /// * `recover_lock_entry`'s one production caller is the CLI's /// `pristine_fetch_is_verifiable`, a yes/no gate on which lock-text -/// refusals to report; it fetches nothing; +/// refusals to report; it fetches nothing (the CLI pins that side); /// * the only registry downloads are the hosted → vendored takeover's /// upstream restore (`patch::redirect::upstream::client`), which /// re-resolves the entry from the registry's own metadata and never @@ -3776,73 +3776,126 @@ fn unsupported_layout_warnings_rewrites_known_codes_and_forwards_unknown_ones() /// Central fetches (`maven_repo`, `jvm_jar`), whose URLs are built from /// coordinates and checked against the upstream checksum files. /// -/// A new call site of any of these must keep that property: read the -/// pristine source from the registry, never from a lock entry that might -/// be Socket-hosted. +/// [`PINNED_REFERENCES`] lists every production reference to those three +/// functions in this crate, by file and line text. What the check catches: +/// any new reference by name — a call through any path, a method call, a +/// `use` (an alias must name the original once), a re-export, or the +/// function passed as a value — in any file, allowlisted or not. What it +/// does not: a call reached through a function value or wrapper an +/// existing pinned line already hands out (a new caller of +/// `upstream::client`'s own methods, say), code generated by a macro, and +/// an edit that keeps a pinned line's text while changing what feeds it. +/// Those stay a review matter: read the pristine source from the +/// registry, never from a lock entry that might be Socket-hosted. +const PINNED_REFERENCES: &[(&str, &str, &str)] = &[ + ( + "patch/redirect/upstream/client.rs", + "download", + "crate::vendor::registry_fetch::download(&self.http, url).await", + ), + ( + "patch/redirect/upstream/client.rs", + "download", + "let bytes = self.download(&dist.tarball).await?;", + ), + ( + "patch/redirect/upstream/client.rs", + "download", + "let zip = self.download(&format!(\"{base}.zip\")).await?;", + ), + ( + "patch/redirect/upstream/client.rs", + "download", + "let go_mod = self.download(&format!(\"{base}.mod\")).await?;", + ), + ( + "patch/redirect/upstream/client.rs", + "download", + "let bytes = self.download(&url).await?;", + ), + // `get_text`: registry metadata documents. + ( + "patch/redirect/upstream/client.rs", + "download", + "let bytes = self.download(url).await?;", + ), + // The self-update's own `download` module and timeout field (name + // collisions: it fetches socket-patch's release binary, no lock pin). + ( + "update/download.rs", + "download", + ".timeout(timeouts.download)", + ), + ("update/mod.rs", "download", "pub mod download;"), + ( + "update/mod.rs", + "download", + "download::sweep_stale_stages(dest_dir);", + ), + ( + "update/mod.rs", + "download", + "let staged = download::download_and_stage(", + ), + ("update/release.rs", "download", "pub download: Duration,"), + ( + "update/release.rs", + "download", + "download: Duration::from_secs(300),", + ), + ("update/release.rs", "download", "download: budget,"), + ( + "patch/jvm_jar.rs", + "fetch_registry_bytes", + "let bytes = crate::vendor::maven_repo::fetch_registry_bytes(", + ), + ( + "vendor/maven_repo.rs", + "fetch_registry_bytes", + "match fetch_registry_bytes(&url, super::registry_fetch::MAX_DOWNLOAD_BYTES).await {", + ), + ( + "vendor/maven_repo.rs", + "fetch_registry_bytes", + "fetch_registry_bytes(", + ), + ( + "vendor/maven_repo.rs", + "fetch_registry_bytes", + "fetch_registry_bytes(url, MAX_POM_BYTES as u64).await", + ), + ( + "vendor/lock_inventory/mod.rs", + "recover_lock_entry", + "pub use self::recover::recover_lock_entry;", + ), +]; + #[test] fn lock_pinned_locations_never_drive_a_download() { - fn walk(dir: &Path, out: &mut Vec) { - for entry in std::fs::read_dir(dir).unwrap() { - let path = entry.unwrap().path(); - if path.is_dir() { - walk(&path, out); - } else if path.extension().is_some_and(|e| e == "rs") { - out.push(path); + use crate::vendor::test_support::production_sources::{production_sources, references}; + let src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut found: Vec<(String, &str, String)> = Vec::new(); + for file in production_sources(&src) { + let code = file.code(); + let lines: Vec<&str> = file.text.lines().collect(); + for name in ["download", "fetch_registry_bytes", "recover_lock_entry"] { + for (line, _) in references(&code, name) { + found.push((file.rel.clone(), name, lines[line - 1].trim().to_string())); } } } - let core = Path::new(env!("CARGO_MANIFEST_DIR")); - let crates = core.parent().unwrap(); - let mut files = Vec::new(); - for krate in ["socket-patch-core", "socket-patch-cli"] { - walk(&crates.join(krate).join("src"), &mut files); - } - // Test modules kept in files of their own (this one included). - files.retain(|path| { - let path = path.to_string_lossy(); - !path.ends_with("tests.rs") && !path.contains("test_support") - }); - let allowed: &[(&str, &[&str])] = &[ - ( - "registry_fetch::download(", - &["socket-patch-core/src/patch/redirect/upstream/client.rs"], - ), - ( - "fetch_registry_bytes(", - &[ - "socket-patch-core/src/patch/jvm_jar.rs", - "socket-patch-core/src/vendor/maven_repo.rs", - ], - ), - ( - "recover_lock_entry(", - &[ - "socket-patch-cli/src/commands/vendor.rs", - "socket-patch-core/src/vendor/lock_inventory/recover.rs", - ], - ), - ]; - for (needle, expected) in allowed { - let mut found: Vec = files - .iter() - .filter(|path| { - let text = std::fs::read_to_string(path).unwrap(); - let production = text.split("#[cfg(test)]").next().unwrap_or_default(); - production.contains(needle) - }) - .map(|path| { - path.strip_prefix(crates) - .unwrap() - .to_string_lossy() - .replace('\\', "/") - }) - .collect(); - found.sort(); - assert_eq!( - found, *expected, - "`{needle}` gained or lost a production call site. A download must never take \ - its URL or hash from a lock entry, which may be Socket-hosted (patched bytes); \ - re-check the new site against this test's docs, then update the list" - ); - } + found.sort(); + let mut expected: Vec<(String, &str, String)> = PINNED_REFERENCES + .iter() + .map(|(rel, name, line)| (rel.to_string(), *name, line.to_string())) + .collect(); + expected.sort(); + assert_eq!( + found, expected, + "a production reference to a registry download or to `recover_lock_entry` was added, \ + removed or changed. A download must never take its URL or hash from a lock entry, \ + which may be Socket-hosted (patched bytes); re-check the site against this test's \ + docs, then update PINNED_REFERENCES" + ); } diff --git a/crates/socket-patch-core/src/vendor/test_support.rs b/crates/socket-patch-core/src/vendor/test_support.rs index 11821eee6..044bafea7 100644 --- a/crates/socket-patch-core/src/vendor/test_support.rs +++ b/crates/socket-patch-core/src/vendor/test_support.rs @@ -503,6 +503,7 @@ pub(crate) fn tree_snapshot(root: &Path) -> std::collections::BTreeMap String { + code_only(&self.text) + } +} + +/// Every production `.rs` file under `src`, sorted by path, each read once. +pub fn production_sources(src: &Path) -> Vec { + fn walk(dir: &Path, out: &mut Vec) { + for entry in std::fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + walk(&path, out); + } else if path.extension().is_some_and(|e| e == "rs") { + out.push(path); + } + } + } + let mut files = Vec::new(); + walk(src, &mut files); + let mut out: Vec = files + .into_iter() + .filter_map(|path| { + let rel = path + .strip_prefix(src) + .unwrap() + .to_string_lossy() + .replace('\\', "/"); + if rel.ends_with("tests.rs") || rel.contains("test_support") { + return None; + } + // Windows CI checks out with CRLF. + let text = std::fs::read_to_string(&path) + .unwrap() + .replace("\r\n", "\n"); + Some(ProductionSource { + rel, + text: strip_test_items(&text), + }) + }) + .collect(); + out.sort_by(|a, b| a.rel.cmp(&b.rel)); + out +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum Kind { + Code, + Comment, + Literal, +} + +/// Classify each byte of `src` as code, comment or literal. +fn classify(src: &str) -> Vec { + let b = src.as_bytes(); + let n = b.len(); + let mut kind = vec![Kind::Code; n]; + let ident = |c: u8| c.is_ascii_alphanumeric() || c == b'_'; + let mut i = 0; + let mark = |kind: &mut Vec, from: usize, to: usize, k: Kind| { + for slot in &mut kind[from..to.min(n)] { + *slot = k; + } + }; + // End (exclusive) of a quoted literal whose opening quote is at `open`. + let quoted_end = |open: usize, quote: u8| { + let mut j = open + 1; + while j < n { + match b[j] { + b'\\' => j += 2, + c if c == quote => return j + 1, + _ => j += 1, + } + } + n + }; + while i < n { + let c = b[i]; + let next = b.get(i + 1).copied(); + if c == b'/' && next == Some(b'/') { + let end = src[i..].find('\n').map_or(n, |at| i + at); + mark(&mut kind, i, end, Kind::Comment); + i = end; + } else if c == b'/' && next == Some(b'*') { + let (mut j, mut depth) = (i + 2, 1); + while j < n && depth > 0 { + if b[j] == b'/' && b.get(j + 1) == Some(&b'*') { + depth += 1; + j += 2; + } else if b[j] == b'*' && b.get(j + 1) == Some(&b'/') { + depth -= 1; + j += 2; + } else { + j += 1; + } + } + mark(&mut kind, i, j, Kind::Comment); + i = j; + } else if (c == b'r' || c == b'b') && (i == 0 || !ident(b[i - 1])) { + // r"…", r#"…"#, b"…", br#"…"#, b'…'. + let mut j = i; + if b[j] == b'b' { + j += 1; + } + let raw = b.get(j) == Some(&b'r'); + if raw { + j += 1; + } + let mut hashes = 0; + while raw && b.get(j) == Some(&b'#') { + hashes += 1; + j += 1; + } + if b.get(j) == Some(&b'"') { + let end = if raw { + let closing = format!("\"{}", "#".repeat(hashes)); + src[j + 1..] + .find(&closing) + .map_or(n, |at| j + 1 + at + closing.len()) + } else { + quoted_end(j, b'"') + }; + mark(&mut kind, i, end, Kind::Literal); + i = end; + } else if !raw && c == b'b' && next == Some(b'\'') { + let end = quoted_end(i + 1, b'\''); + mark(&mut kind, i, end, Kind::Literal); + i = end; + } else { + i += 1; + } + } else if c == b'"' { + let end = quoted_end(i, b'"'); + mark(&mut kind, i, end, Kind::Literal); + i = end; + } else if c == b'\'' { + // A char literal, or a lifetime / label (`'a`, `'outer:`). + if next == Some(b'\\') { + let end = quoted_end(i, b'\''); + mark(&mut kind, i, end, Kind::Literal); + i = end; + } else { + let width = src[i + 1..].chars().next().map_or(1, char::len_utf8); + if b.get(i + 1 + width) == Some(&b'\'') { + mark(&mut kind, i, i + 2 + width, Kind::Literal); + i += 2 + width; + } else { + i += 1; + } + } + } else { + i += 1; + } + } + kind +} + +/// Index of the bracket closing the one at `open`, over code bytes only. +fn matching_close(b: &[u8], kind: &[Kind], open: usize) -> usize { + let mut depth = 0usize; + for (j, &c) in b.iter().enumerate().skip(open) { + if kind[j] != Kind::Code { + continue; + } + match c { + b'(' | b'[' | b'{' => depth += 1, + b')' | b']' | b'}' => { + depth -= 1; + if depth == 0 { + return j; + } + } + _ => {} + } + } + b.len().saturating_sub(1) +} + +/// End (exclusive) of the item starting at `start`: its first `;` outside +/// brackets, or its first top-level `{…}` block (plus a `;` right after). +fn item_end(b: &[u8], kind: &[Kind], start: usize) -> usize { + let mut depth = 0usize; + let mut j = start; + while j < b.len() { + if kind[j] == Kind::Code { + match b[j] { + b'(' | b'[' => depth += 1, + b')' | b']' => depth = depth.saturating_sub(1), + b';' if depth == 0 => return j + 1, + b'{' if depth == 0 => { + let close = matching_close(b, kind, j); + let mut k = close + 1; + while k < b.len() && b[k].is_ascii_whitespace() { + k += 1; + } + return if b.get(k) == Some(&b';') { + k + 1 + } else { + close + 1 + }; + } + _ => {} + } + } + j += 1; + } + b.len() +} + +/// Whether an attribute's inside (`cfg(test)` of `#[cfg(test)]`) gates its +/// item to test builds. +fn is_test_cfg(inside: &str) -> bool { + let compact: String = inside.chars().filter(|c| !c.is_whitespace()).collect(); + compact == "cfg(test)" || compact.starts_with("cfg(all(test,") +} + +/// `src` with every test-only item blanked (see the module docs). +pub fn strip_test_items(src: &str) -> String { + let b = src.as_bytes(); + let kind = classify(src); + let mut out = b.to_vec(); + let blank = |out: &mut [u8], from: usize, to: usize| { + for byte in &mut out[from..to] { + if *byte != b'\n' { + *byte = b' '; + } + } + }; + let mut i = 0; + while i + 1 < b.len() { + if kind[i] != Kind::Code || b[i] != b'#' { + i += 1; + continue; + } + let inner = b[i + 1] == b'!'; + let open = if inner { i + 2 } else { i + 1 }; + if b.get(open) != Some(&b'[') { + i += 1; + continue; + } + let close = matching_close(b, &kind, open); + if !is_test_cfg(&src[open + 1..close]) { + i = close + 1; + continue; + } + if inner { + // `#![cfg(test)]`: the whole file (module) is test-only. + return src + .bytes() + .map(|c| if c == b'\n' { '\n' } else { ' ' }) + .collect(); + } + // The item: past whitespace, comments and further attributes. + let mut j = close + 1; + loop { + while j < b.len() && (b[j].is_ascii_whitespace() || kind[j] == Kind::Comment) { + j += 1; + } + if b.get(j) == Some(&b'#') && b.get(j + 1) == Some(&b'[') { + j = matching_close(b, &kind, j + 1) + 1; + continue; + } + break; + } + let end = item_end(b, &kind, j); + blank(&mut out, i, end); + i = end; + } + // Only ASCII bytes were written, each over a whole removed range whose + // ends are ASCII, so the text stays UTF-8. + String::from_utf8(out).expect("blanking keeps UTF-8") +} + +/// `src` with comments and literal contents blanked (newlines kept). +pub fn code_only(src: &str) -> String { + let kind = classify(src); + let out: Vec = src + .bytes() + .zip(&kind) + .map(|(c, k)| match k { + Kind::Code => c, + _ if c == b'\n' => b'\n', + _ => b' ', + }) + .collect(); + // Non-code bytes became ASCII; code bytes are whole chars. + String::from_utf8(out).unwrap_or_default() +} + +/// Every reference to the identifier `name` in `code` (from +/// [`ProductionSource::code`]) other than its own definition (`fn name`): +/// a call by any path, a method call, an import (`use …::name` and +/// `… as alias`), a function value passed along. Each is `(line, the +/// trimmed line)`, 1-based. +pub fn references(code: &str, name: &str) -> Vec<(usize, String)> { + let ident = |c: char| c.is_alphanumeric() || c == '_'; + let mut out = Vec::new(); + for (index, line) in code.lines().enumerate() { + let mut from = 0; + while let Some(at) = line[from..].find(name) { + let start = from + at; + let end = start + name.len(); + from = end; + let before = line[..start].chars().next_back(); + let after = line[end..].chars().next(); + if before.is_some_and(ident) || after.is_some_and(ident) { + continue; + } + let prefix = line[..start].trim_end(); + if prefix.ends_with("fn") && !prefix[..prefix.len() - 2].ends_with(ident) { + continue; + } + out.push((index + 1, line.trim().to_string())); + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_items_are_removed_in_place_not_truncated() { + let src = "fn a() {}\n#[cfg(test)]\nfn helper() { let s = \"}\"; }\nfn b() { x(); }\n\ + #[cfg(test)]\nmod tests;\n#[cfg(all(test, unix))]\nuse std::fs::{self, File};\n\ + #[cfg(test)]\n#[path = \"t.rs\"]\nmod more { fn y() { '}'; } }\nfn c() {}\n"; + let out = strip_test_items(src); + assert_eq!(out.lines().count(), src.lines().count(), "{out}"); + for kept in ["fn a()", "fn b() { x(); }", "fn c()"] { + assert!(out.contains(kept), "{kept}: {out}"); + } + for dropped in ["helper", "mod tests", "File", "more", "fn y"] { + assert!(!out.contains(dropped), "{dropped}: {out}"); + } + assert!(strip_test_items("#![cfg(test)]\nfn z() {}\n") + .trim() + .is_empty()); + // `any(test, …)` also builds outside tests. + assert!( + strip_test_items("#[cfg(any(test, feature = \"x\"))]\nfn k() {}\n").contains("fn k") + ); + } + + #[test] + fn literals_and_comments_do_not_confuse_the_scan() { + let src = "#[cfg(test)]\nfn t() { let r = r#\"{ \"#; let c = '{'; /* { */ // {\n }\nfn p<'a>(x: &'a str) {}\n"; + let out = strip_test_items(src); + assert!(out.contains("fn p<'a>"), "{out}"); + assert!(!out.contains("fn t"), "{out}"); + let code = code_only("call(\"download(\"); // download(\ndownload(1);\n"); + assert_eq!( + references(&code, "download"), + vec![(2, "download(1);".to_string())] + ); + } + + #[test] + fn references_find_every_spelling_but_the_definition() { + let code = "pub(crate) async fn download(u: &str) {}\n\ + use a::registry_fetch::download as fetch;\n\ + self.download(&url);\n\ + x.map(download);\n\ + redownload(); download_unredacted();\n"; + let lines: Vec = references(code, "download").iter().map(|r| r.0).collect(); + assert_eq!(lines, vec![2, 3, 4]); + } +} diff --git a/crates/socket-patch-core/src/vex/discover/composer.rs b/crates/socket-patch-core/src/vex/discover/composer.rs index 55e16c9cc..f6edcbc2d 100644 --- a/crates/socket-patch-core/src/vex/discover/composer.rs +++ b/crates/socket-patch-core/src/vex/discover/composer.rs @@ -100,7 +100,7 @@ impl InstallPreference { let preferred_install = ctx .read_text(COMPOSER_JSON, out) .await - .and_then(|text| serde_json::from_str::(&text).ok()) + .and_then(|text| crate::vendor::common::parse_json_text(&text).ok()) .and_then(|manifest| manifest.get("config")?.get("preferred-install").cloned()); Self { preferred_install, @@ -1076,6 +1076,25 @@ mod tests { } } + /// A composer.json saved with a UTF-8 BOM still decides the install + /// preference: its `preferred-install: source` vetoes the leftover + /// `source` (before, the BOM read as "no composer.json"). + #[tokio::test] + async fn a_bom_composer_json_still_decides_the_preference() { + let entry = with_source(vendored_entry("psr/log", "3.0.2", UUID_B)); + let p = Project::new(); + p.write("composer.lock", lock(json!([entry]), json!([]))); + p.write( + "composer.json", + format!( + "{}{}", + crate::formats::text::BOM, + composer_json(json!("source")) + ), + ); + assert_source_veto(&run(&p).await); + } + /// Composer's per-package map: the first matching non-`*` pattern /// decides, case-insensitively; the `*` entry (or composer's default) /// covers the rest. From 22544e7be82aaa3bf818f3622ee5ab763c6cc34d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 17:15:10 +0000 Subject: [PATCH 3/3] Bound test-only list items in the source scan; gate composer refusal Second review round. - The production-source scan's `item_end` only stopped at a depth-0 `;` or the end of the first depth-0 block, so a `#[cfg(test)]` struct field, enum variant, match arm or parameter blanked code far past itself (a whole following `impl`), hiding production references from the no-lock-download and BOM guards. An item now ends at its `,` (with generic `<...>` counted so `Result<(), ()>` does not end it) or at the bracket enclosing it. - `code_only` failed open (an empty file) on a UTF-8 slip; it panics now, like `strip_test_items`. - The module docs no longer claim the file an out-of-line test module points at is dropped. - `vendor --revert` of a composer entry refused over any unparseable composer.lock. It still refuses when the lock names the entry's uuid, but a broken lock that never mentions it cannot route composer at the uuid dir and no longer blocks the revert. - The BOM memo test re-seeds with two BOMs before asserting nothing was stored; the CLI guard sorts its expected list. Co-Authored-By: Claude Opus 5.5 --- .../socket-patch-cli/src/commands/vendor.rs | 3 +- .../src/vendor/composer_lock.rs | 55 +++++++++++++++-- .../vendor/test_support/production_sources.rs | 60 ++++++++++++++++--- 3 files changed, 103 insertions(+), 15 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/vendor.rs b/crates/socket-patch-cli/src/commands/vendor.rs index 1a4aac73f..8e55f77d7 100644 --- a/crates/socket-patch-cli/src/commands/vendor.rs +++ b/crates/socket-patch-cli/src/commands/vendor.rs @@ -8197,10 +8197,11 @@ mod lock_download_guard_tests { } } found.sort(); - let expected: Vec<(String, &str, String)> = PINNED_REFERENCES + let mut expected: Vec<(String, &str, String)> = PINNED_REFERENCES .iter() .map(|(rel, name, line)| (rel.to_string(), *name, line.to_string())) .collect(); + expected.sort(); assert_eq!( found, expected, "a production reference to `recover_lock_entry` was added, removed or changed. \ diff --git a/crates/socket-patch-core/src/vendor/composer_lock.rs b/crates/socket-patch-core/src/vendor/composer_lock.rs index b76d7cf35..1b2155a2f 100644 --- a/crates/socket-patch-core/src/vendor/composer_lock.rs +++ b/crates/socket-patch-core/src/vendor/composer_lock.rs @@ -890,9 +890,20 @@ async fn stranded_wired_packages( Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), Err(e) => return Err(format!("composer.lock is unreadable ({e})")), }; - let lock = LOCK_MEMO - .parse(text.as_bytes(), || parse_json_text(&text)) - .map_err(|e| format!("composer.lock is not valid JSON ({e})"))?; + let lock = match LOCK_MEMO.parse(text.as_bytes(), || parse_json_text(&text)) { + Ok(lock) => lock, + // A lock that never names the uuid (in any spelling: the uuid has + // no `/` to escape) cannot point composer at its directory, so a + // broken lock elsewhere does not block this revert. + Err(_) + if !text + .to_ascii_lowercase() + .contains(&uuid.to_ascii_lowercase()) => + { + return Ok(Vec::new()) + } + Err(e) => return Err(format!("composer.lock is not valid JSON ({e})")), + }; Ok(stranded_in(&lock, uuid, restorable)) } @@ -1912,7 +1923,12 @@ mod tests { let bom = with_bom(&render); reseed_memo(&memo, bom.clone(), lock.clone()); assert_eq!(memo.get(&bom).as_deref(), Some(&lock)); - assert!(memo.get(&with_bom(&bom)).is_none()); + // Only one BOM is stripped: two are not the render, so re-seeding + // with them stores nothing (and drops the stale slot). + let double = with_bom(&bom); + reseed_memo(&memo, double.clone(), lock.clone()); + assert!(memo.get(&double).is_none()); + assert!(memo.get(&bom).is_none()); } /// Vendor then `--revert` on `lock_bytes`, asserting the revert @@ -3295,12 +3311,20 @@ mod tests { .await .unwrap() .is_empty()); - // Unparseable lock: fail closed (what it wires cannot be read). - tokio::fs::write(&lock_path, b"not json").await.unwrap(); + // Unparseable lock naming the uuid: fail closed (what it wires + // cannot be read). One that never names it wires nothing of it. + tokio::fs::write(&lock_path, format!("not json {UUID}")) + .await + .unwrap(); let err = stranded_wired_packages(&lock_path, UUID, &none) .await .unwrap_err(); assert!(err.contains("not valid JSON"), "{err}"); + tokio::fs::write(&lock_path, b"not json").await.unwrap(); + assert!(stranded_wired_packages(&lock_path, UUID, &none) + .await + .unwrap() + .is_empty()); // No package sections at all. tokio::fs::write( &lock_path, @@ -4526,5 +4550,24 @@ mod tests { conflicted, "lock untouched" ); + + // A broken lock that never names the uuid cannot point composer at + // its directory: it does not block the revert. + let unrelated = format!( + "<<<<<<< HEAD\n{text}=======\n{}>>>>>>> theirs\n", + text.replace('/', "\\/") + ); + tokio::fs::write(root.join(COMPOSER_LOCK), &unrelated) + .await + .unwrap(); + let outcome = revert_composer(&entry, root, true).await; + assert!( + !outcome + .error + .as_deref() + .is_some_and(|e| e.contains("not valid JSON")), + "{:?}", + outcome.error + ); } } diff --git a/crates/socket-patch-core/src/vendor/test_support/production_sources.rs b/crates/socket-patch-core/src/vendor/test_support/production_sources.rs index 78c0e37b7..a49e469fc 100644 --- a/crates/socket-patch-core/src/vendor/test_support/production_sources.rs +++ b/crates/socket-patch-core/src/vendor/test_support/production_sources.rs @@ -4,9 +4,13 @@ //! //! "Production" is every `.rs` file under `src/` except the test-only //! files (`*tests.rs`, `test_support`), with every test-only item removed: -//! an item behind `#[cfg(test)]` or `#[cfg(all(test, …))]` (a test module, -//! inline or `mod x;`, a test helper, a test `use`), and the whole file -//! when it opens with `#![cfg(test)]`. Items are removed in place, never by +//! an item behind `#[cfg(test)]` or `#[cfg(all(test, …))]` (an inline test +//! module or a `mod x;` declaration, a test helper, a test `use`, a field, +//! variant, arm or parameter), and the whole file when it opens with +//! `#![cfg(test)]`. The FILE an out-of-line test module (`#[cfg(test)] mod +//! x;`) points at is still read unless its name ends in `tests.rs`, so a +//! test-only file (the crawlers' `oracle.rs`) must not hold what a guard +//! forbids, or must open with `#![cfg(test)]`. Items are removed in place, never by //! truncating at the first `#[cfg(test)]`, so production code after a test //! item is still scanned. Line endings are normalized to LF, and removed //! bytes become spaces (newlines kept), so line numbers stay the file's. @@ -208,14 +212,32 @@ fn matching_close(b: &[u8], kind: &[Kind], open: usize) -> usize { /// End (exclusive) of the item starting at `start`: its first `;` outside /// brackets, or its first top-level `{…}` block (plus a `;` right after). +/// An item never reaches past the bracket that encloses it, and one in a +/// comma list (a struct field, an enum variant, a match arm, a parameter) +/// ends at its `,`. Generic `<…>` count as brackets for that comma (a +/// `<` or `>` hugged by code on both sides, `->` and `=>` excepted), so +/// `Result<(), ()>` or `HashMap` do not end it. fn item_end(b: &[u8], kind: &[Kind], start: usize) -> usize { let mut depth = 0usize; + let mut angle = 0usize; let mut j = start; + let code = |i: usize| kind.get(i) == Some(&Kind::Code); while j < b.len() { - if kind[j] == Kind::Code { + if code(j) { match b[j] { b'(' | b'[' => depth += 1, - b')' | b']' => depth = depth.saturating_sub(1), + b')' | b']' | b'}' if depth == 0 => return j, + b')' | b']' => depth -= 1, + b'<' if j > 0 + && code(j - 1) + && !b[j - 1].is_ascii_whitespace() + && b.get(j + 1) + .is_some_and(|c| !c.is_ascii_whitespace() && *c != b'=') => + { + angle += 1 + } + b'>' if angle > 0 && j > 0 && !matches!(b[j - 1], b'-' | b'=') => angle -= 1, + b',' if depth == 0 && angle == 0 => return j + 1, b';' if depth == 0 => return j + 1, b'{' if depth == 0 => { let close = matching_close(b, kind, j); @@ -223,7 +245,7 @@ fn item_end(b: &[u8], kind: &[Kind], start: usize) -> usize { while k < b.len() && b[k].is_ascii_whitespace() { k += 1; } - return if b.get(k) == Some(&b';') { + return if b.get(k) == Some(&b';') || b.get(k) == Some(&b',') { k + 1 } else { close + 1 @@ -313,8 +335,9 @@ pub fn code_only(src: &str) -> String { _ => b' ', }) .collect(); - // Non-code bytes became ASCII; code bytes are whole chars. - String::from_utf8(out).unwrap_or_default() + // Non-code bytes became ASCII; code bytes are whole chars. A slip must + // fail the guard loudly, never hand it an empty (reference-free) file. + String::from_utf8(out).expect("blanking keeps UTF-8") } /// Every reference to the identifier `name` in `code` (from @@ -372,6 +395,27 @@ mod tests { ); } + /// A test-only field, variant, arm or parameter ends at its `,` or at + /// the bracket that encloses it, never at the next item's block. + #[test] + fn list_items_end_at_their_comma_or_closer() { + let src = "struct S {\n #[cfg(test)]\n probe: u32,\n}\nimpl S { fn get(&self) { kept_a(); } }\n\ + struct T {\n #[cfg(test)]\n m: HashMap,\n keep: u8,\n}\n\ + enum E {\n #[cfg(test)]\n Probe(u8),\n Real,\n}\n\ + fn f(#[cfg(test)] x: u8, y: u8) { kept_b(); }\n\ + #[cfg(test)]\nfn r() -> Result<(), ()> { gone(); }\nfn after() { kept_c(); }\n\ + fn m(v: u8) { match v {\n #[cfg(test)]\n 0 => gone2(),\n _ => kept_d(),\n } }\n"; + let out = strip_test_items(src); + for kept in [ + "kept_a", "keep: u8", "Real", "y: u8", "kept_b", "kept_c", "kept_d", + ] { + assert!(out.contains(kept), "{kept}: {out}"); + } + for dropped in ["probe", "HashMap", "Probe", "x: u8", "gone()", "gone2"] { + assert!(!out.contains(dropped), "{dropped}: {out}"); + } + } + #[test] fn literals_and_comments_do_not_confuse_the_scan() { let src = "#[cfg(test)]\nfn t() { let r = r#\"{ \"#; let c = '{'; /* { */ // {\n }\nfn p<'a>(x: &'a str) {}\n";