diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 6506b62ad..9a12353ff 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -1442,6 +1442,7 @@ command's integration tests check its output. | `vendor_ledger_missing` | `failed` (artifact-level: `uuid` + `details.{ecosystem,path}`, no purl) | repair (v5.0) and `vendor --check`: a lockfile references `.socket/vendor///` but the vendor ledger has no entry for it; repair no longer rebuilds ledger entries from lockfiles. Recovery: restore `.socket/vendor/state.json` from version control and re-run `repair`, or `git checkout -- ` and re-vendor. | | `vendor_wiring_unknown_revert_blocked` | `skipped` (beside the `failed`/`revert_failed` event) | vendor --revert: the ledger entry was reconstructed by a pre-v5 `repair` without wiring records and the live lockfile still resolves through the artifact — the revert refuses (fail-closed) instead of deleting a tarball the lock points at. Recovery: `socket-patch repair`, then restore the pre-vendor lock (or re-lock without the override) and re-run the revert. repair: an npm ledger entry whose `flavor` this release does not know (written by a newer socket-patch) is skipped, never health-checked or rebuilt, and the artifact, wiring and ledger stay as found (a lone `skipped` event; the run's exit is unaffected). Recovery: upgrade socket-patch. | | `stale_install` | `skipped` | vex (in-run `scan --mode hosted --vex`): a hosted stale-install probe found positively unpatched installed bytes, so the purl is omitted even under `--vex-no-verify` (see the gem / Python stale-install guards). | +| `dependency_not_installed` | `skipped` | vex: a hosted npm-family copy carries every patched file, but the patch changes its `package.json` and a dependency it declares (other than an optional one) does not resolve from the copy (no `node_modules/` in it or above its real path), for example because the lockfile never locked it (a yarn classic pin written before the served-manifest check, #1462). Omitted until the dependency is installed. | | `record_unavailable` | `skipped` | vex (manifest-less): a lockfile-wired patch has no local record (manifest, this run's hosted records or a pre-v5 redirect ledger, vendor ledger) and none could be fetched — `--offline`, transport error, 404, or a refused (paid) patch. Omitted, never attested from the `socket-patch.vendor.json` marker. | | `record_mismatch` | `skipped` | vex (manifest-less): the record found for a wired patch names another package or another patch uuid than the wiring. | | `vendor_unwired` | `skipped` | vex: a vendor-ledger entry whose committed artifact no lockfile/config wires any more (reverted lock, leftover ledger or artifact). Applies under `--no-verify` too. | @@ -1499,7 +1500,7 @@ command's integration tests check its output. | `vendor_bun_reinstall_required` | `skipped` (advisory; human: `Warning: …`); rollback/remove `warnings[]`; `scan --prune` top-level `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. Also `failed` on `repair` (and `vendor_check_failed` on `vendor --check`) for a lock an older release already vendored that way, with a block wired to the entry naming a descriptor no block locks and the recorded pre-vendor block did not (#1379); the lock is left as it is. Remedy: `yarn install` (without `--frozen-lockfile`) to lock them, then commit `yarn.lock`. | -| `redirect_yarn_classic_dep_manifest_unlocked` | `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_unlocked` | `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. The served tarball is read on every run, so a pin an earlier release wrote without this check (#1462) gets the same warning and is never confirmed either; it stays in the lock until the dependency is locked or the pin is rolled back. Same remedy as `vendor_dep_manifest_unlocked`. | | `redirect_yarn_classic_dep_manifest_rewritten` | `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, the npm-family tarball flavors — npm, pnpm, bun, yarn classic, yarn berry — NuGet, and the JVM trees of Maven, Gradle, sbt and scala-cli): inside a git work tree, `git check-ignore --no-index` reports the new artifact's uuid directory (JVM: its tree root, such as `.socket/vendor/maven2`) 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, dry run included. A file rule such as `*.tgz`, `*.nupkg` or `*.jar` is overridden by the `!*` `.gitignore` vendoring writes into the uuid dir or tree root; if a written tarball, directory payload or nupkg still reads as ignored, the run refuses and removes the uuid dir it created. | | `vendor_artifact_gitignore_unchecked` | warning | vendor (vlt, the npm-family tarball flavors and NuGet): 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. | diff --git a/crates/socket-patch-cli/src/commands/vex.rs b/crates/socket-patch-cli/src/commands/vex.rs index a74652229..4cac3b911 100644 --- a/crates/socket-patch-cli/src/commands/vex.rs +++ b/crates/socket-patch-cli/src/commands/vex.rs @@ -23,6 +23,7 @@ use clap::Args; use socket_patch_core::api::client::ApiClient; use socket_patch_core::manifest::schema::{PatchManifest, PatchRecord}; use socket_patch_core::telemetry::{track_vex_failed, track_vex_generated, TelemetryAuth}; +use socket_patch_core::vex::verify::DEPENDENCY_NOT_INSTALLED; use socket_patch_core::vex::{ build_document, detect_product, BuildOptions, Document, FailedPatch, VendorContext, VerifyOutcome, @@ -1781,6 +1782,10 @@ fn omission_phrase(reason: &str) -> &'static str { against" } "stale_install" => "the installed copy is not patched", + DEPENDENCY_NOT_INSTALLED => { + "the patched package.json declares a dependency that is not installed beside \ + the package" + } "mirror_overrides_source" => { "a Bundler mirror overrides the hosted source and installed bytes were not verified" } @@ -1986,6 +1991,7 @@ mod tests { "vendor_manifest_unverifiable", "stale_install", "mirror_overrides_source", + DEPENDENCY_NOT_INSTALLED, RECORD_UNAVAILABLE, RECORD_MISMATCH, VENDOR_UNWIRED, diff --git a/crates/socket-patch-cli/tests/e2e_vex_lockfile/yarn.rs b/crates/socket-patch-cli/tests/e2e_vex_lockfile/yarn.rs index 55b365309..a8dfc7436 100644 --- a/crates/socket-patch-cli/tests/e2e_vex_lockfile/yarn.rs +++ b/crates/socket-patch-cli/tests/e2e_vex_lockfile/yarn.rs @@ -672,6 +672,52 @@ fn hosted_lockfile_only_matrix() { } } +/// #1462: a hosted pin an earlier release wrote for a patch whose +/// package.json adds a dependency (`is-odd`) the lock never locked. yarn +/// installs the patched bytes without that dependency, so although every +/// patched file verifies, the patch is omitted until the dependency is +/// installed beside it. +#[test] +fn hosted_patch_adding_an_uninstalled_dependency_is_omitted() { + const MANIFEST: &str = "package/package.json"; + let pristine_manifest = format!(r#"{{"name":"{NAME}","version":"{VERSION}"}}"#); + let patched_manifest = format!( + r#"{{"name":"{NAME}","version":"{VERSION}","dependencies":{{"is-odd":"^3.0.0"}}}}"# + ); + let mut view = patch_view(UUID, PURL); + view["files"][MANIFEST] = serde_json::json!({ + "beforeHash": compute_git_sha256_from_bytes(pristine_manifest.as_bytes()), + "afterHash": compute_git_sha256_from_bytes(patched_manifest.as_bytes()), + }); + let (rt, server) = serve_patch_views(vec![(UUID, view)]); + for flavor in FLAVORS { + let tmp = tempfile::tempdir().unwrap(); + let cwd = tmp.path(); + hosted_checkout(cwd, flavor); + install(cwd, PATCHED); + put( + cwd, + "node_modules/left-pad/package.json", + patched_manifest.as_bytes(), + ); + + let (code, env) = vex_json(cwd, &["--proxy-url", &server.uri()]); + let cell = format!("{flavor:?} dependency not installed"); + assert_omitted(cwd, code, &env, "dependency_not_installed", &cell); + + // Hoisted beside it, the dependency resolves: attested. + put( + cwd, + "node_modules/is-odd/package.json", + br#"{"name":"is-odd","version":"3.0.1"}"#, + ); + let (code, env) = vex_json(cwd, &["--proxy-url", &server.uri()]); + let cell = format!("{flavor:?} dependency installed"); + assert_attested(cwd, code, &env, UUID, "redirected", &cell); + } + drop(rt); +} + // ────────────────────────────────────────────────────────────────────── // VENDORED — lockfile only (cells a, b, e) // ────────────────────────────────────────────────────────────────────── 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 2e4c469e6..393ad9ea5 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 @@ -341,3 +341,100 @@ async fn mock_api_with_sha1(server: &MockServer, tarball: &[u8]) { .await; mock_api(server, tarball, Some(tarball.to_vec())).await; } + +/// The patched tarball of #1462: its package.json adds `is-odd`, which +/// [`LOCK`] doesn't lock. +fn tarball_adding_a_dependency() -> Vec { + 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"), + ]) +} + +/// [`write_classic_project`] with `left-pad` already pinned to the hosted +/// artifact the way v4.0.0 pinned it: `resolved` carries the tarball's +/// sha1, and the block's dependencies were never checked against the +/// served package.json. Returns the pinned lock. +fn write_v4_pinned_project(root: &Path, api: &str, tarball: &[u8]) -> String { + write_classic_project(root); + let url = format!("{api}/patch/npm/left-pad/1.3.0/{TOKEN}/{UUID}/left-pad-1.3.0.tgz"); + let lock = format!( + "# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.\n\ + # yarn lockfile v1\n\n\n\ + left-pad@1.3.0:\n version \"1.3.0\"\n \ + resolved \"{url}#{}\"\n integrity {}\n", + sha1_hex(tarball), + sha512_sri(tarball) + ); + std::fs::write(root.join("yarn.lock"), &lock).unwrap(); + lock +} + +/// #1462: a lock an earlier release pinned to a tarball whose package.json +/// adds a dependency the lock doesn't lock. Yarn installs the patched +/// package without it, so the rescan must not confirm the pin: it names +/// the unlocked dependency and reports the patch as not hosted, whether +/// the grant carries the sha1 (the tarball would otherwise never be +/// read) or not. +async fn assert_v4_pin_with_unlocked_dependency_is_not_confirmed(with_sha1: bool) { + let server = MockServer::start().await; + let tarball = tarball_adding_a_dependency(); + if with_sha1 { + mock_api_with_sha1(&server, &tarball).await; + } else { + mock_api(&server, &tarball, Some(tarball.clone())).await; + } + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("proj"); + let lock = write_v4_pinned_project(&root, &server.uri(), &tarball); + + let (code, doc, stderr) = scan(&root, &server.uri()); + let warning = doc["warnings"] + .as_array() + .unwrap() + .iter() + .find(|w| w["code"] == "redirect_yarn_classic_dep_manifest_unlocked") + .unwrap_or_else(|| panic!("refusal warning (sha1: {with_sha1}): {doc:#}")); + let detail = warning["detail"].as_str().unwrap(); + assert!( + detail.contains("is-odd@^3.0.0") && detail.contains("an earlier run's pin of it"), + "{detail}" + ); + assert!( + common::envelope::hosted_pins(&doc).is_empty(), + "the stale pin is not confirmed (sha1: {with_sha1}): {doc:#}" + ); + assert!( + !common::envelope::mode_events(&doc, "hosted") + .iter() + .any(|e| e["purl"] == PURL && e["action"] == "applied"), + "no applied event (sha1: {with_sha1}): {doc:#}" + ); + // Like a fresh refusal, the patch is listed as not hosted; the exit + // code is unchanged pending the hosted exit-policy decision (#704). + assert!( + common::envelope::mode_events(&doc, "hosted") + .iter() + .any(|e| e["purl"] == PURL && e["action"] == "skipped"), + "listed as not hosted (sha1: {with_sha1}): {doc:#}" + ); + assert_eq!(code, 0, "{doc:#}\n{stderr}"); + assert_eq!( + std::fs::read_to_string(root.join("yarn.lock")).unwrap(), + lock, + "the scan never rewrites the earlier pin" + ); +} + +#[tokio::test] +async fn issue_1462_v4_pin_with_unlocked_dependency_is_not_confirmed_sha1_grant() { + assert_v4_pin_with_unlocked_dependency_is_not_confirmed(true).await; +} + +#[tokio::test] +async fn issue_1462_v4_pin_with_unlocked_dependency_is_not_confirmed_sha512_only_grant() { + assert_v4_pin_with_unlocked_dependency_is_not_confirmed(false).await; +} diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index eea16d612..2db7b72c6 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -1260,10 +1260,12 @@ pub fn yarn_berry_manifest_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). +/// pins the artifact already or locks the package as a registry copy, and +/// the grant carries a sha512. The tarball gives the sha1 for the +/// `resolved` fragment yarn 1 keys its cache slot on when the grant has +/// none (#558), and its own dependencies are checked against the lock +/// (#591) on every run: a pin an earlier release wrote was never checked +/// (#1462). /// 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>( @@ -1285,11 +1287,10 @@ pub fn yarn_classic_artifact_targets<'a>( /// [`yarn_classic_artifact_targets`] over a classic `lock`, reading its /// registry copies through `read_copies`. The lock is read in one pass for -/// the artifacts it pins already, once a dep with a sha1 asks; the -/// registry-copy test reads every block, so it runs after that, at most -/// once per lock, and only when some dep still needs it (#1423): a rescan -/// whose grants all carry a sha1 and are pinned already never reads the -/// blocks. +/// the artifacts it pins already; the registry-copy test reads every +/// block, so it runs after that, at most once per lock, and only when some +/// dep is not pinned yet (#1423): a rescan whose deps are all pinned +/// already never reads the blocks. fn select_classic_artifacts<'a>( candidates: &'a [Candidate], lock: &str, @@ -1304,15 +1305,14 @@ fn select_classic_artifacts<'a>( .map(|c| &c.dep) .filter(|dep| dep.ecosystem == "npm" && dep.integrity.sha512.is_some()) .filter(|dep| { - dep.integrity.sha1.is_none() - || !pinned - .get_or_insert_with(|| quoted_fragment_urls(lock)) - .contains(dep.artifact_url.as_str()) - }) - .filter(|dep| { - registry_copies - .get_or_insert_with(|| read_copies.take().map_or_else(HashSet::new, |r| r(lock))) - .contains(&(crate::patch::redirect::full_name(dep), dep.version.clone())) + pinned + .get_or_insert_with(|| quoted_fragment_urls(lock)) + .contains(dep.artifact_url.as_str()) + || registry_copies + .get_or_insert_with(|| { + read_copies.take().map_or_else(HashSet::new, |r| r(lock)) + }) + .contains(&(crate::patch::redirect::full_name(dep), dep.version.clone())) }) .filter(|dep| seen.insert(dep.artifact_url.clone())) .collect() @@ -3250,10 +3250,10 @@ mod tests { ); } - /// #1423: the served tarball is read only for a dep the lock locks as - /// a registry copy and doesn't pin to the artifact yet. The lock's - /// blocks are read at most once however many deps there are, and not - /// at all on a rescan whose deps are all pinned already. + /// #1423: the served tarball is read only for a dep the lock pins to + /// the artifact or locks as a registry copy. The lock's blocks are + /// read at most once however many deps there are, and not at all on a + /// rescan whose deps are all pinned already. #[test] fn classic_artifact_targets_read_the_lock_blocks_at_most_once() { let mut lock = String::from("# yarn lockfile v1\n\n"); @@ -3277,13 +3277,17 @@ mod tests { classic_registry_copies(lock) }; - // Every dep pinned to its artifact already: nothing to fetch, and - // the blocks are never read. + // Every dep pinned to its artifact already: each tarball is still + // read, since an earlier release pinned it without checking its + // dependencies (#1462), but the blocks are never read. let mut pinned = lock.clone(); for c in &candidates { pinned.push_str(&format!("# \"{}#s1\"\n", c.dep.artifact_url)); } - assert!(select_classic_artifacts(&candidates, &pinned, counted).is_empty()); + assert_eq!( + select_classic_artifacts(&candidates, &pinned, counted).len(), + n + ); assert_eq!(reads.get(), 0); // Unpinned: every registry copy is selected once per artifact; a diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 65c0d9067..fb87b2218 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -4098,13 +4098,24 @@ fn rewrite_yarn_classic_with( result .refused_yarn_classic_uuids .insert(dep.patch_uuid.clone()); + // A pin an earlier release wrote without this check (#1462) + // is left as found, but never confirmed: yarn installs the + // patched package from it without these dependencies. + let outcome = if blocks.iter().any(|b| { + classic_field(&b.lines, "resolved") + .is_some_and(|r| r.split('#').next() == Some(dep.artifact_url.as_str())) + }) { + "an earlier run's pin of it is not confirmed: yarn 1 installs only what the \ + lock names, so it installs the patched package without them" + } else { + "yarn 1 installs only what the lock names, so it is not pinned and stays \ + unpatched" + }; 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", + {outcome}. Lock them first (for example `yarn add {}`), then re-run", dep.version, missing.join(", "), missing.join(" ") diff --git a/crates/socket-patch-core/src/vex/verify.rs b/crates/socket-patch-core/src/vex/verify.rs index 1d1c9f2b2..958eb8158 100644 --- a/crates/socket-patch-core/src/vex/verify.rs +++ b/crates/socket-patch-core/src/vex/verify.rs @@ -479,7 +479,73 @@ async fn verify_hosted_copies( } None => record, }; - verify_every_copy(&copies.paths, record).await + verify_every_copy(&copies.paths, record).await?; + if Ecosystem::from_purl(purl) == Some(Ecosystem::Npm) && touches_npm_manifest(record) { + for path in &copies.paths { + if unresolved_npm_dependency(path).await.is_some() { + return Err(DEPENDENCY_NOT_INSTALLED.to_string()); + } + } + } + Ok(()) +} + +/// The omission tag of a hosted npm copy that carries the patched bytes +/// but whose patched `package.json` declares a dependency node cannot +/// resolve from it (#1462): the lock never locked it, so the package was +/// installed without it. +pub const DEPENDENCY_NOT_INSTALLED: &str = "dependency_not_installed"; + +/// Whether an npm `record` patches the package's own `package.json`, the +/// only way a patch changes the dependencies its package declares. +fn touches_npm_manifest(record: &PatchRecord) -> bool { + record + .files + .keys() + .any(|key| key.strip_prefix("package/").unwrap_or(key) == "package.json") +} + +/// The first `dependencies` entry of the installed npm copy at `pkg_path` +/// that node cannot resolve from it: no `node_modules/` in the copy +/// or in any directory above its real path (node's lookup, which also +/// finds a bundled, hoisted or pnpm/bun store-sibling copy behind a +/// root-linked symlink). Optional dependencies may be absent, and a copy +/// outside a `node_modules` tree (a Plug'n'Play install), an unreadable +/// manifest or a name that is not a safe package name is never evidence. +async fn unresolved_npm_dependency(pkg_path: &Path) -> Option { + let pkg_path = pkg_path.to_path_buf(); + crate::utils::fs::run_blocking(move || { + if !pkg_path + .components() + .any(|c| c.as_os_str() == "node_modules") + { + return None; + } + let text = + crate::utils::fs::read_regular_to_string_sync(&pkg_path.join("package.json")).ok()?; + let pkg: serde_json::Value = serde_json::from_str(&text).ok()?; + let real = std::fs::canonicalize(&pkg_path).ok()?; + let optional = pkg + .get("optionalDependencies") + .and_then(serde_json::Value::as_object); + pkg.get("dependencies")? + .as_object()? + .keys() + .filter(|name| optional.is_none_or(|o| !o.contains_key(*name))) + .filter(|name| crate::vendor::npm_common::is_safe_npm_name(name)) + .find(|name| !node_resolves(&real, name)) + .cloned() + }) + .await +} + +/// Whether node resolves the package `name` from the package directory +/// `from`: `/node_modules/` exists for `from` or an ancestor +/// that is not itself a `node_modules` directory. +fn node_resolves(from: &Path, name: &str) -> bool { + from.ancestors() + .filter(|dir| dir.file_name().is_none_or(|n| n != "node_modules")) + .any(|dir| dir.join("node_modules").join(name).exists()) } /// Whether `rest` (what follows a matched `from` prefix) starts a new name @@ -1868,6 +1934,78 @@ mod tests { dir } + /// #1462: node's lookup from an installed copy finds a bundled, + /// hoisted or pnpm-sibling dependency, also through a root-linked + /// symlink; an optional one may be absent, and a copy outside + /// `node_modules` is no evidence. + #[tokio::test] + async fn unresolved_npm_dependency_follows_node_lookup() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + let put = |rel: &str, text: &str| { + let path = root.join(rel); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(path, text).unwrap(); + }; + let manifest = r#"{"name":"lp","dependencies":{"a":"1","@s/b":"1","opt":"1", + "../../escape":"1"},"optionalDependencies":{"opt":"1"}}"#; + + // Hoisted layout: `a` and `@s/b` at the root; `@s/b` missing first. + put("node_modules/lp/package.json", manifest); + put("node_modules/a/package.json", "{}"); + let hoisted = root.join("node_modules/lp"); + assert_eq!( + unresolved_npm_dependency(&hoisted).await.as_deref(), + Some("@s/b") + ); + // Bundled inside the package itself. + put("node_modules/lp/node_modules/@s/b/package.json", "{}"); + assert_eq!(unresolved_npm_dependency(&hoisted).await, None); + + // pnpm's isolated store: dependencies are siblings of the package. + let store = "node_modules/.pnpm/lp@1.0.0/node_modules"; + put(&format!("{store}/lp/package.json"), manifest); + put(&format!("{store}/a/package.json"), "{}"); + let isolated = root.join(store).join("lp"); + assert_eq!( + unresolved_npm_dependency(&isolated).await.as_deref(), + Some("@s/b") + ); + put(&format!("{store}/@s/b/package.json"), "{}"); + assert_eq!(unresolved_npm_dependency(&isolated).await, None); + + // Not in a node_modules tree: never evidence. + put("unplugged/lp/package.json", manifest); + assert_eq!( + unresolved_npm_dependency(&root.join("unplugged/lp")).await, + None + ); + + // A FIFO where the manifest should be is never opened (it would + // block): no evidence. + #[cfg(unix)] + { + let fifo_dir = root.join("node_modules/fifo"); + std::fs::create_dir_all(&fifo_dir).unwrap(); + let c = + std::ffi::CString::new(fifo_dir.join("package.json").to_str().unwrap()).unwrap(); + assert_eq!(unsafe { libc::mkfifo(c.as_ptr(), 0o644) }, 0); + assert_eq!(unresolved_npm_dependency(&fifo_dir).await, None); + } + + // A root-linked symlink into the store (pnpm, bun isolated, npm + // linked): the lookup starts from the real path. + #[cfg(unix)] + { + std::os::unix::fs::symlink(root.join(store).join("lp"), root.join("node_modules/lp2")) + .unwrap(); + assert_eq!( + unresolved_npm_dependency(&root.join("node_modules/lp2")).await, + None + ); + } + } + fn hosted_ctx(purl: &str, copies: HostedCopies) -> VendorContext { VendorContext { hosted: [(purl.to_string(), copies)].into_iter().collect(),