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`),