From 1d7e1bf4961b6ec9cb547d03976eb447c6c9b05c Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 14:15:30 -0400 Subject: [PATCH] Refuse yarn classic pins with unlocked deps A patch that adds a dependency to the package's own package.json, or moves one to a new range, left yarn classic locks that yarn could not install reproducibly. Vendored mode recomputed the block's dependencies sub-map but added no block for the new descriptor, so online frozen installs fetched it unpinned, --offline installs failed and every yarn install re-saved the lock. Hosted mode never looked at the patched manifest, so yarn never installed the new dependency and the patched package crashed at runtime, while scan and vex reported success. Both writers now compare the patched package.json with the lock. Vendored mode refuses the patch before any wiring is written (vendor_dep_manifest_unlocked) when a descriptor has no block of its own. Hosted mode reads the served tarball (with the #558 sha1 fetch) for every entry it has not pinned yet, refuses the pin with redirect_yarn_classic_dep_manifest_unlocked, and rewrites the sub-maps when every descriptor is already locked. Each refusal names the descriptors and a remedy (lock them first, e.g. with yarn add). Fixes #591 Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/socket-patch-cli/CLI_CONTRACT.md | 5 +- .../src/commands/scan/hosted.rs | 45 ++-- .../tests/covgap_commands_scan_hosted.rs | 61 +++++- .../tests/e2e_redirect_yarn_classic_build.rs | 29 +++ .../tests/hosted_memory_parity.rs | 56 ++++- .../tests/in_process_redirect.rs | 61 +++++- .../tests/scan/hosted_yarn_classic_sha1.rs | 67 ++++++ .../src/formats/yarn/classic_deps.rs | 181 ++++++++++++++++ .../socket-patch-core/src/formats/yarn/mod.rs | 3 + crates/socket-patch-core/src/hosted/engine.rs | 46 ++-- .../src/hosted/memory/discover.rs | 31 +-- .../src/hosted/memory/mod.rs | 18 +- .../src/hosted/memory/stages.rs | 41 ++-- .../src/hosted/npm_manifest.rs | 62 ++++-- .../src/patch/redirect/mod.rs | 194 ++++++++++++++++- .../src/vendor/npm_common.rs | 35 +++- .../src/vendor/yarn_classic_lock.rs | 196 ++++++++++++------ docs/ecosystems.md | 18 +- 18 files changed, 976 insertions(+), 173 deletions(-) create mode 100644 crates/socket-patch-core/src/formats/yarn/classic_deps.rs diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index bb4592f4b..c4e0f61b9 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -738,7 +738,7 @@ to **six flavors**. | eco / flavor | vendored artifact | committed wiring | consumption proof | |---|---|---|---| | npm (package-lock) | deterministic patched tarball `[@scope/]-.tgz`, plus `/.gitignore` (re-includes the tarball against the project's ignores, such as Node.gitignore's `*.tgz`) and `/.gitattributes` (`-text`); every tarball flavor below writes the same pair and refuses `vendor_artifact_gitignored` when git would still drop the tarball | `package-lock.json` only (`npm-shrinkwrap.json` wins when present): every entry matching name+version gets `resolved: "file:…"` + recomputed `integrity`. `package.json` untouched | `npm ci` (integrity-verified). Plain `npm install` preserves the entry; `npm update ` re-resolves and drops it | -| npm / yarn classic | (same tarball) | `yarn.lock` only: matching blocks get `resolved "file:./…#"` + `integrity` (both checksums recomputed; merged-key & `npm:`-alias blocks covered) | `yarn install --frozen-lockfile --offline` (sha1 fragment + sha512 SRI both enforced; byte-stable lock) | +| npm / yarn classic | (same tarball) | `yarn.lock` only: matching blocks get `resolved "file:./…#"` + `integrity` (both checksums recomputed; merged-key & `npm:`-alias blocks covered); a patch that rewrites the package's `package.json` gets the blocks' `dependencies:` / `optionalDependencies:` sub-maps recomputed, and is refused `vendor_dep_manifest_unlocked` before any write when a dependency it adds or a range it changes to has no lock block of its own (#591) | `yarn install --frozen-lockfile --offline` (sha1 fragment + sha512 SRI both enforced; byte-stable lock) | | npm / yarn berry (node-modules linker) | (same tarball) | root `package.json` `resolutions` + `yarn.lock` entry with `checksum: 10c0/` of the berry cache-zip (reproduced from the tarball offline). **PnP is refused** (`.pnp.*` → different artifact pipeline) | `yarn install --immutable --check-cache`, cold cache. Refused if `__metadata.cacheKey ≠ 10c0` or a non-default `compressionLevel`. Both files keep their own layout — a CRLF lock (yarn's output on Windows) is spliced in CRLF, `package.json` is re-serialized with its BOM, indent, line ending and trailing-newline shape — so vendor + `--revert` round-trip byte-exactly; a lock or `package.json` MIXING CRLF and LF is refused before any write (`vendor_yarn_berry_mixed_line_endings`) | | npm / pnpm (lockfileVersion 9) | (same tarball) | root `package.json` `pnpm.overrides` (versioned selector) **+** `pnpm-lock.yaml` surgery (overrides / importer version / packages `resolution.integrity` / snapshots) **+** the same override in `pnpm-workspace.yaml` (pnpm >= 10.5 reads it there; created with a root-only `packages:` scaffold when absent). A project with no `pnpm-workspace.yaml` pinned to pnpm 9.0–10.4 (every pin, read as for the hosted trust config) gets no file: those read package.json, and a root-only workspace makes `pnpm add` fail there (#734); a later vendor on pnpm >= 10.5 adds it. Residual: an unpinned project with no install record still gets the scaffold, so on pnpm 9.0–10.4 it needs `pnpm add -w ` or a `packageManager` pin | `pnpm install --frozen-lockfile --offline`, cold store (integrity-verified; byte-stable on pnpm 9 & 10). Other lockfileVersions: 5.4/6.0 route to the legacy backend below; anything else refused | | npm / pnpm LEGACY (lockfileVersion 5.4 = pnpm 7, 6.0 = pnpm 8; flavor `pnpm-legacy`) | (same tarball) | root `package.json` `pnpm.overrides` **+** legacy lock surgery (overrides / root dep + specifiers / packages rekey to a bare `file:` key with recomputed integrity / in-package dep refs). **No `pnpm-workspace.yaml` is written** (pnpm ≤ 8 reads overrides only from package.json). The lock's SPECIFIER is machine-ABSOLUTE — pnpm ≤ 8 absolutizes `file:` overrides itself — surfaced as `vendor_pnpm_legacy_absolute_specifier`. Legacy WORKSPACE locks (`importers:`) refused | same-path `pnpm install --frozen-lockfile --offline`, cold store (byte-stable on pnpm 7.33.5 / 8.15.9). A checkout at a DIFFERENT path fails the frozen check (path-bound specifier) and must run `pnpm install --offline --no-frozen-lockfile` once (the flag matters on CI, where pnpm defaults frozen on), which installs the vendored tarball and re-resolves only the specifier line | @@ -1350,6 +1350,9 @@ Every `--json` invocation emits a single JSON object that follows the **unified | `vendor_vlt_reinstall_required` | `skipped` (advisory; human: `Warning: …`) | vendor / scan / get `--mode vendored` (vlt), wet and dry runs, and in-sync reruns: (a) the run rewires an optional dependency, or an importer's `node_modules/` of an optional dependency still resolves into `node_modules/.vlt/`: from vlt 0.0.0-30 a plain `vlt install` (1.2.0: also `--force`) keeps that installed upstream copy linked; the detail says to run `vlt ci` (or delete `node_modules` and run `vlt install`) to link the vendored copy, and that vlt 0.0.0-30 … 1.0.4 install no optional dependency from the lock of a project that declares only optional dependencies (upgrade to 1.0.5 or later first); (b) otherwise, an importer's link of the dependency still resolves into `node_modules/.vlt/`: the detail names the links (`node_modules/`, `/node_modules/`) and says `vlt install` (or `vlt ci`) links the vendored copy — on a warm tree after a plain `vlt install` that is true of every vendored direct dependency; (c) an importer's link resolves into the vendored dir of the patch this run replaces (a new patch uuid), which the run removes: the detail names the links and says `vlt install` (or `vlt ci`) links the new vendored copy; (d) a redownload of the payload (vendor, or `repair` after a corrupt or missing payload) could not keep vlt's links to the package's own dependencies (its old `node_modules/` held more than links): the detail says to run `vlt ci` (or delete `node_modules` and run `vlt install`), since a plain `vlt install` does not re-link them. `repair` moves those links back into the downloaded payload when they are only links. The package is vendored either way; a run whose patch fails to apply emits neither. A wet `vendor --revert` (and the revert a vendored → hosted takeover runs, whose advisory joins `redirect.warnings[]`): (a) the revert moves an `optionalDependencies` spec back from the `file:` dir, or an optional importer's `node_modules/` still resolves into the vendored uuid dir: from vlt 0.0.0-30 a plain `vlt install` keeps that link (dangling once the dir is removed), so the detail says to run `vlt ci` (or delete `node_modules` and run `vlt install`) to link the restored copy, with the same vlt 1.0.5 note; (b) otherwise, an importer's link still resolves into the vendored uuid dir: the detail names the links and says `vlt install` (or `vlt ci`) links the restored copy. A dry-run revert emits neither. | | `vendor_bun_reinstall_required` | `skipped` (advisory; human: `Warning: …`); rollback/remove `warnings[]`; `scan --prune` `gc.warnings[]` (human: `GC: …`) | a wet Bun revert (`vendor --revert`, rollback / remove of a vendored entry, `--preserve-state` included) that restored the lock entry while `node_modules/` is a real directory, or the tree has no `node_modules/.bun/` (a hoisted install): Bun's hoisted linker does not re-extract a package whose lock entry moves from the vendored tarball back to the registry record of the same `name@version`, so a plain `bun install` (also `--frozen-lockfile`) reports no changes and keeps the vendored bytes (measured on 1.1.45 … 1.4.2). The detail names `name@version` and says to run `bun install --force` (or delete `node_modules` and run `bun install`); the human revert hint names `bun install --force` too. An isolated install (a link into `node_modules/.bun/`) relinks and a project without `node_modules/` has nothing installed: neither warns, and neither does a dry run, a drift-kept revert, or a revert that restored nothing (`vendor_lockfile_missing`, or `vendor_lock_entry_removed` after `bun remove`, whose copy a plain `bun install` prunes). | | `vendor_flavor_changed` | `failed` | vendor (npm): the purl's vendor ledger entry was written for another lockfile `flavor` than the one the router now detects (for example `npm` → `vlt` after switching package managers). Remedy: `socket-patch vendor --revert` it first, then re-vendor. Refused before any write. | +| `vendor_dep_manifest_unlocked` | refused | vendor (yarn classic): the patch rewrites the package's own `package.json` to depend on a descriptor (`name@range`) no `yarn.lock` block is keyed by — an added dependency, or an existing one moved to a new range. yarn 1 builds its install graph from the lock, so the rewired block would name a dependency it never resolves: online frozen installs fetch it unpinned, `--offline` installs fail and every plain `yarn install` re-saves the lock (#591). Refused after staging and before any wiring is written (the staged uuid dir is removed); the detail names the descriptors. Remedy: lock them first (for example `yarn add `), then re-run. A dry run, which stages nothing, does not foresee it. | +| `redirect_yarn_classic_dep_manifest_unlocked` | `redirect.warnings[]` (warning) | scan/get `--mode hosted` (yarn classic): the served tarball's own `package.json` depends on a descriptor no `yarn.lock` block is keyed by. yarn 1 installs only what the lock names, so a pin would install the patched package without that dependency (#591). The dep is not pinned and never confirmed; the lock is left as it was. Same remedy as `vendor_dep_manifest_unlocked`. | +| `redirect_yarn_classic_dep_manifest_rewritten` | `redirect.warnings[]` (warning) | scan/get `--mode hosted` (yarn classic): the served tarball's own `package.json` declares other dependencies than the pinned block's sub-maps, every descriptor already locked; the block's `dependencies:` / `optionalDependencies:` sub-maps are rewritten to match (#591). | | `vendor_artifact_gitignored` | `failed` | vendor (vlt and the npm-family tarball flavors: npm, pnpm, bun, yarn classic, yarn berry): inside a git work tree, `git check-ignore --no-index` reports the new artifact's uuid directory as ignored by a rule its own `.gitignore` cannot override (such as a root `.socket/` or `vendor/` rule; the detail names the rule). Remedy: drop that rule for `.socket/vendor/`. Refused before any write. A file rule such as `*.tgz` is overridden by the `/.gitignore` vendoring writes; if the written artifact still reads as ignored, the run refuses and removes the uuid dir it created. | | `vendor_artifact_gitignore_unchecked` | warning | vendor (vlt and the npm-family tarball flavors): git is installed but could not answer the ignore check for the written vendored directory (it failed to start, ran past 30 s, or `rev-parse` / `check-ignore` exited with an error); the package is vendored and the detail names what failed. Remedy: make sure no ignore rule covers `.socket/` before committing. Git absent, or a project outside any work tree, raises nothing. | | `vendor_ledger_entry_missing` | `failed` | vendor (vlt): the only installed copy is vlt's link to a committed vendored directory, but the vendor ledger has no entry for the package; restore `.socket/vendor/state.json` from version control (v5.0: `repair` no longer re-synthesizes it). Replaces the `package_not_installed` skip. | diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index b6cd40702..b37eea5d8 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -1042,28 +1042,37 @@ pub(crate) async fn run_redirect_selected( } } } - // A yarn classic pin needs the served tarball's sha1 as its `resolved` - // fragment: yarn 1 keys its cache slot by it (#558). When the grant - // carries only a sha512, the scan downloads the tarball, checks it - // against that sha512 and pins the sha1 of those bytes. A tarball that - // cannot be fetched or verified drops its patch rather than pin a - // fragmentless URL a warm cache serves stale bytes for. - let sha1_targets: Vec<(String, String, DepOverride)> = - engine::yarn_classic_sha1_targets(&candidates, &read.files) - .into_iter() - .filter_map(|dep| { - let sha512 = dep.integrity.sha512.clone()?; - Some((dep.artifact_url.clone(), sha512, dep.clone())) - }) - .collect(); - for (url, sha512, dep) in sha1_targets { + // A yarn classic pin reads the served tarball: its sha1 is the + // `resolved` fragment yarn 1 keys its cache slot on when the grant + // carries none (#558), and its package.json's dependencies must match + // the lock block's sub-maps, every new descriptor locked (#591). The + // tarball is checked against the grant's sha512; one that cannot be + // fetched, verified or read drops its patch. + let classic_targets: Vec<(String, String, DepOverride)> = + engine::yarn_classic_artifact_targets( + &candidates, + &read.files, + &resolve_outer_yarn_mirror_for_process(&common.cwd), + ) + .into_iter() + .filter_map(|dep| { + let sha512 = dep.integrity.sha512.clone()?; + Some((dep.artifact_url.clone(), sha512, dep.clone())) + }) + .collect(); + for (url, sha512, dep) in classic_targets { status.set(format!("Fetching hosted tarball for {}...", dep.name)); - match socket_patch_core::hosted::npm_manifest::fetch_hosted_npm_sha1( + match socket_patch_core::hosted::npm_manifest::fetch_hosted_classic_artifact( api_client, &url, &sha512, ) .await { - Ok(sha1) => engine::set_derived_sha1(&mut candidates, &url, &sha1), + Ok(artifact) => engine::record_classic_artifact( + &mut candidates, + &mut python_metadata, + &url, + &artifact, + ), Err(detail) => { unavailable_python_artifacts.insert(url.clone()); skipped.push(engine::npm_tarball_unavailable(&dep, &detail)); @@ -2063,7 +2072,7 @@ fn describe_skip_reason(reason: &str) -> String { "the hosted tarball's package.json could not be fetched".into() } "npm_tarball_unavailable" => { - "the hosted tarball could not be fetched or did not match its sha512".into() + "the hosted tarball could not be fetched, verified or read".into() } "redirect_bun_lock_unsupported" | "redirect_bun_lockb_invalid" => { "the Bun lockfile blocks the vendored-to-hosted migration (see the warning)".into() diff --git a/crates/socket-patch-cli/tests/covgap_commands_scan_hosted.rs b/crates/socket-patch-cli/tests/covgap_commands_scan_hosted.rs index 385562706..46695f131 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_scan_hosted.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_scan_hosted.rs @@ -2719,6 +2719,59 @@ fn write_yarn_classic_project(root: &Path, package_manager: Option<&str>) { .unwrap(); } +/// A granted reference whose tarball the mock serves (a yarn classic pin +/// reads it, #558 / #591), with the grant's hashes matching it; returns its +/// URL. +async fn mock_served_classic_reference(server: &MockServer) -> String { + use base64::Engine as _; + use sha1::Digest as _; + let manifest = format!(r#"{{"name":"{NAME}","version":"{VERSION}"}}"#); + let mut builder = tar::Builder::new(flate2::write::GzEncoder::new( + Vec::new(), + flate2::Compression::default(), + )); + let mut header = tar::Header::new_gnu(); + header.set_size(manifest.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, "package/package.json", manifest.as_bytes()) + .unwrap(); + let tgz = builder.into_inner().unwrap().finish().unwrap(); + let artifact = format!("/patch/npm/{NAME}/{VERSION}/22222222-2222-4222-8222-222222222222/{UUID}/{NAME}-{VERSION}.tgz"); + let url = format!("{}{artifact}", server.uri()); + mock_reference_results( + server, + json!({ + UUID: { + "status": "granted", + "url": url, + "purl": PURL, + "artifacts": [{ + "kind": "tarball", + "url": url, + "integrity": { + "sha512": format!( + "sha512-{}", + base64::engine::general_purpose::STANDARD + .encode(sha2::Sha512::digest(&tgz)) + ), + "sha1": hex::encode(sha1::Sha1::digest(&tgz)), + } + }], + "registryOverride": null + } + }), + ) + .await; + Mock::given(method("GET")) + .and(path(artifact)) + .respond_with(ResponseTemplate::new(200).set_body_raw(tgz, "application/octet-stream")) + .mount(server) + .await; + url +} + /// #907: a hosted pin in a classic yarn.lock is dropped by the next yarn 2+ /// (berry) install exactly like vendored wiring, so `scan --mode hosted` /// must warn `redirect_yarn_classic_berry_migration_risk` — on a dry run, on @@ -2729,7 +2782,7 @@ async fn hosted_yarn_classic_pin_warns_berry_migration_risk() { for package_manager in [None, Some("yarn@4.18.1")] { let server = MockServer::start().await; mock_discovery(&server, PURL, UUID).await; - mock_granted_reference(&server, UUID, PURL, HOSTED_URL).await; + let hosted_url = mock_served_classic_reference(&server).await; mock_view(&server, UUID, PURL).await; let tmp = tempfile::tempdir().unwrap(); write_yarn_classic_project(tmp.path(), package_manager); @@ -2755,7 +2808,7 @@ async fn hosted_yarn_classic_pin_warns_berry_migration_risk() { } let lock = std::fs::read_to_string(tmp.path().join("yarn.lock")).unwrap(); assert!( - lock.contains(HOSTED_URL), + lock.contains(&hosted_url), "the warning never blocks the pin: {lock}" ); } @@ -2768,7 +2821,7 @@ async fn hosted_yarn_classic_pin_warns_berry_migration_risk() { async fn hosted_yarn_classic_pin_with_yarn1_package_manager_stays_silent() { let server = MockServer::start().await; mock_discovery(&server, PURL, UUID).await; - mock_granted_reference(&server, UUID, PURL, HOSTED_URL).await; + let hosted_url = mock_served_classic_reference(&server).await; mock_view(&server, UUID, PURL).await; let tmp = tempfile::tempdir().unwrap(); write_yarn_classic_project(tmp.path(), Some("yarn@1.22.22")); @@ -2783,5 +2836,5 @@ async fn hosted_yarn_classic_pin_with_yarn1_package_manager_stays_silent() { "a yarn 1 pin suppresses the advisory: {codes:?}" ); let lock = std::fs::read_to_string(tmp.path().join("yarn.lock")).unwrap(); - assert!(lock.contains(HOSTED_URL), "{lock}"); + assert!(lock.contains(&hosted_url), "{lock}"); } diff --git a/crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs index c1d557d72..5cec76794 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs @@ -452,6 +452,25 @@ async fn classic_hosted_project( }))) .mount(&server) .await; + if tamper_served_tarball { + // The scan reads the served tarball once (it checks the dependency + // graph against the lock, #591) and verifies it against the grant, + // so it would refuse tampered bytes up front. Serve the real bytes + // to that read and the tampered ones from then on: the tarball is + // swapped after the pin was written, which only yarn's own check of + // the pinned hashes can catch. + Mock::given(method("GET")) + .and(path(format!( + "/patch/npm/{DEP}/{DEP_VERSION}/{TOKEN}/{UUID}/{DEP}-{DEP_VERSION}.tgz" + ))) + .respond_with( + ResponseTemplate::new(200).set_body_raw(tgz.clone(), "application/octet-stream"), + ) + .up_to_n_times(1) + .with_priority(1) + .mount(&server) + .await; + } Mock::given(method("GET")) .and(path(format!( "/patch/npm/{DEP}/{DEP_VERSION}/{TOKEN}/{UUID}/{DEP}-{DEP_VERSION}.tgz" @@ -1435,6 +1454,16 @@ async fn mock_hosted_grant(tgz: &[u8], orig: &[u8], patched: &[u8], title: &str) }))) .mount(&server) .await; + // The hosted tarball itself: the scan reads it before pinning (#591). + Mock::given(method("GET")) + .and(path(format!( + "/patch/npm/{DEP}/{DEP_VERSION}/{TOKEN}/{UUID}/{DEP}-{DEP_VERSION}.tgz" + ))) + .respond_with( + ResponseTemplate::new(200).set_body_raw(tgz.to_vec(), "application/octet-stream"), + ) + .mount(&server) + .await; server } diff --git a/crates/socket-patch-cli/tests/hosted_memory_parity.rs b/crates/socket-patch-cli/tests/hosted_memory_parity.rs index 09245d1c1..563a5c0de 100644 --- a/crates/socket-patch-cli/tests/hosted_memory_parity.rs +++ b/crates/socket-patch-cli/tests/hosted_memory_parity.rs @@ -24,6 +24,10 @@ struct Case { /// nothing for the formats the engine must rewrite). expect_redirect: bool, dry_run: bool, + /// Serve a real tarball for every npm grant, its integrity rewritten + /// to match: a yarn classic pin reads the served tarball (#558, #591), + /// which the fixtures' placeholder hashes could never verify. + serve_npm_tarballs: bool, } fn case(fixture: &'static str) -> Case { @@ -32,6 +36,47 @@ fn case(fixture: &'static str) -> Case { extra: Vec::new(), expect_redirect: true, dry_run: false, + serve_npm_tarballs: false, + } +} + +/// For each npm patch: a minimal tarball (`package/package.json` naming +/// the package) served at its artifact URL, with the grant's sha512 and +/// sha1 set to that tarball's. +async fn serve_npm_tarballs(server: &MockServer, patches: &mut [common::Patch]) { + use sha1::Digest as _; + use wiremock::matchers::{method, path}; + use wiremock::{Mock, ResponseTemplate}; + for patch in patches.iter_mut() { + let Some(rest) = patch.purl.strip_prefix("pkg:npm/") else { + continue; + }; + let (name, version) = rest.rsplit_once('@').unwrap(); + let manifest = format!(r#"{{"name":"{name}","version":"{version}"}}"#); + let mut builder = tar::Builder::new(flate2::write::GzEncoder::new( + Vec::new(), + flate2::Compression::default(), + )); + let mut header = tar::Header::new_gnu(); + header.set_size(manifest.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, "package/package.json", manifest.as_bytes()) + .unwrap(); + let tgz = builder.into_inner().unwrap().finish().unwrap(); + let url = patch.reference["url"].as_str().unwrap().to_string(); + let artifact = &mut patch.reference["artifacts"][0]; + artifact["integrity"]["sha512"] = Value::String(format!( + "sha512-{}", + base64::engine::general_purpose::STANDARD.encode(sha2::Sha512::digest(&tgz)) + )); + artifact["integrity"]["sha1"] = Value::String(hex::encode(sha1::Sha1::digest(&tgz))); + Mock::given(method("GET")) + .and(path(url.strip_prefix(&server.uri()).unwrap().to_string())) + .respond_with(ResponseTemplate::new(200).set_body_raw(tgz, "application/octet-stream")) + .mount(server) + .await; } } @@ -39,7 +84,10 @@ fn case(fixture: &'static str) -> Case { async fn assert_parity(case: Case) -> Value { let dir = fixtures_root().join("redirect").join(case.fixture); let server = MockServer::start().await; - let patches = patches_from_overrides(&dir.join("overrides.json"), Some(&server.uri())); + let mut patches = patches_from_overrides(&dir.join("overrides.json"), Some(&server.uri())); + if case.serve_npm_tarballs { + serve_npm_tarballs(&server, &mut patches).await; + } mount_api(&server, &patches).await; let mut files = fixture_files(&dir.join("input")); for (rel, bytes) in &case.extra { @@ -146,7 +194,11 @@ async fn parity_pnpm_existing_workspace() { #[tokio::test] async fn parity_yarn_classic() { - assert_parity(case("npm/yarn-classic/basic")).await; + assert_parity(Case { + serve_npm_tarballs: true, + ..case("npm/yarn-classic/basic") + }) + .await; } #[tokio::test] diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index 97f69b1b4..9040d9fbf 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -126,6 +126,61 @@ async fn mock_reference(server: &MockServer) { .await; } +/// A granted reference whose tarball the mock serves (a yarn classic pin +/// reads it, #558 / #591), the grant's hashes matching it: `(url, sha512 +/// SRI, sha1 hex)`. +async fn mock_served_reference(server: &MockServer) -> (String, String, String) { + use base64::Engine as _; + use sha1::Digest as _; + let manifest = format!(r#"{{"name":"{NAME}","version":"{VERSION}"}}"#); + let mut builder = tar::Builder::new(flate2::write::GzEncoder::new( + Vec::new(), + flate2::Compression::default(), + )); + let mut header = tar::Header::new_gnu(); + header.set_size(manifest.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, "package/package.json", manifest.as_bytes()) + .unwrap(); + let tgz = builder.into_inner().unwrap().finish().unwrap(); + let sha512 = format!( + "sha512-{}", + base64::engine::general_purpose::STANDARD.encode(sha2::Sha512::digest(&tgz)) + ); + let sha1 = hex::encode(sha1::Sha1::digest(&tgz)); + let artifact = format!( + "/patch/npm/{NAME}/{VERSION}/22222222-2222-4222-8222-222222222222/{UUID}/{NAME}-{VERSION}.tgz" + ); + let url = format!("{}{artifact}", server.uri()); + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/package"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "results": { + UUID: { + "status": "granted", + "url": url, + "purl": PURL, + "artifacts": [{ + "kind": "tarball", + "url": url, + "integrity": { "sha512": sha512, "sha1": sha1 } + }], + "registryOverride": null + } + } + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path(artifact)) + .respond_with(ResponseTemplate::new(200).set_body_raw(tgz, "application/octet-stream")) + .mount(server) + .await; + (url, sha512, sha1) +} + /// The `view/{uuid}` endpoint `run_redirect` calls to build the patch record /// (file hashes + vulnerabilities) the in-run VEX attests from. async fn mock_view(server: &MockServer) { @@ -1118,7 +1173,7 @@ async fn scan_redirect_refuses_a_mixed_line_ending_yarn_berry_manifest() { async fn scan_redirect_rewrites_correct_entry_in_crlf_classic_lock() { let server = MockServer::start().await; mock_discovery(&server).await; - mock_reference(&server).await; + let (hosted_url, sha512, sha1) = mock_served_reference(&server).await; let tmp = tempfile::tempdir().unwrap(); std::fs::write( @@ -1157,8 +1212,8 @@ async fn scan_redirect_rewrites_correct_entry_in_crlf_classic_lock() { "the decoy entry must stay byte-identical: {lock}" ); assert!( - lock.contains(&format!("resolved \"{HOSTED_URL}#{PATCHED_SHA1}\"\r\n")) - && lock.contains(&format!("integrity {PATCHED_SHA512}\r\n")), + lock.contains(&format!("resolved \"{hosted_url}#{sha1}\"\r\n")) + && lock.contains(&format!("integrity {sha512}\r\n")), "the target entry must pin the hosted patch: {lock}" ); assert!( diff --git a/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs b/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs index 71962b4aa..88a55cb31 100644 --- a/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs +++ b/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs @@ -262,3 +262,70 @@ fn assert_skipped_untouched(root: &Path, server: &MockServer, doc: &Value, stder "no fragmentless hosted pin is written" ); } + +/// #591: the served tarball's package.json adds a dependency yarn.lock +/// doesn't lock. Yarn 1 installs only what the lock names, so a pin would +/// install the patched package without it (and `vex` would attest it): +/// the scan refuses the pin with an actionable warning and leaves the +/// lock alone. The grant carries a sha1 here, so only the dependency +/// check needs the tarball. +#[tokio::test] +async fn issue_591_served_dependency_the_lock_does_not_lock_is_refused() { + let server = MockServer::start().await; + let tarball = tgz(&[ + ( + "package/package.json", + br#"{"name":"left-pad","version":"1.3.0","dependencies":{"is-odd":"^3.0.0"}}"#, + ), + ("package/index.js", b"module.exports = require('is-odd');\n"), + ]); + mock_api_with_sha1(&server, &tarball).await; + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("proj"); + write_classic_project(&root); + + let (code, doc, stderr) = scan(&root, &server.uri()); + assert_eq!(code, 0, "{doc:#}\n{stderr}"); + assert_eq!(doc["redirect"]["redirected"], 0, "{doc:#}"); + let warning = doc["redirect"]["warnings"] + .as_array() + .unwrap() + .iter() + .find(|w| w["code"] == "redirect_yarn_classic_dep_manifest_unlocked") + .unwrap_or_else(|| panic!("refusal warning: {doc:#}")); + let detail = warning["detail"].as_str().unwrap(); + assert!( + detail.contains("is-odd@^3.0.0") && detail.contains("yarn add is-odd@^3.0.0"), + "{detail}" + ); + assert_eq!( + std::fs::read_to_string(root.join("yarn.lock")).unwrap(), + LOCK, + "nothing is pinned with an incomplete dependency graph" + ); +} + +/// [`mock_api`] with a grant that carries the tarball's sha1 too. +async fn mock_api_with_sha1(server: &MockServer, tarball: &[u8]) { + let artifact = format!("/patch/npm/left-pad/1.3.0/{TOKEN}/{UUID}/left-pad-1.3.0.tgz"); + let url = format!("{}{artifact}", server.uri()); + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/package"))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "results": { + UUID: { + "status": "granted", + "url": url, + "purl": PURL, + "artifacts": [{ + "kind": "tarball", "url": url, + "integrity": { "sha512": sha512_sri(tarball), "sha1": sha1_hex(tarball) } + }], + "registryOverride": null + } + } + }))) + .mount(server) + .await; + mock_api(server, tarball, Some(tarball.to_vec())).await; +} diff --git a/crates/socket-patch-core/src/formats/yarn/classic_deps.rs b/crates/socket-patch-core/src/formats/yarn/classic_deps.rs new file mode 100644 index 000000000..77932443e --- /dev/null +++ b/crates/socket-patch-core/src/formats/yarn/classic_deps.rs @@ -0,0 +1,181 @@ +//! A yarn classic block's dependency sub-maps against a patched package's +//! own `package.json` (#591). +//! +//! Yarn 1 builds its install graph from `yarn.lock`: a block's +//! `dependencies:` / `optionalDependencies:` sub-maps name the descriptors +//! (`name@range`) the package needs, and each descriptor must be the key +//! of a block of its own. A patch that adds a dependency or changes a +//! range therefore needs both the sub-map rewritten AND a block for every +//! new descriptor. Without the block, `yarn install --frozen-lockfile` +//! resolves the descriptor from the registry with no pin (and `--offline` +//! fails); without the sub-map, yarn never installs the dependency. +//! Neither writer can resolve a new descriptor itself, so they rewrite +//! the sub-maps only when every descriptor is already locked, and refuse +//! the patch otherwise. + +use serde_json::Value; + +use super::blocks::{body_field_line, LockBlock}; +use super::patterns::split_key_patterns; + +/// The block fields yarn 1 mirrors from a package's manifest, in the +/// order it writes them. +const DEP_FIELDS: [&str; 2] = ["dependencies", "optionalDependencies"]; + +/// `lines` (a block's lines) with its dependency sub-maps replaced by the +/// ones `pkg` declares, written the way yarn 1 writes them: each map's +/// entries sorted by name, after every other field. +pub(crate) fn with_manifest_dep_maps(lines: &[String], pkg: &Value) -> Vec { + let mut out = Vec::with_capacity(lines.len()); + let mut i = 0; + while i < lines.len() { + if i > 0 && body_field_line(&lines[i]).is_some_and(is_dep_map_header) { + // Drop the stale sub-map (header + 4-space entries). + i += 1; + while i < lines.len() && body_field_line(&lines[i]).is_none() { + i += 1; + } + continue; + } + out.push(lines[i].clone()); + i += 1; + } + for (field, deps) in manifest_dep_maps(pkg) { + out.push(format!(" {field}:")); + for (name, range) in deps { + out.push(format!(" {} \"{range}\"", quote_yarn_key(&name))); + } + } + out +} + +/// Every descriptor (`name@range`) `pkg`'s dependency maps declare that no +/// block of `blocks` is keyed by, sorted and deduplicated: the ones a lock +/// rewritten to the patched manifest would leave unresolved. +pub(crate) fn unlocked_descriptors(blocks: &[LockBlock], pkg: &Value) -> Vec { + let locked: std::collections::BTreeSet = blocks + .iter() + .flat_map(|b| split_key_patterns(&b.key)) + .collect(); + let mut missing: Vec = manifest_dep_maps(pkg) + .into_iter() + .flat_map(|(_, deps)| deps) + .map(|(name, range)| format!("{name}@{range}")) + .filter(|descriptor| !locked.contains(descriptor)) + .collect(); + missing.sort(); + missing.dedup(); + missing +} + +/// Whether the sub-maps `lines` carries already are exactly the ones `pkg` +/// declares (so a writer leaves them alone). +pub(crate) fn dep_maps_match(lines: &[String], pkg: &Value) -> bool { + with_manifest_dep_maps(lines, pkg) == lines +} + +fn is_dep_map_header(rest: &str) -> bool { + DEP_FIELDS.iter().any(|f| rest.strip_suffix(':') == Some(f)) +} + +/// `pkg`'s non-empty dependency maps, each sorted by name. A non-string +/// range is skipped, as yarn skips it. +fn manifest_dep_maps(pkg: &Value) -> Vec<(&'static str, Vec<(String, String)>)> { + DEP_FIELDS + .iter() + .filter_map(|&field| { + let map = pkg.get(field).and_then(Value::as_object)?; + let mut deps: Vec<(String, String)> = map + .iter() + .filter_map(|(k, v)| Some((k.clone(), v.as_str()?.to_string()))) + .collect(); + if deps.is_empty() { + return None; + } + deps.sort_unstable(); + Some((field, deps)) + }) + .collect() +} + +/// A sub-map key the way yarn 1's serializer writes it: quoted when it +/// could not be read back bare. +pub(crate) fn quote_yarn_key(key: &str) -> String { + let needs = key.is_empty() + || key.starts_with("true") + || key.starts_with("false") + || !key.chars().next().is_some_and(|c| c.is_ascii_alphabetic()) + || key + .chars() + .any(|c| matches!(c, ':' | ' ' | '\n' | '\t' | '\\' | '"' | ',' | '[' | ']')); + if needs { + format!("\"{key}\"") + } else { + key.to_string() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::formats::yarn::blocks::scan_blocks; + + const LOCK: &str = "# yarn lockfile v1\n\n\ +is-number@^6.0.0:\n version \"6.0.0\"\n\n\ +\"@scope/opt@^2.0.0\":\n version \"2.0.0\"\n\n\ +is-odd@3.0.1:\n version \"3.0.1\"\n dependencies:\n is-number \"^6.0.0\"\n"; + + fn lines(text: &str) -> Vec { + text.lines().map(str::to_string).collect() + } + + #[test] + fn unlocked_descriptors_names_only_the_unresolved_ones() { + let blocks = scan_blocks(LOCK); + let pkg = serde_json::json!({ + "dependencies": {"is-number": "^7.0.0", "wow": "^1.0.0"}, + "optionalDependencies": {"@scope/opt": "^2.0.0"}, + }); + assert_eq!( + unlocked_descriptors(&blocks, &pkg), + vec!["is-number@^7.0.0", "wow@^1.0.0"] + ); + let unchanged = serde_json::json!({"dependencies": {"is-number": "^6.0.0"}}); + assert!(unlocked_descriptors(&blocks, &unchanged).is_empty()); + assert!(unlocked_descriptors(&blocks, &serde_json::json!({})).is_empty()); + } + + #[test] + fn sub_maps_are_rebuilt_in_yarns_order() { + let block = lines("is-odd@3.0.1:\n version \"3.0.1\"\n dependencies:\n is-number \"^6.0.0\"\n integrity sha512-X=="); + let pkg = serde_json::json!({ + "optionalDependencies": {"@scope/opt": "^2.0.0"}, + "dependencies": {"zz": "1", "is-number": "^6.0.0"}, + }); + assert_eq!( + with_manifest_dep_maps(&block, &pkg), + lines( + "is-odd@3.0.1:\n version \"3.0.1\"\n integrity sha512-X==\n dependencies:\n \ + is-number \"^6.0.0\"\n zz \"1\"\n optionalDependencies:\n \"@scope/opt\" \"^2.0.0\"" + ) + ); + let same = + lines("is-odd@3.0.1:\n version \"3.0.1\"\n dependencies:\n is-number \"^6.0.0\""); + assert!(dep_maps_match( + &same, + &serde_json::json!({"dependencies": {"is-number": "^6.0.0"}}) + )); + assert!(!dep_maps_match( + &same, + &serde_json::json!({"dependencies": {"is-number": "^7.0.0"}}) + )); + } + + #[test] + fn quote_yarn_key_quotes_what_yarn_quotes() { + assert_eq!(quote_yarn_key("left-pad"), "left-pad"); + assert_eq!(quote_yarn_key("@scope/x"), "\"@scope/x\""); + assert_eq!(quote_yarn_key("3d-lib"), "\"3d-lib\""); + assert_eq!(quote_yarn_key("true-lib"), "\"true-lib\""); + } +} diff --git a/crates/socket-patch-core/src/formats/yarn/mod.rs b/crates/socket-patch-core/src/formats/yarn/mod.rs index 5e6dba3fd..56b80af0e 100644 --- a/crates/socket-patch-core/src/formats/yarn/mod.rs +++ b/crates/socket-patch-core/src/formats/yarn/mod.rs @@ -3,6 +3,8 @@ //! //! * which grammar a lock is ([`sniff_grammar`], [`is_berry_lock`]); //! * the block walk and field reads ([`blocks`]); +//! * a classic block's dependency sub-maps against a patched manifest +//! ([`classic_deps`]); //! * key, descriptor and locator patterns ([`patterns`]); //! * where yarn 1 installs a block's copy from ([`source`]); //! * the stanza view the hosted berry writers re-key and re-order @@ -17,6 +19,7 @@ pub(crate) mod berry_entry; pub mod berry_gates; pub(crate) mod blocks; +pub(crate) mod classic_deps; pub(crate) mod patterns; pub(crate) mod source; pub(crate) mod stanzas; diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index d3927dc2a..ebad9538d 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -1108,14 +1108,18 @@ pub fn yarn_berry_manifest_targets<'a>( .collect() } -/// The npm deps whose yarn classic pin needs the sha1 of the served -/// tarball (#558): the grant carries a sha512 but no sha1, and the -/// project's `yarn.lock` is a classic lock that names the package. Yarn 1 -/// keys its cache slot by the `resolved` URL's `#` fragment, so the -/// pin must carry one. One per distinct artifact URL. -pub fn yarn_classic_sha1_targets<'a>( +/// The npm deps whose yarn classic pin reads the served tarball, one per +/// distinct artifact URL: the project's `yarn.lock` is a classic lock that +/// names the package, the grant carries a sha512, and either the grant has +/// no sha1 for the `resolved` fragment yarn 1 keys its cache slot on +/// (#558), or the lock doesn't pin this artifact yet, so the tarball's own +/// dependencies must be checked against the lock (#591). +/// A lock the project's offline mirror refuses outright (`yarn_outer`: the +/// mirror settings outside the project files) needs none. +pub fn yarn_classic_artifact_targets<'a>( candidates: &'a [Candidate], files: &BTreeMap, + yarn_outer: &OuterYarnMirror, ) -> Vec<&'a DepOverride> { let Some(lock) = files .get("yarn.lock") @@ -1123,15 +1127,20 @@ pub fn yarn_classic_sha1_targets<'a>( else { return Vec::new(); }; + if crate::patch::redirect::yarn_classic_hosted_refused(files, yarn_outer) { + return Vec::new(); + } let mut seen = BTreeSet::new(); candidates .iter() .map(|c| &c.dep) - .filter(|dep| dep.ecosystem == "npm") - .filter(|dep| dep.integrity.sha1.is_none() && dep.integrity.sha512.is_some()) + .filter(|dep| dep.ecosystem == "npm" && dep.integrity.sha512.is_some()) .filter(|dep| { classic_locks_registry_copy(lock, &crate::patch::redirect::full_name(dep), &dep.version) }) + .filter(|dep| { + dep.integrity.sha1.is_none() || !lock.contains(&format!("\"{}#", dep.artifact_url)) + }) .filter(|dep| seen.insert(dep.artifact_url.clone())) .collect() } @@ -1157,21 +1166,28 @@ fn classic_locks_registry_copy(lock: &str, name: &str, version: &str) -> bool { }) } -/// Record the sha1 derived from the served tarball at `url` on every -/// candidate granted that artifact. -pub fn set_derived_sha1(candidates: &mut [Candidate], url: &str, sha1: &str) { +/// Record what the served tarball at `url` yielded: its sha1 on every +/// candidate granted that artifact without one, and its manifest (keyed by +/// URL) for the rewriter. +pub fn record_classic_artifact( + candidates: &mut [Candidate], + manifests: &mut BTreeMap, + url: &str, + artifact: &crate::hosted::npm_manifest::HostedClassicArtifact, +) { for candidate in candidates .iter_mut() .filter(|c| c.dep.artifact_url == url && c.dep.integrity.sha1.is_none()) { - candidate.dep.integrity.sha1 = Some(sha1.to_string()); + candidate.dep.integrity.sha1 = Some(artifact.sha1.clone()); } + manifests.insert(url.to_string(), artifact.manifest.clone()); } /// The skip recorded for an npm dep whose served tarball could not be -/// fetched or did not match its grant's sha512, so no sha1 could be -/// derived for its yarn classic pin (the grant token in `detail` is -/// redacted to ``). +/// fetched, did not match its grant's sha512 or had no readable +/// package.json, so its yarn classic pin could not be checked (the grant +/// token in `detail` is redacted to ``). pub fn npm_tarball_unavailable(dep: &DepOverride, detail: &str) -> SkippedPatch { SkippedPatch { purl: format!( diff --git a/crates/socket-patch-core/src/hosted/memory/discover.rs b/crates/socket-patch-core/src/hosted/memory/discover.rs index b9a8cde21..563664ba0 100644 --- a/crates/socket-patch-core/src/hosted/memory/discover.rs +++ b/crates/socket-patch-core/src/hosted/memory/discover.rs @@ -15,6 +15,7 @@ use std::time::Duration; use crate::api::client::{ApiError, ApiFuture, PatchApi}; use crate::api::ranking::cmp_search_results; use crate::api::types::{BatchPackagePatches, PackageVendorResult, PatchResponse, SearchResponse}; +use crate::hosted::npm_manifest::HostedClassicArtifact; use crate::utils::purl::{normalize_purl, strip_purl_qualifiers}; use crate::utils::purl_key::PurlKey; @@ -391,26 +392,28 @@ pub(crate) async fn fetch_npm_manifests( .collect() } -/// Served npm tarballs' sha1 once per distinct `(url, sha512)`, for the -/// yarn classic pin's `#` fragment when the grant carries none -/// (#558): the disk flow's `fetch_hosted_npm_sha1` over the provider. -pub(crate) async fn fetch_npm_sha1s( +/// The served npm tarballs a yarn classic pin reads (sha1 and +/// package.json), once per distinct `(url, sha512)`: the disk flow's +/// `fetch_hosted_classic_artifact` over the provider. +pub(crate) async fn fetch_classic_artifacts( provider: &Provider, wanted: &BTreeSet<(String, String)>, max_bytes: u64, -) -> BTreeMap> { +) -> BTreeMap> { let ordered: Vec<&(String, String)> = wanted.iter().collect(); let futures: Vec> = ordered .iter() - .map(|(url, sha512)| -> BoxFuture<'_, Result> { - Box::pin(async move { - let bytes = provider - .download_artifact(url, max_bytes) - .await - .map_err(|error| format!("cannot fetch the hosted tarball: {error}"))?; - crate::hosted::npm_manifest::decode_hosted_npm_sha1(&bytes, sha512) - }) - }) + .map( + |(url, sha512)| -> BoxFuture<'_, Result> { + Box::pin(async move { + let bytes = provider + .download_artifact(url, max_bytes) + .await + .map_err(|error| format!("cannot fetch the hosted tarball: {error}"))?; + crate::hosted::npm_manifest::decode_hosted_classic_artifact(&bytes, sha512) + }) + }, + ) .collect(); let results = join_bounded(futures, provider.concurrency).await; ordered diff --git a/crates/socket-patch-core/src/hosted/memory/mod.rs b/crates/socket-patch-core/src/hosted/memory/mod.rs index c752ca33c..4c40d8845 100644 --- a/crates/socket-patch-core/src/hosted/memory/mod.rs +++ b/crates/socket-patch-core/src/hosted/memory/mod.rs @@ -1106,14 +1106,19 @@ async fn engine( .await, ); } - let npm_sha1s: BTreeSet<(String, String)> = planned + let npm_classic: BTreeSet<(String, String)> = planned .iter() - .flat_map(|(_, p)| p.npm_sha1s.iter().cloned()) + .flat_map(|(_, p)| p.npm_classic.iter().cloned()) .collect(); - let artifact_sha1s = if npm_sha1s.is_empty() { + let artifact_classic = if npm_classic.is_empty() { BTreeMap::new() } else { - discover::fetch_npm_sha1s(&provider, &npm_sha1s, options.limits.max_artifact_bytes).await + discover::fetch_classic_artifacts( + &provider, + &npm_classic, + options.limits.max_artifact_bytes, + ) + .await }; phases.mark("plan"); @@ -1130,7 +1135,7 @@ async fn engine( if stage.capped() { first_plans.insert(index, plan.clone()); } - match stages::rewrite(plan, &artifact_metadata, &artifact_sha1s, stage_options).await { + match stages::rewrite(plan, &artifact_metadata, &artifact_classic, stage_options).await { Ok(done) => rewritten.push((index, done)), Err(RewriteRefused { refusal, skipped }) => { states[index].skipped = skipped; @@ -1217,7 +1222,8 @@ async fn engine( .retain(|c| !root_deferred.contains(&c.dep.patch_uuid)); plan.skipped .extend(states[index].deferred.iter().map(deferred_skip)); - match stages::rewrite(plan, &artifact_metadata, &artifact_sha1s, stage_options).await { + match stages::rewrite(plan, &artifact_metadata, &artifact_classic, stage_options).await + { Ok(done) => again.push((index, done)), Err(RewriteRefused { refusal, skipped }) => { states[index].skipped = skipped; diff --git a/crates/socket-patch-core/src/hosted/memory/stages.rs b/crates/socket-patch-core/src/hosted/memory/stages.rs index f1d611c47..f218262b0 100644 --- a/crates/socket-patch-core/src/hosted/memory/stages.rs +++ b/crates/socket-patch-core/src/hosted/memory/stages.rs @@ -12,6 +12,7 @@ use crate::api::types::PackageVendorResult; use crate::hosted::engine::{ self, Candidate, CandidateFiles, Refusal, RewriteOptions, SkippedPatch, }; +use crate::hosted::npm_manifest::HostedClassicArtifact; use crate::hosted::vlt::Preflight; use crate::patch::redirect::npmrc::OuterAllowRemote; use crate::patch::redirect::yarnrc::OuterYarnMirror; @@ -157,9 +158,9 @@ pub(crate) struct Planned { /// `(artifact url, sha512)` of every npm tarball whose own /// package.json a yarn berry pin needs (#718). pub(crate) npm_manifests: Vec<(String, Option)>, - /// `(artifact url, sha512)` of every npm tarball whose sha1 a yarn - /// classic pin needs because its grant carries none (#558). - pub(crate) npm_sha1s: Vec<(String, String)>, + /// `(artifact url, sha512)` of every npm tarball a yarn classic pin + /// reads (its sha1, #558, and its package.json, #591). + pub(crate) npm_classic: Vec<(String, String)>, /// The vlt artifact preflight, judged offline (no network here, so /// every in-scope dep is withheld instead of pinned: `--offline` /// parity). @@ -213,10 +214,14 @@ pub(crate) async fn plan( .into_iter() .map(|dep| (dep.artifact_url.clone(), dep.integrity.sha512.clone())) .collect(); - let npm_sha1s = engine::yarn_classic_sha1_targets(&candidates, &read.files) - .into_iter() - .filter_map(|dep| Some((dep.artifact_url.clone(), dep.integrity.sha512.clone()?))) - .collect(); + let npm_classic = engine::yarn_classic_artifact_targets( + &candidates, + &read.files, + &OuterYarnMirror::default(), + ) + .into_iter() + .filter_map(|dep| Some((dep.artifact_url.clone(), dep.integrity.sha512.clone()?))) + .collect(); Ok(Planned { project, candidates, @@ -225,7 +230,7 @@ pub(crate) async fn plan( read, wheels, npm_manifests, - npm_sha1s, + npm_classic, vlt_preflight, }) } @@ -249,12 +254,13 @@ pub(crate) struct RewriteRefused { pub(crate) skipped: Vec, } -/// Wheel metadata, served npm manifests and served npm tarballs' sha1s -/// (each keyed by artifact URL) → the engine's rewrite → the guard. +/// Wheel metadata, served npm manifests and the served npm tarballs a +/// yarn classic pin reads (each keyed by artifact URL) → the engine's +/// rewrite → the guard. pub(crate) async fn rewrite( planned: Planned, artifact_metadata: &BTreeMap, String>>, - artifact_sha1s: &BTreeMap>, + artifact_classic: &BTreeMap>, options: StageOptions, ) -> Result { let Planned { @@ -265,7 +271,7 @@ pub(crate) async fn rewrite( read, wheels, npm_manifests, - npm_sha1s, + npm_classic, vlt_preflight, } = planned; let skipped_before = skipped.clone(); @@ -314,9 +320,14 @@ pub(crate) async fn rewrite( } } } - for (url, _) in &npm_sha1s { - match artifact_sha1s.get(url) { - Some(Ok(sha1)) => engine::set_derived_sha1(&mut candidates, url, sha1), + for (url, _) in &npm_classic { + match artifact_classic.get(url) { + Some(Ok(artifact)) => engine::record_classic_artifact( + &mut candidates, + &mut python_metadata, + url, + artifact, + ), Some(Err(detail)) => { if unavailable.insert(url.clone()) { for dep in candidates diff --git a/crates/socket-patch-core/src/hosted/npm_manifest.rs b/crates/socket-patch-core/src/hosted/npm_manifest.rs index 532784c41..8418ba266 100644 --- a/crates/socket-patch-core/src/hosted/npm_manifest.rs +++ b/crates/socket-patch-core/src/hosted/npm_manifest.rs @@ -43,30 +43,48 @@ pub async fn fetch_hosted_npm_manifest( decode_hosted_npm_manifest(&bytes, sha512) } -/// The sha1 (hex) of a served npm tarball, for the yarn classic hosted -/// pin's `resolved "#"` fragment when the grant carries no sha1 -/// (#558). Yarn 1 names its cache slot after that fragment, so a -/// fragmentless URL shares the slot of any fragmentless upstream copy of the -/// same version and installs its bytes. The bytes must match the grant's -/// sha512: that is what the pin's `integrity` line names, and a sha1 taken -/// from any other bytes would pin a tarball yarn then refuses. -pub fn decode_hosted_npm_sha1(bytes: &[u8], sha512: &str) -> Result { +/// What a yarn classic hosted pin reads from the served npm tarball. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct HostedClassicArtifact { + /// The tarball's sha1 (hex), for the pin's `resolved "#"` + /// fragment when the grant carries none (#558). Yarn 1 names its cache + /// slot after that fragment, so a fragmentless URL shares the slot of + /// any fragmentless upstream copy of the same version. + pub sha1: String, + /// The tarball's own `package.json` text: yarn 1 installs the + /// dependencies the lock block's sub-maps name, so a patch that + /// changes them needs the block rewritten, and every new descriptor + /// locked (#591). + pub manifest: String, +} + +/// [`HostedClassicArtifact`] from the served bytes, which must match the +/// grant's sha512: that is what the pin's `integrity` line names, and a +/// sha1 taken from any other bytes would pin a tarball yarn then refuses. +pub fn decode_hosted_classic_artifact( + bytes: &[u8], + sha512: &str, +) -> Result { crate::vendor::registry_fetch::verify_sri(bytes, sha512) .map_err(|_| "hosted tarball does not match its published sha512".to_string())?; - Ok(crate::utils::digest::sha1_hex_of(bytes)) + Ok(HostedClassicArtifact { + sha1: crate::utils::digest::sha1_hex_of(bytes), + manifest: decode_hosted_npm_manifest(bytes, None)?, + }) } -/// Download the served tarball and take its sha1 ([`decode_hosted_npm_sha1`]). -pub async fn fetch_hosted_npm_sha1( +/// Download the served tarball and decode it +/// ([`decode_hosted_classic_artifact`]). +pub async fn fetch_hosted_classic_artifact( client: &ApiClient, url: &str, sha512: &str, -) -> Result { +) -> Result { let bytes = client .download_artifact(url) .await .map_err(|error| format!("cannot fetch the hosted tarball: {error}"))?; - decode_hosted_npm_sha1(&bytes, sha512) + decode_hosted_classic_artifact(&bytes, sha512) } #[cfg(test)] @@ -90,15 +108,19 @@ mod tests { } #[test] - fn sha1_is_taken_from_bytes_matching_the_sha512() { - let bytes = tgz(&[("package/package.json", br#"{"name":"left-pad"}"#)]); + fn classic_artifact_is_read_from_bytes_matching_the_sha512() { + let manifest = br#"{"name":"left-pad","dependencies":{"is-odd":"^3.0.0"}}"#; + let bytes = tgz(&[("package/package.json", manifest)]); let sri = sha512_sri_of(&bytes); - assert_eq!( - decode_hosted_npm_sha1(&bytes, &sri).unwrap(), - crate::utils::digest::sha1_hex_of(&bytes) - ); + let artifact = decode_hosted_classic_artifact(&bytes, &sri).unwrap(); + assert_eq!(artifact.sha1, crate::utils::digest::sha1_hex_of(&bytes)); + assert_eq!(artifact.manifest.as_bytes(), manifest); let other = sha512_sri_of(b"other bytes"); - assert!(decode_hosted_npm_sha1(&bytes, &other).is_err()); + assert!(decode_hosted_classic_artifact(&bytes, &other).is_err()); + let no_manifest = tgz(&[("package/index.js", b"1")]); + assert!( + decode_hosted_classic_artifact(&no_manifest, &sha512_sri_of(&no_manifest)).is_err() + ); } #[test] diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 323ccacaa..950a052db 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -315,9 +315,10 @@ pub struct RewriteResult { serde(skip_serializing_if = "std::collections::BTreeSet::is_empty") )] pub confirmed_yarn_berry_uuids: std::collections::BTreeSet, - /// Patch uuids the yarn classic rewriter refused because the project + /// Patch uuids the yarn classic rewriter refused: the project /// configures a `yarn-offline-mirror` (see - /// [`preflight_yarn_classic_hosted`]). Never confirmed. + /// [`preflight_yarn_classic_hosted`]), or the served tarball depends on + /// a descriptor `yarn.lock` doesn't lock (#591). Never confirmed. #[cfg_attr( test, serde(skip_serializing_if = "std::collections::BTreeSet::is_empty") @@ -730,7 +731,7 @@ fn rewriter_groups<'a>( Box::new(move |result| { rewrite_npm_lock(files, overrides, result); plan_hosted(files, overrides, result); - rewrite_yarn_classic_with(files, overrides, yarn_outer, result); + rewrite_yarn_classic_with(files, overrides, artifact_metadata, yarn_outer, result); rewrite_yarn_berry_with_manifests(files, overrides, artifact_metadata, result); rewrite_bun_lock(files, overrides, result); }), @@ -3569,6 +3570,24 @@ pub fn yarn_classic_offline_mirror( /// mirror, so yarn installs the upstream bytes and fails the patched /// integrity (or, `--offline`, never fetches the patched tarball at all). /// `Ok` for a lock that is not classic (the berry rewriter owns those). +/// Whether the project's yarn offline mirror refuses every hosted yarn +/// classic pin of `files`' `yarn.lock` ([`preflight_yarn_classic_hosted`]), +/// so the hosted flows need not read any served tarball for one. +pub fn yarn_classic_hosted_refused( + files: &BTreeMap, + outer: &yarnrc::OuterYarnMirror, +) -> bool { + files.get("yarn.lock").is_some_and(|lock| { + preflight_yarn_classic_hosted( + lock, + files.get(YARNRC_REL).map(String::as_str), + files.get(npmrc::NPMRC_REL).map(String::as_str), + outer, + ) + .is_err() + }) +} + fn preflight_yarn_classic_hosted( lock: &str, yarnrc: Option<&str>, @@ -3677,14 +3696,19 @@ fn rewrite_yarn_classic( rewrite_yarn_classic_with( files, overrides, + &BTreeMap::new(), &yarnrc::OuterYarnMirror::default(), result, ) } +/// `manifests` holds the served tarballs' own package.json texts, keyed by +/// artifact URL (the hosted flows fetch the ones a classic pin reads; a +/// dep with none keeps its lock block's dependency sub-maps as they are). fn rewrite_yarn_classic_with( files: &BTreeMap, overrides: &[DepOverride], + manifests: &BTreeMap, yarn_outer: &yarnrc::OuterYarnMirror, result: &mut RewriteResult, ) { @@ -3779,6 +3803,38 @@ fn rewrite_yarn_classic_with( }); continue; }; + // The served tarball's own package.json (#591): yarn 1 installs the + // dependencies a block's sub-maps name, each through a block of its + // own. A patch that adds a dependency or changes a range needs the + // sub-maps rewritten and every new descriptor locked; no hosted + // rewrite can resolve one, so such a dep is refused, never pinned + // with a graph yarn would install without it. + let served_manifest: Option = manifests + .get(&dep.artifact_url) + .and_then(|text| serde_json::from_str::(text).ok()) + .filter(Value::is_object); + if let Some(pkg) = &served_manifest { + let missing = crate::formats::yarn::classic_deps::unlocked_descriptors(&blocks, pkg); + if !missing.is_empty() { + result + .refused_yarn_classic_uuids + .insert(dep.patch_uuid.clone()); + result.warnings.push(RewriteWarning { + code: "redirect_yarn_classic_dep_manifest_unlocked".into(), + detail: format!( + "the patched {fname}@{} depends on {}, which yarn.lock does not lock; \ + yarn 1 installs only what the lock names, so it is not pinned and \ + stays unpatched. Lock them first (for example `yarn add {}`), then \ + re-run", + dep.version, + missing.join(", "), + missing.join(" ") + ), + }); + continue; + } + } + let mut manifest_rewritten = false; let mut matched_any = false; let mut pinned_any = false; let mut alias_skipped = false; @@ -3979,11 +4035,18 @@ fn rewrite_yarn_classic_with( continue; } pinned_any = true; - let pinned = repin_classic_block( + let mut pinned = repin_classic_block( &block.lines, &format!("{}#{sha1}", dep.artifact_url), &sha512, ); + if let Some(pkg) = &served_manifest { + if !crate::formats::yarn::classic_deps::dep_maps_match(&pinned, pkg) { + pinned = + crate::formats::yarn::classic_deps::with_manifest_dep_maps(&pinned, pkg); + manifest_rewritten = true; + } + } if pinned != block.lines { // Edits record the block's on-disk bytes (CRLF lines for a // CRLF block), so they match what the file really held. @@ -4003,6 +4066,17 @@ fn rewrite_yarn_classic_with( changed = true; } } + if manifest_rewritten { + result.warnings.push(RewriteWarning { + code: "redirect_yarn_classic_dep_manifest_rewritten".into(), + detail: format!( + "the patched {fname}@{} declares different dependencies; its yarn.lock \ + dependency sub-maps were rewritten to match (every descriptor is already \ + locked)", + dep.version + ), + }); + } if !matched_any && !alias_skipped && !copy_skipped { result.warnings.push(RewriteWarning { code: "redirect_yarn_classic_entry_not_found".into(), @@ -11105,7 +11179,13 @@ mod tests { let mut files = BTreeMap::new(); files.insert("yarn.lock".to_string(), classic_lock_two_entries()); let mut r = RewriteResult::default(); - rewrite_yarn_classic_with(&files, std::slice::from_ref(&ovr), outer, &mut r); + rewrite_yarn_classic_with( + &files, + std::slice::from_ref(&ovr), + &BTreeMap::new(), + outer, + &mut r, + ); assert!( r.files.is_empty() && r.edits.is_empty(), "{outer:?}: {:?}", @@ -11136,7 +11216,13 @@ mod tests { "yarn-offline-mirror false\n".to_string(), ); let mut r = RewriteResult::default(); - rewrite_yarn_classic_with(&files, std::slice::from_ref(&ovr), &refusing[1], &mut r); + rewrite_yarn_classic_with( + &files, + std::slice::from_ref(&ovr), + &BTreeMap::new(), + &refusing[1], + &mut r, + ); assert!(r.warnings.is_empty(), "{:?}", r.warnings); assert!(r.files["yarn.lock"].contains("left-pad-1.3.0.tgz")); // The full chain drives it too. @@ -20840,6 +20926,102 @@ packages: ); } + /// #591: the hosted classic rewriter reads the served tarball's own + /// package.json. A dependency it adds (or a range it changes to) that + /// no block locks would be dropped by yarn 1, which installs only what + /// the lock names: the dep is refused and nothing is written. When + /// every descriptor is locked, the block's sub-maps are rewritten to + /// the served manifest's. + #[test] + fn issue_591_hosted_classic_checks_the_served_manifest_against_the_lock() { + let url = "http://p.test/is-odd-3.0.1.tgz"; + let ovr = npm_override("is-odd", "3.0.1", url, "sha512-P=="); + let lock = "# yarn lockfile v1\n\n\n\ + is-number@^6.0.0:\n version \"6.0.0\"\n \ + resolved \"https://registry.yarnpkg.com/is-number/-/is-number-6.0.0.tgz#aaaa\"\n\n\ + is-odd@3.0.1:\n version \"3.0.1\"\n \ + resolved \"https://registry.yarnpkg.com/is-odd/-/is-odd-3.0.1.tgz#bbbb\"\n \ + integrity sha512-UP==\n dependencies:\n is-number \"^6.0.0\"\n"; + let run = |lock: &str, manifest: &str| { + let files = BTreeMap::from([("yarn.lock".to_string(), lock.to_string())]); + let manifests = BTreeMap::from([(url.to_string(), manifest.to_string())]); + let mut r = RewriteResult::default(); + rewrite_yarn_classic_with( + &files, + std::slice::from_ref(&ovr), + &manifests, + &yarnrc::OuterYarnMirror::default(), + &mut r, + ); + r + }; + + // Added dependency, and a changed range: refused, nothing written. + for (manifest, missing) in [ + ( + r#"{"name":"is-odd","dependencies":{"is-number":"^6.0.0","wow":"^1.0.0"}}"#, + "wow@^1.0.0", + ), + ( + r#"{"name":"is-odd","dependencies":{"is-number":"^7.0.0"}}"#, + "is-number@^7.0.0", + ), + ] { + let r = run(lock, manifest); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "{manifest}: {:?}", + r.files + ); + assert_eq!( + warning_codes(&r), + vec!["redirect_yarn_classic_dep_manifest_unlocked"], + "{manifest}" + ); + assert!(r.warnings[0].detail.contains(missing), "{:?}", r.warnings); + assert!(r.refused_yarn_classic_uuids.contains(&ovr.patch_uuid)); + } + + // The served manifest declares what the lock already says: a plain + // pin, sub-map untouched. + let r = run( + lock, + r#"{"name":"is-odd","dependencies":{"is-number":"^6.0.0"}}"#, + ); + assert!(r.warnings.is_empty(), "{:?}", r.warnings); + assert!( + r.files["yarn.lock"].contains(&format!( + " resolved \"{url}#{NPM_SHA1}\"\n integrity sha512-P==\n \ + dependencies:\n is-number \"^6.0.0\"\n" + )), + "{:?}", + r.files + ); + + // The new range is locked: the sub-map follows the served manifest. + let locked = format!( + "{lock}\nis-number@^7.0.0:\n version \"7.0.0\"\n \ + resolved \"https://registry.yarnpkg.com/is-number/-/is-number-7.0.0.tgz#cccc\"\n" + ); + let r = run( + &locked, + r#"{"name":"is-odd","dependencies":{"is-number":"^7.0.0"}}"#, + ); + assert_eq!( + warning_codes(&r), + vec!["redirect_yarn_classic_dep_manifest_rewritten"] + ); + assert!( + r.files["yarn.lock"].contains(&format!( + " resolved \"{url}#{NPM_SHA1}\"\n integrity sha512-P==\n \ + dependencies:\n is-number \"^7.0.0\"\n" + )), + "{:?}", + r.files + ); + assert!(r.refused_yarn_classic_uuids.is_empty()); + } + /// #558: a yarn classic pin without a sha1 would have no `#` /// fragment, so yarn 1 would file the hosted tarball in the cache slot /// of a fragmentless upstream copy and install its bytes. The rewriter diff --git a/crates/socket-patch-core/src/vendor/npm_common.rs b/crates/socket-patch-core/src/vendor/npm_common.rs index 0a82bf702..c1d03a1ff 100644 --- a/crates/socket-patch-core/src/vendor/npm_common.rs +++ b/crates/socket-patch-core/src/vendor/npm_common.rs @@ -726,12 +726,18 @@ pub(super) async fn done_failure_unstage( uuid_dir_rel: &str, uuid_dir_preexisted: bool, ) -> VendorOutcome { + unstage(project_root, uuid_dir_rel, uuid_dir_preexisted).await; + done_failure(purl, error) +} + +/// Remove the uuid dir a run staged, unless it existed before the run (a +/// same-uuid re-vendor's dir may still be referenced by live wiring). +async fn unstage(project_root: &Path, uuid_dir_rel: &str, uuid_dir_preexisted: bool) { if !uuid_dir_preexisted { let uuid_dir = project_root.join(uuid_dir_rel); let _ = remove_tree(&uuid_dir).await; super::common::prune_empty_vendor_levels(&uuid_dir).await; } - done_failure(purl, error) } /// The vendor ledger tail every npm flavor shares once its wiring is on @@ -831,6 +837,19 @@ pub(super) trait NpmLockBackend { warnings: &mut Vec, ) -> Result, String>; + /// A refusal for a patch whose rewritten `package.json` (`staged_pkg`) + /// the flavor's lock can't follow, checked after staging and before any + /// wiring is written (the driver unstages the uuid dir). `None`: wire + /// as usual. + fn manifest_refusal( + &self, + _plan: &Self::Plan, + _staged_pkg: &Value, + _coords: &NpmCoords, + ) -> Option { + None + } + /// The advisory for a patch that rewrites the package's own /// `package.json`, whose mirrors the flavor's lock either recomputes or /// keeps. @@ -904,6 +923,20 @@ pub(super) async fn vendor_npm_family( (coords.name.as_str(), coords.version.as_str()) ); + if let Some(refusal) = staged + .staged_pkg_json + .as_ref() + .and_then(|pkg| backend.manifest_refusal(&plan, pkg, &coords)) + { + unstage( + project_root, + &coords.uuid_dir_rel, + staged.uuid_dir_preexisted, + ) + .await; + return refusal; + } + let cx = WireCx { project_root, coords: &coords, diff --git a/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs b/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs index 9e9d84594..93a7339cf 100644 --- a/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs @@ -28,9 +28,9 @@ use serde_json::Value; use crate::constants::SOCKET_DIR; use crate::formats::yarn::blocks::{ - block_eol, body_field_line, classic_field, repin_classic_block, replace_block, scan_blocks, - LockBlock, + block_eol, classic_field, repin_classic_block, replace_block, scan_blocks, LockBlock, }; +use crate::formats::yarn::classic_deps::{unlocked_descriptors, with_manifest_dep_maps}; use crate::formats::yarn::patterns::{classic_key_real_name, split_key_patterns}; use crate::formats::yarn::source::{classic_copy_source, CopySource}; use crate::manifest::schema::PatchRecord; @@ -222,6 +222,29 @@ impl NpmLockBackend for YarnClassicBackend { })) } + /// #591: yarn 1 installs a package's dependencies from the lock, so a + /// patch whose `package.json` adds a dependency or changes a range + /// needs a lock block for every new descriptor. Vendoring can't + /// resolve one (the sub-map rewrite alone leaves it dangling: online + /// frozen installs fetch it unpinned, offline ones fail, and every + /// plain `yarn install` re-saves the lock), so it refuses instead. + fn manifest_refusal( + &self, + plan: &YarnClassicPlan, + staged_pkg: &Value, + coords: &NpmCoords, + ) -> Option { + let blocks = scan_blocks_shared(&plan.text); + let missing = unlocked_descriptors(&blocks, staged_pkg); + if missing.is_empty() { + return None; + } + Some(refused( + "vendor_dep_manifest_unlocked", + unlocked_detail(&coords.name, &coords.version, &missing), + )) + } + fn manifest_warning(&self, name: &str, version: &str) -> VendorWarning { VendorWarning::new( "vendor_dep_manifest_rewritten", @@ -234,6 +257,19 @@ impl NpmLockBackend for YarnClassicBackend { } } +/// The refusal detail for a patch whose `package.json` declares +/// dependencies `yarn.lock` doesn't lock (#591). +fn unlocked_detail(name: &str, version: &str, missing: &[String]) -> String { + format!( + "the patch rewrites {name}@{version}'s package.json to depend on {}, which \ + {YARN_LOCK} does not lock; yarn 1 would install them unpinned (and fail \ + `--offline` installs), so {name}@{version} was not vendored. Lock them first \ + (for example `yarn add {}`), then re-run", + missing.join(", "), + missing.join(" ") + ) +} + /// [`vendor_yarn_classic`]'s defensive re-sniff: the flavor router already /// separates classic from berry, but rewriting a berry lock with classic /// grammar would corrupt it — never proceed past a `__metadata:` key. @@ -883,44 +919,10 @@ fn rewrite_classic_block( staged_pkg: Option<&Value>, ) -> Vec { let pinned = repin_classic_block(lines, resolved_value, integrity_value); - let Some(pkg) = staged_pkg else { - return pinned; - }; - let mut out = Vec::with_capacity(pinned.len()); - let mut i = 0; - while i < pinned.len() { - if i > 0 - && body_field_line(&pinned[i]) - .is_some_and(|r| r == "dependencies:" || r == "optionalDependencies:") - { - // Drop the stale sub-map (header + 4-space entries); the - // recomputed ones are appended below in yarn's order. - i += 1; - while i < pinned.len() && body_field_line(&pinned[i]).is_none() { - i += 1; - } - continue; - } - out.push(pinned[i].clone()); - i += 1; + match staged_pkg { + Some(pkg) => with_manifest_dep_maps(&pinned, pkg), + None => pinned, } - for field in ["dependencies", "optionalDependencies"] { - let Some(map) = pkg.get(field).and_then(Value::as_object) else { - continue; - }; - if map.is_empty() { - continue; - } - out.push(format!(" {field}:")); - let mut keys: Vec<&String> = map.keys().collect(); - keys.sort_unstable(); - for k in keys { - if let Some(range) = map.get(k).and_then(Value::as_str) { - out.push(format!(" {} \"{range}\"", quote_yarn_key(k))); - } - } - } - out } /// Does this block's `resolved` already point into `.socket/vendor/npm/` @@ -978,23 +980,6 @@ pub(super) fn forget_block_scans() { BLOCK_MEMO.invalidate(); } -/// yarn v1's lockfile key quoting (stringify.js `shouldWrapKey`): wrap when -/// the key would not parse bare. -fn quote_yarn_key(key: &str) -> String { - let needs = key.is_empty() - || key.starts_with("true") - || key.starts_with("false") - || !key.chars().next().is_some_and(|c| c.is_ascii_alphabetic()) - || key - .chars() - .any(|c| matches!(c, ':' | ' ' | '\n' | '\t' | '\\' | '"' | ',' | '[' | ']')); - if needs { - format!("\"{key}\"") - } else { - key.to_string() - } -} - pub(super) fn lines_to_json(lines: &[String]) -> Value { Value::Array(lines.iter().map(|l| Value::String(l.clone())).collect()) } @@ -1429,10 +1414,17 @@ left-pad@^1.3.0: integrity sha512-XI5MPzVNApjAyhQzphX8BkmKsKUxD4LdyK24iZeQGinBN9yTQT3bFlCBy/aVx2HrNcqQGsdot8ghrjyrvMCoEA== dependencies: old-dep "^1.0.0" + +wow@^1.0.0: + version "1.0.0" + +"@scope/opt@^2.0.0": + version "2.0.0" "#; let mut fx = fixture_with_lock(lock).await; - // The patch rewrites package.json: new dependency + an optional one. + // The patch rewrites package.json: new dependency + an optional one, + // both already locked (#591: an unlocked one is refused). let before: &[u8] = br#"{"name":"left-pad","version":"1.3.0"}"#; let after: &[u8] = br#"{"name":"left-pad","version":"1.3.0","dependencies":{"wow":"^1.0.0"},"optionalDependencies":{"@scope/opt":"^2.0.0"}}"#; let after_hash = compute_git_sha256_from_bytes(after); @@ -1465,12 +1457,91 @@ left-pad@^1.3.0: ); } + /// A lock block for the `wow@^1.0.0` descriptor the manifest-rewriting + /// fixtures' patches add (#591: vendoring needs it locked). + const WOW_BLOCK: &str = "\nwow@^1.0.0:\n version \"1.0.0\"\n"; + + /// Stage a patch on `fx` that rewrites left-pad's package.json to + /// `after`. + async fn patch_manifest(fx: &mut Fixture, after: &[u8]) { + let before: &[u8] = br#"{"name":"left-pad","version":"1.3.0"}"#; + let after_hash = compute_git_sha256_from_bytes(after); + tokio::fs::write(fx.root().join(".socket/blobs").join(&after_hash), after) + .await + .unwrap(); + fx.record.files.insert( + "package/package.json".to_string(), + PatchFileInfo { + before_hash: compute_git_sha256_from_bytes(before), + after_hash, + }, + ); + } + + /// #591: a patch whose package.json adds a dependency yarn.lock doesn't + /// lock is refused before any wiring: rewriting the sub-map alone would + /// leave a dangling descriptor yarn 1 installs unpinned (and an + /// `--offline` install fails). The lock and the vendor dir are left as + /// they were. + #[tokio::test] + async fn issue_591_added_dependency_without_a_lock_block_is_refused() { + let mut fx = fixture_with_lock(Y2_BEFORE).await; + patch_manifest( + &mut fx, + br#"{"name":"left-pad","version":"1.3.0","dependencies":{"is-odd":"^3.0.0"}}"#, + ) + .await; + let detail = expect_refused(fx.vendor(false).await, "vendor_dep_manifest_unlocked"); + assert!( + detail.contains("is-odd@^3.0.0") && detail.contains("yarn add is-odd@^3.0.0"), + "{detail}" + ); + assert_eq!(fx.lock_text().await, Y2_BEFORE, "lock untouched"); + assert!( + !fx.root().join(".socket/vendor/npm").join(UUID).exists(), + "the staged tarball is unstaged" + ); + } + + /// #591 (range change): the patch moves an existing dependency to a + /// range no block locks. Same refusal: the lock's `is-number@^6.0.0` + /// block can't stand in for `^7.0.0`. + #[tokio::test] + async fn issue_591_changed_range_without_a_lock_block_is_refused() { + let lock = format!( + "{Y2_BEFORE} dependencies:\n is-number \"^6.0.0\"\n\n\ + is-number@^6.0.0:\n version \"6.0.0\"\n" + ); + let mut fx = fixture_with_lock(&lock).await; + patch_manifest( + &mut fx, + br#"{"name":"left-pad","version":"1.3.0","dependencies":{"is-number":"^7.0.0"}}"#, + ) + .await; + let detail = expect_refused(fx.vendor(false).await, "vendor_dep_manifest_unlocked"); + assert!(detail.contains("is-number@^7.0.0"), "{detail}"); + assert!(!detail.contains("^6.0.0"), "{detail}"); + assert_eq!(fx.lock_text().await, lock, "lock untouched"); + + // Once the new range is locked, the patch vendors with the + // sub-map rewritten to it. + let locked = format!("{lock}\nis-number@^7.0.0:\n version \"7.0.0\"\n"); + tokio::fs::write(fx.lock_path(), &locked).await.unwrap(); + let (result, entry, _) = expect_done(fx.vendor(false).await); + assert!(result.success && entry.is_some(), "{:?}", result.error); + let text = fx.lock_text().await; + assert!( + text.contains(" dependencies:\n is-number \"^7.0.0\"\n"), + "{text}" + ); + } + /// #920: the `package.json` advisory is emitted once, by the run that /// wires — an in-sync re-run of a manifest-rewriting patch is a quiet /// AlreadyPatched. #[tokio::test] async fn manifest_rewriting_rerun_is_in_sync_without_the_manifest_warning() { - let mut fx = fixture_with_lock(Y2_BEFORE).await; + let mut fx = fixture_with_lock(&format!("{Y2_BEFORE}{WOW_BLOCK}")).await; let before: &[u8] = br#"{"name":"left-pad","version":"1.3.0"}"#; let after: &[u8] = br#"{"name":"left-pad","version":"1.3.0","dependencies":{"wow":"^1.0.0"}}"#; @@ -1514,6 +1585,9 @@ left-pad@^1.3.0: integrity sha512-XI5MPzVNApjAyhQzphX8BkmKsKUxD4LdyK24iZeQGinBN9yTQT3bFlCBy/aVx2HrNcqQGsdot8ghrjyrvMCoEA== dependencies: old-dep "^1.0.0" + +wow@^1.0.0: + version "1.0.0" "#; let mut fx = fixture_with_lock(lock).await; let before: &[u8] = br#"{"name":"left-pad","version":"1.3.0"}"#; @@ -2421,12 +2495,6 @@ left-pad@^1.3.0: ); assert_eq!(pattern_real_name("alias@npm:left-pad"), Some("left-pad")); assert_eq!(pattern_real_name("no-at-sign"), None); - - // yarn's key quoting rule. - assert_eq!(quote_yarn_key("left-pad"), "left-pad"); - assert_eq!(quote_yarn_key("@scope/x"), "\"@scope/x\""); - assert_eq!(quote_yarn_key("3d-lib"), "\"3d-lib\""); - assert_eq!(quote_yarn_key("true-lib"), "\"true-lib\""); } #[test] diff --git a/docs/ecosystems.md b/docs/ecosystems.md index e5f64f1be..ab023d5a8 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -136,10 +136,20 @@ The backticked slug in each row is the value `-e`/`--ecosystems` accepts (e.g. rewritten to the hosted tarball. `resolved` always carries the tarball's `#` fragment: yarn 1 names its cache slot after it, so a fragmentless URL would share the slot of an unpatched copy of the same version and a warm - cache would install those bytes (or fail the integrity check). When the grant - carries no sha1, the scan downloads the served tarball, checks it against the - grant's sha512 and pins the sha1 of those bytes. If that download or check - fails, the patch is skipped as `npm_tarball_unavailable`. A project that sets `yarn-offline-mirror` + cache would install those bytes (or fail the integrity check). For an entry it + hasn't pinned yet (or a grant with no sha1), the scan downloads the served + tarball and checks it against the grant's sha512. It pins the sha1 of those + bytes when the grant has none, and it compares the tarball's own + `package.json` with the lock: yarn 1 installs only the dependencies the lock + names, so when the patch adds a dependency (or changes a range) that no + `yarn.lock` block locks, the pin is refused with + `redirect_yarn_classic_dep_manifest_unlocked` (lock the new descriptor first, + for example with `yarn add`, then re-run). When every descriptor is already + locked, the entry's dependency sub-maps are rewritten to match + (`redirect_yarn_classic_dep_manifest_rewritten`). Vendored mode refuses the + same case with `vendor_dep_manifest_unlocked`. If the download, the check or + the `package.json` read fails, the patch is skipped as + `npm_tarball_unavailable`. A project that sets `yarn-offline-mirror` is refused with `redirect_yarn_classic_offline_mirror`. The mirror is resolved the way yarn 1 resolves it: the project's `.yarnrc` / `.npmrc`, the user's (`~/.yarnrc`, which `yarn config set` writes, and `~/.npmrc`),