From 94066be9793a457257ba7ac13bb0495aba52db87 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 12:26:14 +0000 Subject: [PATCH 1/6] Start fix for #1595 Assisted-by: Claude Code:claude-opus-5-5 From f5cf6e1a0d7527645dd438e3bb2b6deebb5f7124 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 12:54:27 +0000 Subject: [PATCH 2/6] Warn when npm 12 script approvals stop matching npm 12 runs a dependency's install script only when package.json allowScripts approves it, and it matches a version-pinned key such as "esbuild@0.19.12": true (what `npm approve-scripts` writes) only against a registry tarball URL. A hosted scan points the lock at the patch server and a vendored scan at a file: tarball, so the approval silently stopped matching: the next `npm ci` skipped the approved install script (or failed outright under strict-allow-scripts) while the scan exited 0 with no warning. Both modes now read the root allowScripts policy with npm's own matching rules and warn when a patched package with an install script is approved only by version: redirect_npm_allow_scripts (hosted) and vendor_npm_allow_scripts (vendored package-lock). A name-only approval, which npm still honors, and a matching deny raise nothing. The policy is never rewritten, since the only key npm would still accept widens it; the warning names the pinned keys and the name-only remedy. Fixes #1595 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 + .../tests/in_process_redirect.rs | 86 +++++++ .../tests/in_process_vendor.rs | 66 +++++ crates/socket-patch-core/src/hosted/engine.rs | 66 ++++- .../src/patch/redirect/mod.rs | 1 + .../src/patch/redirect/npm_allow_scripts.rs | 237 ++++++++++++++++++ .../src/vendor/npm_flavor.rs | 15 +- .../socket-patch-core/src/vendor/npm_lock.rs | 31 +++ docs/ecosystems.md | 8 + docs/testing/npm-compatibility.md | 7 + 10 files changed, 516 insertions(+), 3 deletions(-) create mode 100644 crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 3756e7abf..f2cf2d4ab 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -1461,6 +1461,7 @@ command's integration tests check its output. | `vendor_workspace_member_skipped` | `skipped` (warning) | vendor / scan / get `--mode vendored` (npm, #688): a lock entry with the package's `name@version` is the project's own source (a workspace member or a `file:` directory), so it is left alone while the lock's registry copies are vendored; patch that source directly if it needs the fix. When it is the only instance, vendoring refuses instead. | | `vendor_copy_unpatched` | `skipped` (warning) | vendor / scan / get `--mode vendored` (yarn berry, #939): the registry entry was wired, but `yarn.lock` also installs the same `name@version` from a `file:` tarball / directory, URL tarball or `portal:` / `link:` dependency declared under another name (`lp2@file:…`), which the name-keyed `resolutions` entry never reaches; the detail names that entry, and that copy stays unpatched (`vex` does not attest the package beside it). | | `vendor_npm_allow_file` | `skipped` (warning, printed as `Warning (vendor_npm_allow_file)`) | vendor / scan / get `--mode vendored` (package-lock, #969): the effective npm `allow-file` setting (env > project `.npmrc` > user > global > builtin) is not `all`, so npm >= 11.14 refuses the vendored `file:` tarball (EALLOWFILE) on install. The setting is respected, never rewritten; the detail names where it comes from and the remedy (`allow-file=all`). `vendor --check` fails the entry for the same reason. | +| `vendor_npm_allow_scripts` | `skipped` (warning, printed as `Warning (vendor_npm_allow_scripts)`) | vendor / scan / get `--mode vendored` (package-lock, #1595): the root package.json `allowScripts` approves a vendored package's install script only by version (`"pkg@x.y.z": true`), which npm 12 matches only against a registry tarball URL, so the next install blocks the approved script (every install fails under `strict-allow-scripts`). Raised only when a lock entry has `hasInstallScript`, no name-only approval (`pkg`, `pkg@*`) covers it and no deny matches. The policy is never rewritten; the detail names the pinned keys and the name-only remedy. | | `vendor_yarn_berry_unsupported` | `failed` | vendor (npm): yarn-berry Plug'n'Play layout; use its native `yarn patch` workflow. | | `vendor_bun_lockb_invalid` | `failed` | vendor / scan / get `--mode vendored`: the binary lock is malformed, unreadable, unsupported or cannot be rewritten safely. The detail names the parser, hash or filesystem error. Refused before patch downloads and before hosted takeover; the package's `failed` event carries `errorCode` and `error`, while `get ` also carries top-level `error.code`. Dry-run predicts the same refusal. | | `vendor_bun_workspace_unsupported` | `failed` | vendor / scan / get `--mode vendored` (bun): the text lock holds `workspace:` packages and its `lockfileVersion` is below 2 — Bun 1.2–1.3 resolve a workspace member's local-tarball path relative to the member; a committed version-2 lock is the proof every consumer runs Bun ≥ 1.4 (deliberate over-approximation: root-only declared packages would install on version 1 too). Detail names the version integer and a version-specific remedy: delete `bun.lock` and re-lock with Bun ≥ 1.4 (an in-place `bun install` keeps the existing version) — then, for a version-1 lock, "or use `--mode hosted`, which accepts version-1 workspace locks"; for a version-0 lock, "or delete `bun.lock`, re-lock with Bun ≥ 1.2 (which writes lockfileVersion 1) and use `--mode hosted`" (hosted refuses version-0 workspace locks, so a bare hosted pointer would send the user into a second refusal). Refused before any write — in the pre-download preflight on `get`/`scan` (see `vendor_bun_lockb_invalid` for the placements); in the shared preflight that `vendor` and the vendor step run BEFORE a hosted → vendored takeover's revert (a hosted-redirected purl stays hosted-wired, ledger and lock untouched; `vendor --dry-run` previews the same `failed` code); and in the engine when the run would write a NEW local tuple. Exempt: purls the vendor ledger wires at the selected uuid, purls whose every `bun.lock` instance is already a `.socket/vendor/npm/` tuple (any uuid), in-sync re-runs and `repair` redownloads. | @@ -1570,6 +1571,7 @@ command's integration tests check its output. | `redirect_vlt_lockfile_version_missing` / `redirect_vlt_old_lockfile_ignored` / `redirect_vlt_scalar_registry_ignored` | `warnings[]` (warning) | scan/get `--mode hosted` (vlt): the lock has no `lockfileVersion` (vlt ≥ 1.0.0-rc.15 re-resolves it) / a legacy default-registry id without `"modifiers"` in `vlt.json` (vlt 0.0.0-16 … 0.0.0-24 ignore the lock) / a scalar `registry` option that vlt 1.0.0-rc.7 … rc.29 honor over the lock. The deps stay redirected, but the run's `--vex` does not attest them. | | `redirect_vlt_sibling_lockfiles` | `warnings[]` (warning) | scan/get `--mode hosted` (vlt): `vlt-lock.json` and another npm-family lock are both present and vlt's install state (`node_modules/.vlt-lock.json` or `node_modules/.vlt/`) is not, so both locks were rewritten and the other lock's rules confirm. | | `redirect_npm_replace_registry_host` | `warnings[]` (warning) | scan/get `--mode hosted` (npm): the effective npm `replace-registry-host` (env var, project `.npmrc`, or user / global / builtin config) is `always` or a pinned hosted hostname, so npm rewrites the hosted pins to the configured registry and every install fails E404; the detail names the layer and the remedies (`replace-registry-host=npmjs` in the project `.npmrc`, or vendored mode). See the npm `replace-registry-host` contract. | +| `redirect_npm_allow_scripts` | `warnings[]` (warning) | scan/get `--mode hosted` (npm, #1595): a root npm lock pins a patched package to its hosted artifact, a lock entry for it has `hasInstallScript`, and package.json `allowScripts` approves it only by version (`"pkg@x.y.z"` or `||`-joined exact versions) with no name-only approval and no deny. npm 12 matches a version pin only against a registry tarball URL, so the approved install script stops running (every install fails under `strict-allow-scripts`). The policy is never rewritten; the detail names the pinned keys and the name-only remedy. | | `redirect_vlt_no_lockfile` | `warnings[]` (warning) | scan/get `--mode hosted` (vlt): `vlt.json` or vlt's install state is present without `vlt-lock.json`; replaces `redirect_npm_no_lockfile` for vlt projects. | | `redirect_npm_shrinkwrap_only` | `warnings[]` (warning) | scan/get `--mode hosted` (npm, #899): the root `npm-shrinkwrap.json` is the only npm lock and carries a hosted redirect (this run's or an earlier one's — every run repeats it until the project gains a `package-lock.json`). npm >= 12 never reads the shrinkwrap and installs the unpatched registry bytes; npm <= 11 installs the patch. Rename the lock to `package-lock.json` (or commit a copy under that name) and re-run. | | `redirect_vlt_artifact_unverifiable` | `warnings[]` (warning), `skipped` (hosted event) | scan/get `--mode hosted` (vlt): before any takeover or rewrite (dry runs included), each granted artifact with a default-registry instance in `vlt-lock.json` (or, for a purl a `flavor: "vlt"` vendored entry claims, its vendored node, probed before the takeover reverts it) is fetched once as vlt fetches it (`accept-encoding: gzip;q=1.0, identity;q=0.5`, no `Authorization`, up to 10 redirects) and must return 200 with no content encoding (or `identity`) and the granted sha512. On failure (`content-encoding `, `sha512 mismatch`, `http `, `fetch error `, `offline`) the dep is withheld from every rewriter when vlt drives or it is vlt-vendored (which also keeps it vendored), and from the vlt rewrite only otherwise (detail "…; vlt-lock.json was not changed for {purl}"; only the sibling lock this run rewrote can confirm it). A lock already pinned by an earlier run is left pinned, and neither confirmed nor attested. Projects without `vlt-lock.json` make no such request. The detail quotes the artifact URL (and any fetch error that echoes it) with its grant-token path level, the one just before the patch uuid, spelled ``; host, uuid and leaf stay. The in-memory hosted engine (`hosted-bundle`, the Node addon) has no network for this fetch, so it judges every in-scope artifact as `--offline` does (withheld, never pinned; the vendored takeover it refuses anyway). Exit 0. | diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index f3afa2262..f041e5af6 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -3861,6 +3861,92 @@ async fn pnpm_warning_strips_userinfo_and_names_only_spliced_hosts() { ); } +/// [`write_project`] whose lock records an install script for the patched +/// package and whose root package.json approves it in npm 12's +/// `allowScripts` with `policy`. +fn write_project_with_allow_scripts(root: &Path, policy: serde_json::Value) { + write_project(root); + std::fs::write( + root.join("package.json"), + serde_json::json!({ + "name": "consumer", + "version": "0.0.0", + "dependencies": { NAME: VERSION }, + "allowScripts": policy, + }) + .to_string(), + ) + .unwrap(); + let path = root.join("package-lock.json"); + let mut lock: serde_json::Value = + serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); + lock["packages"][format!("node_modules/{NAME}")]["hasInstallScript"] = true.into(); + std::fs::write(&path, serde_json::to_vec_pretty(&lock).unwrap()).unwrap(); +} + +/// #1595: npm 12 matches a version-pinned `allowScripts` approval +/// (`"pkg@x.y.z": true`, what `npm approve-scripts` writes) only against a +/// registry-shaped `resolved`, which the hosted redirect replaces, so the +/// approved install script silently stops running. The run must say so +/// (`redirect_npm_allow_scripts`, naming the pinned key and the name-only +/// remedy) and leave package.json untouched; a name-only approval, which +/// npm still honors, must not warn. +#[tokio::test] +#[serial] +async fn hosted_npm_warns_when_pinned_allow_scripts_approval_is_lost() { + let server = MockServer::start().await; + mock_discovery(&server).await; + mock_reference(&server).await; + mock_view(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + let pinned = format!("{NAME}@{VERSION}"); + write_project_with_allow_scripts(tmp.path(), serde_json::json!({ &pinned: true })); + let package_json = std::fs::read(tmp.path().join("package.json")).unwrap(); + let env = run_redirect_subprocess(tmp.path(), &server.uri()); + assert_eq!(vlt_hosted_common::redirected(&env), 1, "anchor: {env}"); + let lock = std::fs::read_to_string(tmp.path().join("package-lock.json")).unwrap(); + assert!(lock.contains("\"hasInstallScript\": true"), "{lock}"); + assert_eq!( + warning_codes(&env), + vec![ + "redirect_npm_allow_remote".to_string(), + "redirect_npm_allow_scripts".to_string() + ], + "{env}" + ); + let detail = env["warnings"] + .as_array() + .unwrap() + .iter() + .find(|w| w["code"] == "redirect_npm_allow_scripts") + .and_then(|w| w["detail"].as_str()) + .unwrap() + .to_string(); + assert!( + detail.contains(&format!("`\"{pinned}\": true`")) + && detail.contains(&format!("`\"{NAME}\": true`")) + && detail.contains("strict-allow-scripts"), + "the warning names the lost pin and the name-only remedy: {detail}" + ); + assert_eq!( + std::fs::read(tmp.path().join("package.json")).unwrap(), + package_json, + "the user's allowScripts policy is never rewritten" + ); + + // A name-only approval keeps matching the redirected entry: no warning. + let tmp = tempfile::tempdir().unwrap(); + write_project_with_allow_scripts(tmp.path(), serde_json::json!({ &pinned: true, NAME: true })); + let env = run_redirect_subprocess(tmp.path(), &server.uri()); + assert_eq!(vlt_hosted_common::redirected(&env), 1, "anchor: {env}"); + assert_eq!( + warning_codes(&env), + vec!["redirect_npm_allow_remote".to_string()], + "{env}" + ); +} + /// A clean, fully-successful hosted run must carry an EXACT warning set — not /// merely "contains X". Presence-only assertions let spurious warnings ride /// along unnoticed: every pnpm/yarn/bun/Rush run shipped a bogus diff --git a/crates/socket-patch-cli/tests/in_process_vendor.rs b/crates/socket-patch-cli/tests/in_process_vendor.rs index bd3377f85..6ae1f6c16 100644 --- a/crates/socket-patch-cli/tests/in_process_vendor.rs +++ b/crates/socket-patch-cli/tests/in_process_vendor.rs @@ -4428,6 +4428,72 @@ snapshots: ); } + /// #1595: npm 12 matches a version-pinned `allowScripts` approval only + /// against a registry tarball URL, so once vendoring rewires the lock + /// to the `file:` tarball the approved install script stops running. + /// The vendored run still vendors, leaves package.json untouched and + /// records `vendor_npm_allow_scripts`; a name-only approval (which npm + /// still honors) records nothing. + #[tokio::test] + #[serial] + async fn vendor_package_lock_warns_when_pinned_allow_scripts_approval_is_lost() { + let server = MockServer::start().await; + let registry = mock_registry(&server).await; + for (policy, expect_warning) in [ + (json!({ format!("{CONV_NAME}@{CONV_VERSION}"): true }), true), + ( + json!({ format!("{CONV_NAME}@{CONV_VERSION}"): true, CONV_NAME: true }), + false, + ), + ] { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write_package_lock_project(root); + let package_json = json!({ + "name": "consumer", + "version": "0.0.0", + "dependencies": { CONV_NAME: CONV_VERSION }, + "allowScripts": policy, + }) + .to_string(); + std::fs::write(root.join("package.json"), &package_json).unwrap(); + let lock_path = root.join("package-lock.json"); + let mut lock: Value = + serde_json::from_slice(&std::fs::read(&lock_path).unwrap()).unwrap(); + lock["packages"][format!("node_modules/{CONV_NAME}")]["hasInstallScript"] = true.into(); + std::fs::write(&lock_path, serde_json::to_vec_pretty(&lock).unwrap()).unwrap(); + + seed_manifest_and_blob(root); + let (code, env) = vendor_online_cli(root, ®istry, PATCH_ORIGIN, &[]); + assert_eq!(code, 0, "vendoring still succeeds: {env:#}"); + find_event(&env, "applied", None); + let warned = events(&env) + .iter() + .find(|e| e["errorCode"] == "vendor_npm_allow_scripts"); + if expect_warning { + let warned = + warned.unwrap_or_else(|| panic!("expected vendor_npm_allow_scripts: {env:#}")); + assert_eq!(warned["action"], "skipped", "{env:#}"); + let text = warned.to_string(); + assert!( + text.contains(&format!("{CONV_NAME}@{CONV_VERSION}")) + && text.contains("vendored `file:` tarball"), + "{text}" + ); + } else { + assert!( + warned.is_none(), + "a name-only approval still matches: {env:#}" + ); + } + assert_eq!( + std::fs::read_to_string(root.join("package.json")).unwrap(), + package_json, + "the user's allowScripts policy is never rewritten" + ); + } + } + /// [`write_package_lock_project`] plus a parent package `bund` that /// BUNDLES the same `name@version` (`inBundle: true`): the hosted run /// rewires only the hoisted entry (`redirect_npm_bundled_instance_skipped`). diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 04b0c6d65..7a052b590 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -2306,8 +2306,9 @@ async fn rewrite_once( &overrides, &options, ); - let (npm_warnings, npmrc_config_write) = + let (mut npm_warnings, npmrc_config_write) = npm_allow_remote(view, &files, &rewrite, &overrides, &options); + npm_warnings.extend(npm_allow_scripts(&files, &rewrite, &overrides)); if let Some((text, edit)) = trust_config_write { rewrite.files.insert(PNPM_WORKSPACE_REL.to_string(), text); // Appended last, after the lock edits it serves. v5 keeps no hosted @@ -2796,6 +2797,69 @@ fn governing_workspace(view: &ProjectView<'_>) -> Option { } } +/// npm >= 12 blocks dependency install scripts unless the root +/// `package.json#allowScripts` approves them, and it matches a pinned key +/// (`"esbuild@0.19.12": true`) only against a registry-shaped `resolved` +/// URL, which a hosted redirect replaces (#1595). Warn +/// (`redirect_npm_allow_scripts`) for every package a root npm lock now +/// pins to a hosted artifact whose install script is approved only by +/// version. The policy is the user's, so it is never rewritten: the only +/// key npm would still honor is a name-only one, which widens it. +fn npm_allow_scripts( + files: &BTreeMap, + rewrite: &RewriteResult, + overrides: &[DepOverride], +) -> Vec { + use crate::patch::redirect::npm_allow_scripts::{ + lock_has_install_script, lost_detail, pinned_approvals_lost, HOSTED_WARNING, + }; + let Some(package_json) = rewrite + .files + .get("package.json") + .or_else(|| files.get("package.json")) + else { + return Vec::new(); + }; + let npm_lock_texts: Vec<&String> = NPM_LOCKS + .iter() + .filter_map(|lock| rewrite.files.get(*lock).or_else(|| files.get(*lock))) + .collect(); + let npm_overrides: Vec<&DepOverride> = + overrides.iter().filter(|o| o.ecosystem == "npm").collect(); + let groups: Vec<[String; 2]> = npm_overrides + .iter() + .map(|o| artifact_url_spellings(&o.artifact_url)) + .collect(); + let present = groups_present(&npm_lock_texts, &groups); + let mut seen: BTreeSet<(String, String)> = BTreeSet::new(); + let mut warnings = Vec::new(); + for (o, _) in npm_overrides.iter().zip(present).filter(|(_, p)| *p) { + let name = crate::patch::redirect::full_name(o); + if !seen.insert((name.clone(), o.version.clone())) { + continue; + } + if !npm_lock_texts + .iter() + .any(|lock| lock_has_install_script(lock, &name, &o.version)) + { + continue; + } + let keys = pinned_approvals_lost(package_json, &name, &o.version); + if keys.is_empty() { + continue; + } + let target = match url_host(&o.artifact_url) { + Some(host) => format!("the hosted patch tarball on {host}"), + None => "the hosted patch tarball".to_string(), + }; + warnings.push(warning( + HOSTED_WARNING, + lost_detail(&name, &o.version, &keys, &target), + )); + } + warnings +} + /// npm >= 12 ships `allow-remote=none`: it refuses (EALLOWREMOTE) every /// tarball whose `resolved` origin is not the configured registry — which /// is exactly what a hosted redirect writes. npm <= 11 installs it diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index b9e81d265..593907689 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -48,6 +48,7 @@ mod group_equivalence_tests; #[cfg(test)] mod lock_index_equivalence_tests; mod maven_index; +pub mod npm_allow_scripts; pub mod npmrc; mod pdm; mod pipenv; diff --git a/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs b/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs new file mode 100644 index 000000000..d59143464 --- /dev/null +++ b/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs @@ -0,0 +1,237 @@ +//! npm 12's `package.json#allowScripts` install-script policy (#1595). +//! +//! npm >= 12 blocks dependency install scripts unless the root +//! `package.json` approves them, and `npm approve-scripts` writes a +//! version-pinned key (`"esbuild@0.19.12": true`). npm matches a pinned +//! key only against the trusted version it reads from a registry-shaped +//! `resolved` URL (`…//-/-.tgz`, arborist's +//! `script-allowed.js` / `versionFromTgz`). A hosted redirect (patch +//! server URL) and a vendored rewire (`file:.socket/vendor/…`) both leave +//! that shape, so the trusted version becomes unknown: a pinned allow +//! stops matching, while a name-only allow (`pkg`, `pkg@*`) and every +//! deny keep matching. The approved script then silently stops running +//! (or, under `strict-allow-scripts`, every install fails). Neither side +//! can keep the pin valid without weakening the policy or writing a +//! machine-specific key, so both runs warn. + +use semver::Version; +use serde_json::Value; + +/// Hosted-mode warning code (`warnings[]`). +pub const HOSTED_WARNING: &str = "redirect_npm_allow_scripts"; +/// Vendored-mode advisory code (`skipped` event). +pub const VENDOR_WARNING: &str = "vendor_npm_allow_scripts"; + +/// How an `allowScripts` key matches a registry package, mirroring +/// arborist's `matchRegistry` for the key types that can name one. +enum KeyMatch { + /// The key does not name this registry package (another name, an + /// alias, a git / file / remote spec, a range npm rejects). + No, + /// A bare name or `name@*`: matches whatever the lock resolves to. + NameOnly, + /// An exact version or `||`-joined exact versions covering the + /// package's version: matches only while npm can read that version + /// from a registry-shaped `resolved`. + Pinned, +} + +/// Split an npm package-arg key into `(name, spec)`; `spec` is `None` for +/// a bare name. A scoped name's leading `@` is part of the name. +fn split_key(key: &str) -> (&str, Option<&str>) { + let search_from = usize::from(key.starts_with('@')); + match key[search_from..].find('@') { + Some(at) => { + let at = at + search_from; + (&key[..at], Some(&key[at + 1..])) + } + None => (key, None), + } +} + +/// `semver.valid(v, { loose: true })` for the exact-version spellings npm +/// accepts in a pin: surrounding whitespace and a leading `=` / `v`. +fn loose_version(text: &str) -> Option { + let text = text.trim(); + let text = text.strip_prefix('=').unwrap_or(text).trim_start(); + let text = text + .strip_prefix('v') + .or_else(|| text.strip_prefix('V')) + .unwrap_or(text); + Version::parse(text).ok() +} + +fn key_match(key: &str, name: &str, version: &Version) -> KeyMatch { + let (key_name, spec) = split_key(key); + if key_name != name { + return KeyMatch::No; + } + let spec = match spec.map(str::trim) { + None | Some("") | Some("*") => return KeyMatch::NameOnly, + Some(spec) => spec, + }; + // `name@npm:other@1` is an alias, which npm never matches; anything + // else that is not exact versions (`^1`, `latest`, a URL) is rejected + // or matched as a non-registry spec. + let pins: Option> = spec.split("||").map(loose_version).collect(); + match pins { + Some(pins) if pins.iter().any(|pin| pin == version) => KeyMatch::Pinned, + _ => KeyMatch::No, + } +} + +/// The `allowScripts` keys in the root `package.json` text that approve +/// `name@version`'s install scripts today but stop matching once its lock +/// entries resolve outside the registry. Empty when nothing is lost: no +/// pinned approval covers it, a name-only approval still does, a deny +/// already blocks it, or the policy (or the file) is unreadable. +pub fn pinned_approvals_lost(package_json: &str, name: &str, version: &str) -> Vec { + let Some(version) = loose_version(version) else { + return Vec::new(); + }; + let Ok(manifest) = serde_json::from_str::(package_json.trim_start_matches('\u{feff}')) + else { + return Vec::new(); + }; + let Some(policy) = manifest.get("allowScripts").and_then(Value::as_object) else { + return Vec::new(); + }; + let mut pinned = Vec::new(); + for (key, value) in policy { + match (key_match(key, name, &version), value) { + (KeyMatch::No, _) => {} + // A matching deny wins before and after the rewrite (npm fails + // a pinned deny closed on an unknown version): nothing changes. + (_, Value::Bool(false)) => return Vec::new(), + (KeyMatch::NameOnly, Value::Bool(true)) => return Vec::new(), + (KeyMatch::Pinned, Value::Bool(true)) => pinned.push(key.clone()), + _ => {} + } + } + pinned +} + +/// Does any `package-lock.json` / `npm-shrinkwrap.json` entry for +/// `name@version` record an install script (`hasInstallScript: true`)? +/// Only such a package has a script for npm's policy to block. Bundled +/// copies are skipped: npm never runs their scripts, approved or not. +pub fn lock_has_install_script(lock_text: &str, name: &str, version: &str) -> bool { + let Ok(lock) = serde_json::from_str::(lock_text.trim_start_matches('\u{feff}')) else { + return false; + }; + let Some(packages) = lock.get("packages").and_then(Value::as_object) else { + return false; + }; + packages.iter().any(|(key, entry)| { + let folder = key.rsplit("node_modules/").next().unwrap_or_default(); + let entry_name = entry.get("name").and_then(Value::as_str).unwrap_or(folder); + key.contains("node_modules/") + && entry_name == name + && entry.get("version").and_then(Value::as_str) == Some(version) + && entry.get("hasInstallScript").and_then(Value::as_bool) == Some(true) + && entry.get("inBundle").and_then(Value::as_bool) != Some(true) + }) +} + +/// The warning detail for `name@version`, whose pinned `keys` no longer +/// match once its lock entries point at `target` (the hosted patch server +/// or the vendored `file:` tarball). +pub fn lost_detail(name: &str, version: &str, keys: &[String], target: &str) -> String { + let keys = keys + .iter() + .map(|key| format!("`\"{key}\": true`")) + .collect::>() + .join(", "); + format!( + "npm >= 12 stops running {name}@{version}'s approved install scripts: package.json \ + `allowScripts` approves it only by version ({keys}), and npm matches a version pin \ + only against a registry tarball URL, while the lock now resolves it to {target}. The \ + next `npm ci` installs the patched files but blocks the install script (with \ + `strict-allow-scripts=true` every install fails). To keep it running, approve the \ + package by name (`\"{name}\": true`) in package.json `allowScripts`; npm <= 11 is \ + unaffected" + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn pkg(policy: &str) -> String { + format!(r#"{{ "name": "app", "allowScripts": {policy} }}"#) + } + + #[test] + fn exact_pin_is_lost() { + let lost = + pinned_approvals_lost(&pkg(r#"{"esbuild@0.19.12": true}"#), "esbuild", "0.19.12"); + assert_eq!(lost, vec!["esbuild@0.19.12".to_string()]); + } + + #[test] + fn scoped_pin_and_disjunction_and_loose_spellings_are_lost() { + let text = pkg( + r#"{"@img/sharp@0.33.0 || 0.33.5": true, "@img/sharp@v0.33.5": true, "@img/sharp@=0.33.5": true}"#, + ); + assert_eq!( + pinned_approvals_lost(&text, "@img/sharp", "0.33.5").len(), + 3 + ); + } + + #[test] + fn name_only_approval_still_covers_it() { + for policy in [ + r#"{"esbuild@0.19.12": true, "esbuild": true}"#, + r#"{"esbuild@0.19.12": true, "esbuild@*": true}"#, + ] { + assert!( + pinned_approvals_lost(&pkg(policy), "esbuild", "0.19.12").is_empty(), + "{policy}" + ); + } + } + + #[test] + fn deny_or_unrelated_keys_lose_nothing() { + for policy in [ + r#"{"esbuild@0.19.12": true, "esbuild": false}"#, + r#"{"esbuild@0.19.11": true}"#, + r#"{"esbuild@^0.19.0": true}"#, + r#"{"esbuild-wasm@0.19.12": true}"#, + r#"{"alias@npm:esbuild@0.19.12": true}"#, + r#"{"esbuild@0.19.12": "yes"}"#, + r#"[]"#, + ] { + assert!( + pinned_approvals_lost(&pkg(policy), "esbuild", "0.19.12").is_empty(), + "{policy}" + ); + } + assert!(pinned_approvals_lost("not json", "esbuild", "0.19.12").is_empty()); + assert!(pinned_approvals_lost(r#"{"name": "app"}"#, "esbuild", "0.19.12").is_empty()); + } + + #[test] + fn bom_prefixed_package_json_is_read() { + let text = format!("\u{feff}{}", pkg(r#"{"esbuild@0.19.12": true}"#)); + assert_eq!(pinned_approvals_lost(&text, "esbuild", "0.19.12").len(), 1); + } + + #[test] + fn install_script_flag_is_read_from_lock_entries() { + let lock = r#"{"lockfileVersion": 3, "packages": { + "": {"name": "app"}, + "node_modules/esbuild": {"version": "0.19.12", "hasInstallScript": true}, + "node_modules/plain": {"version": "1.0.0"}, + "node_modules/p/node_modules/bundled": {"version": "1.0.0", "hasInstallScript": true, "inBundle": true}, + "node_modules/al": {"name": "aliased", "version": "2.0.0", "hasInstallScript": true} + }}"#; + assert!(lock_has_install_script(lock, "esbuild", "0.19.12")); + assert!(!lock_has_install_script(lock, "esbuild", "0.19.11")); + assert!(!lock_has_install_script(lock, "plain", "1.0.0")); + assert!(!lock_has_install_script(lock, "bundled", "1.0.0")); + assert!(lock_has_install_script(lock, "aliased", "2.0.0")); + assert!(!lock_has_install_script(lock, "app", "1.0.0")); + } +} diff --git a/crates/socket-patch-core/src/vendor/npm_flavor.rs b/crates/socket-patch-core/src/vendor/npm_flavor.rs index 95db7b1c9..133f7e560 100644 --- a/crates/socket-patch-core/src/vendor/npm_flavor.rs +++ b/crates/socket-patch-core/src/vendor/npm_flavor.rs @@ -497,8 +497,9 @@ pub async fn vendor_npm_any<'a>( entry.flavor = Some(flavor.as_str().to_string()); } // npm >= 11.14's `allow-file` gates the `file:` specs the - // package-lock wiring writes (#969); the other flavors' package - // managers do not read it. + // package-lock wiring writes (#969), and npm 12's `allowScripts` + // pins stop matching them (#1595); the other flavors' package + // managers read neither. if flavor == NpmLockFlavor::PackageLock && result.success { if let Some((name, version)) = super::npm_common::parse_npm_purl(purl) { if let Some(detail) = @@ -506,6 +507,16 @@ pub async fn vendor_npm_any<'a>( { warnings.push(VendorWarning::new(npm_lock::ALLOW_FILE_WARNING, detail)); } + // npm 12 stops honoring a version-pinned allowScripts + // approval for the rewired `file:` tarball (#1595). + if let Some(detail) = + npm_lock::allow_scripts_lost(project_root, &name, &version).await + { + warnings.push(VendorWarning::new( + crate::patch::redirect::npm_allow_scripts::VENDOR_WARNING, + detail, + )); + } } } } diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index 6f1d95ec4..4f2c374d2 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -1776,6 +1776,37 @@ pub(super) async fn allow_file_refusal_with( )) } +/// The `vendor_npm_allow_scripts` advisory for `name@version` (#1595), or +/// `None`: npm >= 12 matches a version-pinned `package.json#allowScripts` +/// approval only against a registry tarball URL, so once vendoring rewires +/// the lock to the `file:` tarball the approved install script stops +/// running. Only a package some npm lock records `hasInstallScript` for +/// can lose one; the policy is the user's and is never rewritten. +pub(super) async fn allow_scripts_lost( + project_root: &Path, + name: &str, + version: &str, +) -> Option { + use crate::patch::redirect::npm_allow_scripts::{ + lock_has_install_script, lost_detail, pinned_approvals_lost, + }; + use crate::utils::fs::read_regular_to_string; + let package_json = read_regular_to_string(&project_root.join("package.json")) + .await + .ok()?; + let keys = pinned_approvals_lost(&package_json, name, version); + if keys.is_empty() { + return None; + } + let mut has_script = false; + for lock_name in NPM_LOCKS { + if let Ok(text) = read_regular_to_string(&project_root.join(lock_name)).await { + has_script |= lock_has_install_script(&text, name, version); + } + } + has_script.then(|| lost_detail(name, version, &keys, "the vendored `file:` tarball")) +} + /// [`allow_file_refusal_with`] against this process's npm config layers. /// Unit tests see only the project `.npmrc`: the developer's or runner's /// own layers (an `npm_config_allow_file` variable, a `~/.npmrc`) must not diff --git a/docs/ecosystems.md b/docs/ecosystems.md index deda930d5..cc2ab1201 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -133,6 +133,14 @@ Each Legacy format has an upgrade path and an undo path. Both work in v5: E404: the hosted run reads it from the same env / project / user / global / builtin layers and warns `redirect_npm_replace_registry_host` (set `replace-registry-host=npmjs` in the project `.npmrc`, or use vendored mode). + npm 12 runs a dependency's install script only when `package.json` + `allowScripts` approves it, and matches a version-pinned key + (`"esbuild@0.19.12": true`, what `npm approve-scripts` writes) only against a + registry tarball URL. Hosted and vendored lock entries are not one, so when a + patched package with an install script is approved only by version, the run + leaves the policy alone and warns `redirect_npm_allow_scripts` / + `vendor_npm_allow_scripts`: approve it by name (`"esbuild": true`) to keep + the script running (#1595). npm 6 ignores `resolved` for registry dependencies, so a redirected lockfileVersion 1 lock fails closed with EINTEGRITY under npm 6 (`redirect_npm_legacy_client`) and installs under npm diff --git a/docs/testing/npm-compatibility.md b/docs/testing/npm-compatibility.md index 297a45779..711d72513 100644 --- a/docs/testing/npm-compatibility.md +++ b/docs/testing/npm-compatibility.md @@ -72,6 +72,13 @@ npm 12 notes: install unchanged. An explicit `none` (or `root` with a transitive vendored copy) refuses them with EALLOWFILE; the vendored run warns `vendor_npm_allow_file` and `vendor --check` fails (#969). +- `allowScripts` (npm 12): a version-pinned approval (`"pkg@x.y.z": true`) + matches only a registry-shaped `resolved`, so after a hosted or vendored + rewrite npm blocks the package's approved install script (every install + fails under `strict-allow-scripts=true`). A name-only approval keeps + working. The run leaves the policy untouched and warns + `redirect_npm_allow_scripts` / `vendor_npm_allow_scripts` when the patched + entry has `hasInstallScript` and no name-only approval covers it (#1595). Dependencies that ship their own `npm-shrinkwrap.json` (#753): the lock marks such a package `"hasShrinkwrap": true` (`firebase-tools`, `netlify-cli`), and From 46ccb14f179e307e2f7cb9fe6f018c065ecf5979 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 13:16:40 +0000 Subject: [PATCH 3/6] Match npm's allowScripts rules more exactly Follow-up to the allowScripts warning from code review: - After the rewrite npm can no longer read the version, so a deny pinned to any version of the package starts blocking it, even beside a name-only approval. The warning now covers that case and tells the user to remove the pinned deny. - Only lock entries the run actually rewired count, so a copy left on the registry (for example under a shrinkwrapped dependency) no longer raises a false warning. - Only package-lock.json is checked; npm 12 never reads npm-shrinkwrap.json. - lockfileVersion 1 locks, which never record hasInstallScript, count a rewired entry as running a script. - Version pins ignore build metadata, as npm does, and the policy and lock are parsed once, with an early exit when there's no policy. Refs #1595 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/hosted/engine.rs | 50 +- .../src/patch/redirect/npm_allow_scripts.rs | 448 ++++++++++++------ .../socket-patch-core/src/vendor/npm_lock.rs | 35 +- 3 files changed, 356 insertions(+), 177 deletions(-) diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 7a052b590..4503b6a2d 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -2801,51 +2801,43 @@ fn governing_workspace(view: &ProjectView<'_>) -> Option { /// `package.json#allowScripts` approves them, and it matches a pinned key /// (`"esbuild@0.19.12": true`) only against a registry-shaped `resolved` /// URL, which a hosted redirect replaces (#1595). Warn -/// (`redirect_npm_allow_scripts`) for every package a root npm lock now -/// pins to a hosted artifact whose install script is approved only by -/// version. The policy is the user's, so it is never rewritten: the only -/// key npm would still honor is a name-only one, which widens it. +/// (`redirect_npm_allow_scripts`) for every package whose rewired +/// package-lock.json entry runs an install script that loses its approval. +/// The policy is the user's, so it is never rewritten: the only key npm +/// would still honor is a name-only one, which widens it. npm >= 12 never +/// reads npm-shrinkwrap.json, so a shrinkwrap pin cannot lose one. fn npm_allow_scripts( files: &BTreeMap, rewrite: &RewriteResult, overrides: &[DepOverride], ) -> Vec { use crate::patch::redirect::npm_allow_scripts::{ - lock_has_install_script, lost_detail, pinned_approvals_lost, HOSTED_WARNING, + lost_detail, rewired_runs_install_script, AllowScripts, HOSTED_WARNING, NPM12_LOCK, }; - let Some(package_json) = rewrite - .files - .get("package.json") - .or_else(|| files.get("package.json")) + let read = |rel: &str| rewrite.files.get(rel).or_else(|| files.get(rel)); + let Some(policy) = read("package.json").and_then(|text| AllowScripts::parse(text)) else { + return Vec::new(); + }; + let Some(lock) = + read(NPM12_LOCK).and_then(|text| serde_json::from_str::(text).ok()) else { return Vec::new(); }; - let npm_lock_texts: Vec<&String> = NPM_LOCKS - .iter() - .filter_map(|lock| rewrite.files.get(*lock).or_else(|| files.get(*lock))) - .collect(); - let npm_overrides: Vec<&DepOverride> = - overrides.iter().filter(|o| o.ecosystem == "npm").collect(); - let groups: Vec<[String; 2]> = npm_overrides - .iter() - .map(|o| artifact_url_spellings(&o.artifact_url)) - .collect(); - let present = groups_present(&npm_lock_texts, &groups); let mut seen: BTreeSet<(String, String)> = BTreeSet::new(); let mut warnings = Vec::new(); - for (o, _) in npm_overrides.iter().zip(present).filter(|(_, p)| *p) { + for o in overrides.iter().filter(|o| o.ecosystem == "npm") { let name = crate::patch::redirect::full_name(o); if !seen.insert((name.clone(), o.version.clone())) { continue; } - if !npm_lock_texts - .iter() - .any(|lock| lock_has_install_script(lock, &name, &o.version)) - { + let Some(lost) = policy.lost(&name, &o.version) else { continue; - } - let keys = pinned_approvals_lost(package_json, &name, &o.version); - if keys.is_empty() { + }; + let rewired = |_: &str, entry: &serde_json::Value| { + entry.get("resolved").and_then(serde_json::Value::as_str) + == Some(o.artifact_url.as_str()) + }; + if !rewired_runs_install_script(&lock, &name, &o.version, rewired) { continue; } let target = match url_host(&o.artifact_url) { @@ -2854,7 +2846,7 @@ fn npm_allow_scripts( }; warnings.push(warning( HOSTED_WARNING, - lost_detail(&name, &o.version, &keys, &target), + lost_detail(&name, &o.version, &lost, &target), )); } warnings diff --git a/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs b/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs index d59143464..c57c6f92e 100644 --- a/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs +++ b/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs @@ -8,11 +8,14 @@ //! `script-allowed.js` / `versionFromTgz`). A hosted redirect (patch //! server URL) and a vendored rewire (`file:.socket/vendor/…`) both leave //! that shape, so the trusted version becomes unknown: a pinned allow -//! stops matching, while a name-only allow (`pkg`, `pkg@*`) and every -//! deny keep matching. The approved script then silently stops running -//! (or, under `strict-allow-scripts`, every install fails). Neither side -//! can keep the pin valid without weakening the policy or writing a -//! machine-specific key, so both runs warn. +//! stops matching, a pinned deny fails closed (it matches whatever its +//! version), and name-only keys (`pkg`, `pkg@*`) match as before. An +//! approved script then silently stops running (or, under +//! `strict-allow-scripts`, every install fails). Neither mode can keep the +//! approval valid without widening the policy or writing a +//! machine-specific key, so both warn. + +use std::cmp::Ordering; use semver::Version; use serde_json::Value; @@ -22,18 +25,41 @@ pub const HOSTED_WARNING: &str = "redirect_npm_allow_scripts"; /// Vendored-mode advisory code (`skipped` event). pub const VENDOR_WARNING: &str = "vendor_npm_allow_scripts"; -/// How an `allowScripts` key matches a registry package, mirroring -/// arborist's `matchRegistry` for the key types that can name one. -enum KeyMatch { - /// The key does not name this registry package (another name, an - /// alias, a git / file / remote spec, a range npm rejects). - No, +/// The only npm lock npm >= 12 reads (it never reads npm-shrinkwrap.json), +/// so the only one whose rewired entries can lose an approval. +pub const NPM12_LOCK: &str = "package-lock.json"; + +/// How an `allowScripts` key can match a registry package, mirroring +/// arborist's `matchRegistry` for the key types that name one. +enum KeyKind { /// A bare name or `name@*`: matches whatever the lock resolves to. NameOnly, - /// An exact version or `||`-joined exact versions covering the - /// package's version: matches only while npm can read that version - /// from a registry-shaped `resolved`. - Pinned, + /// An exact version or `||`-joined exact versions: matches only when + /// npm reads a covered version from a registry-shaped `resolved`. + Pinned(Vec), +} + +/// The approval npm 12 gives one package under the policy. +#[derive(PartialEq)] +enum Verdict { + Allowed, + Denied, + Unreviewed, +} + +/// The parsed root `allowScripts` policy: `(key, name, kind, allow)` for +/// every key that can name a registry package. Aliases, ranges npm +/// rejects (`^1`, `latest`) and git / file / remote specs never match a +/// registry package, before or after the rewrite, and are dropped. +pub struct AllowScripts { + entries: Vec<(String, String, KeyKind, bool)>, +} + +/// What the rewrite of one package costs under the policy: the pinned +/// allows that stop matching and the pinned denies that start to. +pub struct LostApproval { + pub allows: Vec, + pub denies: Vec, } /// Split an npm package-arg key into `(name, spec)`; `spec` is `None` for @@ -61,95 +87,183 @@ fn loose_version(text: &str) -> Option { Version::parse(text).ok() } -fn key_match(key: &str, name: &str, version: &Version) -> KeyMatch { - let (key_name, spec) = split_key(key); - if key_name != name { - return KeyMatch::No; +impl AllowScripts { + /// The policy in the root `package.json` text, or `None` when it has + /// no `allowScripts` key that could name a registry package (or the + /// file is unreadable): nothing can be lost. + pub fn parse(package_json: &str) -> Option { + let manifest: Value = + serde_json::from_str(package_json.trim_start_matches('\u{feff}')).ok()?; + let policy = manifest.get("allowScripts")?.as_object()?; + let entries: Vec<_> = policy + .iter() + .filter_map(|(key, value)| { + let allow = value.as_bool()?; + let (name, spec) = split_key(key); + let kind = match spec.map(str::trim) { + None | Some("") | Some("*") => KeyKind::NameOnly, + Some(spec) => KeyKind::Pinned( + spec.split("||") + .map(loose_version) + .collect::>>()?, + ), + }; + Some((key.clone(), name.to_string(), kind, allow)) + }) + .collect(); + (!entries.is_empty()).then_some(Self { entries }) } - let spec = match spec.map(str::trim) { - None | Some("") | Some("*") => return KeyMatch::NameOnly, - Some(spec) => spec, - }; - // `name@npm:other@1` is an alias, which npm never matches; anything - // else that is not exact versions (`^1`, `latest`, a URL) is rejected - // or matched as a non-registry spec. - let pins: Option> = spec.split("||").map(loose_version).collect(); - match pins { - Some(pins) if pins.iter().any(|pin| pin == version) => KeyMatch::Pinned, - _ => KeyMatch::No, + + /// npm's verdict for `name`, at `version` when npm can read one from a + /// registry `resolved`, or `None` once it cannot (deny wins). + fn verdict(&self, name: &str, version: Option<&Version>) -> Verdict { + let mut allowed = false; + for (_, key_name, kind, allow) in &self.entries { + if key_name != name { + continue; + } + let matches = match (kind, version) { + (KeyKind::NameOnly, _) => true, + (KeyKind::Pinned(pins), Some(v)) => pins + .iter() + .any(|pin| pin.cmp_precedence(v) == Ordering::Equal), + // Unverifiable version: a deny fails closed, an allow is + // refused. + (KeyKind::Pinned(_), None) => !allow, + }; + if matches && !allow { + return Verdict::Denied; + } + allowed |= matches; + } + if allowed { + Verdict::Allowed + } else { + Verdict::Unreviewed + } } -} -/// The `allowScripts` keys in the root `package.json` text that approve -/// `name@version`'s install scripts today but stop matching once its lock -/// entries resolve outside the registry. Empty when nothing is lost: no -/// pinned approval covers it, a name-only approval still does, a deny -/// already blocks it, or the policy (or the file) is unreadable. -pub fn pinned_approvals_lost(package_json: &str, name: &str, version: &str) -> Vec { - let Some(version) = loose_version(version) else { - return Vec::new(); - }; - let Ok(manifest) = serde_json::from_str::(package_json.trim_start_matches('\u{feff}')) - else { - return Vec::new(); - }; - let Some(policy) = manifest.get("allowScripts").and_then(Value::as_object) else { - return Vec::new(); - }; - let mut pinned = Vec::new(); - for (key, value) in policy { - match (key_match(key, name, &version), value) { - (KeyMatch::No, _) => {} - // A matching deny wins before and after the rewrite (npm fails - // a pinned deny closed on an unknown version): nothing changes. - (_, Value::Bool(false)) => return Vec::new(), - (KeyMatch::NameOnly, Value::Bool(true)) => return Vec::new(), - (KeyMatch::Pinned, Value::Bool(true)) => pinned.push(key.clone()), - _ => {} + /// The approval `name@version`'s install scripts lose once its lock + /// entries resolve outside the registry, or `None` when npm 12 runs + /// them as before (or never did: denied or unreviewed already). + pub fn lost(&self, name: &str, version: &str) -> Option { + let version = loose_version(version)?; + if self.verdict(name, Some(&version)) != Verdict::Allowed + || self.verdict(name, None) == Verdict::Allowed + { + return None; } + let pinned = |allow: bool| -> Vec { + self.entries + .iter() + .filter(|(_, key_name, kind, a)| { + key_name == name && *a == allow && matches!(kind, KeyKind::Pinned(_)) + }) + .filter(|(_, _, kind, _)| match kind { + // Every pinned deny matches after the rewrite; only the + // allows that covered this version were lost. + KeyKind::Pinned(pins) => { + !allow + || pins + .iter() + .any(|pin| pin.cmp_precedence(&version) == Ordering::Equal) + } + KeyKind::NameOnly => false, + }) + .map(|(key, ..)| key.clone()) + .collect() + }; + Some(LostApproval { + allows: pinned(true), + denies: pinned(false), + }) } - pinned } -/// Does any `package-lock.json` / `npm-shrinkwrap.json` entry for -/// `name@version` record an install script (`hasInstallScript: true`)? -/// Only such a package has a script for npm's policy to block. Bundled -/// copies are skipped: npm never runs their scripts, approved or not. -pub fn lock_has_install_script(lock_text: &str, name: &str, version: &str) -> bool { - let Ok(lock) = serde_json::from_str::(lock_text.trim_start_matches('\u{feff}')) else { - return false; - }; - let Some(packages) = lock.get("packages").and_then(Value::as_object) else { - return false; - }; - packages.iter().any(|(key, entry)| { - let folder = key.rsplit("node_modules/").next().unwrap_or_default(); - let entry_name = entry.get("name").and_then(Value::as_str).unwrap_or(folder); - key.contains("node_modules/") - && entry_name == name - && entry.get("version").and_then(Value::as_str) == Some(version) - && entry.get("hasInstallScript").and_then(Value::as_bool) == Some(true) - && entry.get("inBundle").and_then(Value::as_bool) != Some(true) - }) +/// Does a rewired `package-lock.json` entry for `name@version` run an +/// install script? `is_rewired(key, entry)` picks the entries the run +/// points outside the registry. lockfileVersion 2/3 entries say so with +/// `hasInstallScript`; a lockfileVersion 1 lock never records it, so a +/// rewired v1 entry counts as having one. Bundled copies never run +/// scripts. +pub fn rewired_runs_install_script( + lock: &Value, + name: &str, + version: &str, + is_rewired: impl Fn(&str, &Value) -> bool, +) -> bool { + if let Some(packages) = lock.get("packages").and_then(Value::as_object) { + return packages.iter().any(|(key, entry)| { + let Some(idx) = key.rfind("node_modules/") else { + return false; + }; + let folder = &key[idx + "node_modules/".len()..]; + entry.get("name").and_then(Value::as_str).unwrap_or(folder) == name + && entry.get("version").and_then(Value::as_str) == Some(version) + && entry.get("hasInstallScript").and_then(Value::as_bool) == Some(true) + && entry.get("inBundle").and_then(Value::as_bool) != Some(true) + && is_rewired(key, entry) + }); + } + fn walk_v1( + deps: Option<&Value>, + name: &str, + version: &str, + is_rewired: &dyn Fn(&str, &Value) -> bool, + ) -> bool { + let Some(deps) = deps.and_then(Value::as_object) else { + return false; + }; + deps.iter().any(|(key, entry)| { + (key == name + && entry.get("version").and_then(Value::as_str) == Some(version) + && entry.get("bundled").and_then(Value::as_bool) != Some(true) + && is_rewired(key, entry)) + || walk_v1(entry.get("dependencies"), name, version, is_rewired) + }) + } + walk_v1(lock.get("dependencies"), name, version, &is_rewired) } -/// The warning detail for `name@version`, whose pinned `keys` no longer -/// match once its lock entries point at `target` (the hosted patch server -/// or the vendored `file:` tarball). -pub fn lost_detail(name: &str, version: &str, keys: &[String], target: &str) -> String { - let keys = keys - .iter() - .map(|key| format!("`\"{key}\": true`")) - .collect::>() - .join(", "); +/// The warning detail for `name@version`, whose approval is `lost` once +/// its lock entries point at `target` (the hosted patch server or the +/// vendored `file:` tarball). +pub fn lost_detail(name: &str, version: &str, lost: &LostApproval, target: &str) -> String { + let quote = |keys: &[String], value: bool| { + keys.iter() + .map(|key| format!("`\"{key}\": {value}`")) + .collect::>() + .join(", ") + }; + let mut why = Vec::new(); + if !lost.allows.is_empty() { + why.push(format!( + "package.json `allowScripts` approves it only by version ({}), which npm matches \ + only against a registry tarball URL", + quote(&lost.allows, true) + )); + } + if !lost.denies.is_empty() { + why.push(format!( + "npm applies its version-pinned deny ({}) to every version it cannot read from a \ + registry tarball URL", + quote(&lost.denies, false) + )); + } + let remedy = if lost.denies.is_empty() { + format!("approve the package by name (`\"{name}\": true`) in package.json `allowScripts`") + } else { + format!( + "remove its version-pinned deny entries and approve the package by name \ + (`\"{name}\": true`) in package.json `allowScripts`" + ) + }; format!( - "npm >= 12 stops running {name}@{version}'s approved install scripts: package.json \ - `allowScripts` approves it only by version ({keys}), and npm matches a version pin \ - only against a registry tarball URL, while the lock now resolves it to {target}. The \ - next `npm ci` installs the patched files but blocks the install script (with \ - `strict-allow-scripts=true` every install fails). To keep it running, approve the \ - package by name (`\"{name}\": true`) in package.json `allowScripts`; npm <= 11 is \ - unaffected" + "npm >= 12 stops running {name}@{version}'s approved install scripts once the lock \ + resolves it to {target}: {}. The next `npm ci` installs the patched files but blocks \ + the install script (with `strict-allow-scripts=true` every install fails). To keep it \ + running, {remedy}; npm <= 11 is unaffected", + why.join("; and ") ) } @@ -157,26 +271,24 @@ pub fn lost_detail(name: &str, version: &str, keys: &[String], target: &str) -> mod tests { use super::*; - fn pkg(policy: &str) -> String { - format!(r#"{{ "name": "app", "allowScripts": {policy} }}"#) + fn lost_for(policy: &str, name: &str, version: &str) -> Option { + let text = format!(r#"{{ "name": "app", "allowScripts": {policy} }}"#); + AllowScripts::parse(&text)?.lost(name, version) } #[test] fn exact_pin_is_lost() { - let lost = - pinned_approvals_lost(&pkg(r#"{"esbuild@0.19.12": true}"#), "esbuild", "0.19.12"); - assert_eq!(lost, vec!["esbuild@0.19.12".to_string()]); + let lost = lost_for(r#"{"esbuild@0.19.12": true}"#, "esbuild", "0.19.12").unwrap(); + assert_eq!(lost.allows, vec!["esbuild@0.19.12".to_string()]); + assert!(lost.denies.is_empty()); } #[test] - fn scoped_pin_and_disjunction_and_loose_spellings_are_lost() { - let text = pkg( - r#"{"@img/sharp@0.33.0 || 0.33.5": true, "@img/sharp@v0.33.5": true, "@img/sharp@=0.33.5": true}"#, - ); - assert_eq!( - pinned_approvals_lost(&text, "@img/sharp", "0.33.5").len(), - 3 - ); + fn scoped_pin_disjunction_loose_spellings_and_build_metadata_are_lost() { + let policy = r#"{"@img/sharp@0.33.0 || 0.33.5": true, "@img/sharp@v0.33.5": true, + "@img/sharp@=0.33.5": true, "@img/sharp@0.33.5+b": true}"#; + let lost = lost_for(policy, "@img/sharp", "0.33.5").unwrap(); + assert_eq!(lost.allows.len(), 4); } #[test] @@ -184,54 +296,124 @@ mod tests { for policy in [ r#"{"esbuild@0.19.12": true, "esbuild": true}"#, r#"{"esbuild@0.19.12": true, "esbuild@*": true}"#, + r#"{"esbuild": true}"#, ] { - assert!( - pinned_approvals_lost(&pkg(policy), "esbuild", "0.19.12").is_empty(), - "{policy}" - ); + assert!(lost_for(policy, "esbuild", "0.19.12").is_none(), "{policy}"); } } + /// npm fails a pinned deny closed on an unknown version, so a deny for + /// ANOTHER version starts matching the rewired copy, even beside a + /// name-only allow. #[test] - fn deny_or_unrelated_keys_lose_nothing() { + fn other_version_deny_starts_matching() { + let lost = lost_for( + r#"{"esbuild": true, "esbuild@0.18.0": false}"#, + "esbuild", + "0.19.12", + ) + .unwrap(); + assert!(lost.allows.is_empty()); + assert_eq!(lost.denies, vec!["esbuild@0.18.0".to_string()]); + let detail = lost_detail("esbuild", "0.19.12", &lost, "x"); + assert!( + detail.contains("remove its version-pinned deny"), + "{detail}" + ); + + let lost = lost_for( + r#"{"esbuild@0.19.12": true, "esbuild@0.18.0": false}"#, + "esbuild", + "0.19.12", + ) + .unwrap(); + assert_eq!(lost.allows, vec!["esbuild@0.19.12".to_string()]); + assert_eq!(lost.denies, vec!["esbuild@0.18.0".to_string()]); + } + + #[test] + fn denied_unreviewed_or_unrelated_lose_nothing() { for policy in [ r#"{"esbuild@0.19.12": true, "esbuild": false}"#, + r#"{"esbuild@0.19.12": false}"#, r#"{"esbuild@0.19.11": true}"#, r#"{"esbuild@^0.19.0": true}"#, r#"{"esbuild-wasm@0.19.12": true}"#, r#"{"alias@npm:esbuild@0.19.12": true}"#, r#"{"esbuild@0.19.12": "yes"}"#, + r#"{"esbuild": true, "esbuild@^0.18": false}"#, r#"[]"#, ] { - assert!( - pinned_approvals_lost(&pkg(policy), "esbuild", "0.19.12").is_empty(), - "{policy}" - ); + assert!(lost_for(policy, "esbuild", "0.19.12").is_none(), "{policy}"); } - assert!(pinned_approvals_lost("not json", "esbuild", "0.19.12").is_empty()); - assert!(pinned_approvals_lost(r#"{"name": "app"}"#, "esbuild", "0.19.12").is_empty()); + assert!(AllowScripts::parse("not json").is_none()); + assert!(AllowScripts::parse(r#"{"name": "app"}"#).is_none()); + assert!(AllowScripts::parse(r#"{"allowScripts": {}}"#).is_none()); } #[test] fn bom_prefixed_package_json_is_read() { - let text = format!("\u{feff}{}", pkg(r#"{"esbuild@0.19.12": true}"#)); - assert_eq!(pinned_approvals_lost(&text, "esbuild", "0.19.12").len(), 1); + let text = "\u{feff}{\"allowScripts\": {\"esbuild@0.19.12\": true}}"; + assert!(AllowScripts::parse(text) + .unwrap() + .lost("esbuild", "0.19.12") + .is_some()); } #[test] - fn install_script_flag_is_read_from_lock_entries() { - let lock = r#"{"lockfileVersion": 3, "packages": { + fn install_script_read_from_rewired_v3_entries_only() { + let lock: Value = serde_json::from_str( + r#"{"lockfileVersion": 3, "packages": { "": {"name": "app"}, - "node_modules/esbuild": {"version": "0.19.12", "hasInstallScript": true}, - "node_modules/plain": {"version": "1.0.0"}, - "node_modules/p/node_modules/bundled": {"version": "1.0.0", "hasInstallScript": true, "inBundle": true}, - "node_modules/al": {"name": "aliased", "version": "2.0.0", "hasInstallScript": true} - }}"#; - assert!(lock_has_install_script(lock, "esbuild", "0.19.12")); - assert!(!lock_has_install_script(lock, "esbuild", "0.19.11")); - assert!(!lock_has_install_script(lock, "plain", "1.0.0")); - assert!(!lock_has_install_script(lock, "bundled", "1.0.0")); - assert!(lock_has_install_script(lock, "aliased", "2.0.0")); - assert!(!lock_has_install_script(lock, "app", "1.0.0")); + "node_modules/esbuild": {"version": "0.19.12", "resolved": "R", "hasInstallScript": true}, + "node_modules/x/node_modules/esbuild": {"version": "0.19.12", "resolved": "U", "hasInstallScript": true}, + "node_modules/plain": {"version": "1.0.0", "resolved": "R"}, + "node_modules/p/node_modules/bundled": {"version": "1.0.0", "resolved": "R", "hasInstallScript": true, "inBundle": true}, + "node_modules/al": {"name": "aliased", "version": "2.0.0", "resolved": "R", "hasInstallScript": true} + }}"#, + ) + .unwrap(); + let rewired = |_: &str, e: &Value| e["resolved"] == "R"; + assert!(rewired_runs_install_script( + &lock, "esbuild", "0.19.12", rewired + )); + // Only a copy the run left on the registry has a script: no loss. + assert!(!rewired_runs_install_script( + &lock, + "esbuild", + "0.19.12", + |k, _| { k == "node_modules/plain" } + )); + assert!(!rewired_runs_install_script( + &lock, "esbuild", "0.19.11", rewired + )); + assert!(!rewired_runs_install_script( + &lock, "plain", "1.0.0", rewired + )); + assert!(!rewired_runs_install_script( + &lock, "bundled", "1.0.0", rewired + )); + assert!(rewired_runs_install_script( + &lock, "aliased", "2.0.0", rewired + )); + assert!(!rewired_runs_install_script(&lock, "app", "1.0.0", rewired)); + } + + #[test] + fn rewired_v1_entry_counts_as_running_a_script() { + let lock: Value = serde_json::from_str( + r#"{"lockfileVersion": 1, "dependencies": { + "a": {"version": "1.0.0", "resolved": "U", "dependencies": { + "esbuild": {"version": "0.19.12", "resolved": "R"}}}, + "b": {"version": "1.0.0", "resolved": "R", "bundled": true} + }}"#, + ) + .unwrap(); + let rewired = |_: &str, e: &Value| e["resolved"] == "R"; + assert!(rewired_runs_install_script( + &lock, "esbuild", "0.19.12", rewired + )); + assert!(!rewired_runs_install_script(&lock, "a", "1.0.0", rewired)); + assert!(!rewired_runs_install_script(&lock, "b", "1.0.0", rewired)); } } diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index 4f2c374d2..58e4f81e3 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -1778,33 +1778,38 @@ pub(super) async fn allow_file_refusal_with( /// The `vendor_npm_allow_scripts` advisory for `name@version` (#1595), or /// `None`: npm >= 12 matches a version-pinned `package.json#allowScripts` -/// approval only against a registry tarball URL, so once vendoring rewires -/// the lock to the `file:` tarball the approved install script stops -/// running. Only a package some npm lock records `hasInstallScript` for -/// can lose one; the policy is the user's and is never rewritten. +/// approval only against a registry tarball URL (and fails a pinned deny +/// closed), so once vendoring rewires package-lock.json to the `file:` +/// tarball an approved install script can stop running. Only the +/// instances vendoring rewires count (the [`scan_lock_matches`] set), and +/// only package-lock.json: npm >= 12 never reads npm-shrinkwrap.json. The +/// policy is the user's and is never rewritten. pub(super) async fn allow_scripts_lost( project_root: &Path, name: &str, version: &str, ) -> Option { use crate::patch::redirect::npm_allow_scripts::{ - lock_has_install_script, lost_detail, pinned_approvals_lost, + lost_detail, rewired_runs_install_script, AllowScripts, NPM12_LOCK, }; use crate::utils::fs::read_regular_to_string; let package_json = read_regular_to_string(&project_root.join("package.json")) .await .ok()?; - let keys = pinned_approvals_lost(&package_json, name, version); - if keys.is_empty() { + let lost = AllowScripts::parse(&package_json)?.lost(name, version)?; + let bytes = read_regular_to_bytes(&project_root.join(NPM12_LOCK)) + .await + .ok()?; + let lock = parse_json_manifest(&bytes).ok()?; + let overrides = NpmOverrides::read(project_root).await; + let LockScan::Matches(matches) = + scan_lock_matches(&lock, &overrides, name, version, &mut Vec::new()) + else { return None; - } - let mut has_script = false; - for lock_name in NPM_LOCKS { - if let Ok(text) = read_regular_to_string(&project_root.join(lock_name)).await { - has_script |= lock_has_install_script(&text, name, version); - } - } - has_script.then(|| lost_detail(name, version, &keys, "the vendored `file:` tarball")) + }; + let rewired = |key: &str, _: &Value| matches.iter().any(|m| m.key == key); + rewired_runs_install_script(&lock, name, version, rewired) + .then(|| lost_detail(name, version, &lost, "the vendored `file:` tarball")) } /// [`allow_file_refusal_with`] against this process's npm config layers. From 63b4504b4b595fb8b6560ded91e2520665330d3b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 13:20:20 +0000 Subject: [PATCH 4/6] Use the shared BOM helper for package.json The source lint requires production code to strip a UTF-8 BOM through formats::text rather than inline. Refs #1595 Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-core/src/patch/redirect/npm_allow_scripts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs b/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs index c57c6f92e..435c5671f 100644 --- a/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs +++ b/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs @@ -93,7 +93,7 @@ impl AllowScripts { /// file is unreadable): nothing can be lost. pub fn parse(package_json: &str) -> Option { let manifest: Value = - serde_json::from_str(package_json.trim_start_matches('\u{feff}')).ok()?; + serde_json::from_str(crate::formats::text::strip_bom(package_json)).ok()?; let policy = manifest.get("allowScripts")?.as_object()?; let entries: Vec<_> = policy .iter() From 6c41e51962a86bf971fe9a63ed44c47b7a0ef6ae Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 13:32:54 +0000 Subject: [PATCH 5/6] Read a BOM-prefixed package-lock.json in hosted The hosted allowScripts check parsed the rewritten lock without stripping a UTF-8 BOM, which the npm rewriter keeps, so such a lock silently skipped the warning. Refs #1595 Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/in_process_redirect.rs | 14 ++++++++++++++ crates/socket-patch-core/src/hosted/engine.rs | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index f041e5af6..6874ab71d 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -3935,6 +3935,20 @@ async fn hosted_npm_warns_when_pinned_allow_scripts_approval_is_lost() { "the user's allowScripts policy is never rewritten" ); + // A BOM-prefixed lock (kept by the rewriter) is read the same way. + let tmp = tempfile::tempdir().unwrap(); + write_project_with_allow_scripts(tmp.path(), serde_json::json!({ &pinned: true })); + let lock_path = tmp.path().join("package-lock.json"); + let mut bom = b"\xEF\xBB\xBF".to_vec(); + bom.extend(std::fs::read(&lock_path).unwrap()); + std::fs::write(&lock_path, bom).unwrap(); + let env = run_redirect_subprocess(tmp.path(), &server.uri()); + assert_eq!(vlt_hosted_common::redirected(&env), 1, "anchor: {env}"); + assert!( + warning_codes(&env).contains(&"redirect_npm_allow_scripts".to_string()), + "a BOM-prefixed lock still warns: {env}" + ); + // A name-only approval keeps matching the redirected entry: no warning. let tmp = tempfile::tempdir().unwrap(); write_project_with_allow_scripts(tmp.path(), serde_json::json!({ &pinned: true, NAME: true })); diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 4503b6a2d..966ff41f9 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -2819,7 +2819,7 @@ fn npm_allow_scripts( return Vec::new(); }; let Some(lock) = - read(NPM12_LOCK).and_then(|text| serde_json::from_str::(text).ok()) + read(NPM12_LOCK).and_then(|text| crate::vendor::common::parse_json_text(text).ok()) else { return Vec::new(); }; From cd0cf077f4f588956b79d5e1af3295b6894aeb35 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 17:06:14 +0000 Subject: [PATCH 6/6] Compare single-version allowScripts pins as npm does npm 12 parses a single-version key such as `esbuild@0.19.12` as an npa `version` spec and matches it with `trusted.version === fetchSpec`, a plain string comparison. Only `||` disjunctions go through `semver.satisfies` (loose), and every part of one must pass strict `semver.valid`. The model matched every pin by semver precedence, so `v0.19.12`, `=0.19.12` and `0.19.12+b` counted as approving `0.19.12`. That raised a warning for an approval npm never gave, and its remedy would approve a script nobody approved. It also missed a loosely spelled pinned deny next to a name-only allow, which npm ignores before the rewrite but fails closed on afterwards. Single-version pins now compare as strings. Disjunctions keep the loose match, and a part with `=` voids the key, as in npm. Refs #1595 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018kARWYm2Bo52TcsTXveagg --- .../src/patch/redirect/npm_allow_scripts.rs | 109 ++++++++++++++---- 1 file changed, 85 insertions(+), 24 deletions(-) diff --git a/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs b/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs index 435c5671f..fe4e9e123 100644 --- a/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs +++ b/crates/socket-patch-core/src/patch/redirect/npm_allow_scripts.rs @@ -36,7 +36,47 @@ enum KeyKind { NameOnly, /// An exact version or `||`-joined exact versions: matches only when /// npm reads a covered version from a registry-shaped `resolved`. - Pinned(Vec), + Pinned(Pin), +} + +/// The versions a pinned key covers, as arborist compares them. +enum Pin { + /// One version (npa `type: 'version'`): the trimmed spec must equal + /// the registry version as a string, so `v1.2.3`, `=1.2.3` and + /// `1.2.3+build` never match `1.2.3`. + Exact(String), + /// A `||` disjunction (npa `type: 'range'`): every part must be a + /// strict `semver.valid` version, matched by `semver.satisfies` (loose, + /// so build metadata is ignored). + AnyOf(Vec), +} + +impl Pin { + fn parse(spec: &str) -> Option { + if !spec.contains("||") { + // npa only calls a loose-valid version a version; anything + // else is a range or tag that matchRegistry rejects. + loose_version(spec)?; + return Some(Self::Exact(spec.to_string())); + } + spec.split("||") + .map(|part| { + let part = part.trim(); + Version::parse(part.strip_prefix('v').unwrap_or(part)).ok() + }) + .collect::>>() + .map(Self::AnyOf) + } + + fn covers(&self, version: &str) -> bool { + match self { + Self::Exact(spec) => spec == version, + Self::AnyOf(pins) => loose_version(version).is_some_and(|v| { + pins.iter() + .any(|pin| pin.cmp_precedence(&v) == Ordering::Equal) + }), + } + } } /// The approval npm 12 gives one package under the policy. @@ -75,8 +115,8 @@ fn split_key(key: &str) -> (&str, Option<&str>) { } } -/// `semver.valid(v, { loose: true })` for the exact-version spellings npm -/// accepts in a pin: surrounding whitespace and a leading `=` / `v`. +/// `semver.valid(v, { loose: true })`, which also accepts surrounding +/// whitespace and a leading `=` / `v`: how npa decides a spec is a version. fn loose_version(text: &str) -> Option { let text = text.trim(); let text = text.strip_prefix('=').unwrap_or(text).trim_start(); @@ -102,11 +142,7 @@ impl AllowScripts { let (name, spec) = split_key(key); let kind = match spec.map(str::trim) { None | Some("") | Some("*") => KeyKind::NameOnly, - Some(spec) => KeyKind::Pinned( - spec.split("||") - .map(loose_version) - .collect::>>()?, - ), + Some(spec) => KeyKind::Pinned(Pin::parse(spec)?), }; Some((key.clone(), name.to_string(), kind, allow)) }) @@ -116,7 +152,7 @@ impl AllowScripts { /// npm's verdict for `name`, at `version` when npm can read one from a /// registry `resolved`, or `None` once it cannot (deny wins). - fn verdict(&self, name: &str, version: Option<&Version>) -> Verdict { + fn verdict(&self, name: &str, version: Option<&str>) -> Verdict { let mut allowed = false; for (_, key_name, kind, allow) in &self.entries { if key_name != name { @@ -124,9 +160,7 @@ impl AllowScripts { } let matches = match (kind, version) { (KeyKind::NameOnly, _) => true, - (KeyKind::Pinned(pins), Some(v)) => pins - .iter() - .any(|pin| pin.cmp_precedence(v) == Ordering::Equal), + (KeyKind::Pinned(pin), Some(v)) => pin.covers(v), // Unverifiable version: a deny fails closed, an allow is // refused. (KeyKind::Pinned(_), None) => !allow, @@ -147,8 +181,8 @@ impl AllowScripts { /// entries resolve outside the registry, or `None` when npm 12 runs /// them as before (or never did: denied or unreviewed already). pub fn lost(&self, name: &str, version: &str) -> Option { - let version = loose_version(version)?; - if self.verdict(name, Some(&version)) != Verdict::Allowed + loose_version(version)?; + if self.verdict(name, Some(version)) != Verdict::Allowed || self.verdict(name, None) == Verdict::Allowed { return None; @@ -162,12 +196,7 @@ impl AllowScripts { .filter(|(_, _, kind, _)| match kind { // Every pinned deny matches after the rewrite; only the // allows that covered this version were lost. - KeyKind::Pinned(pins) => { - !allow - || pins - .iter() - .any(|pin| pin.cmp_precedence(&version) == Ordering::Equal) - } + KeyKind::Pinned(pin) => !allow || pin.covers(version), KeyKind::NameOnly => false, }) .map(|(key, ..)| key.clone()) @@ -284,11 +313,43 @@ mod tests { } #[test] - fn scoped_pin_disjunction_loose_spellings_and_build_metadata_are_lost() { - let policy = r#"{"@img/sharp@0.33.0 || 0.33.5": true, "@img/sharp@v0.33.5": true, - "@img/sharp@=0.33.5": true, "@img/sharp@0.33.5+b": true}"#; + fn scoped_pin_and_disjunction_are_lost() { + let policy = r#"{"@img/sharp@0.33.0 || 0.33.5": true, "@img/sharp@ 0.33.5 ": true, + "@img/sharp@v0.33.0 || 0.33.5+b": true}"#; let lost = lost_for(policy, "@img/sharp", "0.33.5").unwrap(); - assert_eq!(lost.allows.len(), 4); + assert_eq!(lost.allows.len(), 3); + } + + /// npm compares a single-version pin as a string, so loose spellings + /// never approved the registry copy: nothing is lost, and the remedy + /// must not approve a script nobody approved. + #[test] + fn loose_single_version_spellings_never_matched() { + for spec in ["v0.19.12", "=0.19.12", "0.19.12+b"] { + let policy = format!(r#"{{"esbuild@{spec}": true}}"#); + assert!(lost_for(&policy, "esbuild", "0.19.12").is_none(), "{spec}"); + } + // A disjunction part npm's strict semver.valid rejects voids the key. + assert!(lost_for( + r#"{"esbuild@=0.19.12 || 0.20.0": true}"#, + "esbuild", + "0.19.12" + ) + .is_none()); + } + + /// A loosely spelled deny never matched the registry copy but fails + /// closed after the rewrite, overriding a name-only allow. + #[test] + fn loose_single_version_deny_starts_matching() { + let lost = lost_for( + r#"{"esbuild": true, "esbuild@v0.19.12": false}"#, + "esbuild", + "0.19.12", + ) + .unwrap(); + assert!(lost.allows.is_empty()); + assert_eq!(lost.denies, vec!["esbuild@v0.19.12".to_string()]); } #[test]