From f93d4b7f4d427693664cd03bd8c4f4c192ace64f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 23:21:58 +0000 Subject: [PATCH 1/3] Start fix for #1233 Assisted-by: Claude Code:claude-opus-5-5 From 09e5e2f48346e9bcfce74acd6b4e2f065316aa78 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 23:37:19 +0000 Subject: [PATCH 2/3] Expand ${VAR} in npm settings like npm does npm runs every config value through env-replace, in every layer. socket-patch read replace-registry-host, allow-remote and allow-file raw, so replace-registry-host=${RRH} with RRH=always looked like the literal "${RRH}". The hosted scan then gave no warning and reported success while every npm ci failed with E404 (#1233). The same gap made allow-remote=${AR} and allow-file=${AF} read as non-"all" values. Values from the env, project, user, global and builtin layers are now trimmed and env-replaced before they are compared, like npm's parseField. An unset ${VAR} stays literal, as it does in npm. Fixes #1233 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- .../tests/vendor/redirect_npm_allow_remote.rs | 55 ++++++ .../src/patch/redirect/npmrc.rs | 180 ++++++++++++++++-- .../socket-patch-core/src/vendor/npm_lock.rs | 65 ++++++- 4 files changed, 277 insertions(+), 25 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 17dd3fe45..f3deb15ee 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -161,7 +161,7 @@ For a **9.0 root lock**, the CLI ensures `pnpm-workspace.yaml` carries `trustLoc **npm hosted-mode `allow-remote` contract**: npm >=12 defaults `allow-remote=none` and refuses (EALLOWREMOTE) every lockfile entry whose `resolved` tarball is not served by the configured registry — exactly what a hosted redirect writes into `package-lock.json` / `npm-shrinkwrap.json`. Whenever a run leaves a ROOT npm lock carrying a granted hosted artifact URL (spliced this run, or already redirected by an earlier one — a missed config heals on re-run), the CLI ensures `allow-remote=all` in the project-root `.npmrc`: the file is created holding exactly `allow-remote=all\n` when absent, otherwise one `allow-remote=all` line is spliced in after the last non-empty top-level line (before any ini `[section]` header), in the file's own line ending, with the BOM, CRLF and trailing-newline shape preserved. The write lands in `redirect.rewrittenFiles` (edit kind `redirect_npmrc_allow_remote`), respects `--dry-run` (nothing written; the warning says what would be — including for a vendored → hosted takeover the dry run only previews), and is disabled by `--no-npm-allow-remote-config` / `SOCKET_NO_NPM_ALLOW_REMOTE_CONFIG`. The `.npmrc` grammar is npm's own `ini` parser's (cross-checked against it): lines split on any run of `\r` / `\n` (a bare `\r` ends a line), only the exact key `allow-remote` counts after ini unquoting (npm ignores `allow_remote` / `ALLOW-REMOTE` in a `.npmrc`; such a line is left alone and the real key appended), comment lines are ignored, a `[section]` header is recognized only as npm does — on the UNTRIMMED line (an indented or BOM-prefixed `[sec]` is a plain top-level key) — and ends the top-level scope, quotes and inline comments are stripped, the LAST top-level assignment wins, and the value is case-sensitive. An explicit other value (`allow-remote=none` / `root` / anything but `all`) is RESPECTED and never rewritten — the pnpm `trustLockfile: false` precedent — in the project `.npmrc` AND in every other npm config layer npm would consult: an `npm_config_allow_remote` environment variable (any spelling npm normalizes; it beats every `.npmrc`, so a project write could not take effect), and — when the project file sets nothing — the user (`npm_config_userconfig` / `~/.npmrc`), global (`npm_config_globalconfig` / `/etc/npmrc`, prefix from `npm_config_prefix`, the user/builtin config, `PREFIX` or the `node` binary's install root) and builtin (npm's own `npmrc` beside the `node` binary: `/lib/node_modules/npm/npmrc`, `\node_modules\npm\npmrc` on Windows) config files — path values `${VAR}`-expanded and `~`-expanded like npm, env names case-insensitive on Windows, where a committed project line would silently override a machine / org policy. A symlinked, non-regular or unreadable `.npmrc`, or one with bare-`\r` line endings (npm splits on them, the line splice does not), is left untouched. Every variant emits the `redirect_npm_allow_remote` warning (written / would write / already set / explicit value respected — naming the project file, the env var, or the user/global/builtin config path — / opted out / unreadable or unsupported), always with the tradeoff: `allow-remote=all` lets npm install ANY url-resolved dependency, not just Socket's patched ones, while the per-entry sha512 integrity pins stay enforced; the remedy for the non-writing variants is `allow-remote=all` in `.npmrc` or `npm ci --allow-remote=all`. npm <=11 is unaffected (11 defaults to `all`, <=10 has no such setting). **Unwind (v5.0: no ledger)**: once `rollback`, `remove` or a hosted → vendored takeover has restored the last hosted entry of the root `package-lock.json` / `npm-shrinkwrap.json` to its upstream registry entry (see "Hosted unwind coverage"), a project `.npmrc` holding exactly `allow-remote=all\n` (the file hosted mode creates) is deleted; any other `.npmrc` that still has a top-level `allow-remote=all` line is left untouched and the `npm_allow_remote_left` warning says the line may be removed if nothing else needs it (v5 keeps no record of whether hosted mode added it, so it is never removed behind the user's back). The rewrite's stage file is created with the `.npmrc`'s own permission bits (a 0600 token-bearing file is never staged world-readable). **Vendored mode is unaffected by `allow-remote`**: its `file:.socket/vendor/…` resolutions are npm `file` specs, which npm >= 11.14 gates by `allow-file` (default `all`), never `allow-remote` — verified by the real npm 12 vendored matrix. **npm vendored `allow-file` contract (#969)**: `allow-file=none` refuses every `file:` dependency, and any other non-`all` value (`root`) admits one only when the project root or a workspace declares it and resolves to that very lock node (arborist's `_isRoot`); otherwise every `npm ci` / `npm install` fails EALLOWFILE. The effective value is read from the same npm config layers as `allow-remote` (an `npm_config_allow_file` environment variable, then the project `.npmrc`, then the user / global / builtin config files). When it refuses a `package-lock.json` / `npm-shrinkwrap.json` instance of a vendored `name@version` (the set vendoring rewires), the vendored run (`vendor`, `scan` / `get --mode vendored`, dry runs and in-sync re-runs included) still vendors, leaves the setting untouched, and records the `vendor_npm_allow_file` advisory naming the setting's source, the refused lock entries and the remedy (`allow-file=all` in the project `.npmrc`, unsetting the env var, or `npm ci --allow-file=all`); `vendor --check` fails that entry `vendor_check_failed` with the same reason. Other npm-family flavors (yarn, pnpm, bun, vlt) do not read `allow-file` and are not checked. -**npm hosted-mode `replace-registry-host` contract**: npm >=8 rewrites the origin of a lock's `resolved` URL to the configured registry when its `replace-registry-host` setting is `always`, or equals the URL's hostname (`npmjs`, the default, stands for `registry.npmjs.org`; `never` matches nothing). A hosted pin rewritten that way is fetched from `/patch/npm/…` and every `npm ci` / `npm install` fails E404 (closed: never unpatched bytes; a warm npm cache can hide it locally). Whenever a run leaves a root npm lock carrying a granted hosted artifact URL (the same trigger as the `allow-remote` contract, so an "already on hosted patches" re-run reports it too), the CLI resolves `replace-registry-host` across npm's layers — an `npm_config_replace_registry_host` environment variable (any spelling npm normalizes), then the project `.npmrc`, then the user / global / builtin config files (located exactly like the `allow-remote` layers) — and, when the effective value rewrites a pinned host, emits the `redirect_npm_replace_registry_host` warning naming the layer (env var / project file / config path) and the remedies: `replace-registry-host=npmjs` in the project `.npmrc` (or unsetting the env var), or vendored mode, whose `file:` resolutions npm never rewrites. The setting is never rewritten; the redirect itself still lands and the exit status is unchanged. +**npm hosted-mode `replace-registry-host` contract**: npm >=8 rewrites the origin of a lock's `resolved` URL to the configured registry when its `replace-registry-host` setting is `always`, or equals the URL's hostname (`npmjs`, the default, stands for `registry.npmjs.org`; `never` matches nothing). A hosted pin rewritten that way is fetched from `/patch/npm/…` and every `npm ci` / `npm install` fails E404 (closed: never unpatched bytes; a warm npm cache can hide it locally). Whenever a run leaves a root npm lock carrying a granted hosted artifact URL (the same trigger as the `allow-remote` contract, so an "already on hosted patches" re-run reports it too), the CLI resolves `replace-registry-host` across npm's layers — an `npm_config_replace_registry_host` environment variable (any spelling npm normalizes), then the project `.npmrc`, then the user / global / builtin config files (located exactly like the `allow-remote` layers), each value env-replaced the way npm reads it (`${VAR}` / `${VAR?}`, so `replace-registry-host=${RRH}` with `RRH=always` counts as `always`; the same holds for `allow-remote` and the vendored `allow-file` check) — and, when the effective value rewrites a pinned host, emits the `redirect_npm_replace_registry_host` warning naming the layer (env var / project file / config path) and the remedies: `replace-registry-host=npmjs` in the project `.npmrc` (or unsetting the env var), or vendored mode, whose `file:` resolutions npm never rewrites. The setting is never rewritten; the redirect itself still lands and the exit status is unchanged. **pnpm per-member locks (#492)**: a workspace whose root `pnpm-workspace.yaml` sets `sharedWorkspaceLockfile: false` (or, when the YAML leaves the key unset, whose root `.npmrc` sets `shared-workspace-lockfile=false`; on disk, when neither file sets it, `pnpm_config_shared_workspace_lockfile` / `npm_config_shared_workspace_lockfile`; booleans in any letter case, as pnpm's YAML reader takes them) installs each member from the member's own `pnpm-lock.yaml`. Run from the workspace root, hosted mode reads every member lock the `packages:` globs name (quoted `!` negations applied; `node_modules`, `bower_components`, `test` and `tests` trees and symlinked directories skipped) beside the root lock, which may be absent (pnpm 7): each is rewritten like the root lock, a 9.0 member lock engages the `trustLockfile: true` auto-config in the ROOT `pnpm-workspace.yaml` (heal-on-rerun included), and discovery (`list`, `vex`, `rollback`, `remove`) reads the same member locks. A root lock whose `importers:` lists a member is a shared lock, so member locks beside it are stale and never read or attested. When the members' locks cannot be listed or read (no `packages:` key, whose pnpm <= 8 default finds projects in every directory; a `packages:` value that is not a plain list of globs; no member lock and no root lock, or a member lock that exists but is not readable text), every pnpm lock (the root's included) is left untouched and the run warns `redirect_pnpm_member_locks_unresolved` instead of `redirect_pnpm_no_lockfile`; no pnpm pin is confirmed. The lockfile inventory reads the same member locks, so their dependencies are discovered without an installed tree. The in-memory engine gives the same result as a disk run from the workspace root: a directory whose `pnpm-lock.yaml` has no `pnpm-workspace.yaml` beside it is a workspace member, never a root of its own, when (with `npm` among the `ecosystems`) the nearest ancestor `pnpm-workspace.yaml` is readable and its `packages:` globs (read as above, no braces, classes or extglobs) list it, it holds a `package.json` / `package.yaml` / `package.json5`, the workspace root installs it from its own lock (`sharedWorkspaceLockfile: false` with the member list resolved) or from a root lock beside which its lock is a stale leftover, and socket.yml admits the workspace root. Its lock is then demoted into that workspace root, which becomes a root even with no lock of its own (pnpm 7), the way a Cargo member's lock is demoted; the member stays a root only when it holds another lock marker, without its pnpm lock. The workspace root then pins (or refuses) its member locks as above, trusts them in its own file, refuses a member's branch lock (#556) and leaves a member lock beside a shared root lock unread. A directory that fails any of those tests keeps its lock as a root of its own (pinned like any root, or refused with `redirect_pnpm_settings_elsewhere` while the trust auto-config is on and the ancestor file may list it). Path selection adds the workspace root above every candidate to `roots`, fetches it, and fetches the member locks, branch locks and manifests below it in every directory holding a package manifest; given those `roots` as `projectRoots`, the session drops a confirmed member named beside its workspace root (unless it holds another lock) and a lockless workspace root that pins no member, so it runs the roots it would detect. A confirmed member named in `projectRoots` without its workspace root is not pinned: its lock is dropped and the run-level `pnpm_member_lock_ignored` warning names the workspace root to scan. diff --git a/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs b/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs index 7dd013aa0..a618ecbfe 100644 --- a/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs +++ b/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs @@ -909,6 +909,61 @@ async fn replace_registry_host_rewriting_the_pin_is_warned() { ); } +/// #1233: npm env-replaces every config value, so +/// `replace-registry-host=${RRH}` with `RRH=always` rewrites every hosted +/// pin exactly like `=always`. The scan read the literal `${RRH}`, matched +/// no host and stayed silent; it now warns in the project and user layers. +#[tokio::test] +async fn replace_registry_host_via_env_interpolation_is_warned() { + let server = MockServer::start().await; + mock_api(&server).await; + + // Project `.npmrc`. + let tmp = tempfile::tempdir().unwrap(); + write_npm_project(tmp.path(), "package-lock.json"); + std::fs::write(tmp.path().join(".npmrc"), "replace-registry-host=${RRH}\n").unwrap(); + let (code, doc, _) = + scan_hosted_env(tmp.path(), &server.uri(), &["--json"], &[("RRH", "always")]); + assert_eq!(code, 0, "{doc:#}"); + let detail = replace_host_warning(&doc).unwrap_or_else(|| panic!("no {REPLACE_HOST}: {doc:#}")); + assert!( + detail.contains("the project .npmrc sets `replace-registry-host=always`"), + "{detail}" + ); + + // User config. + let tmp = tempfile::tempdir().unwrap(); + let cfg = tempfile::tempdir().unwrap(); + write_npm_project(tmp.path(), "package-lock.json"); + let user = cfg.path().join("user.npmrc"); + std::fs::write(&user, "replace-registry-host=${RRH}\n").unwrap(); + let user_s = user.to_str().unwrap(); + let (code, doc, _) = scan_hosted_env( + tmp.path(), + &server.uri(), + &["--json"], + &[ + ("NPM_CONFIG_USERCONFIG", user_s), + ("npm_config_userconfig", user_s), + ("RRH", "always"), + ], + ); + assert_eq!(code, 0, "{doc:#}"); + let detail = replace_host_warning(&doc).unwrap_or_else(|| panic!("no {REPLACE_HOST}: {doc:#}")); + assert!( + detail.contains("the user npm config") && detail.contains(user_s), + "{detail}" + ); + + // An unset `${RRH}` stays literal for npm too, and matches no host. + let tmp = tempfile::tempdir().unwrap(); + write_npm_project(tmp.path(), "package-lock.json"); + std::fs::write(tmp.path().join(".npmrc"), "replace-registry-host=${RRH}\n").unwrap(); + let (code, doc, _) = scan_hosted_env(tmp.path(), &server.uri(), &["--json"], &[]); + assert_eq!(code, 0, "{doc:#}"); + assert_eq!(replace_host_warning(&doc), None, "{doc:#}"); +} + /// Review finding: `remove` dropped the hosted leg's warnings. A /// redirect-created `.npmrc` the user has since added to is no longer the /// scaffold, so the restore keeps it byte-for-byte and warns that the diff --git a/crates/socket-patch-core/src/patch/redirect/npmrc.rs b/crates/socket-patch-core/src/patch/redirect/npmrc.rs index b700ec639..5f4de9d1f 100644 --- a/crates/socket-patch-core/src/patch/redirect/npmrc.rs +++ b/crates/socket-patch-core/src/patch/redirect/npmrc.rs @@ -281,6 +281,9 @@ pub struct OuterAllowRemote { /// `replace-registry-host` in the same outer layers (see /// [`effective_replace_registry_host`]). pub replace_registry_host: OuterSetting, + /// The process facts the layers were resolved with. npm env-replaces + /// the project `.npmrc`'s values too, against the same environment. + pub config_env: NpmConfigEnv, } /// One npm config key as the layers OUTSIDE the project `.npmrc` set it. @@ -294,7 +297,8 @@ pub struct OuterSetting { pub file: Option, } -/// One explicit `allow-remote` assignment in a non-project npm config file. +/// One explicit assignment in a non-project npm config file (its value +/// already env-replaced, see [`NpmConfigEnv::config_value`]). #[derive(Debug, Clone, PartialEq)] pub struct OuterFileValue { /// `"user"`, `"global"` or `"builtin"`. @@ -307,7 +311,7 @@ pub struct OuterFileValue { /// (`@npmcli/config`): environment, home directory, and the node binary /// (npm derives the default global prefix from `process.execPath`, and its /// own install root — the builtin config's home — sits beside it). -#[derive(Debug, Default, Clone)] +#[derive(Debug, Default, Clone, PartialEq)] pub struct NpmConfigEnv { pub vars: Vec<(String, String)>, pub home: Option, @@ -428,6 +432,14 @@ impl NpmConfigEnv { out } + /// A (non-path) config value as npm's `parseField` reads it: trimmed, + /// then env-replaced. npm does this to every string value in every + /// layer (env, project, user, global, builtin), so `allow-remote=${AR}` + /// with `AR=all` is `all` to npm. + pub fn config_value(&self, value: &str) -> String { + self.env_replace(value.trim_matches(is_js_ws)) + } + /// A `path`-typed config value as npm's `parseField` reads it: trimmed, /// env-replaced, then a leading `~/` (also `~\` on Windows) expands /// against [`Self::home`]. @@ -446,7 +458,8 @@ impl NpmConfigEnv { /// prefix case-insensitive, then non-leading `_` → `-` and lowercased. /// Empty values are ignored (npm skips them). When several spellings /// are set a non-`benign` one wins (process env order is unspecified, - /// so the conservative reading is reported). + /// so the conservative reading is reported). The value is returned raw: + /// callers env-replace it as its type needs. fn npm_config(&self, key: &str, benign: &str) -> Option<(String, String)> { let mut found: Option<(String, String)> = None; for (k, v) in &self.vars { @@ -468,7 +481,11 @@ impl NpmConfigEnv { c.to_ascii_lowercase() }); } - if norm == key && found.as_ref().is_none_or(|(_, prev)| prev == benign) { + if norm == key + && found + .as_ref() + .is_none_or(|(_, prev)| self.config_value(prev) == benign) + { found = Some((k.clone(), v.clone())); } } @@ -509,12 +526,13 @@ pub fn resolve_outer_allow_remote( ) -> OuterAllowRemote { let layers = outer_config_layers(env, read); OuterAllowRemote { - env: env.npm_config("allow-remote", "all"), - file: outer_file_value(&layers, "allow-remote"), + env: env_value(env, "allow-remote", "all"), + file: outer_file_value(env, &layers, "allow-remote"), replace_registry_host: OuterSetting { - env: env.npm_config(REPLACE_REGISTRY_HOST, "npmjs"), - file: outer_file_value(&layers, REPLACE_REGISTRY_HOST), + env: env_value(env, REPLACE_REGISTRY_HOST, "npmjs"), + file: outer_file_value(env, &layers, REPLACE_REGISTRY_HOST), }, + config_env: env.clone(), } } @@ -530,11 +548,18 @@ pub fn resolve_outer_npm_setting( ) -> OuterSetting { let layers = outer_config_layers(env, read); OuterSetting { - env: env.npm_config(key, benign), - file: outer_file_value(&layers, key), + env: env_value(env, key, benign), + file: outer_file_value(env, &layers, key), } } +/// The `npm_config_*` variable setting `key`, its value env-replaced the +/// way npm reads it. +fn env_value(env: &NpmConfigEnv, key: &str, benign: &str) -> Option<(String, String)> { + env.npm_config(key, benign) + .map(|(var, value)| (var, env.config_value(&value))) +} + /// One non-project npm config file: `(layer, path, text)`. type OuterLayer = (&'static str, Option, Option); @@ -597,14 +622,19 @@ fn outer_config_layers( ] } -/// The highest-precedence top-level `key` assignment among `layers`. -fn outer_file_value(layers: &[OuterLayer], key: &str) -> Option { +/// The highest-precedence top-level `key` assignment among `layers`, +/// env-replaced like npm reads it. +fn outer_file_value( + env: &NpmConfigEnv, + layers: &[OuterLayer], + key: &str, +) -> Option { layers.iter().find_map(|(layer, path, text)| { let value = npmrc_top_level_value(text.as_deref()?, key)?; Some(OuterFileValue { layer, path: path.clone()?, - value, + value: env.config_value(&value), }) }) } @@ -633,19 +663,23 @@ pub enum SettingSource { /// The `replace-registry-host` value npm would use, following its layer /// order (env > project > user > global > builtin), or `None` when no -/// layer sets it (npm's default `npmjs`). +/// layer sets it (npm's default `npmjs`). Every value is env-replaced the +/// way npm reads it (#1233). pub fn effective_replace_registry_host( project: Option<&str>, outer: &OuterAllowRemote, ) -> Option<(String, SettingSource)> { - let outer = &outer.replace_registry_host; - if let Some((var, value)) = &outer.env { + let setting = &outer.replace_registry_host; + if let Some((var, value)) = &setting.env { return Some((value.clone(), SettingSource::Env(var.clone()))); } if let Some(value) = project.and_then(|t| npmrc_top_level_value(t, REPLACE_REGISTRY_HOST)) { - return Some((value, SettingSource::Project)); + return Some(( + outer.config_env.config_value(&value), + SettingSource::Project, + )); } - outer.file.as_ref().map(|f| { + setting.file.as_ref().map(|f| { ( f.value.clone(), SettingSource::File { @@ -699,7 +733,7 @@ pub fn plan_npmrc_allow_remote(existing: Option<&str>) -> NpmrcPlan { /// ([`NpmrcPlan::EnvSet`]) beats everything; then the project value; then /// an explicit non-`all` user / global / builtin value /// ([`NpmrcPlan::OuterSet`]). Every explicit value is respected, never -/// overridden by a write. +/// overridden by a write, and read env-replaced like npm reads it. pub fn plan_npmrc_allow_remote_with(existing: Option<&str>, outer: &OuterAllowRemote) -> NpmrcPlan { if let Some((var, value)) = &outer.env { if value != "all" { @@ -709,7 +743,10 @@ pub fn plan_npmrc_allow_remote_with(existing: Option<&str>, outer: &OuterAllowRe }; } } - match existing.and_then(npmrc_allow_remote) { + match existing + .and_then(npmrc_allow_remote) + .map(|v| outer.config_env.config_value(&v)) + { Some(v) if v == "all" => return NpmrcPlan::AlreadyAll, Some(v) => return NpmrcPlan::UserSet(v), None => {} @@ -1046,7 +1083,13 @@ mod tests { &cfg_env(&[("npm_config_allow_remote", ""), ("npm_config_x", "none")]), |_| None, ); - assert_eq!(outer, OuterAllowRemote::default()); + assert_eq!( + outer, + OuterAllowRemote { + config_env: outer.config_env.clone(), + ..OuterAllowRemote::default() + } + ); let outer = resolve_outer_allow_remote(&cfg_env(&[("npm_config_allow_remote", "all")]), |_| None); assert_eq!( @@ -1371,4 +1414,99 @@ mod tests { assert!(replace_registry_host_rewrites("[::1]", "[::1]:80")); assert!(replace_registry_host_rewrites("[::1]", "[::1]")); } + + /// #1233: npm env-replaces every config value (`@npmcli/config` + /// `parseField` -> `envReplace`), in every layer. A + /// `replace-registry-host=${RRH}` with `RRH=always` is `always` to npm, + /// so it must read as `always` here too, wherever it is set. + #[test] + fn replace_registry_host_values_are_env_replaced_like_npm() { + use std::collections::HashMap; + use std::path::{Path, PathBuf}; + let host = "patch.socket.dev"; + let rrh = [("RRH", "always")]; + + // Project `.npmrc`. + let outer = resolve_outer_allow_remote(&cfg_env(&rrh), |_| None); + let (value, source) = + effective_replace_registry_host(Some("replace-registry-host=${RRH}\n"), &outer) + .expect("project layer sets it"); + assert_eq!((value.as_str(), source), ("always", SettingSource::Project)); + assert!(replace_registry_host_rewrites(&value, host)); + // `${RRH?}` and a padded value read the same way. + let (value, _) = + effective_replace_registry_host(Some("replace-registry-host = ${RRH?} \n"), &outer) + .unwrap(); + assert_eq!(value, "always"); + + // User `.npmrc`. + let files: HashMap = HashMap::from([( + PathBuf::from("/home/u/.npmrc"), + "replace-registry-host=${RRH}\n", + )]); + let read = |p: &Path| files.get(p).map(|s| s.to_string()); + let outer = resolve_outer_allow_remote(&cfg_env(&rrh), read); + let (value, source) = effective_replace_registry_host(None, &outer).unwrap(); + assert_eq!(value, "always"); + assert!(matches!(source, SettingSource::File { layer: "user", .. })); + + // An `npm_config_*` variable is env-replaced too. + let outer = resolve_outer_allow_remote( + &cfg_env(&[ + ("RRH", host), + ("npm_config_replace_registry_host", "${RRH}"), + ]), + |_| None, + ); + let (value, _) = effective_replace_registry_host(None, &outer).unwrap(); + assert!(replace_registry_host_rewrites(&value, host), "{value}"); + + // Unset: npm keeps `${RRH}` verbatim, which matches no host. + let outer = resolve_outer_allow_remote(&cfg_env(&[]), |_| None); + let (value, _) = + effective_replace_registry_host(Some("replace-registry-host=${RRH}\n"), &outer) + .unwrap(); + assert_eq!(value, "${RRH}"); + assert!(!replace_registry_host_rewrites(&value, host)); + } + + /// #1233: `allow-remote` is env-replaced like every npm value, so a + /// `${AR}` that expands to `all` is already `all`, in every layer, and + /// one that expands to anything else is respected under its real value. + #[test] + fn allow_remote_values_are_env_replaced_like_npm() { + use std::collections::HashMap; + use std::path::{Path, PathBuf}; + let all = [("AR", "all")]; + let outer = resolve_outer_allow_remote(&cfg_env(&all), |_| None); + assert_eq!( + plan_npmrc_allow_remote_with(Some("allow-remote=${AR}\n"), &outer), + NpmrcPlan::AlreadyAll + ); + let outer = resolve_outer_allow_remote(&cfg_env(&[("AR", "none")]), |_| None); + assert_eq!( + plan_npmrc_allow_remote_with(Some("allow-remote=${AR}\n"), &outer), + NpmrcPlan::UserSet("none".into()) + ); + + // Env layer: `npm_config_allow_remote=${AR}` with `AR=all` is `all`. + let outer = resolve_outer_allow_remote( + &cfg_env(&[("AR", "all"), ("npm_config_allow_remote", "${AR}")]), + |_| None, + ); + assert_eq!( + plan_npmrc_allow_remote_with(Some("fund=false\n"), &outer), + NpmrcPlan::Append("fund=false\nallow-remote=all\n".into()) + ); + + // User layer: `${AR}` -> `all` does not block the project write. + let files: HashMap = + HashMap::from([(PathBuf::from("/home/u/.npmrc"), "allow-remote=${AR}\n")]); + let read = |p: &Path| files.get(p).map(|s| s.to_string()); + let outer = resolve_outer_allow_remote(&cfg_env(&all), read); + assert_eq!( + plan_npmrc_allow_remote_with(Some("fund=false\n"), &outer), + NpmrcPlan::Append("fund=false\nallow-remote=all\n".into()) + ); + } } diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index 6f1d95ec4..a94ab8f21 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -1602,16 +1602,18 @@ enum AllowFileSource { } /// The effective `allow-file` value npm would read, in its layer order -/// (env > project > user > global > builtin), when it is NOT `all`. +/// (env > project > user > global > builtin), when it is NOT `all`. Values +/// are env-replaced like npm reads them (`outer`'s already are). fn effective_allow_file( project_npmrc: Option<&str>, outer: &crate::patch::redirect::npmrc::OuterSetting, + env: &crate::patch::redirect::npmrc::NpmConfigEnv, ) -> Option<(AllowFileSource, String)> { use crate::patch::redirect::npmrc::npmrc_top_level_value; let (source, value) = if let Some((var, value)) = &outer.env { (AllowFileSource::Env(var.clone()), value.clone()) } else if let Some(value) = project_npmrc.and_then(|t| npmrc_top_level_value(t, "allow-file")) { - (AllowFileSource::Project, value) + (AllowFileSource::Project, env.config_value(&value)) } else { let file = outer.file.as_ref()?; ( @@ -1735,7 +1737,7 @@ pub(super) async fn allow_file_refusal_with( let read = |path: &Path| read_regular_to_string_sync(path).ok(); let outer = resolve_outer_npm_setting(env, read, "allow-file", "all"); let project = read(&project_root.join(NPMRC_REL)); - let (source, value) = effective_allow_file(project.as_deref(), &outer)?; + let (source, value) = effective_allow_file(project.as_deref(), &outer, env)?; let refused = allow_file_refused_instances(project_root, name, version, &value).await; if refused.is_empty() { return None; @@ -6347,4 +6349,61 @@ mod tests { None ); } + + /// #1233: npm env-replaces `allow-file` like every config value, so a + /// `${AF}` is read as what it expands to in each layer. + #[tokio::test] + async fn allow_file_values_are_env_replaced_like_npm() { + let (tmp, home) = allow_file_project(Some("allow-file=${AF}\n")).await; + let root = tmp.path().join("proj"); + // `AF=all`: npm admits every file: dependency. + assert_eq!( + allow_file_refusal_with( + &root, + "is-number", + "7.0.0", + &allow_file_env(&home, &[("AF", "all")]) + ) + .await, + None + ); + // `AF=none`: refused, and reported under its real value. + let detail = allow_file_refusal_with( + &root, + "left-pad", + "1.3.0", + &allow_file_env(&home, &[("AF", "none")]), + ) + .await + .expect("allow-file=${AF} with AF=none refuses"); + assert!( + detail.contains("refuses every `file:` dependency"), + "{detail}" + ); + // The env and user layers expand the same way. + tokio::fs::remove_file(root.join(".npmrc")).await.unwrap(); + tokio::fs::write(home.join(".npmrc"), "allow-file=${AF}\n") + .await + .unwrap(); + assert_eq!( + allow_file_refusal_with( + &root, + "is-number", + "7.0.0", + &allow_file_env(&home, &[("AF", "all")]) + ) + .await, + None + ); + assert_eq!( + allow_file_refusal_with( + &root, + "is-number", + "7.0.0", + &allow_file_env(&home, &[("AF", "all"), ("npm_config_allow_file", "${AF}")]) + ) + .await, + None + ); + } } From 9be0112716f7d8fb57370975f8dd5abfd888a518 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 23:56:18 +0000 Subject: [PATCH 3/3] Expand only replace-registry-host; quote both Review of the first commit: expanding allow-remote and allow-file against the local environment could flip a committed `${VAR}` to "all" on one machine and hide a refusal that a CI runner without the variable still hits. For those two settings the raw `${VAR}` already reads as "not all", which is the safe direction, so they go back to raw reads. replace-registry-host stays expanded, because expanding it can only add the warning. The warning now quotes the value as written and what npm expands it to, so the user can find the line or variable it names. config_path reuses the new config_value helper. Refs #1233 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- .../tests/vendor/redirect_npm_allow_remote.rs | 11 +- crates/socket-patch-core/src/hosted/engine.rs | 6 +- .../socket-patch-core/src/hosted/guidance.rs | 18 +- .../src/patch/redirect/npmrc.rs | 212 ++++++++---------- .../socket-patch-core/src/vendor/npm_lock.rs | 65 +----- 6 files changed, 120 insertions(+), 194 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index f3deb15ee..a61d72020 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -161,7 +161,7 @@ For a **9.0 root lock**, the CLI ensures `pnpm-workspace.yaml` carries `trustLoc **npm hosted-mode `allow-remote` contract**: npm >=12 defaults `allow-remote=none` and refuses (EALLOWREMOTE) every lockfile entry whose `resolved` tarball is not served by the configured registry — exactly what a hosted redirect writes into `package-lock.json` / `npm-shrinkwrap.json`. Whenever a run leaves a ROOT npm lock carrying a granted hosted artifact URL (spliced this run, or already redirected by an earlier one — a missed config heals on re-run), the CLI ensures `allow-remote=all` in the project-root `.npmrc`: the file is created holding exactly `allow-remote=all\n` when absent, otherwise one `allow-remote=all` line is spliced in after the last non-empty top-level line (before any ini `[section]` header), in the file's own line ending, with the BOM, CRLF and trailing-newline shape preserved. The write lands in `redirect.rewrittenFiles` (edit kind `redirect_npmrc_allow_remote`), respects `--dry-run` (nothing written; the warning says what would be — including for a vendored → hosted takeover the dry run only previews), and is disabled by `--no-npm-allow-remote-config` / `SOCKET_NO_NPM_ALLOW_REMOTE_CONFIG`. The `.npmrc` grammar is npm's own `ini` parser's (cross-checked against it): lines split on any run of `\r` / `\n` (a bare `\r` ends a line), only the exact key `allow-remote` counts after ini unquoting (npm ignores `allow_remote` / `ALLOW-REMOTE` in a `.npmrc`; such a line is left alone and the real key appended), comment lines are ignored, a `[section]` header is recognized only as npm does — on the UNTRIMMED line (an indented or BOM-prefixed `[sec]` is a plain top-level key) — and ends the top-level scope, quotes and inline comments are stripped, the LAST top-level assignment wins, and the value is case-sensitive. An explicit other value (`allow-remote=none` / `root` / anything but `all`) is RESPECTED and never rewritten — the pnpm `trustLockfile: false` precedent — in the project `.npmrc` AND in every other npm config layer npm would consult: an `npm_config_allow_remote` environment variable (any spelling npm normalizes; it beats every `.npmrc`, so a project write could not take effect), and — when the project file sets nothing — the user (`npm_config_userconfig` / `~/.npmrc`), global (`npm_config_globalconfig` / `/etc/npmrc`, prefix from `npm_config_prefix`, the user/builtin config, `PREFIX` or the `node` binary's install root) and builtin (npm's own `npmrc` beside the `node` binary: `/lib/node_modules/npm/npmrc`, `\node_modules\npm\npmrc` on Windows) config files — path values `${VAR}`-expanded and `~`-expanded like npm, env names case-insensitive on Windows, where a committed project line would silently override a machine / org policy. A symlinked, non-regular or unreadable `.npmrc`, or one with bare-`\r` line endings (npm splits on them, the line splice does not), is left untouched. Every variant emits the `redirect_npm_allow_remote` warning (written / would write / already set / explicit value respected — naming the project file, the env var, or the user/global/builtin config path — / opted out / unreadable or unsupported), always with the tradeoff: `allow-remote=all` lets npm install ANY url-resolved dependency, not just Socket's patched ones, while the per-entry sha512 integrity pins stay enforced; the remedy for the non-writing variants is `allow-remote=all` in `.npmrc` or `npm ci --allow-remote=all`. npm <=11 is unaffected (11 defaults to `all`, <=10 has no such setting). **Unwind (v5.0: no ledger)**: once `rollback`, `remove` or a hosted → vendored takeover has restored the last hosted entry of the root `package-lock.json` / `npm-shrinkwrap.json` to its upstream registry entry (see "Hosted unwind coverage"), a project `.npmrc` holding exactly `allow-remote=all\n` (the file hosted mode creates) is deleted; any other `.npmrc` that still has a top-level `allow-remote=all` line is left untouched and the `npm_allow_remote_left` warning says the line may be removed if nothing else needs it (v5 keeps no record of whether hosted mode added it, so it is never removed behind the user's back). The rewrite's stage file is created with the `.npmrc`'s own permission bits (a 0600 token-bearing file is never staged world-readable). **Vendored mode is unaffected by `allow-remote`**: its `file:.socket/vendor/…` resolutions are npm `file` specs, which npm >= 11.14 gates by `allow-file` (default `all`), never `allow-remote` — verified by the real npm 12 vendored matrix. **npm vendored `allow-file` contract (#969)**: `allow-file=none` refuses every `file:` dependency, and any other non-`all` value (`root`) admits one only when the project root or a workspace declares it and resolves to that very lock node (arborist's `_isRoot`); otherwise every `npm ci` / `npm install` fails EALLOWFILE. The effective value is read from the same npm config layers as `allow-remote` (an `npm_config_allow_file` environment variable, then the project `.npmrc`, then the user / global / builtin config files). When it refuses a `package-lock.json` / `npm-shrinkwrap.json` instance of a vendored `name@version` (the set vendoring rewires), the vendored run (`vendor`, `scan` / `get --mode vendored`, dry runs and in-sync re-runs included) still vendors, leaves the setting untouched, and records the `vendor_npm_allow_file` advisory naming the setting's source, the refused lock entries and the remedy (`allow-file=all` in the project `.npmrc`, unsetting the env var, or `npm ci --allow-file=all`); `vendor --check` fails that entry `vendor_check_failed` with the same reason. Other npm-family flavors (yarn, pnpm, bun, vlt) do not read `allow-file` and are not checked. -**npm hosted-mode `replace-registry-host` contract**: npm >=8 rewrites the origin of a lock's `resolved` URL to the configured registry when its `replace-registry-host` setting is `always`, or equals the URL's hostname (`npmjs`, the default, stands for `registry.npmjs.org`; `never` matches nothing). A hosted pin rewritten that way is fetched from `/patch/npm/…` and every `npm ci` / `npm install` fails E404 (closed: never unpatched bytes; a warm npm cache can hide it locally). Whenever a run leaves a root npm lock carrying a granted hosted artifact URL (the same trigger as the `allow-remote` contract, so an "already on hosted patches" re-run reports it too), the CLI resolves `replace-registry-host` across npm's layers — an `npm_config_replace_registry_host` environment variable (any spelling npm normalizes), then the project `.npmrc`, then the user / global / builtin config files (located exactly like the `allow-remote` layers), each value env-replaced the way npm reads it (`${VAR}` / `${VAR?}`, so `replace-registry-host=${RRH}` with `RRH=always` counts as `always`; the same holds for `allow-remote` and the vendored `allow-file` check) — and, when the effective value rewrites a pinned host, emits the `redirect_npm_replace_registry_host` warning naming the layer (env var / project file / config path) and the remedies: `replace-registry-host=npmjs` in the project `.npmrc` (or unsetting the env var), or vendored mode, whose `file:` resolutions npm never rewrites. The setting is never rewritten; the redirect itself still lands and the exit status is unchanged. +**npm hosted-mode `replace-registry-host` contract**: npm >=8 rewrites the origin of a lock's `resolved` URL to the configured registry when its `replace-registry-host` setting is `always`, or equals the URL's hostname (`npmjs`, the default, stands for `registry.npmjs.org`; `never` matches nothing). A hosted pin rewritten that way is fetched from `/patch/npm/…` and every `npm ci` / `npm install` fails E404 (closed: never unpatched bytes; a warm npm cache can hide it locally). Whenever a run leaves a root npm lock carrying a granted hosted artifact URL (the same trigger as the `allow-remote` contract, so an "already on hosted patches" re-run reports it too), the CLI resolves `replace-registry-host` across npm's layers — an `npm_config_replace_registry_host` environment variable (any spelling npm normalizes), then the project `.npmrc`, then the user / global / builtin config files (located exactly like the `allow-remote` layers), its value env-replaced the way npm reads it (`${VAR}` / `${VAR?}`: `replace-registry-host=${RRH}` with `RRH=always` counts as `always`, and the warning quotes both spellings; an unset `${VAR}` stays literal, as in npm) — and, when the effective value rewrites a pinned host, emits the `redirect_npm_replace_registry_host` warning naming the layer (env var / project file / config path) and the remedies: `replace-registry-host=npmjs` in the project `.npmrc` (or unsetting the env var), or vendored mode, whose `file:` resolutions npm never rewrites. The setting is never rewritten; the redirect itself still lands and the exit status is unchanged. **pnpm per-member locks (#492)**: a workspace whose root `pnpm-workspace.yaml` sets `sharedWorkspaceLockfile: false` (or, when the YAML leaves the key unset, whose root `.npmrc` sets `shared-workspace-lockfile=false`; on disk, when neither file sets it, `pnpm_config_shared_workspace_lockfile` / `npm_config_shared_workspace_lockfile`; booleans in any letter case, as pnpm's YAML reader takes them) installs each member from the member's own `pnpm-lock.yaml`. Run from the workspace root, hosted mode reads every member lock the `packages:` globs name (quoted `!` negations applied; `node_modules`, `bower_components`, `test` and `tests` trees and symlinked directories skipped) beside the root lock, which may be absent (pnpm 7): each is rewritten like the root lock, a 9.0 member lock engages the `trustLockfile: true` auto-config in the ROOT `pnpm-workspace.yaml` (heal-on-rerun included), and discovery (`list`, `vex`, `rollback`, `remove`) reads the same member locks. A root lock whose `importers:` lists a member is a shared lock, so member locks beside it are stale and never read or attested. When the members' locks cannot be listed or read (no `packages:` key, whose pnpm <= 8 default finds projects in every directory; a `packages:` value that is not a plain list of globs; no member lock and no root lock, or a member lock that exists but is not readable text), every pnpm lock (the root's included) is left untouched and the run warns `redirect_pnpm_member_locks_unresolved` instead of `redirect_pnpm_no_lockfile`; no pnpm pin is confirmed. The lockfile inventory reads the same member locks, so their dependencies are discovered without an installed tree. The in-memory engine gives the same result as a disk run from the workspace root: a directory whose `pnpm-lock.yaml` has no `pnpm-workspace.yaml` beside it is a workspace member, never a root of its own, when (with `npm` among the `ecosystems`) the nearest ancestor `pnpm-workspace.yaml` is readable and its `packages:` globs (read as above, no braces, classes or extglobs) list it, it holds a `package.json` / `package.yaml` / `package.json5`, the workspace root installs it from its own lock (`sharedWorkspaceLockfile: false` with the member list resolved) or from a root lock beside which its lock is a stale leftover, and socket.yml admits the workspace root. Its lock is then demoted into that workspace root, which becomes a root even with no lock of its own (pnpm 7), the way a Cargo member's lock is demoted; the member stays a root only when it holds another lock marker, without its pnpm lock. The workspace root then pins (or refuses) its member locks as above, trusts them in its own file, refuses a member's branch lock (#556) and leaves a member lock beside a shared root lock unread. A directory that fails any of those tests keeps its lock as a root of its own (pinned like any root, or refused with `redirect_pnpm_settings_elsewhere` while the trust auto-config is on and the ancestor file may list it). Path selection adds the workspace root above every candidate to `roots`, fetches it, and fetches the member locks, branch locks and manifests below it in every directory holding a package manifest; given those `roots` as `projectRoots`, the session drops a confirmed member named beside its workspace root (unless it holds another lock) and a lockless workspace root that pins no member, so it runs the roots it would detect. A confirmed member named in `projectRoots` without its workspace root is not pinned: its lock is dropped and the run-level `pnpm_member_lock_ignored` warning names the workspace root to scan. diff --git a/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs b/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs index a618ecbfe..86daddc96 100644 --- a/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs +++ b/crates/socket-patch-cli/tests/vendor/redirect_npm_allow_remote.rs @@ -912,7 +912,8 @@ async fn replace_registry_host_rewriting_the_pin_is_warned() { /// #1233: npm env-replaces every config value, so /// `replace-registry-host=${RRH}` with `RRH=always` rewrites every hosted /// pin exactly like `=always`. The scan read the literal `${RRH}`, matched -/// no host and stayed silent; it now warns in the project and user layers. +/// no host and stayed silent; it now warns in the project and user layers, +/// naming the spelling the user wrote and what npm expands it to. #[tokio::test] async fn replace_registry_host_via_env_interpolation_is_warned() { let server = MockServer::start().await; @@ -927,7 +928,9 @@ async fn replace_registry_host_via_env_interpolation_is_warned() { assert_eq!(code, 0, "{doc:#}"); let detail = replace_host_warning(&doc).unwrap_or_else(|| panic!("no {REPLACE_HOST}: {doc:#}")); assert!( - detail.contains("the project .npmrc sets `replace-registry-host=always`"), + detail.contains( + "the project .npmrc sets `replace-registry-host=${RRH}` (npm expands it to `always`)" + ), "{detail}" ); @@ -951,7 +954,9 @@ async fn replace_registry_host_via_env_interpolation_is_warned() { assert_eq!(code, 0, "{doc:#}"); let detail = replace_host_warning(&doc).unwrap_or_else(|| panic!("no {REPLACE_HOST}: {doc:#}")); assert!( - detail.contains("the user npm config") && detail.contains(user_s), + detail.contains("the user npm config") + && detail.contains(user_s) + && detail.contains("(npm expands it to `always`)"), "{detail}" ); diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 25b2ac5e9..88a7fdecd 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -2607,16 +2607,16 @@ fn npm_allow_remote( // so every install 404s. The setting is the user's, so it is reported // (from whichever layer sets it), never overridden. let project = npmrc.as_ref().ok().and_then(|t| t.as_deref()); - if let Some((value, source)) = effective_replace_registry_host(project, &outer) { + if let Some(setting) = effective_replace_registry_host(project, &outer) { let blocked: Vec<&str> = npm_hosts .iter() .copied() - .filter(|host| replace_registry_host_rewrites(&value, host)) + .filter(|host| replace_registry_host_rewrites(&setting.value, host)) .collect(); if !blocked.is_empty() { npm_warnings.push(warning( NPM_REPLACE_REGISTRY_HOST_CODE, - npm_replace_registry_host_detail(&blocked, &value, &source), + npm_replace_registry_host_detail(&blocked, &setting), )); } } diff --git a/crates/socket-patch-core/src/hosted/guidance.rs b/crates/socket-patch-core/src/hosted/guidance.rs index 458fd1535..9917ee668 100644 --- a/crates/socket-patch-core/src/hosted/guidance.rs +++ b/crates/socket-patch-core/src/hosted/guidance.rs @@ -560,11 +560,21 @@ pub const NPM_REPLACE_REGISTRY_HOST_CODE: &str = "redirect_npm_replace_registry_ /// user's — so the warning names where it is set and both remedies. pub fn npm_replace_registry_host_detail( hosts: &[&str], - value: &str, - source: &crate::patch::redirect::npmrc::SettingSource, + setting: &crate::patch::redirect::npmrc::EffectiveSetting, ) -> String { use crate::patch::redirect::npmrc::SettingSource; - let (where_, fix) = match source { + // Name the spelling the user can find (`${RRH}`), and what npm makes + // of it. + let value = if setting.raw.trim() == setting.value { + format!("`replace-registry-host={}`", setting.value) + } else { + format!( + "`replace-registry-host={}` (npm expands it to `{}`)", + setting.raw.trim(), + setting.value + ) + }; + let (where_, fix) = match &setting.source { SettingSource::Env(var) => ( format!("the environment variable {var} sets"), format!( @@ -586,7 +596,7 @@ pub fn npm_replace_registry_host_detail( }; format!( "the npm lockfile now resolves patched dependencies from the hosted patch server ({}), \ - but {where_} `replace-registry-host={value}`, which makes npm >=8 rewrite those \ + but {where_} {value}, which makes npm >=8 rewrite those \ `resolved` URLs to the configured registry: every `npm ci` / `npm install` then \ fails E404 (a warm npm cache can hide this locally; a fresh checkout or CI runner \ fails). To install the hosted patches, {fix}; or switch this project to vendored \ diff --git a/crates/socket-patch-core/src/patch/redirect/npmrc.rs b/crates/socket-patch-core/src/patch/redirect/npmrc.rs index 5f4de9d1f..abf9a024c 100644 --- a/crates/socket-patch-core/src/patch/redirect/npmrc.rs +++ b/crates/socket-patch-core/src/patch/redirect/npmrc.rs @@ -281,8 +281,9 @@ pub struct OuterAllowRemote { /// `replace-registry-host` in the same outer layers (see /// [`effective_replace_registry_host`]). pub replace_registry_host: OuterSetting, - /// The process facts the layers were resolved with. npm env-replaces - /// the project `.npmrc`'s values too, against the same environment. + /// The process facts the layers were resolved with: npm env-replaces + /// a value against them (see [`NpmConfigEnv::config_value`]), + /// whichever layer sets it, the project `.npmrc` included. pub config_env: NpmConfigEnv, } @@ -297,8 +298,7 @@ pub struct OuterSetting { pub file: Option, } -/// One explicit assignment in a non-project npm config file (its value -/// already env-replaced, see [`NpmConfigEnv::config_value`]). +/// One explicit `allow-remote` assignment in a non-project npm config file. #[derive(Debug, Clone, PartialEq)] pub struct OuterFileValue { /// `"user"`, `"global"` or `"builtin"`. @@ -432,19 +432,19 @@ impl NpmConfigEnv { out } - /// A (non-path) config value as npm's `parseField` reads it: trimmed, - /// then env-replaced. npm does this to every string value in every - /// layer (env, project, user, global, builtin), so `allow-remote=${AR}` - /// with `AR=all` is `all` to npm. + /// A config value as npm's `parseField` reads it: trimmed, then + /// env-replaced. npm does this to every string value in every layer + /// (env, project, user, global, builtin), so + /// `replace-registry-host=${RRH}` with `RRH=always` is `always` to npm. pub fn config_value(&self, value: &str) -> String { - self.env_replace(value.trim_matches(is_js_ws)) + self.env_replace(js_trim(value)) } - /// A `path`-typed config value as npm's `parseField` reads it: trimmed, - /// env-replaced, then a leading `~/` (also `~\` on Windows) expands - /// against [`Self::home`]. + /// A `path`-typed config value as npm's `parseField` reads it: + /// [`Self::config_value`], then a leading `~/` (also `~\` on Windows) + /// expands against [`Self::home`]. fn config_path(&self, value: &str) -> std::path::PathBuf { - let value = self.env_replace(value.trim_matches(is_js_ws)); + let value = self.config_value(value); let rest = value .strip_prefix("~/") .or_else(|| value.strip_prefix("~\\").filter(|_| self.windows)); @@ -458,8 +458,7 @@ impl NpmConfigEnv { /// prefix case-insensitive, then non-leading `_` → `-` and lowercased. /// Empty values are ignored (npm skips them). When several spellings /// are set a non-`benign` one wins (process env order is unspecified, - /// so the conservative reading is reported). The value is returned raw: - /// callers env-replace it as its type needs. + /// so the conservative reading is reported). fn npm_config(&self, key: &str, benign: &str) -> Option<(String, String)> { let mut found: Option<(String, String)> = None; for (k, v) in &self.vars { @@ -481,11 +480,7 @@ impl NpmConfigEnv { c.to_ascii_lowercase() }); } - if norm == key - && found - .as_ref() - .is_none_or(|(_, prev)| self.config_value(prev) == benign) - { + if norm == key && found.as_ref().is_none_or(|(_, prev)| prev == benign) { found = Some((k.clone(), v.clone())); } } @@ -526,11 +521,11 @@ pub fn resolve_outer_allow_remote( ) -> OuterAllowRemote { let layers = outer_config_layers(env, read); OuterAllowRemote { - env: env_value(env, "allow-remote", "all"), - file: outer_file_value(env, &layers, "allow-remote"), + env: env.npm_config("allow-remote", "all"), + file: outer_file_value(&layers, "allow-remote"), replace_registry_host: OuterSetting { - env: env_value(env, REPLACE_REGISTRY_HOST, "npmjs"), - file: outer_file_value(env, &layers, REPLACE_REGISTRY_HOST), + env: env.npm_config(REPLACE_REGISTRY_HOST, "npmjs"), + file: outer_file_value(&layers, REPLACE_REGISTRY_HOST), }, config_env: env.clone(), } @@ -548,18 +543,11 @@ pub fn resolve_outer_npm_setting( ) -> OuterSetting { let layers = outer_config_layers(env, read); OuterSetting { - env: env_value(env, key, benign), - file: outer_file_value(env, &layers, key), + env: env.npm_config(key, benign), + file: outer_file_value(&layers, key), } } -/// The `npm_config_*` variable setting `key`, its value env-replaced the -/// way npm reads it. -fn env_value(env: &NpmConfigEnv, key: &str, benign: &str) -> Option<(String, String)> { - env.npm_config(key, benign) - .map(|(var, value)| (var, env.config_value(&value))) -} - /// One non-project npm config file: `(layer, path, text)`. type OuterLayer = (&'static str, Option, Option); @@ -622,19 +610,14 @@ fn outer_config_layers( ] } -/// The highest-precedence top-level `key` assignment among `layers`, -/// env-replaced like npm reads it. -fn outer_file_value( - env: &NpmConfigEnv, - layers: &[OuterLayer], - key: &str, -) -> Option { +/// The highest-precedence top-level `key` assignment among `layers`. +fn outer_file_value(layers: &[OuterLayer], key: &str) -> Option { layers.iter().find_map(|(layer, path, text)| { let value = npmrc_top_level_value(text.as_deref()?, key)?; Some(OuterFileValue { layer, path: path.clone()?, - value: env.config_value(&value), + value, }) }) } @@ -661,25 +644,35 @@ pub enum SettingSource { }, } +/// The effective `replace-registry-host` setting: where it is set, the +/// value as written there, and the value npm uses. +#[derive(Debug, Clone, PartialEq)] +pub struct EffectiveSetting { + /// The value npm uses: [`raw`](Self::raw) trimmed and env-replaced. + pub value: String, + /// The value as written in the file or variable (e.g. `${RRH}`). + pub raw: String, + pub source: SettingSource, +} + /// The `replace-registry-host` value npm would use, following its layer /// order (env > project > user > global > builtin), or `None` when no -/// layer sets it (npm's default `npmjs`). Every value is env-replaced the -/// way npm reads it (#1233). +/// layer sets it (npm's default `npmjs`). npm env-replaces the value in +/// every layer (#1233), so `${RRH}` reads as what `RRH` holds; an unset +/// `${RRH}` stays literal, as it does for npm. pub fn effective_replace_registry_host( project: Option<&str>, outer: &OuterAllowRemote, -) -> Option<(String, SettingSource)> { +) -> Option { let setting = &outer.replace_registry_host; - if let Some((var, value)) = &setting.env { - return Some((value.clone(), SettingSource::Env(var.clone()))); - } - if let Some(value) = project.and_then(|t| npmrc_top_level_value(t, REPLACE_REGISTRY_HOST)) { - return Some(( - outer.config_env.config_value(&value), - SettingSource::Project, - )); - } - setting.file.as_ref().map(|f| { + let (raw, source) = if let Some((var, value)) = &setting.env { + (value.clone(), SettingSource::Env(var.clone())) + } else if let Some(value) = + project.and_then(|t| npmrc_top_level_value(t, REPLACE_REGISTRY_HOST)) + { + (value, SettingSource::Project) + } else { + let f = setting.file.as_ref()?; ( f.value.clone(), SettingSource::File { @@ -687,6 +680,11 @@ pub fn effective_replace_registry_host( path: f.path.clone(), }, ) + }; + Some(EffectiveSetting { + value: outer.config_env.config_value(&raw), + raw, + source, }) } @@ -733,7 +731,7 @@ pub fn plan_npmrc_allow_remote(existing: Option<&str>) -> NpmrcPlan { /// ([`NpmrcPlan::EnvSet`]) beats everything; then the project value; then /// an explicit non-`all` user / global / builtin value /// ([`NpmrcPlan::OuterSet`]). Every explicit value is respected, never -/// overridden by a write, and read env-replaced like npm reads it. +/// overridden by a write. pub fn plan_npmrc_allow_remote_with(existing: Option<&str>, outer: &OuterAllowRemote) -> NpmrcPlan { if let Some((var, value)) = &outer.env { if value != "all" { @@ -743,10 +741,7 @@ pub fn plan_npmrc_allow_remote_with(existing: Option<&str>, outer: &OuterAllowRe }; } } - match existing - .and_then(npmrc_allow_remote) - .map(|v| outer.config_env.config_value(&v)) - { + match existing.and_then(npmrc_allow_remote) { Some(v) if v == "all" => return NpmrcPlan::AlreadyAll, Some(v) => return NpmrcPlan::UserSet(v), None => {} @@ -1346,6 +1341,12 @@ mod tests { /// against a pinned origin the way `@npmcli/arborist` does. #[test] fn replace_registry_host_layers_and_matching() { + fn effective( + project: Option<&str>, + outer: &OuterAllowRemote, + ) -> Option<(String, SettingSource)> { + effective_replace_registry_host(project, outer).map(|s| (s.value, s.source)) + } use std::collections::HashMap; use std::path::{Path, PathBuf}; let files: HashMap = HashMap::from([ @@ -1362,12 +1363,12 @@ mod tests { // Nothing set anywhere: npm's default (`npmjs`) applies. let outer = resolve_outer_allow_remote(&cfg_env(&[]), |_| None); - assert_eq!(effective_replace_registry_host(None, &outer), None); + assert_eq!(effective(None, &outer), None); // The user file beats the global one. let outer = resolve_outer_allow_remote(&cfg_env(&[]), read); assert_eq!( - effective_replace_registry_host(Some("fund=false\n"), &outer), + effective(Some("fund=false\n"), &outer), Some(( "always".into(), SettingSource::File { @@ -1378,7 +1379,7 @@ mod tests { ); // ...the project file beats both... assert_eq!( - effective_replace_registry_host(Some("replace-registry-host=npmjs\n"), &outer), + effective(Some("replace-registry-host=npmjs\n"), &outer), Some(("npmjs".into(), SettingSource::Project)) ); // ...and the env beats every file, in any spelling npm normalizes. @@ -1388,7 +1389,7 @@ mod tests { ] { let outer = resolve_outer_allow_remote(&cfg_env(&[(var, "always")]), read); assert_eq!( - effective_replace_registry_host(Some("replace-registry-host=never\n"), &outer), + effective(Some("replace-registry-host=never\n"), &outer), Some(("always".into(), SettingSource::Env(var.into()))) ); } @@ -1418,7 +1419,8 @@ mod tests { /// #1233: npm env-replaces every config value (`@npmcli/config` /// `parseField` -> `envReplace`), in every layer. A /// `replace-registry-host=${RRH}` with `RRH=always` is `always` to npm, - /// so it must read as `always` here too, wherever it is set. + /// so it must read as `always` here too, wherever it is set, while the + /// spelling the user wrote is kept for the warning. #[test] fn replace_registry_host_values_are_env_replaced_like_npm() { use std::collections::HashMap; @@ -1428,16 +1430,22 @@ mod tests { // Project `.npmrc`. let outer = resolve_outer_allow_remote(&cfg_env(&rrh), |_| None); - let (value, source) = - effective_replace_registry_host(Some("replace-registry-host=${RRH}\n"), &outer) - .expect("project layer sets it"); - assert_eq!((value.as_str(), source), ("always", SettingSource::Project)); - assert!(replace_registry_host_rewrites(&value, host)); + let got = effective_replace_registry_host(Some("replace-registry-host=${RRH}\n"), &outer) + .expect("project layer sets it"); + assert_eq!( + got, + EffectiveSetting { + value: "always".into(), + raw: "${RRH}".into(), + source: SettingSource::Project, + } + ); + assert!(replace_registry_host_rewrites(&got.value, host)); // `${RRH?}` and a padded value read the same way. - let (value, _) = + let got = effective_replace_registry_host(Some("replace-registry-host = ${RRH?} \n"), &outer) .unwrap(); - assert_eq!(value, "always"); + assert_eq!(got.value, "always"); // User `.npmrc`. let files: HashMap = HashMap::from([( @@ -1446,9 +1454,12 @@ mod tests { )]); let read = |p: &Path| files.get(p).map(|s| s.to_string()); let outer = resolve_outer_allow_remote(&cfg_env(&rrh), read); - let (value, source) = effective_replace_registry_host(None, &outer).unwrap(); - assert_eq!(value, "always"); - assert!(matches!(source, SettingSource::File { layer: "user", .. })); + let got = effective_replace_registry_host(None, &outer).unwrap(); + assert_eq!((got.value.as_str(), got.raw.as_str()), ("always", "${RRH}")); + assert!(matches!( + got.source, + SettingSource::File { layer: "user", .. } + )); // An `npm_config_*` variable is env-replaced too. let outer = resolve_outer_allow_remote( @@ -1458,55 +1469,14 @@ mod tests { ]), |_| None, ); - let (value, _) = effective_replace_registry_host(None, &outer).unwrap(); - assert!(replace_registry_host_rewrites(&value, host), "{value}"); + let got = effective_replace_registry_host(None, &outer).unwrap(); + assert!(replace_registry_host_rewrites(&got.value, host), "{got:?}"); // Unset: npm keeps `${RRH}` verbatim, which matches no host. let outer = resolve_outer_allow_remote(&cfg_env(&[]), |_| None); - let (value, _) = - effective_replace_registry_host(Some("replace-registry-host=${RRH}\n"), &outer) - .unwrap(); - assert_eq!(value, "${RRH}"); - assert!(!replace_registry_host_rewrites(&value, host)); - } - - /// #1233: `allow-remote` is env-replaced like every npm value, so a - /// `${AR}` that expands to `all` is already `all`, in every layer, and - /// one that expands to anything else is respected under its real value. - #[test] - fn allow_remote_values_are_env_replaced_like_npm() { - use std::collections::HashMap; - use std::path::{Path, PathBuf}; - let all = [("AR", "all")]; - let outer = resolve_outer_allow_remote(&cfg_env(&all), |_| None); - assert_eq!( - plan_npmrc_allow_remote_with(Some("allow-remote=${AR}\n"), &outer), - NpmrcPlan::AlreadyAll - ); - let outer = resolve_outer_allow_remote(&cfg_env(&[("AR", "none")]), |_| None); - assert_eq!( - plan_npmrc_allow_remote_with(Some("allow-remote=${AR}\n"), &outer), - NpmrcPlan::UserSet("none".into()) - ); - - // Env layer: `npm_config_allow_remote=${AR}` with `AR=all` is `all`. - let outer = resolve_outer_allow_remote( - &cfg_env(&[("AR", "all"), ("npm_config_allow_remote", "${AR}")]), - |_| None, - ); - assert_eq!( - plan_npmrc_allow_remote_with(Some("fund=false\n"), &outer), - NpmrcPlan::Append("fund=false\nallow-remote=all\n".into()) - ); - - // User layer: `${AR}` -> `all` does not block the project write. - let files: HashMap = - HashMap::from([(PathBuf::from("/home/u/.npmrc"), "allow-remote=${AR}\n")]); - let read = |p: &Path| files.get(p).map(|s| s.to_string()); - let outer = resolve_outer_allow_remote(&cfg_env(&all), read); - assert_eq!( - plan_npmrc_allow_remote_with(Some("fund=false\n"), &outer), - NpmrcPlan::Append("fund=false\nallow-remote=all\n".into()) - ); + let got = effective_replace_registry_host(Some("replace-registry-host=${RRH}\n"), &outer) + .unwrap(); + assert_eq!(got.value, "${RRH}"); + assert!(!replace_registry_host_rewrites(&got.value, host)); } } diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index a94ab8f21..6f1d95ec4 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -1602,18 +1602,16 @@ enum AllowFileSource { } /// The effective `allow-file` value npm would read, in its layer order -/// (env > project > user > global > builtin), when it is NOT `all`. Values -/// are env-replaced like npm reads them (`outer`'s already are). +/// (env > project > user > global > builtin), when it is NOT `all`. fn effective_allow_file( project_npmrc: Option<&str>, outer: &crate::patch::redirect::npmrc::OuterSetting, - env: &crate::patch::redirect::npmrc::NpmConfigEnv, ) -> Option<(AllowFileSource, String)> { use crate::patch::redirect::npmrc::npmrc_top_level_value; let (source, value) = if let Some((var, value)) = &outer.env { (AllowFileSource::Env(var.clone()), value.clone()) } else if let Some(value) = project_npmrc.and_then(|t| npmrc_top_level_value(t, "allow-file")) { - (AllowFileSource::Project, env.config_value(&value)) + (AllowFileSource::Project, value) } else { let file = outer.file.as_ref()?; ( @@ -1737,7 +1735,7 @@ pub(super) async fn allow_file_refusal_with( let read = |path: &Path| read_regular_to_string_sync(path).ok(); let outer = resolve_outer_npm_setting(env, read, "allow-file", "all"); let project = read(&project_root.join(NPMRC_REL)); - let (source, value) = effective_allow_file(project.as_deref(), &outer, env)?; + let (source, value) = effective_allow_file(project.as_deref(), &outer)?; let refused = allow_file_refused_instances(project_root, name, version, &value).await; if refused.is_empty() { return None; @@ -6349,61 +6347,4 @@ mod tests { None ); } - - /// #1233: npm env-replaces `allow-file` like every config value, so a - /// `${AF}` is read as what it expands to in each layer. - #[tokio::test] - async fn allow_file_values_are_env_replaced_like_npm() { - let (tmp, home) = allow_file_project(Some("allow-file=${AF}\n")).await; - let root = tmp.path().join("proj"); - // `AF=all`: npm admits every file: dependency. - assert_eq!( - allow_file_refusal_with( - &root, - "is-number", - "7.0.0", - &allow_file_env(&home, &[("AF", "all")]) - ) - .await, - None - ); - // `AF=none`: refused, and reported under its real value. - let detail = allow_file_refusal_with( - &root, - "left-pad", - "1.3.0", - &allow_file_env(&home, &[("AF", "none")]), - ) - .await - .expect("allow-file=${AF} with AF=none refuses"); - assert!( - detail.contains("refuses every `file:` dependency"), - "{detail}" - ); - // The env and user layers expand the same way. - tokio::fs::remove_file(root.join(".npmrc")).await.unwrap(); - tokio::fs::write(home.join(".npmrc"), "allow-file=${AF}\n") - .await - .unwrap(); - assert_eq!( - allow_file_refusal_with( - &root, - "is-number", - "7.0.0", - &allow_file_env(&home, &[("AF", "all")]) - ) - .await, - None - ); - assert_eq!( - allow_file_refusal_with( - &root, - "is-number", - "7.0.0", - &allow_file_env(&home, &[("AF", "all"), ("npm_config_allow_file", "${AF}")]) - ) - .await, - None - ); - } }