From 94e51ec00d07b5a39d3d2f24c26f671094d59451 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 14 Aug 2026 09:27:49 -0700 Subject: [PATCH 1/6] fix(redirect): composer redirects confirm, pin, and stay in-bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The composer.lock rewriter wrote `\/`-escaped URLs while the post-rewrite confirmation probe searched only raw and percent-encoded spellings, so every successful composer redirect reported `redirected: 0`, persisted no patch records into the ledger, and left `vex` with nothing to attest. The rewriter now emits composer-native raw slashes (PHP `JSON_UNESCAPED_SLASHES`) and shares one `artifact_url_present` predicate with the probe — both spellings accepted — so writer and prober can never drift again. Three more rewriter defects fixed: - Entry lookup is bounded by a string-literal-aware brace matcher, so a source-only (dist-less) target fails closed with `redirect_composer_no_dist` instead of silently rewriting the NEXT package's dist url+shasum — a checksum-clean wrong-code install. - Names match case-insensitively and the locked version must match the patched one through composer's leading-`v` normalization; a drifted lock fails closed with `redirect_composer_version_mismatch`. - A dist block without a `shasum` key now gets the sha1 pin inserted (composer's key order) instead of redirecting unverified. New codes: `redirect_composer_no_dist_url`, `redirect_composer_lock_malformed`. The basic golden is converted to the raw-slash format real composer writes, with five new cases (escaped-slash lock, no-shasum-key, source-only bystander, version-mismatch, mixed-case + v-prefix). A new in-process e2e proves the full loop: rewritten lock, `redirected: 1`, patch record and revert edit in the ledger. Co-authored-by: Claude Fable 5 --- .../src/commands/scan/hosted.rs | 9 +- .../tests/in_process_redirect.rs | 189 +++++++++ .../src/patch/redirect/mod.rs | 398 +++++++++++++++++- .../composer-lock/basic/expected-edits.json | 4 +- .../basic/expected/composer.lock | 12 +- .../composer-lock/basic/input/composer.lock | 12 +- .../escaped-slash-lock/expected-edits.json | 10 + .../escaped-slash-lock/expected/composer.lock | 29 ++ .../escaped-slash-lock/input/composer.lock | 29 ++ .../escaped-slash-lock/overrides.json | 14 + .../mixed-case-v-prefix/expected-edits.json | 10 + .../expected/composer.lock | 19 + .../mixed-case-v-prefix/input/composer.lock | 19 + .../mixed-case-v-prefix/overrides.json | 14 + .../no-shasum-key/expected-edits.json | 10 + .../no-shasum-key/expected/composer.lock | 29 ++ .../no-shasum-key/input/composer.lock | 28 ++ .../no-shasum-key/overrides.json | 14 + .../source-only-bystander/expected-edits.json | 1 + .../source-only-bystander/input/composer.lock | 28 ++ .../source-only-bystander/overrides.json | 14 + .../version-mismatch/expected-edits.json | 1 + .../version-mismatch/input/composer.lock | 19 + .../version-mismatch/overrides.json | 14 + 24 files changed, 904 insertions(+), 22 deletions(-) create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/expected-edits.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/expected/composer.lock create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/input/composer.lock create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/overrides.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/expected-edits.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/expected/composer.lock create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/input/composer.lock create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/overrides.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/expected-edits.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/expected/composer.lock create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/input/composer.lock create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/overrides.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/expected-edits.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/input/composer.lock create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/overrides.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/expected-edits.json create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/input/composer.lock create mode 100644 crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/overrides.json diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index f2ec0b3a..8ff119db 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -416,7 +416,14 @@ pub(super) async fn run_redirect( .filter(|(_, _, artifact_url, index_url, suffixed_version)| { let encoded = socket_patch_core::utils::uri::encode_uri_component(artifact_url); final_texts.iter().any(|text| { - text.contains(artifact_url.as_str()) + // The rewriters' own predicate — raw, or the `\/`-escaped + // slashes an old composer.lock spells them with — so a + // writer's spelling can never be one this probe misses. It + // was: the composer rewriter emitted `\/`-escaped urls this + // probe never looked for, so a fully successful composer + // redirect reported `redirected: 0`, fetched no patch record + // into the ledger, and left the patch unattestable by `vex`. + socket_patch_core::patch::redirect::artifact_url_present(text, artifact_url) // The berry rewriter writes the URL percent-encoded into the // lock's `::__archiveUrl=` binding, so the raw form is absent. || text.contains(encoded.as_str()) diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index d302aa48..e0a89da3 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -1970,3 +1970,192 @@ async fn redirect_json_mode_write_failures_emit_error_envelope() { let out = run_leg(tmp.path(), &server).await; assert_error_envelope(&out, "ledger-write failure"); } + +// ── composer ───────────────────────────────────────────────────────────── + +const COMPOSER_PURL: &str = "pkg:composer/monolog/monolog@2.0.0"; +const COMPOSER_UUID: &str = "66666666-6666-4666-8666-666666666666"; +const COMPOSER_URL: &str = "http://patch.test/patch/composer/monolog/monolog/2.0.0/\ + 77777777-7777-4777-8777-777777777777/\ + 66666666-6666-4666-8666-666666666666/monolog-2.0.0.zip"; +const COMPOSER_SHA1: &str = "abcdef0123456789abcdef0123456789abcdef01"; + +/// A composer project discovered through `vendor/composer/installed.json`, +/// with the lock the redirect rewriter edits. The lock is composer-native: +/// `JSON_UNESCAPED_SLASHES`, 4-space indent. +fn write_composer_project(root: &Path) { + std::fs::write( + root.join("composer.json"), + "{ \"require\": { \"monolog/monolog\": \"2.0.0\" } }\n", + ) + .unwrap(); + let installed = root.join("vendor").join("composer"); + std::fs::create_dir_all(&installed).unwrap(); + std::fs::write( + installed.join("installed.json"), + r#"{ "packages": [ { "name": "monolog/monolog", "version": "2.0.0" } ] } +"#, + ) + .unwrap(); + std::fs::create_dir_all(root.join("vendor/monolog/monolog")).unwrap(); + std::fs::write( + root.join("composer.lock"), + r#"{ + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "monolog/monolog", + "version": "2.0.0", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/Seldaek/monolog/zipball/abc123", + "reference": "abc123def456", + "shasum": "" + } + } + ], + "packages-dev": [] +} +"#, + ) + .unwrap(); +} + +async fn mock_composer_api(server: &MockServer) { + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/batch"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "packages": [{ + "purl": COMPOSER_PURL, + "patches": [{ + "uuid": COMPOSER_UUID, "purl": COMPOSER_PURL, "tier": "free", + "cveIds": [], "ghsaIds": [], "severity": "high", + "title": "composer redirect fixture" + }] + }], + "canAccessPaidPatches": false, + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path_regex(format!( + "^/v0/orgs/{ORG}/patches/by-package/.+$" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "patches": [{ + "uuid": COMPOSER_UUID, "purl": COMPOSER_PURL, + "publishedAt": "2024-01-01T00:00:00Z", + "description": "x", "license": "MIT", "tier": "free", + "vulnerabilities": {} + }], + "canAccessPaidPatches": false, + }))) + .mount(server) + .await; + // composer pins `dist.shasum` — a sha1, not npm's sha512. + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/package"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "results": { + COMPOSER_UUID: { + "status": "granted", + "url": COMPOSER_URL, + "purl": COMPOSER_PURL, + "artifacts": [{ + "kind": "tarball", + "url": COMPOSER_URL, + "integrity": { "sha1": COMPOSER_SHA1 } + }], + "registryOverride": null + } + } + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path(format!("/v0/orgs/{ORG}/patches/view/{COMPOSER_UUID}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "uuid": COMPOSER_UUID, + "purl": COMPOSER_PURL, + "publishedAt": "2024-01-01T00:00:00Z", + "files": { + "src/Logger.php": { + "beforeHash": "a".repeat(64), + "afterHash": "b".repeat(64), + } + }, + "vulnerabilities": { + GHSA: { + "cves": ["CVE-2024-9"], + "summary": "composer redirect fixture", + "severity": "high", + "description": "d" + } + }, + "description": "x", "license": "MIT", "tier": "free" + }))) + .mount(server) + .await; +} + +/// End-to-end regression for the composer redirect that reported itself as +/// having done nothing: the rewriter wrote the hosted url with `\/`-escaped +/// slashes while the post-rewrite confirmation probe searched only the raw and +/// percent-encoded spellings, so a fully successful rewrite yielded +/// `redirected: 0`, no patch record in the ledger, and nothing for `vex` to +/// attest. The rewriter now emits composer-native raw slashes and the probe +/// asks the rewriter's own predicate, so the lock edit and the confirmation +/// cannot disagree. Subprocess so the `--json` envelope can be read back. +#[tokio::test] +#[serial] +async fn composer_redirect_is_confirmed_and_recorded() { + let server = MockServer::start().await; + mock_composer_api(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_composer_project(tmp.path()); + + let env = run_redirect_subprocess(tmp.path(), &server.uri()); + assert_eq!(env["status"], "success", "envelope: {env}"); + assert_eq!( + env["redirect"]["redirected"], 1, + "the composer redirect must be CONFIRMED, not silently unconfirmed: {env}" + ); + assert_eq!( + env["redirect"]["rewrittenFiles"][0], "composer.lock", + "composer.lock must be the rewritten file: {env}" + ); + assert!( + warning_codes(&env).is_empty(), + "a clean composer redirect emits no warnings; got {:?}", + warning_codes(&env) + ); + + let lock = std::fs::read_to_string(tmp.path().join("composer.lock")).unwrap(); + assert!( + lock.contains(&format!("\"url\": \"{COMPOSER_URL}\"")), + "dist.url must be the hosted patch with composer-native raw slashes; got:\n{lock}" + ); + assert!( + !lock.contains("\\/"), + "composer writes lock JSON with JSON_UNESCAPED_SLASHES — no escaped slashes may \ + be introduced; got:\n{lock}" + ); + assert!( + lock.contains(&format!("\"shasum\": \"{COMPOSER_SHA1}\"")), + "dist.shasum must pin the patched artifact's sha1; got:\n{lock}" + ); + + // The confirmation is what drives the record fetch: no confirmation, no + // record, and `socket-patch vex` can never attest the patch. + let ledger = + std::fs::read_to_string(tmp.path().join(".socket/vendor/redirect-state.json")).unwrap(); + assert!( + ledger.contains(COMPOSER_PURL) && ledger.contains(GHSA), + "the ledger must carry the fetched patch record for the redirected purl: {ledger}" + ); + assert!( + ledger.contains("redirect_composer_dist"), + "the ledger must carry the revert edit for the lock rewrite: {ledger}" + ); +} diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 0ff40eb5..b8202473 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -20,6 +20,7 @@ use regex::Regex; use serde::{Deserialize, Serialize}; use serde_json::{json, Value}; +use crate::crawlers::composer_crawler::normalize_version; use crate::crawlers::python_crawler::canonicalize_pypi_name; use crate::vendor::yarn_berry_lock::yarnrc_compression_level; @@ -1309,6 +1310,125 @@ fn rewrite_uv_lock( } // ── composer.lock ──────────────────────────────────────────────────────────── +/// Whether `text` points at `artifact_url` in any spelling a rewritten file may +/// carry: the raw url every rewriter emits — composer.lock included, since +/// composer writes its lock through PHP's `JSON_UNESCAPED_SLASHES` — or the +/// `\/`-escaped slashes an older composer wrote, which redirect the install just +/// as well. Shared by the composer rewriter's already-redirected check and the +/// CLI's post-rewrite confirmation probe so the writer's spelling and the +/// probe's cannot drift: the probe searched only raw and percent-encoded urls +/// while the composer rewriter emitted `\/`, so a fully successful composer +/// redirect reported nothing redirected — no patch record reached the ledger and +/// `vex` had nothing to attest. +pub fn artifact_url_present(text: &str, artifact_url: &str) -> bool { + text.contains(artifact_url) || text.contains(&artifact_url.replace('/', "\\/")) +} + +/// Byte offset of the `}` closing the JSON object that CONTAINS `from`, which +/// must be a position inside that object. Brace counting skips string literals, +/// so a brace inside a description or URL cannot move the boundary. +fn json_object_end_from(text: &str, from: usize) -> Option { + let mut depth = 0usize; + let mut in_string = false; + let mut escaped = false; + for (offset, ch) in text[from..].char_indices() { + if in_string { + match ch { + _ if escaped => escaped = false, + '\\' => escaped = true, + '"' => in_string = false, + _ => {} + } + continue; + } + match ch { + '"' => in_string = true, + '{' => depth += 1, + '}' if depth == 0 => return Some(from + offset), + '}' => depth -= 1, + _ => {} + } + } + None +} + +/// Value of the first `"": ""` pair in `text` (composer writes its +/// lock with exactly one space after the colon, the same shape the surgical +/// `dist` regexes below assume). +fn json_string_field<'a>(text: &'a str, key: &str) -> Option<&'a str> { + let pattern = format!("\"{key}\": \""); + let start = text.find(&pattern)? + pattern.len(); + let end = text[start..].find('"')? + start; + Some(&text[start..end]) +} + +/// Outcome of locating a package entry in a composer.lock. +enum ComposerEntry { + /// Inclusive byte range from the entry's `"name"` key to the `}` closing + /// the entry — composer writes `name` first, so this covers every key the + /// rewriter edits. + Found(usize, usize), + /// The name matched but the lock pins this OTHER version. + VersionMismatch(String), + NotFound, +} + +/// Locate `pkg`'s entry in a composer.lock (either `packages[]` or +/// `packages-dev[]` — the scan is over the whole file). +/// +/// Names match CASE-INSENSITIVELY, the way the composer crawler and the vendor +/// backend already match them: packagist canonicalizes to lowercase, but +/// hand-written mixed-case locks install fine and would otherwise silently miss +/// the redirect. The locked version must match the patched one through +/// composer's leading-`v` normalization (locks carry the pretty `v6.4.1`, PURLs +/// the bare `6.4.1`); matching on name alone repointed whatever version the +/// lock happened to hold at a patch built for a different one. +fn find_composer_entry(content: &str, pkg: &str, version: &str) -> ComposerEntry { + let mut mismatched: Option = None; + for (name_idx, _) in content.match_indices("\"name\": \"") { + let Some(end) = json_object_end_from(content, name_idx) else { + continue; + }; + let entry = &content[name_idx..=end]; + if !json_string_field(entry, "name").is_some_and(|n| n.eq_ignore_ascii_case(pkg)) { + continue; + } + // Every package entry carries `version`; an `authors[]`/`support` + // object that happens to have a matching `name` does not. + let Some(locked) = json_string_field(entry, "version") else { + continue; + }; + if normalize_version(locked) == normalize_version(version) { + return ComposerEntry::Found(name_idx, end); + } + mismatched = Some(locked.to_string()); + } + match mismatched { + Some(locked) => ComposerEntry::VersionMismatch(locked), + None => ComposerEntry::NotFound, + } +} + +/// Append `"shasum": ""` as the last key of a `"dist": { … }` block, +/// indented like the keys already in it. VCS/zipball dists omit `shasum` +/// entirely; redirecting such a block without inserting the pin left the hosted +/// artifact unverified, so composer would install whatever the URL returned. +/// `block` is the whole dist object and already holds at least a `url`. +fn append_composer_shasum(block: &str, sha1: &str) -> String { + let Some(close) = block.rfind('}') else { + return block.to_string(); + }; + let head = block[..close].trim_end(); + let indent: String = head[head.rfind('\n').map_or(0, |i| i + 1)..] + .chars() + .take_while(|c| c.is_whitespace()) + .collect(); + format!( + "{head},\n{indent}\"shasum\": \"{sha1}\"{}", + &block[head.len()..] + ) +} + fn rewrite_composer_lock( files: &BTreeMap, overrides: &[DepOverride], @@ -1321,6 +1441,7 @@ fn rewrite_composer_lock( if composer.is_empty() || !files.contains_key("composer.lock") { return; } + const DIST_KEY: &str = "\"dist\": {"; let mut content = files["composer.lock"].clone(); let type_re = Regex::new(r#"("type": ")[^"]*(")"#).unwrap(); let url_re = Regex::new(r#"("url": ")[^"]*(")"#).unwrap(); @@ -1335,16 +1456,38 @@ fn rewrite_composer_lock( }); continue; }; - let Some(name_idx) = content.find(&format!("\"name\": \"{composer_name}\"")) else { - result.warnings.push(RewriteWarning { - code: "redirect_composer_pkg_not_found".into(), - detail: format!("no composer.lock package named {composer_name}"), - }); - continue; - }; - let Some(dist_start) = content[name_idx..] - .find("\"dist\": {") - .map(|r| name_idx + r) + let (entry_start, entry_end) = + match find_composer_entry(&content, &composer_name, &dep.version) { + ComposerEntry::Found(start, end) => (start, end), + ComposerEntry::VersionMismatch(locked) => { + result.warnings.push(RewriteWarning { + code: "redirect_composer_version_mismatch".into(), + detail: format!( + "composer.lock pins {composer_name}@{locked}, not the patched {}", + dep.version + ), + }); + continue; + } + ComposerEntry::NotFound => { + result.warnings.push(RewriteWarning { + code: "redirect_composer_pkg_not_found".into(), + detail: format!( + "no composer.lock package named {composer_name}@{}", + dep.version + ), + }); + continue; + } + }; + // The dist block MUST belong to the located entry. Scanning forward + // from the name for the next `"dist": {` walked into the FOLLOWING + // package whenever the target was installed from source, repointing a + // bystander's url + shasum — a checksum-clean install of the wrong + // code. A target with no dist of its own pins nothing: fail closed. + let Some(dist_start) = content[entry_start..=entry_end] + .find(DIST_KEY) + .map(|offset| entry_start + offset) else { result.warnings.push(RewriteWarning { code: "redirect_composer_no_dist".into(), @@ -1352,20 +1495,41 @@ fn rewrite_composer_lock( }); continue; }; - let Some(dist_end) = content[dist_start..].find('}').map(|r| dist_start + r) else { + let Some(dist_end) = json_object_end_from(&content, dist_start + DIST_KEY.len()) else { + result.warnings.push(RewriteWarning { + code: "redirect_composer_lock_malformed".into(), + detail: format!("{composer_name}'s dist block is unterminated"), + }); continue; }; let block = content[dist_start..=dist_end].to_string(); - let escaped_url = dep.artifact_url.replace('/', "\\/"); + // Already redirected (either slash spelling): recording an edit whose + // `original` IS the hosted url would grow the ledger on every re-run + // and poison a future revert. + if artifact_url_present(&block, &dep.artifact_url) && block.contains(&sha1) { + continue; + } + if !block.contains("\"url\": \"") { + result.warnings.push(RewriteWarning { + code: "redirect_composer_no_dist_url".into(), + detail: format!("{composer_name}'s dist block has no url to redirect"), + }); + continue; + } let mut rewritten = type_re.replace(&block, "${1}zip${2}").to_string(); rewritten = url_re - .replace(&rewritten, format!("${{1}}{escaped_url}${{2}}").as_str()) + .replace( + &rewritten, + format!("${{1}}{}${{2}}", dep.artifact_url).as_str(), + ) .to_string(); - if rewritten.contains("\"shasum\": \"") { - rewritten = shasum_re + rewritten = if rewritten.contains("\"shasum\": \"") { + shasum_re .replace(&rewritten, format!("${{1}}{sha1}${{2}}").as_str()) - .to_string(); - } + .to_string() + } else { + append_composer_shasum(&rewritten, &sha1) + }; if rewritten != block { content = format!( "{}{}{}", @@ -3682,4 +3846,204 @@ snapshots: second.edits ); } + + const COMPOSER_ARTIFACT_URL: &str = + "https://patch.socket.dev/patch/composer/acme/target/1.0.0/\ + 11111111-1111-1111-1111-111111111111/\ + 44444444-4444-4444-4444-444444444444/target-1.0.0.zip"; + const COMPOSER_SHA1: &str = "abcdef0123456789abcdef0123456789abcdef01"; + + fn composer_override(version: &str) -> DepOverride { + DepOverride { + ecosystem: "composer".into(), + name: "target".into(), + namespace: Some("acme".into()), + version: version.into(), + token: String::new(), + patch_uuid: "44444444-4444-4444-4444-444444444444".into(), + artifact_url: COMPOSER_ARTIFACT_URL.into(), + berry_zip_url: None, + registry_override: None, + integrity: Integrity { + sha1: Some(COMPOSER_SHA1.into()), + ..Default::default() + }, + } + } + + /// A composer.lock holding `acme/target` (dist shaped by `target_dist`) + /// followed by an untouchable bystander that DOES have a dist. + fn composer_lock_with(target_dist: &str) -> String { + format!( + "{{ + \"packages\": [ + {{ + \"name\": \"acme/target\", + \"version\": \"1.0.0\",{target_dist} + }}, + {{ + \"name\": \"innocent/bystander\", + \"version\": \"2.0.0\", + \"dist\": {{ + \"type\": \"zip\", + \"url\": \"https://api.github.com/repos/innocent/bystander/zipball/beef\", + \"reference\": \"beef\", + \"shasum\": \"\" + }} + }} + ], + \"packages-dev\": [] +}} +" + ) + } + + fn composer_result(lock: &str, version: &str) -> RewriteResult { + let mut files = BTreeMap::new(); + files.insert("composer.lock".to_string(), lock.to_string()); + rewrite_registry_redirect(&files, &[composer_override(version)]) + } + + /// A source-only target (composer.lock records `source`, no `dist` — a VCS + /// install) must fail closed. The rewriter used to find the package by name + /// and then scan FORWARD for the next `"dist": {` with no package boundary, + /// so it repointed the FOLLOWING package's url AND shasum at the target's + /// patch: a checksum-clean install of the wrong code. + #[test] + fn composer_source_only_target_never_touches_the_next_package() { + let lock = composer_lock_with( + " + \"source\": { + \"type\": \"git\", + \"url\": \"https://github.com/acme/target.git\", + \"reference\": \"cafe\" + }", + ); + let r = composer_result(&lock, "1.0.0"); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "no dist belongs to acme/target, so nothing may be rewritten: files={:?} edits={:?}", + r.files.keys(), + r.edits + ); + assert_eq!(warning_codes(&r), vec!["redirect_composer_no_dist"]); + } + + /// A dist block with NO `shasum` key (VCS/zipball dists omit it) must get + /// the pin inserted, not redirected unpinned: composer would otherwise + /// install whatever the hosted url returned with nothing verifying it. + #[test] + fn composer_dist_without_shasum_key_gets_the_pin_inserted() { + let lock = composer_lock_with( + " + \"dist\": { + \"type\": \"zip\", + \"url\": \"https://example.test/vcs/acme/target/zipball/cafe\", + \"reference\": \"cafe\" + }", + ); + let r = composer_result(&lock, "1.0.0"); + let out = r + .files + .get("composer.lock") + .unwrap_or_else(|| panic!("the dist must be redirected; warnings={:?}", r.warnings)); + assert!( + out.contains(&format!( + "\"reference\": \"cafe\",\n \"shasum\": \"{COMPOSER_SHA1}\"" + )), + "the sha1 must be pinned as the dist's last key, at the block's own indent: {out}" + ); + assert!( + serde_json::from_str::(out).is_ok(), + "the surgical insertion must leave valid JSON: {out}" + ); + assert!( + out.contains("zipball/beef") && !out.contains("bystander/zipball/cafe"), + "the bystander's dist must be untouched: {out}" + ); + assert!(r.warnings.is_empty(), "no warnings: {:?}", r.warnings); + + // Re-run over the pinned output: nothing left to change. + let mut again = BTreeMap::new(); + again.insert("composer.lock".to_string(), out.clone()); + let second = rewrite_registry_redirect(&again, &[composer_override("1.0.0")]); + assert!( + second.files.is_empty() && second.edits.is_empty(), + "re-run must be a no-op: files={:?} edits={:?}", + second.files.keys(), + second.edits + ); + } + + /// The locked version must match the patched one. Matching on name alone + /// repointed whichever version the lock happened to hold at a patch built + /// for a different one. + #[test] + fn composer_version_mismatch_fails_closed() { + let lock = composer_lock_with( + " + \"dist\": { + \"type\": \"zip\", + \"url\": \"https://example.test/acme/target/zipball/cafe\", + \"reference\": \"cafe\", + \"shasum\": \"\" + }", + ); + let r = composer_result(&lock, "9.9.9"); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "a lock pinning another version must not be rewritten: {:?}", + r.files.keys() + ); + assert_eq!( + warning_codes(&r), + vec!["redirect_composer_version_mismatch"] + ); + } + + /// An already-redirected lock is left alone whichever way it spells the + /// hosted url — a lock written by older composer carries `\/`-escaped + /// slashes. Re-recording an edit whose `original` IS the hosted url would + /// grow the committed ledger on every run and poison a future revert. + #[test] + fn composer_rerun_over_an_escaped_slash_redirect_is_a_noop() { + let escaped = COMPOSER_ARTIFACT_URL.replace('/', "\\/"); + let lock = composer_lock_with(&format!( + " + \"dist\": {{ + \"type\": \"zip\", + \"url\": \"{escaped}\", + \"reference\": \"cafe\", + \"shasum\": \"{COMPOSER_SHA1}\" + }}" + )); + let r = composer_result(&lock, "1.0.0"); + assert!( + r.files.is_empty() && r.edits.is_empty() && r.warnings.is_empty(), + "an already-redirected lock must be a no-op: files={:?} edits={:?} warnings={:?}", + r.files.keys(), + r.edits, + r.warnings + ); + } + + /// A dist block with no `url` has nothing to redirect: pinning a shasum + /// onto it would claim a redirect that cannot happen. + #[test] + fn composer_dist_without_url_fails_closed() { + let lock = composer_lock_with( + " + \"dist\": { + \"type\": \"path\", + \"reference\": \"cafe\" + }", + ); + let r = composer_result(&lock, "1.0.0"); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "nothing may be rewritten: {:?}", + r.files.keys() + ); + assert_eq!(warning_codes(&r), vec!["redirect_composer_no_dist_url"]); + } } diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/expected-edits.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/expected-edits.json index f8ef3714..538817b4 100644 --- a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/expected-edits.json +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/expected-edits.json @@ -4,7 +4,7 @@ "kind": "redirect_composer_dist", "action": "rewritten", "key": "monolog/monolog", - "original": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https:\\/\\/api.github.com\\/repos\\/Seldaek\\/monolog\\/zipball\\/abc123\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"\"\n }", - "new": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https:\\/\\/patch.socket.dev\\/patch\\/composer\\/monolog\\/monolog\\/2.0.0\\/11111111-1111-1111-1111-111111111111\\/44444444-4444-4444-4444-444444444444\\/monolog-2.0.0.zip\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"abcdef0123456789abcdef0123456789abcdef01\"\n }" + "original": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https://api.github.com/repos/Seldaek/monolog/zipball/abc123\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"\"\n }", + "new": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"abcdef0123456789abcdef0123456789abcdef01\"\n }" } ] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/expected/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/expected/composer.lock index 79d7686c..8937ef5e 100644 --- a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/expected/composer.lock +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/expected/composer.lock @@ -9,10 +9,20 @@ "version": "2.0.0", "dist": { "type": "zip", - "url": "https:\/\/patch.socket.dev\/patch\/composer\/monolog\/monolog\/2.0.0\/11111111-1111-1111-1111-111111111111\/44444444-4444-4444-4444-444444444444\/monolog-2.0.0.zip", + "url": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", "reference": "abc123def456", "shasum": "abcdef0123456789abcdef0123456789abcdef01" } + }, + { + "name": "psr/log", + "version": "1.1.4", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/log/zipball/d49695b909c3b7628b6289db5479a1c204601f11", + "reference": "d49695b909c3b7628b6289db5479a1c204601f11", + "shasum": "" + } } ], "packages-dev": [] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/input/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/input/composer.lock index 24fb942d..333e00f4 100644 --- a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/input/composer.lock +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/basic/input/composer.lock @@ -9,10 +9,20 @@ "version": "2.0.0", "dist": { "type": "zip", - "url": "https:\/\/api.github.com\/repos\/Seldaek\/monolog\/zipball\/abc123", + "url": "https://api.github.com/repos/Seldaek/monolog/zipball/abc123", "reference": "abc123def456", "shasum": "" } + }, + { + "name": "psr/log", + "version": "1.1.4", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/log/zipball/d49695b909c3b7628b6289db5479a1c204601f11", + "reference": "d49695b909c3b7628b6289db5479a1c204601f11", + "shasum": "" + } } ], "packages-dev": [] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/expected-edits.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/expected-edits.json new file mode 100644 index 00000000..3b0043c4 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/expected-edits.json @@ -0,0 +1,10 @@ +[ + { + "path": "composer.lock", + "kind": "redirect_composer_dist", + "action": "rewritten", + "key": "monolog/monolog", + "original": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https:\\/\\/api.github.com\\/repos\\/Seldaek\\/monolog\\/zipball\\/abc123\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"\"\n }", + "new": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"abcdef0123456789abcdef0123456789abcdef01\"\n }" + } +] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/expected/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/expected/composer.lock new file mode 100644 index 00000000..cf8e4363 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/expected/composer.lock @@ -0,0 +1,29 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state" + ], + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "monolog/monolog", + "version": "2.0.0", + "dist": { + "type": "zip", + "url": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", + "reference": "abc123def456", + "shasum": "abcdef0123456789abcdef0123456789abcdef01" + } + }, + { + "name": "psr/log", + "version": "1.1.4", + "dist": { + "type": "zip", + "url": "https:\/\/api.github.com\/repos\/php-fig\/log\/zipball\/d49695b909c3b7628b6289db5479a1c204601f11", + "reference": "d49695b909c3b7628b6289db5479a1c204601f11", + "shasum": "" + } + } + ], + "packages-dev": [] +} diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/input/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/input/composer.lock new file mode 100644 index 00000000..2669e33b --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/input/composer.lock @@ -0,0 +1,29 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state" + ], + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "monolog/monolog", + "version": "2.0.0", + "dist": { + "type": "zip", + "url": "https:\/\/api.github.com\/repos\/Seldaek\/monolog\/zipball\/abc123", + "reference": "abc123def456", + "shasum": "" + } + }, + { + "name": "psr/log", + "version": "1.1.4", + "dist": { + "type": "zip", + "url": "https:\/\/api.github.com\/repos\/php-fig\/log\/zipball\/d49695b909c3b7628b6289db5479a1c204601f11", + "reference": "d49695b909c3b7628b6289db5479a1c204601f11", + "shasum": "" + } + } + ], + "packages-dev": [] +} diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/overrides.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/overrides.json new file mode 100644 index 00000000..a83719b2 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/escaped-slash-lock/overrides.json @@ -0,0 +1,14 @@ +[ + { + "ecosystem": "composer", + "name": "monolog", + "namespace": "monolog", + "version": "2.0.0", + "token": "11111111-1111-1111-1111-111111111111", + "patchUuid": "44444444-4444-4444-4444-444444444444", + "artifactUrl": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", + "integrity": { + "sha1": "abcdef0123456789abcdef0123456789abcdef01" + } + } +] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/expected-edits.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/expected-edits.json new file mode 100644 index 00000000..538817b4 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/expected-edits.json @@ -0,0 +1,10 @@ +[ + { + "path": "composer.lock", + "kind": "redirect_composer_dist", + "action": "rewritten", + "key": "monolog/monolog", + "original": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https://api.github.com/repos/Seldaek/monolog/zipball/abc123\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"\"\n }", + "new": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"abcdef0123456789abcdef0123456789abcdef01\"\n }" + } +] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/expected/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/expected/composer.lock new file mode 100644 index 00000000..4b6479d3 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/expected/composer.lock @@ -0,0 +1,19 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state" + ], + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "Monolog/MonoLog", + "version": "v2.0.0", + "dist": { + "type": "zip", + "url": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", + "reference": "abc123def456", + "shasum": "abcdef0123456789abcdef0123456789abcdef01" + } + } + ], + "packages-dev": [] +} diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/input/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/input/composer.lock new file mode 100644 index 00000000..d87b6340 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/input/composer.lock @@ -0,0 +1,19 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state" + ], + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "Monolog/MonoLog", + "version": "v2.0.0", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/Seldaek/monolog/zipball/abc123", + "reference": "abc123def456", + "shasum": "" + } + } + ], + "packages-dev": [] +} diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/overrides.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/overrides.json new file mode 100644 index 00000000..a83719b2 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/mixed-case-v-prefix/overrides.json @@ -0,0 +1,14 @@ +[ + { + "ecosystem": "composer", + "name": "monolog", + "namespace": "monolog", + "version": "2.0.0", + "token": "11111111-1111-1111-1111-111111111111", + "patchUuid": "44444444-4444-4444-4444-444444444444", + "artifactUrl": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", + "integrity": { + "sha1": "abcdef0123456789abcdef0123456789abcdef01" + } + } +] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/expected-edits.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/expected-edits.json new file mode 100644 index 00000000..adce35c5 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/expected-edits.json @@ -0,0 +1,10 @@ +[ + { + "path": "composer.lock", + "kind": "redirect_composer_dist", + "action": "rewritten", + "key": "monolog/monolog", + "original": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https://example.test/vcs/monolog/monolog/zipball/abc123\",\n \"reference\": \"abc123def456\"\n }", + "new": "\"dist\": {\n \"type\": \"zip\",\n \"url\": \"https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip\",\n \"reference\": \"abc123def456\",\n \"shasum\": \"abcdef0123456789abcdef0123456789abcdef01\"\n }" + } +] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/expected/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/expected/composer.lock new file mode 100644 index 00000000..8937ef5e --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/expected/composer.lock @@ -0,0 +1,29 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state" + ], + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "monolog/monolog", + "version": "2.0.0", + "dist": { + "type": "zip", + "url": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", + "reference": "abc123def456", + "shasum": "abcdef0123456789abcdef0123456789abcdef01" + } + }, + { + "name": "psr/log", + "version": "1.1.4", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/log/zipball/d49695b909c3b7628b6289db5479a1c204601f11", + "reference": "d49695b909c3b7628b6289db5479a1c204601f11", + "shasum": "" + } + } + ], + "packages-dev": [] +} diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/input/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/input/composer.lock new file mode 100644 index 00000000..6513111e --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/input/composer.lock @@ -0,0 +1,28 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state" + ], + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "monolog/monolog", + "version": "2.0.0", + "dist": { + "type": "zip", + "url": "https://example.test/vcs/monolog/monolog/zipball/abc123", + "reference": "abc123def456" + } + }, + { + "name": "psr/log", + "version": "1.1.4", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/log/zipball/d49695b909c3b7628b6289db5479a1c204601f11", + "reference": "d49695b909c3b7628b6289db5479a1c204601f11", + "shasum": "" + } + } + ], + "packages-dev": [] +} diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/overrides.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/overrides.json new file mode 100644 index 00000000..a83719b2 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/no-shasum-key/overrides.json @@ -0,0 +1,14 @@ +[ + { + "ecosystem": "composer", + "name": "monolog", + "namespace": "monolog", + "version": "2.0.0", + "token": "11111111-1111-1111-1111-111111111111", + "patchUuid": "44444444-4444-4444-4444-444444444444", + "artifactUrl": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", + "integrity": { + "sha1": "abcdef0123456789abcdef0123456789abcdef01" + } + } +] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/expected-edits.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/expected-edits.json new file mode 100644 index 00000000..fe51488c --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/expected-edits.json @@ -0,0 +1 @@ +[] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/input/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/input/composer.lock new file mode 100644 index 00000000..f708f9fb --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/input/composer.lock @@ -0,0 +1,28 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state" + ], + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "monolog/monolog", + "version": "2.0.0", + "source": { + "type": "git", + "url": "https://github.com/Seldaek/monolog.git", + "reference": "abc123def456" + } + }, + { + "name": "psr/log", + "version": "1.1.4", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/log/zipball/d49695b909c3b7628b6289db5479a1c204601f11", + "reference": "d49695b909c3b7628b6289db5479a1c204601f11", + "shasum": "" + } + } + ], + "packages-dev": [] +} diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/overrides.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/overrides.json new file mode 100644 index 00000000..a83719b2 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/source-only-bystander/overrides.json @@ -0,0 +1,14 @@ +[ + { + "ecosystem": "composer", + "name": "monolog", + "namespace": "monolog", + "version": "2.0.0", + "token": "11111111-1111-1111-1111-111111111111", + "patchUuid": "44444444-4444-4444-4444-444444444444", + "artifactUrl": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", + "integrity": { + "sha1": "abcdef0123456789abcdef0123456789abcdef01" + } + } +] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/expected-edits.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/expected-edits.json new file mode 100644 index 00000000..fe51488c --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/expected-edits.json @@ -0,0 +1 @@ +[] diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/input/composer.lock b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/input/composer.lock new file mode 100644 index 00000000..782a450b --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/input/composer.lock @@ -0,0 +1,19 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state" + ], + "content-hash": "abc123def456abc123def456abc1", + "packages": [ + { + "name": "monolog/monolog", + "version": "1.9.0", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/Seldaek/monolog/zipball/999999", + "reference": "999999aaaaaa", + "shasum": "" + } + } + ], + "packages-dev": [] +} diff --git a/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/overrides.json b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/overrides.json new file mode 100644 index 00000000..a83719b2 --- /dev/null +++ b/crates/socket-patch-core/tests/fixtures/redirect/composer/composer-lock/version-mismatch/overrides.json @@ -0,0 +1,14 @@ +[ + { + "ecosystem": "composer", + "name": "monolog", + "namespace": "monolog", + "version": "2.0.0", + "token": "11111111-1111-1111-1111-111111111111", + "patchUuid": "44444444-4444-4444-4444-444444444444", + "artifactUrl": "https://patch.socket.dev/patch/composer/monolog/monolog/2.0.0/11111111-1111-1111-1111-111111111111/44444444-4444-4444-4444-444444444444/monolog-2.0.0.zip", + "integrity": { + "sha1": "abcdef0123456789abcdef0123456789abcdef01" + } + } +] From e55d6cf0b9866d1c1b2bc94ca0739425cbb0e7c6 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 14 Aug 2026 09:28:06 -0700 Subject: [PATCH 2/6] fix(crawler): honor composer vendor-dir and install-path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discovery hardcoded `cwd/vendor`, so a project using composer.json `config.vendor-dir` or `COMPOSER_VENDOR_DIR` scanned as "not yet installed (lockfile-only)" and apply resolved its packages as not-found — installed vulnerable code could never be patched, with no error. The vendor dir now resolves the way composer does (`COMPOSER_VENDOR_DIR` > `config.vendor-dir` relative to the manifest > `vendor`), shared by crawl, find, and apply. Unsafe config values (traversal, absolute) fail closed rather than falling back to a tree composer never installed into. installed.json's `install-path` was ignored, so packages installed through composer/installers (WordPress plugins, Drupal modules — the classic PHP CVE carriers) were invisible and unpatchable even though their exact path sat unread in the record. It is now the authoritative package dir, gated lexically to the project root; a rejected path drops the entry instead of falling back to the conventional dir a poisoned record could squat. lock_inventory now shares the crawler's `normalize_version`, so a composer-legal `V1.2.3` tag no longer double-counts as an installed row plus a phantom lockfile-only row (both previously POSTed to the API). Co-authored-by: Claude Fable 5 --- .../src/crawlers/composer_crawler.rs | 370 ++++++++++++++- .../src/vendor/lock_inventory.rs | 15 +- .../tests/crawler_composer_e2e.rs | 447 ++++++++++++++++++ 3 files changed, 803 insertions(+), 29 deletions(-) diff --git a/crates/socket-patch-core/src/crawlers/composer_crawler.rs b/crates/socket-patch-core/src/crawlers/composer_crawler.rs index 63904a9f..47815c63 100644 --- a/crates/socket-patch-core/src/crawlers/composer_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/composer_crawler.rs @@ -10,12 +10,18 @@ use crate::utils::process::{CommandRunner, SystemCommandRunner}; /// vendor directories. pub struct ComposerCrawler; -/// A single package entry distilled from installed.json. Only the two +/// A single package entry distilled from installed.json. Only the three /// fields the crawler needs are retained; everything else (source, /// dist, autoload, ...) is ignored. struct ComposerPackageEntry { name: String, version: String, + /// `install-path` as recorded, relative to the `vendor/composer/` + /// directory that holds installed.json (`../monolog/monolog` for a + /// conventional install, `../../web/app/plugins/x` for a + /// composer/installers target). `None` for Composer 1 entries and any + /// entry that omits it. + install_path: Option, } impl ComposerCrawler { @@ -29,9 +35,11 @@ impl ComposerCrawler { /// In global mode, checks `$COMPOSER_HOME/vendor/` (env var, command /// fallback, or platform defaults). /// - /// In local mode, checks `/vendor/` but only if the directory - /// contains `composer/installed.json` and the cwd looks like a PHP - /// project (`composer.json` or `composer.lock` present). + /// In local mode, checks the project's vendor directory + /// (`COMPOSER_VENDOR_DIR` / composer.json `config.vendor-dir` / + /// `vendor`, see [`resolve_local_vendor_dir`]) but only if the + /// directory contains `composer/installed.json` and the cwd looks like + /// a PHP project (`composer.json` or `composer.lock` present). pub async fn get_vendor_paths( &self, options: &CrawlerOptions, @@ -51,7 +59,9 @@ impl ComposerCrawler { } // Local mode - let vendor_dir = options.cwd.join("vendor"); + let Some(vendor_dir) = resolve_local_vendor_dir(&options.cwd).await else { + return Ok(Vec::new()); + }; let installed_json = vendor_dir.join("composer").join("installed.json"); if !is_dir(&vendor_dir).await || !is_file(&installed_json).await { @@ -77,15 +87,18 @@ impl ComposerCrawler { let vendor_paths = self.get_vendor_paths(options).await.unwrap_or_default(); for vendor_path in &vendor_paths { + let project_root = resolve_project_root(vendor_path).await; let entries = read_installed_json(vendor_path).await; for entry in entries { if let Some((namespace, name)) = entry.name.split_once('/') { // Skip packages that installed.json lists but that are - // not actually on disk (stale metadata, custom install - // paths). This keeps crawl_all consistent with - // find_by_purls, which only returns packages whose - // vendor directory exists. - let pkg_path = vendor_path.join(namespace).join(name); + // not actually on disk (stale metadata, a metapackage). + // This keeps crawl_all consistent with find_by_purls, + // which only returns packages whose directory exists. + let Some(pkg_path) = resolve_package_dir(vendor_path, &project_root, &entry) + else { + continue; + }; if !is_dir(&pkg_path).await { continue; } @@ -138,14 +151,15 @@ impl ComposerCrawler { // package names are case-insensitive and the canonical PURL is // lowercase, but installed.json records the *pretty* (case-preserved) // name and Composer writes the vendor directory with that same - // casing. Key the map by the lowercased name and carry the original - // name so the real on-disk path can be reconstructed even on + // casing. Key the map by the lowercased name and carry the whole + // entry so the real on-disk path can be reconstructed even on // case-sensitive filesystems. let entries = read_installed_json(vendor_path).await; - let installed: HashMap = entries + let installed: HashMap = entries .into_iter() - .map(|e| (e.name.to_ascii_lowercase(), (e.name, e.version))) + .map(|e| (e.name.to_ascii_lowercase(), e)) .collect(); + let project_root = resolve_project_root(vendor_path).await; for purl in purls { if let Some(((namespace, name), version)) = @@ -153,7 +167,7 @@ impl ComposerCrawler { { let full_name = format!("{namespace}/{name}").to_ascii_lowercase(); - let Some((installed_name, installed_version)) = installed.get(&full_name) else { + let Some(entry) = installed.get(&full_name) else { continue; }; @@ -161,17 +175,17 @@ impl ComposerCrawler { // normalized version so a `v`-prefixed installed.json // version (`v6.4.1`) matches a bare PURL version (`6.4.1`) // and vice versa. - if normalize_version(installed_version) != normalize_version(version) { + if normalize_version(&entry.version) != normalize_version(version) { continue; } - // Resolve the on-disk directory using the original casing - // recorded in installed.json, which is what Composer wrote to - // disk — the canonical (lowercase) PURL name would miss it on - // a case-sensitive filesystem. - let pkg_dir = match installed_name.split_once('/') { - Some((ns, n)) => vendor_path.join(ns).join(n), - None => continue, + // Resolve the on-disk directory from installed.json's own + // record — its `install-path` when present, else the + // conventional layout under the original (case-preserved) + // casing Composer wrote to disk; the canonical (lowercase) + // PURL name would miss it on a case-sensitive filesystem. + let Some(pkg_dir) = resolve_package_dir(vendor_path, &project_root, entry) else { + continue; }; if !is_dir(&pkg_dir).await { @@ -282,6 +296,199 @@ pub(crate) fn normalize_version(version: &str) -> &str { version } +/// How far above the vendor directory [`resolve_project_root`] looks for +/// the composer manifest. `config.vendor-dir` may nest the vendor tree +/// (`lib/deps`), so the project root is not always the immediate parent; +/// the walk is bounded so an unrelated `composer.json` far up the +/// filesystem can't widen the write boundary. +const PROJECT_ROOT_SEARCH_DEPTH: usize = 3; + +/// Read `config.vendor-dir` out of a composer.json body, mirroring the +/// slice of Composer's `Config::get('vendor-dir')` that matters here: +/// trailing separators are trimmed (`"vendor/"` is legal) and an empty +/// value counts as unset. +/// +/// Composer additionally expands `$HOME`/`~`/`%VAR%` placeholders +/// (`Platform::expandPath`); that is deliberately NOT implemented — an +/// unexpanded value simply resolves to a directory that does not exist, +/// so discovery reports nothing rather than guessing at a path. +fn parse_config_vendor_dir(composer_json: &str) -> Option { + let doc: serde_json::Value = serde_json::from_str(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()) +} + +/// Resolve the vendor directory of a local project the way Composer does: +/// `COMPOSER_VENDOR_DIR` wins, else composer.json `config.vendor-dir` +/// (relative to the manifest directory), else `vendor`. +/// +/// Composer relocates the WHOLE vendor tree — `composer/installed.json` +/// included — so assuming `/vendor` makes every installed package +/// invisible: scan reports them as lockfile-only ("not yet installed") +/// and apply resolves them as `package_not_found`. +/// +/// Returns `None` when composer.json configures a value that is not a +/// plain relative subpath. That value comes from the project being +/// scanned and names the directory apply later WRITES patch content into, +/// so `../../elsewhere` or `/etc` is refused outright rather than +/// silently downgraded to `vendor/` (which would patch an unrelated +/// tree). Absolute paths are legal in Composer but are refused for the +/// same reason; a project using one discovers nothing, exactly as today. +/// `COMPOSER_VENDOR_DIR` is not gated — it comes from the invoking +/// environment, the same trust level as `CARGO_HOME` / `NUGET_PACKAGES` / +/// `MAVEN_REPO_LOCAL`, which are all honored verbatim. +async fn resolve_local_vendor_dir(cwd: &Path) -> Option { + // A set-but-empty value counts as unset (twin of the MAVEN_REPO_LOCAL + // and NUGET_PACKAGES rules): honoring `""` would resolve the vendor + // tree to the project root itself. + if let Some(from_env) = std::env::var("COMPOSER_VENDOR_DIR") + .ok() + .map(|v| v.trim_end_matches(['/', '\\']).to_string()) + .filter(|v| !v.is_empty()) + { + // `join` substitutes an absolute value for the base, matching + // Composer's own relative-to-the-manifest-dir resolution. + return Some(cwd.join(from_env)); + } + + match read_config_vendor_dir(&cwd.join("composer.json")).await { + Some(configured) if path_safety::is_safe_multi_segment(&configured) => { + Some(cwd.join(configured)) + } + Some(_) => None, + None => Some(cwd.join("vendor")), + } +} + +/// Read `config.vendor-dir` from a composer.json on disk. Opened with +/// [`crate::utils::fs::open_regular_file`] for the same reason +/// installed.json is: the manifest belongs to the untrusted project, and +/// a FIFO planted at that path would wedge a plain read forever. +async fn read_config_vendor_dir(manifest_path: &Path) -> Option { + use tokio::io::AsyncReadExt; + + let (mut file, metadata) = crate::utils::fs::open_regular_file(manifest_path) + .await + .ok()?; + let mut content = String::with_capacity(metadata.len() as usize); + file.read_to_string(&mut content).await.ok()?; + parse_config_vendor_dir(&content) +} + +/// The directory an installed.json `install-path` may not escape. +/// +/// `install-path` legitimately points OUTSIDE the vendor tree — that is +/// the entire point of composer/installers (`extra.installer-paths`, +/// `type: wordpress-plugin`) — so the boundary cannot be the vendor root. +/// But installed.json is untrusted, tamperable input and the resolved +/// directory is a patch WRITE target, so it must stay inside the project: +/// the nearest ancestor of the vendor directory carrying a composer +/// manifest, else the vendor directory's immediate parent. +/// +/// Derived from the vendor path alone so scan (`crawl_all`) and apply +/// (`find_by_purls`, which is only ever handed the vendor directory) +/// agree on the boundary; disagreeing would surface packages in scan that +/// apply then refuses to resolve. +async fn resolve_project_root(vendor_path: &Path) -> PathBuf { + let mut fallback = None; + for ancestor in vendor_path + .ancestors() + .skip(1) + .take(PROJECT_ROOT_SEARCH_DEPTH) + { + if fallback.is_none() { + fallback = Some(ancestor.to_path_buf()); + } + if is_file(&ancestor.join("composer.json")).await + || is_file(&ancestor.join("composer.lock")).await + { + return normalize_lexically(ancestor).unwrap_or_else(|| ancestor.to_path_buf()); + } + } + let root = fallback.unwrap_or_else(|| vendor_path.to_path_buf()); + normalize_lexically(&root).unwrap_or(root) +} + +/// Resolve `.`/`..` without touching the filesystem, so a path can be +/// containment-checked BEFORE it is opened (a canonicalizing check would +/// have to stat the very path being validated, and would fail on +/// not-yet-existing directories). Returns `None` when `..` pops above the +/// path's own root — nothing legitimate does that, so it fails closed. +/// +/// Symlinks are not resolved: a symlink INSIDE the project pointing out +/// of it is a pre-existing trust decision of the project's own tree, the +/// same assumption the rest of the crawler layer makes. +fn normalize_lexically(path: &Path) -> Option { + use std::path::Component; + + let mut out = PathBuf::new(); + let mut depth = 0usize; + for component in path.components() { + match component { + Component::Prefix(_) | Component::RootDir => out.push(component.as_os_str()), + Component::CurDir => {} + Component::ParentDir => { + if depth == 0 { + return None; + } + out.pop(); + depth -= 1; + } + Component::Normal(segment) => { + out.push(segment); + depth += 1; + } + } + } + Some(out) +} + +/// Resolve an installed.json `install-path` against the vendor tree. +/// +/// Composer records it relative to `vendor/composer/` (the directory +/// holding installed.json), not to the vendor root. Returns `None` when +/// the resolved directory leaves `project_root` +/// ([`resolve_project_root`]) — fail closed, no fallback to +/// `vendor//`: an entry that claims to live somewhere out of +/// tree must not redirect the patch onto an unrelated directory. +fn resolve_install_path( + vendor_path: &Path, + project_root: &Path, + install_path: &str, +) -> Option { + if install_path.contains('\0') { + return None; + } + // `join` substitutes an absolute `install-path` (Composer writes one + // for some path repositories) for the base; the containment check + // below is what keeps it in bounds either way. + let joined = vendor_path.join("composer").join(install_path); + let resolved = normalize_lexically(&joined)?; + resolved.starts_with(project_root).then_some(resolved) +} + +/// The on-disk directory of an installed.json entry. +/// +/// `install-path` is authoritative when recorded: Composer 2 writes it for +/// every package, and for composer/installers targets (WordPress plugins, +/// Drupal modules, `extra.installer-paths`) it is the ONLY record of where +/// the package really lives. Entries without one (Composer 1, hand-written +/// metadata) keep the conventional `vendor//` layout. +fn resolve_package_dir( + vendor_path: &Path, + project_root: &Path, + entry: &ComposerPackageEntry, +) -> Option { + match entry.install_path.as_deref() { + Some(install_path) => resolve_install_path(vendor_path, project_root, install_path), + None => { + let (namespace, name) = entry.name.split_once('/')?; + Some(vendor_path.join(namespace).join(name)) + } + } +} + /// Whether an installed.json package name is safe to join onto the /// vendor root. Both `crawl_all` and `find_by_purls` split the recorded /// name at `/` and join the pieces onto the vendor directory, and the @@ -351,9 +558,19 @@ async fn read_installed_json(vendor_path: &Path) -> Vec { if name.is_empty() || version.is_empty() || !is_safe_composer_name(name) { return None; } + // `install-path` is NOT gated here: it is legitimately a + // `..`-prefixed path out of `vendor/composer/`, so the + // coordinate gate cannot be a per-segment one. It is validated + // when resolved instead ([`resolve_install_path`]). + let install_path = entry + .get("install-path") + .and_then(|p| p.as_str()) + .filter(|p| !p.is_empty()) + .map(str::to_string); Some(ComposerPackageEntry { name: name.to_string(), version: version.to_string(), + install_path, }) }) .collect() @@ -1085,6 +1302,113 @@ mod tests { ); } + #[test] + fn test_parse_config_vendor_dir() { + assert_eq!( + parse_config_vendor_dir(r#"{"config":{"vendor-dir":"lib/deps"}}"#).as_deref(), + Some("lib/deps") + ); + // Composer rtrims trailing separators before using the value. + assert_eq!( + parse_config_vendor_dir(r#"{"config":{"vendor-dir":"lib/deps/"}}"#).as_deref(), + Some("lib/deps") + ); + // No config block, no key, wrong type, empty value, malformed JSON — + // all "unset", so the caller falls back to `vendor`. + assert_eq!(parse_config_vendor_dir("{}"), None); + assert_eq!(parse_config_vendor_dir(r#"{"config":{}}"#), None); + assert_eq!( + parse_config_vendor_dir(r#"{"config":{"vendor-dir":7}}"#), + None + ); + assert_eq!( + parse_config_vendor_dir(r#"{"config":{"vendor-dir":""}}"#), + None + ); + assert_eq!( + parse_config_vendor_dir(r#"{"config":{"vendor-dir":"/"}}"#), + None + ); + assert_eq!(parse_config_vendor_dir("{ not json"), None); + } + + #[test] + fn test_normalize_lexically() { + let n = |p: &str| normalize_lexically(Path::new(p)); + // `.` drops out, `..` pops the previous segment. + assert_eq!( + n("/a/b/composer/../monolog/monolog").unwrap(), + PathBuf::from("/a/b/monolog/monolog") + ); + assert_eq!( + n("/a/b/composer/./installers").unwrap(), + PathBuf::from("/a/b/composer/installers") + ); + assert_eq!(n("/a/b/c/../../../web/x").unwrap(), PathBuf::from("/web/x")); + // Popping above the path's own root fails closed. + assert_eq!(n("/a/../.."), None); + assert_eq!(n("../x"), None); + // Relative paths stay relative. + assert_eq!(n("a/b/../c").unwrap(), PathBuf::from("a/c")); + } + + #[tokio::test] + async fn test_resolve_project_root_finds_manifest_above_nested_vendor() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("proj"); + let vendor = root.join("lib").join("deps"); + tokio::fs::create_dir_all(&vendor).await.unwrap(); + tokio::fs::write(root.join("composer.json"), "{}") + .await + .unwrap(); + + // A `config.vendor-dir` can nest the vendor tree, so the project root + // is not the vendor dir's parent — it is the nearest ancestor holding + // the manifest. + assert_eq!(resolve_project_root(&vendor).await, root); + + // With no manifest anywhere above, the immediate parent is the + // boundary rather than an unbounded walk up the filesystem. + let orphan = dir.path().join("orphan").join("vendor"); + tokio::fs::create_dir_all(&orphan).await.unwrap(); + assert_eq!( + resolve_project_root(&orphan).await, + dir.path().join("orphan") + ); + } + + #[tokio::test] + async fn test_crawl_all_without_install_path_uses_conventional_layout() { + // Composer 1 (and hand-written metadata) records no install-path; + // those entries must keep resolving to vendor//. + let dir = tempfile::tempdir().unwrap(); + let vendor_dir = dir.path().join("vendor"); + let composer_dir = vendor_dir.join("composer"); + tokio::fs::create_dir_all(&composer_dir).await.unwrap(); + tokio::fs::write( + composer_dir.join("installed.json"), + r#"[{"name": "monolog/monolog", "version": "3.5.0"}]"#, + ) + .await + .unwrap(); + tokio::fs::create_dir_all(vendor_dir.join("monolog").join("monolog")) + .await + .unwrap(); + tokio::fs::write(dir.path().join("composer.json"), "{}") + .await + .unwrap(); + + let crawler = ComposerCrawler::new(); + let options = CrawlerOptions { + cwd: dir.path().to_path_buf(), + global: false, + global_prefix: None, + }; + let packages = crawler.crawl_all(&options).await; + assert_eq!(packages.len(), 1); + assert_eq!(packages[0].path, vendor_dir.join("monolog").join("monolog")); + } + #[tokio::test] async fn test_find_by_purls_version_mismatch() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/socket-patch-core/src/vendor/lock_inventory.rs b/crates/socket-patch-core/src/vendor/lock_inventory.rs index 914d9fee..8843ff13 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory.rs @@ -22,6 +22,7 @@ use std::path::Path; use serde_json::Value; +use crate::crawlers::composer_crawler::normalize_version; use crate::crawlers::python_crawler::canonicalize_pypi_name; use crate::patch::path_safety; use crate::utils::purl::{percent_decode_purl_component, strip_purl_qualifiers}; @@ -637,7 +638,8 @@ async fn inventory_bun(root: &Path) -> Option> { /// Inventory `composer.lock` `packages`/`packages-dev`. The `dist.shasum` /// (sha1 of the dist zip) is frequently empty — such entries stay /// discovery-only. Names lowercase to the canonical packagist form; -/// versions drop the pretty leading `v`. +/// versions drop the pretty leading `v`/`V` through the crawler's +/// [`normalize_version`], so installed and lockfile rows agree. async fn inventory_composer_lock(project_root: &Path) -> Option> { let bytes = tokio::fs::read(project_root.join("composer.lock")) .await @@ -656,11 +658,12 @@ async fn inventory_composer_lock(project_root: &Path) -> Option/composer/installed.json` with an +/// `install-path` relative to that `composer/` directory, and the package +/// itself at `//`. `config_vendor_dir` is written +/// into composer.json's `config` block when supplied. +async fn stage_relocated_project( + root: &Path, + vendor_rel: &str, + config_vendor_dir: Option<&str>, +) -> std::path::PathBuf { + let vendor = root.join(vendor_rel); + tokio::fs::create_dir_all(vendor.join("monolog").join("monolog")) + .await + .unwrap(); + let composer_dir = vendor.join("composer"); + tokio::fs::create_dir_all(&composer_dir).await.unwrap(); + tokio::fs::write( + composer_dir.join("installed.json"), + br#"{"packages":[{"name":"monolog/monolog","version":"3.5.0","install-path":"../monolog/monolog"}]}"#, + ) + .await + .unwrap(); + + let manifest = match config_vendor_dir { + Some(dir) => format!(r#"{{"config":{{"vendor-dir":"{dir}"}}}}"#), + None => "{}".to_string(), + }; + tokio::fs::write(root.join("composer.json"), manifest) + .await + .unwrap(); + vendor +} + +/// Composer relocates the ENTIRE vendor tree — `composer/installed.json` +/// included — when composer.json sets `config.vendor-dir`, so assuming +/// `/vendor` finds nothing: scan then reports every installed package +/// as lockfile-only ("not yet installed") and apply resolves each one as +/// `package_not_found`. Verified against composer 2.10.2: `"vendor-dir": +/// "lib/deps"` puts installed.json at `lib/deps/composer/installed.json`. +#[tokio::test] +#[serial_test::parallel] +async fn config_vendor_dir_relocates_discovery() { + let tmp = tempfile::tempdir().unwrap(); + // Nested (`lib/deps`), which composer allows and which also proves the + // project root is still found for the install-path boundary check. + let vendor = stage_relocated_project(tmp.path(), "lib/deps", Some("lib/deps")).await; + // No `vendor/` anywhere: the only discoverable tree is the relocated one. + assert!(!tmp.path().join("vendor").exists()); + + let crawler = ComposerCrawler; + let paths = crawler + .get_vendor_paths(&options_at(tmp.path())) + .await + .unwrap(); + assert_eq!(paths, vec![vendor.clone()]); + + let packages = crawler.crawl_all(&options_at(tmp.path())).await; + assert_eq!( + packages.len(), + 1, + "relocated vendor tree must be crawled; got {packages:?}" + ); + assert_eq!(packages[0].purl, ORG_PURL); + assert_eq!(packages[0].path, vendor.join("monolog").join("monolog")); +} + +/// A trailing separator is legal in `config.vendor-dir` (Composer rtrims it +/// before use), so `"vendor-dir": "lib/deps/"` must resolve the same as +/// `"lib/deps"` — a naive join would produce an empty final segment and +/// fail the coordinate gate. +#[tokio::test] +#[serial_test::parallel] +async fn config_vendor_dir_trailing_slash_is_trimmed() { + let tmp = tempfile::tempdir().unwrap(); + let vendor = stage_relocated_project(tmp.path(), "lib/deps", Some("lib/deps/")).await; + + let crawler = ComposerCrawler; + let paths = crawler + .get_vendor_paths(&options_at(tmp.path())) + .await + .unwrap(); + assert_eq!(paths, vec![vendor]); +} + +/// `COMPOSER_VENDOR_DIR` outranks composer.json's `config.vendor-dir` in +/// Composer's own `Config::get`, so it must outrank it here too. Verified +/// against composer 2.10.2: `COMPOSER_VENDOR_DIR=third_party composer +/// install` writes `third_party/composer/installed.json`. +#[tokio::test] +#[serial_test::serial] +async fn composer_vendor_dir_env_outranks_config_and_default() { + let tmp = tempfile::tempdir().unwrap(); + // composer.json points at `lib/deps` and a decoy `vendor/` tree exists; + // the env var names a third directory, which must win over both. + let vendor = stage_relocated_project(tmp.path(), "third_party", Some("lib/deps")).await; + let decoy = tmp.path().join("vendor").join("composer"); + tokio::fs::create_dir_all(&decoy).await.unwrap(); + tokio::fs::write(decoy.join("installed.json"), b"{\"packages\":[]}") + .await + .unwrap(); + + let prev = std::env::var("COMPOSER_VENDOR_DIR").ok(); + std::env::set_var("COMPOSER_VENDOR_DIR", "third_party"); + + let crawler = ComposerCrawler; + let paths = crawler + .get_vendor_paths(&options_at(tmp.path())) + .await + .unwrap(); + let packages = crawler.crawl_all(&options_at(tmp.path())).await; + + match prev { + Some(v) => std::env::set_var("COMPOSER_VENDOR_DIR", v), + None => std::env::remove_var("COMPOSER_VENDOR_DIR"), + } + + assert_eq!(paths, vec![vendor.clone()], "env var must win"); + assert_eq!(packages.len(), 1, "got {packages:?}"); + assert_eq!(packages[0].path, vendor.join("monolog").join("monolog")); +} + +/// A set-but-empty `COMPOSER_VENDOR_DIR` counts as unset (twin of the +/// MAVEN_REPO_LOCAL / NUGET_PACKAGES rules): honoring `""` would resolve +/// the vendor tree to the project root itself, so `vendor/composer/` would +/// be looked for at `/composer/`. +#[tokio::test] +#[serial_test::serial] +async fn empty_composer_vendor_dir_env_falls_back_to_default() { + let tmp = tempfile::tempdir().unwrap(); + let vendor = stage_relocated_project(tmp.path(), "vendor", None).await; + + let prev = std::env::var("COMPOSER_VENDOR_DIR").ok(); + std::env::set_var("COMPOSER_VENDOR_DIR", ""); + + let crawler = ComposerCrawler; + let paths = crawler + .get_vendor_paths(&options_at(tmp.path())) + .await + .unwrap(); + + match prev { + Some(v) => std::env::set_var("COMPOSER_VENDOR_DIR", v), + None => std::env::remove_var("COMPOSER_VENDOR_DIR"), + } + + assert_eq!(paths, vec![vendor], "empty env var must not shadow vendor/"); +} + +/// composer.json belongs to the project being SCANNED and the vendor +/// directory it names is where apply later WRITES patch content, so a +/// `config.vendor-dir` that escapes the project is refused outright — and +/// refused fail-closed, NOT downgraded to `vendor/`, which would patch an +/// unrelated tree that Composer never installed into. +#[tokio::test] +#[serial_test::parallel] +async fn config_vendor_dir_escaping_project_is_refused() { + let outer = tempfile::tempdir().unwrap(); + let root = outer.path().join("proj"); + tokio::fs::create_dir_all(&root).await.unwrap(); + // A fully staged vendor tree OUTSIDE the project, so the refusal is the + // coordinate gate and not a missing directory. + stage_relocated_project(outer.path(), "escaped", None).await; + tokio::fs::write( + root.join("composer.json"), + br#"{"config":{"vendor-dir":"../escaped"}}"#, + ) + .await + .unwrap(); + // A conventional vendor/ tree also exists: the refusal must not silently + // fall back to it either. + let decoy = root.join("vendor").join("composer"); + tokio::fs::create_dir_all(&decoy).await.unwrap(); + tokio::fs::write( + decoy.join("installed.json"), + br#"{"packages":[{"name":"monolog/monolog","version":"3.5.0"}]}"#, + ) + .await + .unwrap(); + tokio::fs::create_dir_all(root.join("vendor").join("monolog").join("monolog")) + .await + .unwrap(); + + let crawler = ComposerCrawler; + let paths = crawler.get_vendor_paths(&options_at(&root)).await.unwrap(); + assert!( + paths.is_empty(), + "escaping config.vendor-dir must fail closed; got {paths:?}" + ); + let packages = crawler.crawl_all(&options_at(&root)).await; + assert!( + packages.is_empty(), + "escaping config.vendor-dir must not fall back to vendor/; got {packages:?}" + ); +} + +/// An ABSOLUTE `config.vendor-dir` is legal in Composer but refused here +/// for the same reason: it names an apply write target and composer.json is +/// tamperable. Discovery reports nothing, exactly as it did before custom +/// vendor directories were understood at all — no silent redirect. +#[tokio::test] +#[serial_test::parallel] +async fn absolute_config_vendor_dir_is_refused() { + let outer = tempfile::tempdir().unwrap(); + let root = outer.path().join("proj"); + tokio::fs::create_dir_all(&root).await.unwrap(); + let absolute = stage_relocated_project(outer.path(), "abs-vendor", None).await; + tokio::fs::write( + root.join("composer.json"), + format!(r#"{{"config":{{"vendor-dir":"{}"}}}}"#, absolute.display()), + ) + .await + .unwrap(); + + let crawler = ComposerCrawler; + let paths = crawler.get_vendor_paths(&options_at(&root)).await.unwrap(); + assert!(paths.is_empty(), "got {paths:?}"); +} + +// ── installed.json install-path ──────────────────────────────── + +const PLUGIN_PURL: &str = "pkg:composer/socket/probe-plugin@1.2.3"; +const INSTALLERS_PURL: &str = "pkg:composer/composer/installers@2.3.0"; + +/// composer/installers (`type: wordpress-plugin`, `extra.installer-paths`) +/// installs packages OUTSIDE `vendor//`, and installed.json's +/// `install-path` is the only record of where they landed. Reconstructing +/// the conventional layout makes them invisible to scan and unpatchable by +/// apply, even though the metadata says exactly where they are. +/// +/// The fixture mirrors composer 2.10.2 byte-for-byte: with +/// `"web/app/plugins/{$name}/"` mapped to `type:wordpress-plugin`, it wrote +/// `"install-path": "../../../web/app/plugins/probe-plugin"` (three levels +/// up from `lib/deps/composer/`) — and, for `composer/installers` itself, +/// the `./`-relative `"./installers"`. +#[tokio::test] +#[serial_test::parallel] +async fn install_path_resolves_package_outside_vendor() { + let tmp = tempfile::tempdir().unwrap(); + let vendor = tmp.path().join("vendor"); + let composer_dir = vendor.join("composer"); + tokio::fs::create_dir_all(&composer_dir).await.unwrap(); + tokio::fs::write( + composer_dir.join("installed.json"), + br#"{"packages":[ + {"name":"composer/installers","version":"v2.3.0","install-path":"./installers"}, + {"name":"socket/probe-plugin","version":"1.2.3","install-path":"../../web/app/plugins/probe-plugin"} + ]}"#, + ) + .await + .unwrap(); + tokio::fs::write(tmp.path().join("composer.json"), b"{}") + .await + .unwrap(); + + let plugin_dir = tmp + .path() + .join("web") + .join("app") + .join("plugins") + .join("probe-plugin"); + tokio::fs::create_dir_all(&plugin_dir).await.unwrap(); + let installers_dir = composer_dir.join("installers"); + tokio::fs::create_dir_all(&installers_dir).await.unwrap(); + // Control: NEITHER package sits at the conventional location, so a + // reconstructed `vendor//` finds nothing to corroborate. + assert!(!vendor.join("socket").join("probe-plugin").exists()); + + let crawler = ComposerCrawler; + let packages = crawler.crawl_all(&options_at(tmp.path())).await; + assert_eq!(packages.len(), 2, "got {packages:?}"); + let plugin = packages.iter().find(|p| p.purl == PLUGIN_PURL).unwrap(); + assert_eq!(plugin.path, plugin_dir); + // `./installers` (a CurDir component) resolves inside vendor/composer/. + let installers = packages.iter().find(|p| p.purl == INSTALLERS_PURL).unwrap(); + assert_eq!(installers.path, installers_dir); + + // apply resolves through find_by_purls and must agree with the crawl — + // otherwise the patch button offers a package apply can't locate. + let found = crawler + .find_by_purls(&vendor, &[PLUGIN_PURL.to_string()]) + .await + .unwrap(); + assert_eq!(found.len(), 1, "got {found:?}"); + assert_eq!(found.get(PLUGIN_PURL).unwrap().path, plugin_dir); +} + +/// installed.json is untrusted, tamperable input and the directory it names +/// is a patch WRITE target, so an `install-path` that leaves the project +/// must be dropped — by both the scan path and apply's resolver. The +/// boundary is the project, not the vendor root: a legitimate +/// composer/installers target lives outside `vendor/` (see the test above), +/// so `..` alone cannot be the signal. +#[tokio::test] +#[serial_test::parallel] +async fn install_path_escaping_project_root_is_rejected() { + let outer = tempfile::tempdir().unwrap(); + let root = outer.path().join("proj"); + let vendor = root.join("vendor"); + let composer_dir = vendor.join("composer"); + tokio::fs::create_dir_all(&composer_dir).await.unwrap(); + tokio::fs::write(root.join("composer.json"), b"{}") + .await + .unwrap(); + + // Both escape targets EXIST on disk, so the on-disk corroboration alone + // does not stop them; only the containment gate does. + let outside = outer.path().join("evil").join("pkg"); + tokio::fs::create_dir_all(&outside).await.unwrap(); + tokio::fs::create_dir_all(vendor.join("monolog").join("monolog")) + .await + .unwrap(); + tokio::fs::write( + composer_dir.join("installed.json"), + format!( + r#"{{"packages":[ + {{"name":"monolog/monolog","version":"3.5.0","install-path":"../monolog/monolog"}}, + {{"name":"relative/evil","version":"1.0.0","install-path":"../../../evil/pkg"}}, + {{"name":"absolute/evil","version":"1.0.0","install-path":"{}"}} + ]}}"#, + outside.display() + ), + ) + .await + .unwrap(); + + let crawler = ComposerCrawler; + let packages = crawler.crawl_all(&options_at(&root)).await; + assert_eq!( + packages.len(), + 1, + "only the in-project package may survive; got {:?}", + packages.iter().map(|p| &p.path).collect::>() + ); + assert_eq!(packages[0].purl, ORG_PURL); + + let found = crawler + .find_by_purls( + &vendor, + &[ + "pkg:composer/relative/evil@1.0.0".to_string(), + "pkg:composer/absolute/evil@1.0.0".to_string(), + ], + ) + .await + .unwrap(); + assert!( + found.is_empty(), + "install-path escaped the project root: {:?}", + found.values().map(|p| &p.path).collect::>() + ); +} + +/// A rejected `install-path` must NOT fall back to `vendor//`: +/// installed.json says the package lives elsewhere, so patching whatever +/// happens to sit at the conventional path would edit the wrong tree. +#[tokio::test] +#[serial_test::parallel] +async fn rejected_install_path_does_not_fall_back_to_conventional_dir() { + let outer = tempfile::tempdir().unwrap(); + let root = outer.path().join("proj"); + let vendor = root.join("vendor"); + let composer_dir = vendor.join("composer"); + tokio::fs::create_dir_all(&composer_dir).await.unwrap(); + tokio::fs::write(root.join("composer.json"), b"{}") + .await + .unwrap(); + // The conventional directory exists and would otherwise be accepted. + tokio::fs::create_dir_all(vendor.join("monolog").join("monolog")) + .await + .unwrap(); + tokio::fs::create_dir_all(outer.path().join("elsewhere")) + .await + .unwrap(); + tokio::fs::write( + composer_dir.join("installed.json"), + br#"{"packages":[{"name":"monolog/monolog","version":"3.5.0","install-path":"../../../elsewhere"}]}"#, + ) + .await + .unwrap(); + + let crawler = ComposerCrawler; + assert!( + crawler.crawl_all(&options_at(&root)).await.is_empty(), + "a rejected install-path must not resolve to vendor/monolog/monolog" + ); + assert!(crawler + .find_by_purls(&vendor, &[ORG_PURL.to_string()]) + .await + .unwrap() + .is_empty()); +} + +// ── version normalization parity with the lockfile inventory ──── + +/// `V1.2.3` is a legal Composer tag. The crawler strips `v` AND `V` from +/// installed.json versions, so if the lockfile inventory strips only the +/// lowercase form the same package yields TWO different PURLs — one +/// installed `@1.2.3` row plus a phantom lockfile-only `@V1.2.3` row, both +/// POSTed to the API and both shown to the user. Pin the two normalizations +/// to the same output. +#[tokio::test] +#[serial_test::parallel] +async fn uppercase_v_version_normalizes_identically_in_crawl_and_lock_inventory() { + let tmp = tempfile::tempdir().unwrap(); + let vendor = tmp.path().join("vendor"); + let composer_dir = vendor.join("composer"); + tokio::fs::create_dir_all(&composer_dir).await.unwrap(); + tokio::fs::create_dir_all(vendor.join("monolog").join("monolog")) + .await + .unwrap(); + tokio::fs::write( + composer_dir.join("installed.json"), + br#"{"packages":[{"name":"monolog/monolog","version":"V3.5.0","install-path":"../monolog/monolog"}]}"#, + ) + .await + .unwrap(); + tokio::fs::write(tmp.path().join("composer.json"), b"{}") + .await + .unwrap(); + tokio::fs::write( + tmp.path().join("composer.lock"), + br#"{"packages":[{"name":"monolog/monolog","version":"V3.5.0","dist":{"type":"zip","url":"https://example.com/m.zip","shasum":""}}]}"#, + ) + .await + .unwrap(); + + let crawled = ComposerCrawler.crawl_all(&options_at(tmp.path())).await; + assert_eq!(crawled.len(), 1, "got {crawled:?}"); + assert_eq!(crawled[0].purl, ORG_PURL); + + let inventoried = + socket_patch_core::vendor::lock_inventory::inventory_project(tmp.path()).await; + let composer_rows: Vec<_> = inventoried + .iter() + .filter(|e| e.ecosystem == "composer") + .collect(); + assert_eq!(composer_rows.len(), 1, "got {composer_rows:?}"); + assert_eq!( + composer_rows[0].purl, crawled[0].purl, + "lockfile and installed rows must normalize to ONE purl, else the \ + package double-counts as installed + lockfile-only" + ); +} From f13889cd12f3de247619839434138203216b9f55 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 14 Aug 2026 09:28:06 -0700 Subject: [PATCH 3/6] fix(setup): keep composer.json bytes, mode, and matrix honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `setup` re-serialized composer-authored manifests from their native 4-space indent (PHP `JSON_PRETTY_PRINT`) to 2-space, so it produced a whole-file diff and `--remove` could never restore the original bytes. It now detects and keeps the file's own indentation and trailing-newline shape via the shared detect_indent/serialize_json helpers; a composer-written manifest round-trips byte-identically. The write also went through the non-mode-preserving atomic writer — the only manifest editor that did — resetting e.g. 0744 to 0644. Switched to `atomic_write_bytes_preserving_mode` like every sibling editor; the in-tree RED test documenting the bug is un-ignored and passing. The setup-matrix composer case never staged a composer.json before running `setup`, so it reported `no_files` and the case was deterministically red under `--features setup-e2e` (and a silent 0-test green without it), mislabeling working functionality as a baseline gap. The driver now stages a minimal manifest like the npm arms, and `baseline_supported` flips to true so a future failure reads REGRESSION instead of BASELINE GAP. Co-authored-by: Claude Fable 5 --- .../tests/setup_matrix_composer.rs | 7 +- .../src/setup/composer/mod.rs | 123 +++++++++++++++--- tests/setup_matrix/matrix.json | 2 +- tests/setup_matrix/run-case.sh | 18 ++- 4 files changed, 129 insertions(+), 21 deletions(-) diff --git a/crates/socket-patch-cli/tests/setup_matrix_composer.rs b/crates/socket-patch-cli/tests/setup_matrix_composer.rs index c6eeabb3..79aa2257 100644 --- a/crates/socket-patch-cli/tests/setup_matrix_composer.rs +++ b/crates/socket-patch-cli/tests/setup_matrix_composer.rs @@ -48,8 +48,11 @@ mod host_guard { /// A realistic composer-only project: a PHP manifest requiring the /// same package the matrix targets, and nothing the npm/Python/Cargo - /// detectors would recognise. - const COMPOSER_JSON: &str = "{\n \"name\": \"acme/widget\",\n \"require\": {\n \"monolog/monolog\": \"3.5.0\"\n }\n}\n"; + /// detectors would recognise. Indented with 4 spaces, the way composer + /// itself writes the file (PHP `JSON_PRETTY_PRINT`) — so the + /// byte-for-byte restore below also pins that `setup` does not reformat + /// a composer-authored manifest to serde's 2-space default. + const COMPOSER_JSON: &str = "{\n \"name\": \"acme/widget\",\n \"require\": {\n \"monolog/monolog\": \"3.5.0\"\n }\n}\n"; /// Run the CLI with `args` in `cwd`; returns `(exit_code, stdout, stderr)`. /// Delegates to the shared `common::run_with_env`, which seeds-then-scrubs diff --git a/crates/socket-patch-core/src/setup/composer/mod.rs b/crates/socket-patch-core/src/setup/composer/mod.rs index 3e2f8d69..991eefae 100644 --- a/crates/socket-patch-core/src/setup/composer/mod.rs +++ b/crates/socket-patch-core/src/setup/composer/mod.rs @@ -11,16 +11,18 @@ //! //! `composer.json` is JSON, so — like the npm `package_json` backend — edits go //! through `serde_json` (with the workspace's `preserve_order` feature, so the -//! user's key order survives) and are written back with -//! `to_string_pretty(..) + "\n"`. The contract mirrors the other backends: -//! idempotent, `dry_run`-aware, `Updated`/`AlreadyConfigured`/`Error`, and a -//! `--remove` that strips exactly what `setup` added. +//! user's key order survives) and are written back in the file's own +//! formatting (see [`serialize_like_input`]). The contract mirrors the other +//! backends: idempotent, `dry_run`-aware, `Updated`/`AlreadyConfigured`/ +//! `Error`, and a `--remove` that strips exactly what `setup` added. use std::path::{Path, PathBuf}; use serde_json::{Map, Value}; use tokio::fs; +use crate::vendor::common::{detect_indent, serialize_json}; + /// The command `setup` appends to each composer script event. The socket-patch /// CLI is invoked from `PATH` (composer has no `npx`-style fetch), offline (the /// patches are committed under `.socket/`) and silent (so it doesn't clutter @@ -113,6 +115,29 @@ fn parse_checked(content: &str) -> Result { Ok(doc) } +/// Re-serialize the edited document in the formatting the file already used. +/// +/// Composer writes `composer.json` through PHP's `JSON_PRETTY_PRINT`, which +/// indents with 4 spaces, while serde's `to_string_pretty` is hard-wired to 2 — +/// so re-serializing turned a two-key edit into a whole-file diff and left +/// `--remove` unable to restore the original bytes. `detect_indent` / +/// `serialize_json` are the same helpers the vendor backends use when they +/// rewrite composer.json and the lockfiles. A file saved without a trailing +/// newline keeps that too, since `serialize_json` always appends one. +fn serialize_like_input(doc: &Value, original: &str) -> String { + let indent = detect_indent(original); + let mut text = match serialize_json(doc, &indent) { + // Always valid UTF-8: serde_json emits escaped ASCII/UTF-8 only. + Ok(bytes) => String::from_utf8_lossy(&bytes).into_owned(), + // Serializing a `Value` cannot fail; fall back to the 2-space form. + Err(_) => serde_json::to_string_pretty(doc).unwrap_or_default() + "\n", + }; + if !original.ends_with('\n') { + text.pop(); + } + text +} + /// Append [`APPLY_COMMAND`] to both hook events, normalising each to an array. /// `None` if already present in every event (idempotent no-op). fn composer_add(content: &str) -> Result, String> { @@ -141,7 +166,7 @@ fn composer_add(content: &str) -> Result, String> { } return Ok(None); } - Ok(Some(serde_json::to_string_pretty(&doc).unwrap() + "\n")) + Ok(Some(serialize_like_input(&doc, content))) } /// Strip [`APPLY_COMMAND`] from both hook events, pruning emptied events and an @@ -167,7 +192,7 @@ fn composer_remove(content: &str) -> Result, String> { // would teleport the last root key into this slot. root.shift_remove("scripts"); } - Ok(Some(serde_json::to_string_pretty(&doc).unwrap() + "\n")) + Ok(Some(serialize_like_input(&doc, content))) } /// Add [`APPLY_COMMAND`] to one event, normalising string → array. Returns @@ -257,12 +282,17 @@ async fn edit( None => Ok(false), Some(new) => { if !dry_run { - // The crate-wide atomic writer (stage+fsync+rename): the - // user's committed composer.json must never be left torn - // by a crash mid-write. - crate::utils::fs::atomic_write_bytes(composer_json, new.as_bytes()) - .await - .map_err(|e| e.to_string())?; + // Atomic (stage+fsync+rename), so the user's committed + // composer.json is never left torn by a crash mid-write, + // and mode-preserving, because the rename swaps in a fresh + // inode that would otherwise take umask defaults — the + // same writer every sibling manifest editor uses. + crate::utils::fs::atomic_write_bytes_preserving_mode( + composer_json, + new.as_bytes(), + ) + .await + .map_err(|e| e.to_string())?; } Ok(true) } @@ -382,6 +412,70 @@ mod tests { assert_eq!(removed, BASIC, "add→remove restores the original bytes"); } + /// Composer writes `composer.json` with PHP's `JSON_PRETTY_PRINT`, i.e. + /// 4-space indent — the shape virtually every real-world manifest has. + const COMPOSER_AUTHORED: &str = + "{\n \"name\": \"acme/app\",\n \"require\": {\n \"php\": \">=8.1\"\n }\n}\n"; + + #[test] + fn test_add_preserves_composer_four_space_indent() { + // Regression: re-serializing at serde's fixed 2-space indent reformatted + // a composer-authored manifest top to bottom, turning a one-key edit + // into a whole-file diff. + let out = composer_add(COMPOSER_AUTHORED).unwrap().unwrap(); + assert!( + out.contains("\n \"name\": \"acme/app\","), + "depth-1 indent not preserved:\n{out}" + ); + assert!( + out.contains("\n \"php\": \">=8.1\""), + "depth-2 indent not preserved:\n{out}" + ); + assert!( + out.contains("\n \"scripts\": {"), + "our own added key must use the file's indent:\n{out}" + ); + assert!(is_hook_present(&out)); + } + + #[test] + fn test_round_trip_restores_composer_authored_manifest() { + // The whole point of preserving the indent: `--remove` must give the + // user back the exact bytes composer wrote, not a reformatted file. + let added = composer_add(COMPOSER_AUTHORED).unwrap().unwrap(); + let removed = composer_remove(&added).unwrap().unwrap(); + assert_eq!( + removed, COMPOSER_AUTHORED, + "add→remove must restore composer's own formatting" + ); + } + + #[test] + fn test_round_trip_preserves_tab_indent() { + let inp = + "{\n\t\"name\": \"acme/app\",\n\t\"require\": {\n\t\t\"php\": \">=8.1\"\n\t}\n}\n"; + let added = composer_add(inp).unwrap().unwrap(); + assert!( + added.contains("\n\t\"scripts\": {"), + "tab indent not preserved:\n{added}" + ); + assert_eq!(composer_remove(&added).unwrap().unwrap(), inp); + } + + #[test] + fn test_round_trip_preserves_absent_trailing_newline() { + // `serialize_json` always appends a trailing newline, so a manifest + // saved without one would gain a phantom last-line diff that `--remove` + // could never take back. + let inp = COMPOSER_AUTHORED.trim_end_matches('\n'); + let added = composer_add(inp).unwrap().unwrap(); + assert!( + !added.ends_with('\n'), + "must not add a trailing newline the file did not have:\n{added:?}" + ); + assert_eq!(composer_remove(&added).unwrap().unwrap(), inp); + } + #[test] fn test_user_string_event_already_ours_is_noop() { // An event whose string value is exactly our command counts as present. @@ -606,11 +700,6 @@ mod tests { #[cfg(unix)] #[tokio::test] - #[ignore = "RED: `edit()` uses the plain `atomic_write_bytes`, so the stage \ - inode is created with umask defaults and the rename resets the \ - user's composer.json mode (0o744 -> 0o644). Every sibling manifest \ - editor uses `atomic_write_bytes_preserving_mode`; switching this \ - call over is the one-line fix, which was not part of this change."] async fn test_edit_preserves_manifest_permissions() { use std::os::unix::fs::PermissionsExt; // Regression: `composer.json` is a file the *user* owns and we merely diff --git a/tests/setup_matrix/matrix.json b/tests/setup_matrix/matrix.json index c5b4c308..f7ddeae3 100644 --- a/tests/setup_matrix/matrix.json +++ b/tests/setup_matrix/matrix.json @@ -179,7 +179,7 @@ { "ecosystem": "composer", "pm": "composer", "image": "composer", "hook_family": "composer-event", - "baseline_supported": false, + "baseline_supported": true, "package": "monolog/monolog", "version": "3.5.0", "purl": "pkg:composer/monolog/monolog@3.5.0", "manifest_key": "package/src/Monolog/Logger.php", "apply_ecosystems": "composer" }, diff --git a/tests/setup_matrix/run-case.sh b/tests/setup_matrix/run-case.sh index c399b675..15a34991 100755 --- a/tests/setup_matrix/run-case.sh +++ b/tests/setup_matrix/run-case.sh @@ -306,7 +306,23 @@ EOF ;; go) printf 'module sm-proj\n\ngo 1.21\n' > go.mod ;; - mvn|composer|dotnet) : ;; + composer) + # `setup` wires its hook into composer.json's script events, so the + # manifest must exist BEFORE setup runs — without it setup reports + # `no_files`, the hook is never written, and `composer require` below + # can't re-apply the patch. The dependency is left to `composer require` + # (same division of labour as npm/yarn/bun above). 4-space indent is + # what composer itself writes, so a reformat by setup would show up as + # a diff here too. + cat > composer.json < Date: Fri, 14 Aug 2026 09:28:21 -0700 Subject: [PATCH 4/6] fix(vendor): make composer revert safe when wiring is lost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverting a repair-reconstructed composer entry (wiring lost, ledger rebuilt from the wired lock) reported success while deleting the vendored artifacts and leaving composer.lock pointing at the deleted paths — the next install failed "Source path ... is not found" with no guidance. The pristine dist survives only in `wiring[].original`, which such an entry does not have, so synthesizing a restore would mean guessing; revert now refuses the entry instead, keeps the artifacts and the ledger row, and names the escape hatch (`composer update --no-install `, then revert again). Dry and wet runs agree. The GC orphan sweep deleted un-ledgered uuid dirs under `.socket/vendor` on the false premise that no ledger means no live wiring — with state.json lost, dirs still referenced by a lockfile were swept and fresh installs broke. Candidates are now cross-checked against `scan_vendor_references` (the same all-ecosystem scan repair reconstructs from); referenced dirs are kept and surfaced as `vendor_orphan_still_wired`, and reclaimed counts no longer include them. The docker vendor capstone's networked `composer update` gains the sibling suite's 3-attempt retry — that fetch is the documented dominant flake. Co-authored-by: Claude Fable 5 --- .../socket-patch-cli/src/commands/vendor.rs | 100 ++++++-- .../tests/docker_e2e_vendor_composer.rs | 11 +- .../tests/e2e_vendor_composer_build.rs | 233 ++++++++++++++++++ .../src/vendor/composer_lock.rs | 91 +++++++ 4 files changed, 408 insertions(+), 27 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/vendor.rs b/crates/socket-patch-cli/src/commands/vendor.rs index e95638ad..a18ecf59 100644 --- a/crates/socket-patch-cli/src/commands/vendor.rs +++ b/crates/socket-patch-cli/src/commands/vendor.rs @@ -198,33 +198,68 @@ async fn dispatch_in_use_one(entry: &VendorEntry, project_root: &Path) -> Option } } +/// What the orphan sweep did with the uuid dirs no ledger entry owns. +#[derive(Default)] +struct OrphanSweep { + /// Un-ledgered AND unreferenced — deleted (unless `dry_run`). + removed: Vec, + /// Un-ledgered but a project lockfile still points into them — kept. + still_wired: Vec, +} + /// Uuid dirs under `.socket/vendor//` with no owning `(eco, uuid)` /// ledger entry (a hand-edited state file, or artifacts left by an -/// interrupted run). The lockfile wiring for these is already gone or -/// owned by a recorded entry, so removal is safe; removed unless -/// `dry_run`. Unparseable dirs are never returned (and never deleted). -/// Returns the orphans so callers can emit events / counts. -async fn sweep_orphan_vendor_dirs( - cwd: &Path, - state: &VendorState, - dry_run: bool, -) -> Vec { +/// interrupted run). Unparseable dirs are never returned (and never +/// deleted). Returns the orphans so callers can emit events / counts. +/// +/// A missing ledger entry does NOT prove missing wiring: `repair` +/// reconstructs entries from lockfiles that still point into +/// `.socket/vendor/` precisely because that state occurs (a deleted +/// state.json, a partial commit). Deleting such a dir would break the next +/// install, so every candidate is checked against the wiring-bearing files +/// first — the same lockfile scan `repair` reconstructs from — and a +/// referenced dir is kept for the caller to warn about. +async fn sweep_orphan_vendor_dirs(cwd: &Path, state: &VendorState, dry_run: bool) -> OrphanSweep { let recorded_units: HashSet<(&str, &str)> = state .entries .values() .map(|e| (e.ecosystem.as_str(), e.uuid.as_str())) .collect(); - let mut orphans = Vec::new(); - for unit in vendor::path::sweep_vendor_dirs(cwd).await { - if recorded_units.contains(&(unit.eco.as_str(), unit.uuid.as_str())) { + let candidates: Vec = vendor::path::sweep_vendor_dirs(cwd) + .await + .into_iter() + .filter(|unit| !recorded_units.contains(&(unit.eco.as_str(), unit.uuid.as_str()))) + .collect(); + let mut out = OrphanSweep::default(); + if candidates.is_empty() { + return out; + } + let wired: HashSet<(String, String)> = + crate::commands::repair_vendor::scan_vendor_references(cwd) + .await + .into_iter() + .map(|(eco, uuid, _path)| (eco, uuid)) + .collect(); + for unit in candidates { + if wired.contains(&(unit.eco.clone(), unit.uuid.clone())) { + out.still_wired.push(unit); continue; } if !dry_run { let _ = remove_tree(&unit.dir).await; } - orphans.push(unit); + out.removed.push(unit); } - orphans + out +} + +/// How an orphan uuid dir is named in events: the PURL recovered from its +/// leaf when the layout is recognizable, else `/`. +fn orphan_label(unit: &vendor::path::SweptVendorDir) -> String { + unit.purls + .first() + .cloned() + .unwrap_or_else(|| format!("{}/{}", unit.eco, unit.uuid)) } /// Does `eco` fall inside this run's `--ecosystems` scope? @@ -1221,17 +1256,30 @@ async fn run_revert(args: &VendorArgs, env: &mut Envelope) -> i32 { } // Orphan sweep: uuid dirs on disk with no ledger entry (a hand-edited - // state file, or artifacts left by an interrupted run). The lockfile - // wiring for these is already gone or owned by a recorded entry, so - // removal is safe; unparseable dirs are reported, never deleted. - for unit in sweep_orphan_vendor_dirs(&common.cwd, &state, common.dry_run).await { - let label = unit - .purls - .first() - .cloned() - .unwrap_or_else(|| format!("{}/{}", unit.eco, unit.uuid)); + // state file, or artifacts left by an interrupted run). Unparseable dirs + // are reported, never deleted — and neither are dirs a lockfile still + // points at (their wiring outlived the ledger). + let sweep = sweep_orphan_vendor_dirs(&common.cwd, &state, common.dry_run).await; + for unit in &sweep.still_wired { + let label = orphan_label(unit); + record_warning( + env, + &label, + &VendorWarning::new( + "vendor_orphan_still_wired", + format!( + "a project lockfile still points at .socket/vendor/{}/{}, which no ledger \ + entry owns; the artifacts were kept (run `socket-patch repair` to re-adopt \ + them into the ledger, then revert again)", + unit.eco, unit.uuid + ), + ), + common, + ); + } + for unit in &sweep.removed { env.record( - PatchEvent::new(PatchAction::Removed, label) + PatchEvent::new(PatchAction::Removed, orphan_label(unit)) .with_reason("vendor_orphan_removed", "vendored dir had no ledger entry"), ); } @@ -1412,9 +1460,11 @@ pub(crate) async fn run_vendor_gc( } } - // (c) orphan uuid dirs, against the post-removal ledger. + // (c) orphan uuid dirs, against the post-removal ledger. Dirs a lockfile + // still points at are kept, so they are not counted as reclaimed. out.orphan_dirs = sweep_orphan_vendor_dirs(&common.cwd, &state, dry_run) .await + .removed .len(); out } diff --git a/crates/socket-patch-cli/tests/docker_e2e_vendor_composer.rs b/crates/socket-patch-cli/tests/docker_e2e_vendor_composer.rs index 78fcfa00..fd344306 100644 --- a/crates/socket-patch-cli/tests/docker_e2e_vendor_composer.rs +++ b/crates/socket-patch-cli/tests/docker_e2e_vendor_composer.rs @@ -84,8 +84,15 @@ cat > composer.json <<'EOF' EOF # 1. REAL fixture: composer update resolves + installs psr/log from packagist. -composer update --no-interaction > /tmp/install.log 2>&1 || { - cat /tmp/install.log >&2; fail "composer update (fixture install) failed"; } +# psr/log arrives over the real network (packagist metadata + the GitHub +# zipball); transient stream/connection errors are the dominant flake in this +# suite — retry with backoff before declaring the fixture broken. +for attempt in 1 2 3; do + composer update --no-interaction > /tmp/install.log 2>&1 && break + if [ "$attempt" = 3 ]; then cat /tmp/install.log >&2; fail "composer update (fixture install) failed"; fi + echo "composer update attempt $attempt failed; retrying" >&2 + sleep $((attempt * 5)) +done PSR_VER=$(php -r ' $l = json_decode(file_get_contents("composer.lock"), true); diff --git a/crates/socket-patch-cli/tests/e2e_vendor_composer_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_composer_build.rs index 2840a5d2..85e5a288 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_composer_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_composer_build.rs @@ -47,6 +47,8 @@ const GHSA: &str = "GHSA-vend-composer-host"; /// The dependency under test — dep-free, tiny, and the same fixture the /// docker twin uses. const DEP: &str = "psr/log"; +/// Version the hand-written (composer-free) revert fixtures below pin. +const FIXTURE_VERSION: &str = "3.0.2"; // ── self-contained helpers ──────────────────────────────────────────── @@ -476,3 +478,234 @@ fn composer_vendor_fresh_checkout_install_and_revert() { ".socket/vendor must be fully removed after revert" ); } + +// ── revert against ledger state the capstone above never produces ───── +// +// Both regressions below are about `.socket/vendor/` state that outlived (or +// was rebuilt without) its wiring record, which the capstone's clean +// vendor→revert round trip cannot reach. They hand-write the wired lock + +// artifact instead of driving composer, so they need neither the toolchain +// nor the network and run in the normal `test` job (no `#[ignore]`). + +/// `repair`-reconstructed ledger entry: recovered from the lockfile path, so +/// it owns the artifact but records NO pre-vendor wiring (see +/// `repair_vendor.rs`'s `synth_entry`). +const UUID_RECONSTRUCTED: &str = "1a2b3c4d-5e6f-4a7b-8c9d-0e1f2a3b4c5d"; +/// Un-ledgered artifact dir that composer.lock still points at. +const UUID_ORPHAN_WIRED: &str = "2b3c4d5e-6f7a-4b8c-9d0e-1f2a3b4c5d6e"; +/// Un-ledgered artifact dir nothing references. +const UUID_ORPHAN_DEAD: &str = "3c4d5e6f-7a8b-4c9d-8e0f-2a3b4c5d6e7f"; + +const FIXTURE_PHP: &[u8] = + b" String { + let copy_rel = format!(".socket/vendor/composer/{uuid}/{DEP}@{FIXTURE_VERSION}"); + let src = proj.join(©_rel).join("src"); + std::fs::create_dir_all(&src).unwrap(); + std::fs::write(src.join("LoggerInterface.php"), FIXTURE_PHP).unwrap(); + copy_rel +} + +/// composer.json + a composer.lock ALREADY wired to `uuid`'s copy — the +/// exact surgery `vendor` writes (path dist, uuid `reference`, `symlink: +/// false`, `source` gone) and what a fresh clone of a vendored project has. +fn write_wired_project(proj: &Path, uuid: &str) -> String { + let copy_rel = write_vendored_copy(proj, uuid); + std::fs::write( + proj.join("composer.json"), + r#"{ + "name": "socket/vendor-revert-fixture", + "require": { + "psr/log": "3.0.*" + } +} +"#, + ) + .unwrap(); + let lock = serde_json::json!({ + "_readme": ["This file locks the dependencies of your project to a known state"], + "content-hash": "7a59d114f58e9b02546b21d7e57430d3", + "packages": [{ + "name": DEP, + "version": FIXTURE_VERSION, + "dist": { "type": "path", "url": copy_rel, "reference": uuid }, + "transport-options": { "symlink": false }, + "type": "library", + }], + "packages-dev": [], + "minimum-stability": "stable", + "plugin-api-version": "2.6.0", + }); + std::fs::write( + proj.join("composer.lock"), + format!("{}\n", serde_json::to_string_pretty(&lock).unwrap()), + ) + .unwrap(); + copy_rel +} + +/// The `.socket/vendor/state.json` a `repair` reconstruction leaves: artifact +/// + uuid recovered from the lock path, `wiring` empty. +fn write_reconstructed_ledger(proj: &Path, uuid: &str, copy_rel: &str) { + let purl = format!("pkg:composer/{DEP}@{FIXTURE_VERSION}"); + let state = serde_json::json!({ + "version": 1, + "entries": { purl.clone(): { + "ecosystem": "composer", + "basePurl": purl, + "uuid": uuid, + "artifact": { "path": copy_rel }, + "wiring": [], + }} + }); + std::fs::write( + proj.join(".socket/vendor/state.json"), + serde_json::to_string_pretty(&state).unwrap(), + ) + .unwrap(); +} + +fn events(env: &serde_json::Value) -> &Vec { + env["events"].as_array().expect("events[]") +} + +/// REGRESSION: reverting a `repair`-reconstructed entry must not strand +/// composer.lock. There is no recorded registry `dist` to put back, so the +/// revert has to REFUSE and keep the artifacts — deleting them while the lock +/// still points at them made the next `composer install` fail with "Source +/// path … is not found", and the run reported success. +#[test] +fn revert_of_reconstructed_entry_refuses_and_keeps_artifacts() { + let tmp = tempfile::tempdir().unwrap(); + let proj = tmp.path().join("proj"); + std::fs::create_dir_all(&proj).unwrap(); + let copy_rel = write_wired_project(&proj, UUID_RECONSTRUCTED); + write_reconstructed_ledger(&proj, UUID_RECONSTRUCTED, ©_rel); + + let lock_path = proj.join("composer.lock"); + let lock_before = std::fs::read(&lock_path).unwrap(); + let state_before = std::fs::read(proj.join(".socket/vendor/state.json")).unwrap(); + + let (code, stdout, stderr) = run_socket( + &proj, + &[ + "vendor", + "--revert", + "--json", + "--cwd", + proj.to_str().unwrap(), + ], + ); + assert_eq!( + code, 1, + "an unrestorable entry must fail the revert.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let env = parse_envelope(&stdout); + assert_eq!(env["status"], "partialFailure", "envelope: {env}"); + assert_eq!(env["summary"]["removed"], 0, "nothing reverted: {env}"); + let failed = events(&env) + .iter() + .find(|e| e["action"] == "failed") + .unwrap_or_else(|| panic!("expected a failed event: {env}")); + assert_eq!(failed["errorCode"], "revert_failed", "{failed}"); + let detail = failed["error"].as_str().expect("error detail"); + assert!( + detail.contains(DEP) && detail.contains("composer update"), + "the refusal must name the package and the re-resolve escape hatch: {detail}" + ); + + assert!( + proj.join(©_rel) + .join("src/LoggerInterface.php") + .exists(), + "a refused revert must NOT delete the artifacts the lock still consumes" + ); + assert_eq!( + std::fs::read(&lock_path).unwrap(), + lock_before, + "composer.lock must be left exactly as it was" + ); + assert_eq!( + std::fs::read(proj.join(".socket/vendor/state.json")).unwrap(), + state_before, + "the entry must stay in the ledger so a later repair/revert can retry" + ); +} + +/// REGRESSION: with state.json gone, the orphan sweep must not delete a uuid +/// dir composer.lock still points at — un-ledgered does not mean un-wired +/// (that is exactly the state `repair` reconstructs from). A genuinely +/// unreferenced dir in the same run must still be swept. +#[test] +fn orphan_sweep_keeps_lock_referenced_dir_when_ledger_is_gone() { + let tmp = tempfile::tempdir().unwrap(); + let proj = tmp.path().join("proj"); + std::fs::create_dir_all(&proj).unwrap(); + let wired_rel = write_wired_project(&proj, UUID_ORPHAN_WIRED); + let dead_rel = write_vendored_copy(&proj, UUID_ORPHAN_DEAD); + assert!( + !proj.join(".socket/vendor/state.json").exists(), + "fixture models a project whose ledger was deleted" + ); + + let lock_before = std::fs::read(proj.join("composer.lock")).unwrap(); + let (code, stdout, stderr) = run_socket( + &proj, + &[ + "vendor", + "--revert", + "--json", + "--cwd", + proj.to_str().unwrap(), + ], + ); + assert_eq!( + code, 0, + "sweeping is not a failure.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let env = parse_envelope(&stdout); + + assert!( + proj.join(&wired_rel) + .join("src/LoggerInterface.php") + .exists(), + "the lock-referenced artifact must survive the sweep: {env}" + ); + assert!( + !proj + .join(format!(".socket/vendor/composer/{UUID_ORPHAN_DEAD}")) + .exists(), + "the unreferenced orphan must still be swept: {env}" + ); + assert_eq!( + std::fs::read(proj.join("composer.lock")).unwrap(), + lock_before, + "the sweep must not touch composer.lock" + ); + assert!( + events(&env) + .iter() + .any(|e| e["errorCode"] == "vendor_orphan_still_wired" + && e["reason"] + .as_str() + .is_some_and(|r| r.contains(UUID_ORPHAN_WIRED))), + "the kept dir must be surfaced as an advisory: {env}" + ); + assert!( + events(&env).iter().any(|e| e["action"] == "removed" + && e["errorCode"] == "vendor_orphan_removed" + && e["purl"] + .as_str() + .is_some_and(|p| p.contains(&format!("{DEP}@{FIXTURE_VERSION}")) + || p.contains(UUID_ORPHAN_DEAD))), + "the swept dir must be reported: {env}" + ); + assert_eq!( + dead_rel, + format!(".socket/vendor/composer/{UUID_ORPHAN_DEAD}/{DEP}@{FIXTURE_VERSION}"), + "fixture path convention" + ); +} diff --git a/crates/socket-patch-core/src/vendor/composer_lock.rs b/crates/socket-patch-core/src/vendor/composer_lock.rs index 0f7b393f..969e5444 100644 --- a/crates/socket-patch-core/src/vendor/composer_lock.rs +++ b/crates/socket-patch-core/src/vendor/composer_lock.rs @@ -27,6 +27,7 @@ //! (`JSON_PRETTY_PRINT`) + trailing newline; serde_json does not escape `/` //! (matching `JSON_UNESCAPED_SLASHES`). +use std::collections::HashSet; use std::path::Path; use serde_json::{json, Map, Value}; @@ -357,6 +358,15 @@ pub async fn vendor_composer( /// update`, a hand edit, or a newer vendor run — is left alone with a /// `vendor_lock_entry_drifted` warning. /// +/// Refused fail-closed when composer.lock still wires a package to our uuid +/// dir that NO wiring record can restore (a `repair`-reconstructed entry +/// carries no pre-vendor fragment): the registry `dist` the surgery replaced +/// exists nowhere else — not in the lock (we overwrote it), not in the +/// artifact — so an un-rewrite is impossible offline, and deleting the +/// artifacts anyway would leave the lock pointing at a gone path (`composer +/// install` then dies with "Source path … is not found"). Nothing is deleted +/// in that case; the error names the re-resolve escape hatch. +/// /// Note: the *installed* `vendor//` keeps the patched bytes until the /// next `composer install` re-mirrors from the registry; revert surfaces that /// as the `vendor_installed_copy_stale` advisory. @@ -378,6 +388,24 @@ pub async fn revert_composer( let lock_path = project_root.join(COMPOSER_LOCK); let mut warnings = Vec::new(); + // Nothing may be deleted while composer.lock still consumes it. Checked + // BEFORE the restore loop (and before any write) so the answer is the + // same for `--dry-run` and a wet run. + let stranded = stranded_wired_packages(&lock_path, &entry.uuid, &restorable_keys(entry)).await; + if !stranded.is_empty() { + let listed = stranded.join(", "); + let args = stranded.join(" "); + return RevertOutcome::failed(format!( + "refusing revert: composer.lock still points {listed} at {uuid_dir_rel}, but the \ + ledger entry records no pre-vendor lock fragment to restore (an entry \ + reconstructed by `socket-patch repair` recovers the artifact, never the \ + registry dist the surgery replaced). The vendored artifacts were LEFT IN \ + PLACE so the project still installs. To undo the vendoring, re-resolve the \ + package from the registry first (`composer update --no-install {args}`), then \ + re-run `socket-patch vendor --revert`" + )); + } + // Wiring is restored in reverse application order (one record today). for w in entry.wiring.iter().rev() { if w.kind != WIRING_KIND { @@ -679,6 +707,69 @@ fn composer_json_bytes(value: &Value) -> std::io::Result> { serialize_json(value, " ") } +/// The `
:` keys this entry can actually put back: +/// a recognized wiring kind, a well-formed key, and a recorded `original`. +fn restorable_keys(entry: &VendorEntry) -> HashSet { + entry + .wiring + .iter() + .filter(|w| w.kind == WIRING_KIND && w.original.is_some()) + .filter_map(|w| w.key.as_deref()) + .filter_map(|k| k.split_once(':')) + .filter(|(section, _)| *section == "packages" || *section == "packages-dev") + .map(|(section, pkg)| format!("{section}:{}", pkg.to_lowercase())) + .collect() +} + +/// Lock packages still wired to `uuid` that `restorable` cannot un-rewrite — +/// the set that a delete-the-artifacts revert would strand. Names are +/// 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. +async fn stranded_wired_packages( + lock_path: &Path, + uuid: &str, + restorable: &HashSet, +) -> Vec { + let Ok(text) = tokio::fs::read_to_string(lock_path).await else { + return Vec::new(); + }; + let Ok(lock) = serde_json::from_str::(&text) else { + return Vec::new(); + }; + let mut out: Vec = Vec::new(); + for section in ["packages", "packages-dev"] { + let Some(arr) = lock.get(section).and_then(Value::as_array) else { + continue; + }; + for e in arr { + let wired_to_us = e + .get("dist") + .and_then(|d| d.get("url")) + .and_then(Value::as_str) + .and_then(parse_vendor_path) + .is_some_and(|p| p.eco == "composer" && p.uuid == uuid); + if !wired_to_us { + continue; + } + let Some(name) = e.get("name").and_then(Value::as_str) else { + continue; + }; + let name = name.to_lowercase(); + // Section-qualified: `restore_lock_entry` only searches the + // section the wiring recorded, so an entry that moved between + // packages[] and packages-dev[] is unrestorable too. + if !restorable.contains(&format!("{section}:{name}")) && !out.contains(&name) { + out.push(name); + } + } + } + out +} + /// Restore one `composer_lock_package` wiring record. `Ok(true)` = restored /// (or would be, on dry run); `Ok(false)` = drifted, left alone; `Err` = a /// real I/O / serialization failure. From 734a8e0918c70db8cb9b87702cd320b24c044fdf Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 14 Aug 2026 09:28:21 -0700 Subject: [PATCH 5/6] ci: actually run the e2e_composer suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The e2e_composer matrix job invoked its suite with `-- --ignored`, but e2e_composer.rs has no #[ignore] tests, so the job permanently passed while running zero tests. The shared run step now takes a per-entry `test_filter` and composer sets `--include-ignored` — the two real tests run, and a future #[ignore]-gated composer capstone still runs on this leg. Every other matrix entry renders the same command as before. Co-authored-by: Claude Fable 5 --- .github/workflows/ci.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e8bb12e7..425a77ee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -584,6 +584,12 @@ jobs: suite: e2e_maven - os: ubuntu-latest suite: e2e_composer + # composer is a shipped ecosystem, so e2e_composer's tests are + # NOT `#[ignore]`-gated the way the experimental-ecosystem suites + # (maven, nuget) are — the matrix default `--ignored` filter + # selected zero tests here and the leg passed vacuously. + # `--include-ignored` runs them, plus any capstone added later. + test_filter: --include-ignored - os: ubuntu-latest suite: e2e_nuget # Host vendor build-proof capstones: fresh-checkout install + @@ -741,7 +747,10 @@ jobs: tools: composer:2 - name: Run e2e tests - run: cargo test -p socket-patch-cli --all-features --test ${{ matrix.suite }} -- --ignored + # Suites are `#[ignore]`-gated out of the unpinned `test` job by + # default, hence `--ignored`; an entry that sets `test_filter` + # overrides the selector for itself only. + run: cargo test -p socket-patch-cli --all-features --test ${{ matrix.suite }} -- ${{ matrix.test_filter || '--ignored' }} # ---------------------------------------------------------------------- # Docker-driven real-package e2e suite. From 65ad6e831b7ce14ca2a13b6f1542b1145d462804 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 14 Aug 2026 10:06:07 -0700 Subject: [PATCH 6/6] fix(crawler): accept ./-style vendor-dir, fix Windows test JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Composer-legal `config.vendor-dir` values with `.` segments or backslash separators (`./vendor`, `./lib/deps`, `lib\deps`) were refused by the safety gate with no fallback, regressing those projects to zero discovery — worse than the old hardcoded `vendor/` (Bugbot review finding). The value is now reduced to plain `a/b` segments first (`.` drops, `..` resolves lexically and fails closed above or at the project root, absolute stays refused), then gated. The crawler e2e tests injected absolute paths into fixture JSON via `display()`, which on Windows embeds raw backslashes and makes the document unparseable — installed.json discovery collapsed to zero and install_path_escaping_project_root_is_rejected failed for the wrong reason (and the absolute vendor-dir refusal test passed vacuously). Both injections now JSON-escape backslashes. Co-authored-by: Claude Fable 5 --- .../src/crawlers/composer_crawler.rs | 61 +++++++++++++++++-- .../tests/crawler_composer_e2e.rs | 40 +++++++++++- 2 files changed, 95 insertions(+), 6 deletions(-) diff --git a/crates/socket-patch-core/src/crawlers/composer_crawler.rs b/crates/socket-patch-core/src/crawlers/composer_crawler.rs index 47815c63..f684b613 100644 --- a/crates/socket-patch-core/src/crawlers/composer_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/composer_crawler.rs @@ -353,14 +353,37 @@ async fn resolve_local_vendor_dir(cwd: &Path) -> Option { } match read_config_vendor_dir(&cwd.join("composer.json")).await { - Some(configured) if path_safety::is_safe_multi_segment(&configured) => { - Some(cwd.join(configured)) - } - Some(_) => None, + Some(configured) => normalize_config_vendor_dir(&configured) + .filter(|normalized| path_safety::is_safe_multi_segment(normalized)) + .map(|normalized| cwd.join(normalized)), None => Some(cwd.join("vendor")), } } +/// Reduce a `config.vendor-dir` value to plain `a/b` segments before the +/// safety gate. Composer accepts `./`-prefixed and `.`-interleaved values +/// (`./vendor`, `lib/./deps`) and either separator; refusing those shapes +/// outright regressed projects that previously resolved fine at the +/// hardcoded `vendor/`. `..` is resolved lexically the way Composer's own +/// path resolution does; a value that climbs above the project root (or +/// reduces to it) fails closed as `None`. +fn normalize_config_vendor_dir(raw: &str) -> Option { + if raw.starts_with(['/', '\\']) { + return None; + } + let mut segments: Vec<&str> = Vec::new(); + for segment in raw.split(['/', '\\']) { + match segment { + "" | "." => {} + ".." => { + segments.pop()?; + } + other => segments.push(other), + } + } + (!segments.is_empty()).then(|| segments.join("/")) +} + /// Read `config.vendor-dir` from a composer.json on disk. Opened with /// [`crate::utils::fs::open_regular_file`] for the same reason /// installed.json is: the manifest belongs to the untrusted project, and @@ -1332,6 +1355,36 @@ mod tests { assert_eq!(parse_config_vendor_dir("{ not json"), None); } + #[test] + fn test_normalize_config_vendor_dir() { + let n = normalize_config_vendor_dir; + // Composer-legal `./` prefixes and `.` segments reduce to the + // plain path; either separator is accepted. + assert_eq!(n("./vendor").as_deref(), Some("vendor")); + assert_eq!(n("./lib/deps").as_deref(), Some("lib/deps")); + assert_eq!(n("lib/./deps").as_deref(), Some("lib/deps")); + assert_eq!(n("lib\\deps").as_deref(), Some("lib/deps")); + assert_eq!(n("lib/../deps").as_deref(), Some("deps")); + assert_eq!(n("vendor").as_deref(), Some("vendor")); + // Escaping the project, reducing to it, or absolute — fail closed. + assert_eq!(n(".."), None); + assert_eq!(n("../elsewhere"), None); + assert_eq!(n("lib/../.."), None); + assert_eq!(n("."), None); + assert_eq!(n("a/.."), None); + assert_eq!(n("/etc/vendor"), None); + assert_eq!(n("\\\\share\\vendor"), None); + // A drive-letter segment survives normalization; the + // `is_safe_multi_segment` gate downstream rejects the colon. + assert_eq!( + n("C:\\Users\\x\\vendor").as_deref(), + Some("C:/Users/x/vendor") + ); + assert!(!crate::patch::path_safety::is_safe_multi_segment( + "C:/Users/x/vendor" + )); + } + #[test] fn test_normalize_lexically() { let n = |p: &str| normalize_lexically(Path::new(p)); diff --git a/crates/socket-patch-core/tests/crawler_composer_e2e.rs b/crates/socket-patch-core/tests/crawler_composer_e2e.rs index a5f29f41..0b64a9bf 100644 --- a/crates/socket-patch-core/tests/crawler_composer_e2e.rs +++ b/crates/socket-patch-core/tests/crawler_composer_e2e.rs @@ -773,6 +773,33 @@ async fn config_vendor_dir_relocates_discovery() { assert_eq!(packages[0].path, vendor.join("monolog").join("monolog")); } +/// Composer accepts `./`-prefixed vendor-dir values (`./lib/deps`); refusing +/// the `.` segment outright regressed such projects to zero discovery, worse +/// than the old hardcoded `vendor/`. The normalizer reduces the value before +/// the safety gate. +#[tokio::test] +#[serial_test::parallel] +async fn config_vendor_dir_dot_prefix_is_normalized() { + let tmp = tempfile::tempdir().unwrap(); + let vendor = stage_relocated_project(tmp.path(), "lib/deps", Some("./lib/deps")).await; + assert!(!tmp.path().join("vendor").exists()); + + let crawler = ComposerCrawler; + let paths = crawler + .get_vendor_paths(&options_at(tmp.path())) + .await + .unwrap(); + assert_eq!(paths, vec![vendor.clone()]); + + let packages = crawler.crawl_all(&options_at(tmp.path())).await; + assert_eq!( + packages.len(), + 1, + "./-prefixed vendor-dir must be crawled; got {packages:?}" + ); + assert_eq!(packages[0].purl, ORG_PURL); +} + /// A trailing separator is legal in `config.vendor-dir` (Composer rtrims it /// before use), so `"vendor-dir": "lib/deps/"` must resolve the same as /// `"lib/deps"` — a naive join would produce an empty final segment and @@ -915,7 +942,13 @@ async fn absolute_config_vendor_dir_is_refused() { let absolute = stage_relocated_project(outer.path(), "abs-vendor", None).await; tokio::fs::write( root.join("composer.json"), - format!(r#"{{"config":{{"vendor-dir":"{}"}}}}"#, absolute.display()), + // Backslashes JSON-escaped so the manifest stays parseable on + // Windows — an unparseable manifest would fall back to `vendor/` + // and pass this refusal test vacuously. + format!( + r#"{{"config":{{"vendor-dir":"{}"}}}}"#, + absolute.display().to_string().replace('\\', "\\\\") + ), ) .await .unwrap(); @@ -1026,7 +1059,10 @@ async fn install_path_escaping_project_root_is_rejected() { {{"name":"relative/evil","version":"1.0.0","install-path":"../../../evil/pkg"}}, {{"name":"absolute/evil","version":"1.0.0","install-path":"{}"}} ]}}"#, - outside.display() + // JSON-escape Windows path separators: raw backslashes made the + // whole installed.json unparseable there, so even the in-project + // package vanished and the test failed for the wrong reason. + outside.display().to_string().replace('\\', "\\\\") ), ) .await