From 852d5379e1d2627644265ca7d89ed93057a24b82 Mon Sep 17 00:00:00 2001 From: kyungseopk1m Date: Tue, 23 Jun 2026 22:56:40 +0900 Subject: [PATCH] fix(npm-resolver): surface registry fetch errors in tarball URL check (#12519) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lockfile verification fetches registry metadata to bind each entry's tarball URL (and to apply the minimumReleaseAge / trust-downgrade policies). On a fetch failure the abbreviated-metadata fetch collapsed every failure mode — 403, 401, network error, 5xx — into the same "missing" value as a version genuinely absent from the metadata, so a transport failure was reported as a tampering-style ERR_PNPM_TARBALL_URL_MISMATCH. Rather than mint a dedicated violation code/message for the transport case, the verifier now propagates the registry's own fetch error and the install aborts with it: - runTarballUrlCheck / runAgeCheck / runTrustCheck rethrow the underlying fetch error rather than folding it into a policy violation. The abbreviated-metadata age shortcut still swallows the error and falls back to per-version lookups. - The verification gate captures a thrown error per entry and rethrows the first after the fan-out settles, so concurrent siblings hitting the same dead registry do not raise unhandled rejections. A transport failure takes precedence over collected policy violations: the run never finished, so the batch is incomplete and the actionable failure is the transport error; a re-run surfaces any remaining violations once the registry is reachable. - Credential leak fixed at the source: `@pnpm/error`'s redactUrlCredentials strips user:pass@ userinfo from a URL in FetchError messages, and the npm resolver redacts the message, stack, and cause of META_FETCH_FAIL. It is a single forward scan, not a regex (the input is uncontrolled, so a backtracking pattern is a ReDoS vector), and it strips up to the last @ in the authority so a raw @ inside the password cannot leak its tail. pacquet parity: ResolutionVerification::FetchFailed is added (the runner aborts with it rather than collecting it as a violation); the gate aborts via VerifyError::RegistryMetaFetchFailed (ERR_PNPM_META_FETCH_FAIL), collect_resolution_policy_violations returns a Result, and the pnpr server maps the abort to a 502. The surfaced fetch error is credential-redacted. The TARBALL_URL_FETCH_FAILED code, its VerifyError variant and hint, and the pnpr-client interning are removed. Note: pacquet surfaces its existing fetch error under ERR_PNPM_META_FETCH_FAIL rather than pnpm's literal ERR_PNPM_FETCH_403 — pacquet's fetch errors do not use the per-status code scheme today, so aligning that globally is a separate effort. The behavior matches: abort with the registry's error, never a tampering label, no credential leak. Fixes pnpm/pnpm#12489. --- .../tarball-verifier-surface-fetch-error.md | 8 ++ .../lockfile-verification/src/errors.rs | 12 ++ .../src/verify_lockfile_resolutions.rs | 52 ++++++-- .../src/verify_lockfile_resolutions/tests.rs | 57 ++++++++- .../src/create_npm_resolution_verifier.rs | 121 +++++++++++++----- .../create_npm_resolution_verifier/tests.rs | 117 +++++++++++++++++ .../src/lookup_context.rs | 7 +- .../resolving-resolver-base/src/tests.rs | 4 +- .../resolving-resolver-base/src/verifier.rs | 12 ++ pnpm11/core/error/src/index.ts | 49 ++++++- pnpm11/core/error/test/index.ts | 34 ++++- .../src/install/verifyLockfileResolutions.ts | 43 +++++-- .../test/install/verifyLockfileResolutions.ts | 21 +++ .../src/createNpmResolutionVerifier.ts | 84 +++++++----- pnpm11/resolving/npm-resolver/src/fetch.ts | 13 +- .../test/createNpmResolutionVerifier.test.ts | 44 +++++++ pnpr/crates/pnpr/src/resolver.rs | 10 +- 17 files changed, 593 insertions(+), 95 deletions(-) create mode 100644 .changeset/tarball-verifier-surface-fetch-error.md diff --git a/.changeset/tarball-verifier-surface-fetch-error.md b/.changeset/tarball-verifier-surface-fetch-error.md new file mode 100644 index 0000000000..ab3778ffec --- /dev/null +++ b/.changeset/tarball-verifier-surface-fetch-error.md @@ -0,0 +1,8 @@ +--- +"pnpm": patch +"@pnpm/resolving.npm-resolver": patch +"@pnpm/installing.deps-installer": patch +"@pnpm/error": patch +--- + +Lockfile verification no longer reports a registry metadata fetch failure (for example a `403`/`401` on a private registry, or a network error) as `ERR_PNPM_TARBALL_URL_MISMATCH`. When the registry can't be reached to verify an entry, the install now aborts with the registry's own fetch error (such as `ERR_PNPM_FETCH_403`, which already explains the authentication situation) instead of mislabeling a transport failure as lockfile tampering. Registry fetch errors no longer leak basic-auth credentials embedded in the registry URL (`https://user:pass@host/`) into their message. diff --git a/pacquet/crates/lockfile-verification/src/errors.rs b/pacquet/crates/lockfile-verification/src/errors.rs index 34ecbd103f..c1a6fabb30 100644 --- a/pacquet/crates/lockfile-verification/src/errors.rs +++ b/pacquet/crates/lockfile-verification/src/errors.rs @@ -57,6 +57,18 @@ pub enum VerifyError { breakdown: String, }, + /// The registry couldn't be reached to verify an entry + /// (auth/network/5xx). Surfaces the registry's own fetch error — which + /// already explains the auth situation — rather than a tampering-style + /// mismatch or a lockfile-policy batch. The message is credential-redacted + /// at the verifier before it reaches here. + #[display("{message}")] + #[diagnostic(code(ERR_PNPM_META_FETCH_FAIL))] + RegistryMetaFetchFailed { + #[error(not(source))] + message: String, + }, + #[display("{count} lockfile entries failed verification:\n{breakdown}")] #[diagnostic(code(ERR_PNPM_RESOLUTION_SHAPE_MISMATCH), help("{HINT}"))] ResolutionShapeMismatch { diff --git a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs index 3e98be3399..6a29e67a4c 100644 --- a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs +++ b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs @@ -170,7 +170,13 @@ pub async fn verify_lockfile_resolutions( let mut emit_guard = TerminalEmitGuard::::failed(entries, started_at, lockfile_path_str.clone()); - let violations = run_fan_out(candidates, verifiers, opts.concurrency).await; + let violations = match run_fan_out(candidates, verifiers, opts.concurrency).await { + Ok(violations) => violations, + // The registry couldn't be reached to verify an entry: abort with its + // own error (already credential-redacted) instead of a policy batch. + // `emit_guard` is still armed to emit `failed` on drop. + Err(message) => return Err(VerifyError::RegistryMetaFetchFailed { message }), + }; if violations.is_empty() { emit_guard.cancel(LockfileVerificationMessage::Done { entries, @@ -200,14 +206,16 @@ pub async fn collect_resolution_policy_violations( lockfile: &Lockfile, verifiers: &[Arc], concurrency: Option, -) -> Vec { +) -> Result, String> { if verifiers.is_empty() || lockfile.packages.is_none() { - return Vec::new(); + return Ok(Vec::new()); } // Shape violations and invalid aliases are deliberately not // collected here: they are hard tampering failures, not policy // picks a caller may auto-exclude. let (candidates, _shape_violations, _invalid_aliases) = collect_candidates(lockfile); + // `Err(message)` is a transport failure the caller must surface rather than + // treat as "no violations" — see [`run_fan_out`]. run_fan_out(candidates, verifiers, concurrency).await } @@ -424,7 +432,7 @@ async fn run_fan_out( candidates: Vec, verifiers: &[Arc], concurrency: Option, -) -> Vec { +) -> Result, String> { let limit = concurrency.unwrap_or(DEFAULT_CONCURRENCY).max(1); let semaphore = Arc::new(Semaphore::new(limit)); let mut futures = FuturesUnordered::new(); @@ -450,35 +458,57 @@ async fn run_fan_out( evaluate_candidate(candidate, &verifiers).await }); } + // A transport failure (the registry couldn't be reached to verify an + // entry) aborts the whole pass with the registry's own error rather than + // collecting it as a policy violation. Drain the rest of the fan-out so no + // in-flight task is dropped mid-await, but keep only the first abort. let mut violations = Vec::new(); + let mut fetch_error: Option = None; while let Some(result) = futures.next().await { - if let Some(violation) = result { - violations.push(violation); + match result { + Ok(Some(violation)) => violations.push(violation), + Ok(None) => {} + Err(message) => { + if fetch_error.is_none() { + fetch_error = Some(message); + } + } } } - violations + // A registry that couldn't be reached takes precedence over collected + // violations: the pass never finished, so the batch is incomplete and the + // actionable failure is the transport error. + match fetch_error { + Some(message) => Err(message), + None => Ok(violations), + } } +/// Outcome of evaluating one candidate against the active verifiers. +/// `Err(message)` is a transport failure (the registry couldn't be +/// reached to verify the entry) — the runner aborts the whole pass with +/// it rather than collecting it as a policy violation. async fn evaluate_candidate( candidate: Candidate, verifiers: &[Arc], -) -> Option { +) -> Result, String> { for verifier in verifiers { let ctx = VerifyCtx { name: &candidate.name, version: &candidate.version }; match verifier.verify(&candidate.resolution, ctx).await { ResolutionVerification::Ok => continue, ResolutionVerification::Err { code, reason } => { - return Some(ResolutionPolicyViolation { + return Ok(Some(ResolutionPolicyViolation { name: candidate.name, version: candidate.version, resolution: candidate.resolution, code, reason, - }); + })); } + ResolutionVerification::FetchFailed { message } => return Err(message), } } - None + Ok(None) } /// Sort violations by `name@version` and build the matching diff --git a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs index 19bec0c1db..c715aa39be 100644 --- a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs +++ b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs @@ -145,6 +145,57 @@ impl ResolutionVerifier for FailFor { } } +/// Abort every candidate with a transport failure (the registry couldn't be +/// reached to verify it). +struct FetchFails { + message: &'static str, + policy: serde_json::Map, +} + +impl FetchFails { + fn new(message: &'static str) -> Arc { + Arc::new(Self { message, policy: serde_json::Map::new() }) + } +} + +impl ResolutionVerifier for FetchFails { + fn verify<'a>( + &'a self, + _resolution: &'a LockfileResolution, + _ctx: VerifyCtx<'a>, + ) -> VerifyFuture<'a> { + let message = self.message.to_string(); + Box::pin(async move { ResolutionVerification::FetchFailed { message } }) + } + + fn policy(&self) -> &serde_json::Map { + &self.policy + } + + fn can_trust_past_check(&self, _cached: &serde_json::Map) -> bool { + true + } +} + +#[tokio::test] +async fn a_fetch_failure_aborts_with_the_registry_error() { + let lockfile = parse(SINGLE_PKG_LOCKFILE); + let verifier = + FetchFails::new("Failed to fetch metadata from https://registry.example/acme: 403"); + let err = verify_lockfile_resolutions::( + &lockfile, + &[verifier as Arc], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect_err("a transport failure must abort verification"); + // It surfaces the registry's own error, not a lockfile-policy batch. + let VerifyError::RegistryMetaFetchFailed { message } = err else { + panic!("expected RegistryMetaFetchFailed, got: {err:?}"); + }; + assert!(message.contains("403"), "message: {message}"); +} + #[tokio::test] async fn no_verifiers_is_a_noop() { static EVENTS: Mutex> = Mutex::new(Vec::new()); @@ -280,7 +331,8 @@ async fn per_candidate_fan_out_stops_at_first_failure() { &[first as Arc, second as Arc], None, ) - .await; + .await + .expect("no fetch failure"); assert_eq!(violations.len(), 1, "stop at first failing verifier"); assert_eq!(violations[0].code, "MINIMUM_RELEASE_AGE_VIOLATION"); } @@ -294,7 +346,8 @@ async fn collect_returns_data_for_all_violations() { &[verifier as Arc], None, ) - .await; + .await + .expect("no fetch failure"); assert_eq!(violations.len(), 2); } diff --git a/pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs b/pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs index b3f91eb46a..53522aa3be 100644 --- a/pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs +++ b/pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs @@ -449,7 +449,7 @@ impl NpmResolutionVerifier { lockfile_tarball: &str, ) -> Option { let registry_tarball = match self.fetch_abbreviated_meta(registry, name).await { - Ok(Some(meta)) => { + Ok(meta) => { if let Some(sink) = self.observed_dist_stats.as_ref() && let Some(stats) = meta.version_dist_stats.as_ref().and_then(|stats| stats.get(version)) @@ -458,7 +458,15 @@ impl NpmResolutionVerifier { } meta.version_tarballs.and_then(|tarballs| tarballs.get(version).cloned()) } - Ok(None) | Err(_) => None, + Err(message) => { + // Couldn't reach the registry to verify (auth/network/5xx). + // Propagate the registry's own fetch error (already + // credential-redacted) so the install aborts with it rather + // than mislabeling a transport failure as a tampering-style URL + // mismatch. Still fail-closed: the entry never reaches the + // filesystem because the install never proceeds. + return Some(ResolutionVerification::FetchFailed { message }); + } }; match registry_tarball { Some(url) if same_tarball_url(lockfile_tarball, &url) => None, @@ -485,12 +493,10 @@ impl NpmResolutionVerifier { let cutoff = self.cutoff.expect("cutoff is Some when age check is active"); let published = match self.fetch_published_at(registry, name, version).await { Ok(value) => value, - Err(reason) => { - return Some(ResolutionVerification::Err { - code: MINIMUM_RELEASE_AGE_VIOLATION_CODE, - reason: uncheckable("minimumReleaseAge", &reason), - }); - } + // A transport failure propagates the registry's own fetch error so + // the install aborts with it; a successful fetch that merely lacks a + // timestamp is handled below. + Err(message) => return Some(ResolutionVerification::FetchFailed { message }), }; let Some(published) = published else { // No source surfaced a publish timestamp; mirror the @@ -535,12 +541,10 @@ impl NpmResolutionVerifier { ) -> Option { let meta = match self.fetch_full_meta_for_trust(registry, name).await { Ok(meta) => meta, - Err(reason) => { - return Some(ResolutionVerification::Err { - code: TRUST_DOWNGRADE_VIOLATION_CODE, - reason: uncheckable("trustPolicy", &reason), - }); - } + // A transport failure propagates the registry's own fetch error so + // the install aborts with it rather than folding it into a policy + // violation. + Err(message) => return Some(ResolutionVerification::FetchFailed { message }), }; let trust_opts = TrustCheckOptions { trust_policy_exclude: self.trust_policy_exclude.as_ref(), @@ -600,8 +604,7 @@ impl NpmResolutionVerifier { name: &PkgName, version: &str, ) -> Result, String> { - if let Some(value) = self.try_abbreviated_modified_shortcut(registry, name, version).await? - { + if let Some(value) = self.try_abbreviated_modified_shortcut(registry, name, version).await { return Ok(Some(value)); } if let Some(map) = self.read_local_meta_time(registry, name).await @@ -632,20 +635,23 @@ impl NpmResolutionVerifier { registry: &str, name: &PkgName, version: &str, - ) -> Result, String> { + ) -> Option { let cutoff = self.cutoff.expect("cutoff is Some when age check is active"); - let Some(meta) = self.fetch_abbreviated_meta(registry, name).await? else { - return Ok(None); + // A fetch failure here is fine: ignore the error and fall back to + // per-version lookups, the same as a successful-but-uninformative + // metadata response. + let Ok(meta) = self.fetch_abbreviated_meta(registry, name).await else { + return None; }; - let Some(modified) = meta.modified else { return Ok(None) }; - let Some(parsed) = parse_packument_timestamp(&modified) else { return Ok(None) }; + let modified = meta.modified?; + let parsed = parse_packument_timestamp(&modified)?; if parsed >= cutoff { - return Ok(None); + return None; } if !meta.version_tarballs.as_ref().is_some_and(|map| map.contains_key(version)) { - return Ok(None); + return None; } - Ok(Some(modified)) + Some(modified) } /// Per-`(registry, name)` abbreviated-meta lookup. The result is @@ -662,9 +668,11 @@ impl NpmResolutionVerifier { /// 2. The on-disk + network cached fetcher /// ([`fetch_full_metadata_cached()`] with `full_metadata: false`) /// when no shared entry is available. - /// 3. A failure (decode / network / cache-write IO) caches - /// `None` so subsequent calls fall through to the next layer - /// of [`Self::resolve_published_at`] without retrying. + /// 3. A failure (decode / network / cache-write IO) caches a + /// credential-safe `Err(reason)` so subsequent calls reuse the + /// same verdict without retrying. The tarball-URL check surfaces + /// this error; the age shortcut ignores it and falls through to + /// the next layer of [`Self::resolve_published_at`]. /// /// Mirrors upstream's /// [`fetchAbbreviatedMeta`](https://github.com/pnpm/pnpm/blob/2a9bd897bf/resolving/npm-resolver/src/createNpmResolutionVerifier.ts#L626-L653). @@ -672,7 +680,7 @@ impl NpmResolutionVerifier { &self, registry: &str, name: &PkgName, - ) -> Result, String> { + ) -> Result { let key = package_key(registry, &name.to_string()); let cell = { let mut cache = self.lookup_context.abbreviated_meta.lock().await; @@ -681,7 +689,7 @@ impl NpmResolutionVerifier { let value = cell .get_or_init(|| async { if let Some(shared) = self.read_shared_meta(name) { - return Some(project_abbreviated_meta(&shared)); + return Ok(project_abbreviated_meta(&shared)); } let opts = FetchFullMetadataCachedOptions { registry, @@ -692,13 +700,18 @@ impl NpmResolutionVerifier { filter_metadata: false, retry_opts: self.retry_opts, }; + // Carry a fetch failure (auth/network/5xx) as the `Err` value + // instead of collapsing it to a missing projection: the + // tarball-URL check needs to tell a transport failure apart + // from a version genuinely absent from the metadata, otherwise + // it reports a 403 as a tampering-style mismatch. match fetch_full_metadata_cached(&name.to_string(), &opts).await { - Ok(meta) => Some(project_abbreviated_meta(&meta)), - Err(_) => None, + Ok(meta) => Ok(project_abbreviated_meta(&meta)), + Err(error) => Err(redact_url_credentials(&error.to_string())), } }) .await; - Ok(value.clone()) + value.clone() } /// Try the resolver's shared [`PackageMetaCache`] for a packument @@ -771,7 +784,7 @@ impl NpmResolutionVerifier { }; fetch_attestation_published_at(&name.to_string(), version, &opts) .await - .map_err(|err| err.to_string()) + .map_err(|err| redact_url_credentials(&err.to_string())) } async fn fetch_full_meta_time( @@ -852,7 +865,9 @@ impl NpmResolutionVerifier { filter_metadata: false, retry_opts: self.retry_opts, }; - fetch_full_metadata_cached(&name.to_string(), &opts).await.map_err(|err| err.to_string()) + fetch_full_metadata_cached(&name.to_string(), &opts) + .await + .map_err(|err| redact_url_credentials(&err.to_string())) } } @@ -1075,6 +1090,44 @@ fn project_abbreviated_meta(meta: &Package) -> crate::lookup_context::Abbreviate } } +/// Strip `user:pass@` (or `user@`) that appears right after a URL scheme in +/// any message text, e.g. `… https://user:pass@host/pkg …` → +/// `… https://host/pkg …`. A registry configured as `https://user:pass@host/` +/// would otherwise leak its embedded basic-auth into the fetch error the +/// verifier surfaces when it aborts on a transport failure. +fn redact_url_credentials(text: &str) -> String { + let mut out = String::with_capacity(text.len()); + let mut rest = text; + while let Some(pos) = rest.find("://") { + let (before, after) = rest.split_at(pos + "://".len()); + out.push_str(before); + // Only treat "://" as a URL authority boundary when a scheme character + // (schemes end in an ASCII alphanumeric) precedes it, so an unrelated + // "://" in the message isn't mangled. + let has_scheme = pos > 0 && rest.as_bytes()[pos - 1].is_ascii_alphanumeric(); + rest = strip_leading_userinfo(after).filter(|_| has_scheme).unwrap_or(after); + } + out.push_str(rest); + out +} + +/// If the authority leading `text` contains `userinfo@`, return the slice after +/// the **last** `@` within it; otherwise `None`. The authority ends at the first +/// `/`, `?`, `#`, or whitespace. Stripping to the last `@` keeps a raw `@` inside +/// the password (`user:p@ss@host`) from leaking its tail. +fn strip_leading_userinfo(authority: &str) -> Option<&str> { + let mut last_at = None; + for (idx, ch) in authority.char_indices() { + match ch { + '@' => last_at = Some(idx + ch.len_utf8()), + '/' | '?' | '#' => break, + c if c.is_whitespace() => break, + _ => {} + } + } + last_at.map(|end| &authority[end..]) +} + fn same_tarball_url(left: &str, right: &str) -> bool { canonical_tarball_url(left) == canonical_tarball_url(right) } diff --git a/pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs b/pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs index fb22da730d..dade85b26a 100644 --- a/pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs +++ b/pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs @@ -10,6 +10,7 @@ use ssri::Integrity; use super::{ CreateNpmResolutionVerifierOptions, create_npm_resolution_verifier, observed_dist_stats_sink, + redact_url_credentials, }; const FAKE_INTEGRITY: &str = "sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=="; @@ -1073,3 +1074,119 @@ async fn binding_check_records_dist_stats_into_the_sink() { assert_eq!(recorded.unpacked_size, Some(123_456)); assert_eq!(recorded.file_count, Some(42)); } + +/// A 403 on the metadata fetch (e.g. a CI token that is authenticated but not +/// authorized to read a private package) must not be reported as a lockfile +/// tarball-URL mismatch: the lockfile is correct, the fetch is the problem. The +/// verifier propagates the registry's own fetch error so the install aborts. +#[tokio::test] +async fn propagates_metadata_fetch_failure_instead_of_a_tampering_mismatch() { + let mut server = mockito::Server::new_async().await; + let registry = format!("{}/", server.url()); + let server_url = server.url(); + let _meta_mock = server + .mock("GET", "/private-pkg") + .with_status(403) + .with_body(r#"{"error":"Forbidden"}"#) + .create_async() + .await; + + let opts = default_opts(®istry); + let verifier = create_npm_resolution_verifier(opts); + let resolution = LockfileResolution::Tarball(TarballResolution { + tarball: format!("{server_url}/private-pkg/-/private-pkg-1.0.0.tgz"), + integrity: Some(fake_integrity()), + git_hosted: None, + path: None, + }); + let name: PkgName = "private-pkg".parse().expect("parse"); + let result = verifier.verify(&resolution, ctx(&name, "1.0.0")).await; + + // A transport failure aborts via FetchFailed, never a tampering-style + // TARBALL_URL_MISMATCH. + let ResolutionVerification::FetchFailed { message } = result else { + panic!("expected FetchFailed, got {result:?}"); + }; + assert!(message.contains("403"), "message: {message}"); +} + +/// The metadata fetch succeeds but does not list the pinned version. That is a +/// genuine verification failure (not a transport error), so it stays +/// `TARBALL_URL_MISMATCH` rather than aborting via `FetchFailed`. +#[tokio::test] +async fn version_absent_from_fetched_metadata_stays_tarball_url_mismatch() { + let mut server = mockito::Server::new_async().await; + let registry = format!("{}/", server.url()); + let server_url = server.url(); + let packument = serde_json::json!({ + "name": "present-pkg", + "dist-tags": { "latest": "1.0.0" }, + "versions": { + "1.0.0": { + "name": "present-pkg", + "version": "1.0.0", + "dist": { + "integrity": FAKE_INTEGRITY, + "shasum": "0000000000000000000000000000000000000000", + "tarball": format!("{server_url}/present-pkg/-/present-pkg-1.0.0.tgz"), + } + } + } + }); + let _meta_mock = server + .mock("GET", "/present-pkg") + .with_status(200) + .with_body(packument.to_string()) + .create_async() + .await; + + let opts = default_opts(®istry); + let verifier = create_npm_resolution_verifier(opts); + let resolution = LockfileResolution::Tarball(TarballResolution { + tarball: format!("{server_url}/present-pkg/-/present-pkg-2.0.0.tgz"), + integrity: Some(fake_integrity()), + git_hosted: None, + path: None, + }); + let name: PkgName = "present-pkg".parse().expect("parse"); + let result = verifier.verify(&resolution, ctx(&name, "2.0.0")).await; + + let ResolutionVerification::Err { code, .. } = result else { + panic!("expected Err, got {result:?}"); + }; + assert_eq!(code, "TARBALL_URL_MISMATCH"); +} + +#[test] +fn redact_url_credentials_strips_embedded_basic_auth() { + assert_eq!( + redact_url_credentials( + "Failed to fetch metadata from https://user:pass@host/pkg: timed out" + ), + "Failed to fetch metadata from https://host/pkg: timed out", + ); + // user-only userinfo (no password) is stripped too. + assert_eq!( + redact_url_credentials("got https://token@registry.example/foo"), + "got https://registry.example/foo", + ); + // A raw "@" inside the password is stripped up to the last "@" in the + // authority, so the password tail can't leak. + assert_eq!( + redact_url_credentials("Failed to fetch metadata from https://user:p@ss@host/pkg: 403"), + "Failed to fetch metadata from https://host/pkg: 403", + ); + // An "@" in the path/query (after the authority) is preserved. + assert_eq!( + redact_url_credentials("got https://host/path?to=a@b"), + "got https://host/path?to=a@b", + ); + // A credential-free URL is left untouched. + assert_eq!( + redact_url_credentials("Failed to fetch metadata from https://host/pkg: timed out"), + "Failed to fetch metadata from https://host/pkg: timed out", + ); + // A bare "://" with no preceding scheme character is not treated as a URL + // authority, so an "@" further along is preserved. + assert_eq!(redact_url_credentials("a :// b@c"), "a :// b@c"); +} diff --git a/pacquet/crates/resolving-npm-resolver/src/lookup_context.rs b/pacquet/crates/resolving-npm-resolver/src/lookup_context.rs index 7428f5bf1a..0cd030760e 100644 --- a/pacquet/crates/resolving-npm-resolver/src/lookup_context.rs +++ b/pacquet/crates/resolving-npm-resolver/src/lookup_context.rs @@ -75,7 +75,12 @@ pub(crate) struct PublishedAtLookupContext { pub published_at: SingleflightMap, String>>, pub full_meta: SingleflightMap>, String>>, pub full_meta_for_trust: SingleflightMap, String>>, - pub abbreviated_meta: SingleflightMap>, + /// `Ok(projection)` on a successful fetch, `Err(reason)` on a fetch + /// failure (auth/network/5xx). The error is carried as a value rather than + /// discarded so the tarball-URL check can tell a transport failure apart + /// from a version genuinely absent from the metadata; the age shortcut + /// ignores it and falls back to per-version lookups. + pub abbreviated_meta: SingleflightMap>, pub local_meta: SingleflightMap>>, } diff --git a/pacquet/crates/resolving-resolver-base/src/tests.rs b/pacquet/crates/resolving-resolver-base/src/tests.rs index eed9921815..7735fcfb37 100644 --- a/pacquet/crates/resolving-resolver-base/src/tests.rs +++ b/pacquet/crates/resolving-resolver-base/src/tests.rs @@ -30,7 +30,9 @@ fn resolution_verification_err_round_trip() { assert_eq!(code, "MINIMUM_RELEASE_AGE_VIOLATION"); assert_eq!(reason, "was published yesterday"); } - ResolutionVerification::Ok => panic!("expected Err"), + ResolutionVerification::Ok | ResolutionVerification::FetchFailed { .. } => { + panic!("expected Err") + } } } diff --git a/pacquet/crates/resolving-resolver-base/src/verifier.rs b/pacquet/crates/resolving-resolver-base/src/verifier.rs index b8e44d2ec1..b4c94732aa 100644 --- a/pacquet/crates/resolving-resolver-base/src/verifier.rs +++ b/pacquet/crates/resolving-resolver-base/src/verifier.rs @@ -29,6 +29,18 @@ pub enum ResolutionVerification { /// breakdown. Allowed to allocate. reason: String, }, + /// The registry couldn't be reached to verify the entry + /// (auth/network/5xx). Unlike [`ResolutionVerification::Err`], this + /// is not a per-entry policy pick to collect into the batch — the + /// verification never completed, so the runner aborts the install + /// with the registry's own fetch error rather than mislabeling a + /// transport failure as lockfile tampering. Mirrors pnpm, where the + /// verifier rethrows the underlying `FetchError`. + FetchFailed { + /// The registry fetch error, already rendered and stripped of + /// any credentials embedded in the URL. + message: String, + }, } /// A [`ResolutionVerifier`]'s rejection materialized for one diff --git a/pnpm11/core/error/src/index.ts b/pnpm11/core/error/src/index.ts index 73a42e2c78..9c5deeee16 100644 --- a/pnpm11/core/error/src/index.ts +++ b/pnpm11/core/error/src/index.ts @@ -45,7 +45,7 @@ export class FetchError extends PnpmError { if (request.authHeaderValue) { _request.authHeaderValue = hideAuthInformation(request.authHeaderValue) } - const message = `GET ${request.url}: ${response.statusText} - ${response.status}` + const message = `GET ${redactUrlCredentials(request.url)}: ${response.statusText} - ${response.status}` // NOTE: For security reasons, some registries respond with 404 on authentication errors as well. // So we print authorization info on 404 errors as well. if (response.status === 401 || response.status === 403 || response.status === 404) { @@ -62,6 +62,53 @@ export class FetchError extends PnpmError { } } +/** + * Strip `user:pass@` (or `user@`) userinfo that follows a URL scheme in any + * text, e.g. `GET https://user:pass@host/pkg: …` → `GET https://host/pkg: …`. + * A registry configured as `https://user:pass@host/` would otherwise leak its + * embedded basic-auth credentials into every error message that interpolates + * the request URL (terminal output, CI logs). `FetchError` already hides the + * auth *header*; this covers credentials carried in the URL itself. + * + * Implemented as a single forward scan rather than a regex: `text` is + * uncontrolled (it interpolates the request URL), so a backtracking pattern is + * a ReDoS vector, and the scan strips up to the **last** `@` in the authority + * so a raw `@` inside the password (`user:p@ss@host`) doesn't leak its tail. + */ +export function redactUrlCredentials (text: string): string { + let result = '' + let cursor = 0 + while (cursor < text.length) { + const schemeSep = text.indexOf('://', cursor) + if (schemeSep === -1) return result + text.slice(cursor) + const authorityStart = schemeSep + 3 + result += text.slice(cursor, authorityStart) + cursor = authorityStart + // Only treat `://` as a URL authority boundary when a scheme character + // (schemes end in an ASCII alphanumeric) sits right before it; otherwise a + // bare `://` in the text is left untouched. + if (schemeSep === 0 || !isSchemeTailChar(text.charCodeAt(schemeSep - 1))) continue + // Userinfo runs to the last `@` within the authority, which itself ends at + // the first `/`, `?`, `#`, or whitespace. + let lastAt = -1 + for (let i = authorityStart; i < text.length; i++) { + const code = text.charCodeAt(i) + if (code === 0x2f || code === 0x3f || code === 0x23 || isAsciiWhitespace(code)) break + if (code === 0x40) lastAt = i + } + if (lastAt !== -1) cursor = lastAt + 1 + } + return result +} + +function isSchemeTailChar (code: number): boolean { + return (code >= 0x30 && code <= 0x39) || (code >= 0x41 && code <= 0x5a) || (code >= 0x61 && code <= 0x7a) +} + +function isAsciiWhitespace (code: number): boolean { + return code === 0x20 || code === 0x09 || code === 0x0a || code === 0x0b || code === 0x0c || code === 0x0d +} + function hideAuthInformation (authHeaderValue: string): string { const [authType, token] = authHeaderValue.split(' ') if (token == null) return '[hidden]' diff --git a/pnpm11/core/error/test/index.ts b/pnpm11/core/error/test/index.ts index 7d3e046758..24d5db205d 100644 --- a/pnpm11/core/error/test/index.ts +++ b/pnpm11/core/error/test/index.ts @@ -1,5 +1,5 @@ import { expect, test } from '@jest/globals' -import { FetchError, PnpmError } from '@pnpm/error' +import { FetchError, PnpmError, redactUrlCredentials } from '@pnpm/error' test('PnpmError exposes cause when provided', () => { const cause = new Error('original failure') @@ -48,3 +48,35 @@ test('FetchError escapes non-standard auth header', () => { expect(error.hint).toBe('An authorization header was used: [hidden]') expect(error.request.authHeaderValue).toBe('[hidden]') }) + +test('FetchError strips basic-auth credentials embedded in the request URL', () => { + const error = new FetchError( + { url: 'https://user:pass@registry.example/@scope%2fpkg' }, + { status: 403, statusText: 'Forbidden' } + ) + expect(error.message).toBe('GET https://registry.example/@scope%2fpkg: Forbidden - 403') +}) + +test('redactUrlCredentials', () => { + // user:pass@ and user@ userinfo are stripped, regardless of scheme. + expect(redactUrlCredentials('GET https://user:pass@host/pkg: timed out')) + .toBe('GET https://host/pkg: timed out') + expect(redactUrlCredentials('git+ssh://token@host/repo.git')) + .toBe('git+ssh://host/repo.git') + // A raw "@" inside the password is stripped up to the last "@" in the + // authority, so the password tail can't leak. + expect(redactUrlCredentials('GET https://user:p@ss@host/pkg: 403')) + .toBe('GET https://host/pkg: 403') + // An "@" in the path/query (after the authority) is preserved. + expect(redactUrlCredentials('https://host/path?to=a@b')) + .toBe('https://host/path?to=a@b') + // A credential-free URL is left untouched. + expect(redactUrlCredentials('GET https://host/pkg: timed out')) + .toBe('GET https://host/pkg: timed out') + // A bare "://" with no preceding scheme character is not a URL authority, so + // a later "@" is preserved. + expect(redactUrlCredentials('a :// b@c')).toBe('a :// b@c') + // Multiple credentialed URLs in one message are all redacted. + expect(redactUrlCredentials('a https://u:p@h1/x and b https://t@h2/y')) + .toBe('a https://h1/x and b https://h2/y') +}) diff --git a/pnpm11/installing/deps-installer/src/install/verifyLockfileResolutions.ts b/pnpm11/installing/deps-installer/src/install/verifyLockfileResolutions.ts index 066f9f2b00..149b3f7d18 100644 --- a/pnpm11/installing/deps-installer/src/install/verifyLockfileResolutions.ts +++ b/pnpm11/installing/deps-installer/src/install/verifyLockfileResolutions.ts @@ -262,6 +262,10 @@ function buildVerificationError (violations: ResolutionPolicyViolation[]): PnpmE const details = omitted > 0 ? `${breakdown}\n …and ${omitted} more` : breakdown + // Registry fetch failures (auth/network/5xx) don't reach this batch — the + // verifier throws the registry's own error and the gate aborts with it. So + // every violation here is a genuine policy rejection, and the hint points at + // the lockfile rather than at connectivity. return new PnpmError( errorCode, `${violations.length} lockfile entries failed verification:\n${details}`, @@ -422,23 +426,40 @@ async function iterateLockfileViolations ( concurrency: number | undefined ): Promise { const violations: ResolutionPolicyViolation[] = [] + // A verifier may throw rather than return a violation when it can't reach the + // registry to verify an entry (auth/network/5xx) — that's not a per-entry + // policy pick, it's an incomplete verification, so the registry's own error + // should abort the install. Capture the first such error and rethrow it after + // the fan-out settles: rethrowing straight into Promise.all would leave the + // sibling tasks (all failing against the same dead registry) as unhandled + // rejections once Promise.all rejects on the first. + let fetchError: unknown const limit = pLimit(concurrency ?? DEFAULT_CONCURRENCY) await Promise.all( Array.from(candidates.values(), ({ name, version, nonSemverVersion, resolution }) => limit(async () => { - // Fan out across every active verifier; each handles its own - // protocol short-circuit (e.g. the npm verifier returns ok:true for - // git resolutions). We stop at the first failure per entry so a - // multi-verifier setup doesn't produce duplicate violations for the - // same (name, version). - for (const verifier of verifiers) { - // eslint-disable-next-line no-await-in-loop - const result = await verifier.verify(resolution, { name, version, nonSemverVersion }) - if (!result.ok) { - violations.push({ name, version, resolution, code: result.code, reason: result.reason }) - break + try { + // Fan out across every active verifier; each handles its own + // protocol short-circuit (e.g. the npm verifier returns ok:true for + // git resolutions). We stop at the first failure per entry so a + // multi-verifier setup doesn't produce duplicate violations for the + // same (name, version). + for (const verifier of verifiers) { + // eslint-disable-next-line no-await-in-loop + const result = await verifier.verify(resolution, { name, version, nonSemverVersion }) + if (!result.ok) { + violations.push({ name, version, resolution, code: result.code, reason: result.reason }) + break + } } + } catch (err) { + fetchError ??= err } })) ) + // A registry that couldn't be reached takes precedence over collected + // violations: the run never finished verifying, so the batch is incomplete + // and the actionable failure is the transport error. Once it's resolved the + // re-run surfaces any remaining violations. + if (fetchError != null) throw fetchError return violations } diff --git a/pnpm11/installing/deps-installer/test/install/verifyLockfileResolutions.ts b/pnpm11/installing/deps-installer/test/install/verifyLockfileResolutions.ts index b880145b50..27c21a7946 100644 --- a/pnpm11/installing/deps-installer/test/install/verifyLockfileResolutions.ts +++ b/pnpm11/installing/deps-installer/test/install/verifyLockfileResolutions.ts @@ -71,6 +71,27 @@ test('throws with the verifier-supplied code and reason on a single failure', as }) }) +test('propagates a verifier throw (registry fetch failure) instead of folding it into a batch', async () => { + // A verifier throws — rather than returning a violation — when it can't reach + // the registry to verify an entry. That transport error must surface as-is + // (the install aborts with the registry's own error), not be turned into a + // lockfile-verification batch. + const lockfile = makeLockfile({ + 'is-odd@0.1.2': { resolution: tarballResolution('sha512-a') }, + 'private-pkg@1.0.0': { resolution: tarballResolution('sha512-b') }, + }) + const fetchError = Object.assign(new Error('GET https://registry.example/private-pkg: Forbidden - 403'), { + code: 'ERR_PNPM_FETCH_403', + }) + const verifier = wrap(async (_, { name }) => { + if (name === 'private-pkg') throw fetchError + return { ok: false, code: 'MINIMUM_RELEASE_AGE_VIOLATION', reason: 'too fresh' } + }) + + // The thrown transport error wins over the collected policy violation. + await expect(verifyLockfileResolutions(lockfile, [verifier])).rejects.toBe(fetchError) +}) + test('throws a generic code with per-entry codes in the breakdown when violations span policies', async () => { const lockfile = makeLockfile({ 'is-odd@0.1.2': { resolution: tarballResolution('sha512-a') }, diff --git a/pnpm11/resolving/npm-resolver/src/createNpmResolutionVerifier.ts b/pnpm11/resolving/npm-resolver/src/createNpmResolutionVerifier.ts index e8830fed54..e48dfebd08 100644 --- a/pnpm11/resolving/npm-resolver/src/createNpmResolutionVerifier.ts +++ b/pnpm11/resolving/npm-resolver/src/createNpmResolutionVerifier.ts @@ -335,16 +335,11 @@ async function runAgeCheck ( cutoff: number, ignoreMissingTimeField: boolean ): Promise<{ ok: false, code: string, reason: string } | undefined> { - let published: string | undefined - try { - published = await fetchPublishedAt(context, registry, name, version) - } catch (err) { - return { - ok: false, - code: MINIMUM_RELEASE_AGE_VIOLATION_CODE, - reason: uncheckable('minimumReleaseAge', err instanceof Error ? err.message : String(err)), - } - } + // A transport failure (auth/network/5xx) propagates the registry's own fetch + // error (e.g. ERR_PNPM_FETCH_403); the gate aborts the install with it rather + // than folding it into a policy violation. A successful fetch that simply + // lacks a publish timestamp for this version is handled below. + const published = await fetchPublishedAt(context, registry, name, version) if (!published) { // No source — attestation, local mirror, or full metadata — // surfaced a publish timestamp for this version. The resolver's @@ -400,7 +395,15 @@ async function runTarballUrlCheck ( version: string, lockfileTarball: string ): Promise<{ ok: false, code: string, reason: string } | undefined> { - const meta = await fetchAbbreviatedMeta(context, registry, name) + const { meta, error } = await fetchAbbreviatedMeta(context, registry, name) + if (error != null) { + // Couldn't reach the registry to verify (auth/network/5xx). Propagate the + // registry's own fetch error (e.g. ERR_PNPM_FETCH_403, which already + // explains the auth situation) instead of mislabeling a transport failure + // as a tampering-style URL mismatch. The gate aborts the install with that + // error — still fail-closed, the entry never reaches the filesystem. + throw error + } const registryTarball = meta?.versionTarballs?.get(version) if (registryTarball != null && sameTarballUrl(lockfileTarball, registryTarball)) { return undefined @@ -454,19 +457,11 @@ async function runTrustCheck ( trustPolicyIgnoreAfter?: number } ): Promise<{ ok: false, code: string, reason: string } | undefined> { - let meta: PackageMeta - try { - meta = await fetchFullMetaForTrust(context, registry, name) - } catch (err) { - // `fetchFullMetadataCached` rejects (network error, 404, etc.); the - // verifier fails closed so a missing manifest can't be mistaken - // for a passing trust check. - return { - ok: false, - code: TRUST_DOWNGRADE_VIOLATION_CODE, - reason: uncheckable('trustPolicy', err instanceof Error ? err.message : String(err)), - } - } + // A transport failure (auth/network/5xx) propagates the registry's own fetch + // error; the gate aborts the install with it rather than folding it into a + // policy violation. Still fail-closed: a missing manifest can't be mistaken + // for a passing trust check because the install never proceeds. + const meta = await fetchFullMetaForTrust(context, registry, name) try { failIfTrustDowngraded(meta, version, opts) @@ -611,10 +606,11 @@ interface PublishedAtLookupContext { * package-level `modified` plus the set of currently-listed version * names — so the multi-hundred-KB packument can be GC'd as soon as * the fetch returns (the cache only needs to dedupe network/disk - * round-trips, not full document storage). Resolves to `undefined` - * on failure. + * round-trips, not full document storage). Resolves to `{ meta }` on + * success or `{ error }` on a fetch failure — it never rejects, so the + * cached promise is safe to share between callers. */ - abbreviatedMetaCache: Map> + abbreviatedMetaCache: Map> /** * Per-(registry+name+version) memo of the final published-at answer * the verifier hands to the policy check. One install verifies each @@ -724,7 +720,9 @@ async function tryAbbreviatedModifiedShortcut ( name: string, version: string ): Promise { - const meta = await fetchAbbreviatedMeta(context, registry, name) + // A fetch failure here is fine: ignore `error` and fall back to per-version + // lookups, the same as a successful-but-uninformative metadata response. + const { meta } = await fetchAbbreviatedMeta(context, registry, name) const modified = meta?.modified if (typeof modified !== 'string') return undefined const modifiedMs = Date.parse(modified) @@ -742,7 +740,7 @@ function fetchAbbreviatedMeta ( context: PublishedAtLookupContext, registry: string, name: string -): Promise { +): Promise { const cacheKey = `${registry}\x00${name}` let cachedPromise = context.abbreviatedMetaCache.get(cacheKey) if (cachedPromise == null) { @@ -753,13 +751,22 @@ function fetchAbbreviatedMeta ( // can only return this registry's own packument. const shared = readSharedMeta(context.sharedMetaCache, registry, name) if (shared != null) { - cachedPromise = Promise.resolve(projectAbbreviatedMeta(shared)) + cachedPromise = Promise.resolve({ meta: projectAbbreviatedMeta(shared) }) } else { + // Carry a fetch failure (auth/network/5xx) as `error` instead of + // collapsing it to `undefined`: the tarball-URL check rethrows it (so the + // registry's own error surfaces, not a tampering-style mismatch) while + // the age shortcut ignores it and falls back to per-version lookups. + // Keeping it a resolved value — not a rejected promise — lets the two + // callers share one cached promise without an unhandled rejection. cachedPromise = fetchAbbreviatedMetadataCached(context.fetchOpts, name, { registry, authHeaderValue: context.getAuthHeaderValueByURI(registry, { pkgName: name }), cacheDir: context.cacheDir, - }).then(projectAbbreviatedMeta, () => undefined) + }).then( + (meta) => ({ meta: projectAbbreviatedMeta(meta) }), + (error: unknown) => ({ error }) + ) } context.abbreviatedMetaCache.set(cacheKey, cachedPromise) } @@ -846,6 +853,21 @@ interface AbbreviatedMetaProjection { versionTarballs?: Map } +/** + * Result of an abbreviated-metadata fetch. The fetch error is carried as a + * value rather than rejected so the per-install cache can hold a single + * resolved promise — a cached rejection would surface as an unhandled + * rejection and be shared across every caller of the same key. The + * tarball-URL check rethrows this error; the age shortcut ignores it and + * falls back to per-version lookups. + * + * Modeled as a discriminated union so a result carries exactly one of `meta` + * or `error` — never both, never neither. + */ +type AbbreviatedMetaResult = + | { meta: AbbreviatedMetaProjection, error?: undefined } + | { error: unknown, meta?: undefined } + function readLocalMetaTime ( context: PublishedAtLookupContext, registry: string, diff --git a/pnpm11/resolving/npm-resolver/src/fetch.ts b/pnpm11/resolving/npm-resolver/src/fetch.ts index ff6f5a9c1d..8d027c5b3b 100644 --- a/pnpm11/resolving/npm-resolver/src/fetch.ts +++ b/pnpm11/resolving/npm-resolver/src/fetch.ts @@ -1,4 +1,5 @@ import url from 'node:url' +import util from 'node:util' import { requestRetryLogger } from '@pnpm/core-loggers' import { @@ -6,6 +7,7 @@ import { type FetchErrorRequest, type FetchErrorResponse, PnpmError, + redactUrlCredentials, } from '@pnpm/error' import type { FetchFromRegistry, RetryTimeoutOptions } from '@pnpm/fetching.types' import { globalWarn } from '@pnpm/logger' @@ -143,7 +145,16 @@ export async function fetchMetadataFromFromRegistry ( timeout: fetchOpts.timeout, }) as RegistryResponse } catch (error: any) { // eslint-disable-line - reject(new PnpmError('META_FETCH_FAIL', `GET ${uri}: ${error.message as string}`, { attempts: attempt, cause: error })) + // Redact credentials embedded in the URL from the cause as well, not + // just the top-level message: a reporter or debugger that renders + // `error.cause` would otherwise print the raw URL-bearing message. The + // `stack` string embeds the original (pre-mutation) message, so redact + // it too — mutating `message` alone leaves the credentials in `stack`. + if (util.types.isNativeError(error)) { + if (typeof error.message === 'string') error.message = redactUrlCredentials(error.message) + if (typeof error.stack === 'string') error.stack = redactUrlCredentials(error.stack) + } + reject(new PnpmError('META_FETCH_FAIL', redactUrlCredentials(`GET ${uri}: ${error.message as string}`), { attempts: attempt, cause: error })) return } if (response.status === 304) { diff --git a/pnpm11/resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts b/pnpm11/resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts index 6bc968b447..a3c5ef6563 100644 --- a/pnpm11/resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts +++ b/pnpm11/resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts @@ -563,3 +563,47 @@ test('createNpmResolutionVerifier() canTrustPastCheck rejects when the trust-exc trustPolicyIgnoreAfter: null, })).toBe(false) }) + +test('createNpmResolutionVerifier() propagates the registry fetch error instead of reporting a tampering-style mismatch', async () => { + // A 403 on the metadata fetch (e.g. a CI token that is authenticated but not + // authorized to read a private package) must not be reported as a lockfile + // tarball-URL mismatch: the lockfile is correct, the fetch is the problem. + // The verifier rethrows the registry's own error so the install aborts with + // ERR_PNPM_FETCH_403 (which already explains the auth situation). + const pool = getMockAgent().get('https://registry.npmjs.org') + pool.intercept({ path: '/private-pkg', method: 'GET' }).reply(403, { error: 'Forbidden' }).persist() + + const verifier = createNpmResolutionVerifier(makeVerifierOpts()) + await expect(verifier.verify( + makeTarballResolution('private-pkg', '1.0.0'), + { name: 'private-pkg', version: '1.0.0' } + )).rejects.toMatchObject({ code: 'ERR_PNPM_FETCH_403' }) +}) + +test('createNpmResolutionVerifier() still flags a version absent from fetched metadata as TARBALL_URL_MISMATCH', async () => { + // The metadata fetch succeeds but does not list the pinned version. That is a + // genuine verification failure (not a transport error), so it must stay + // TARBALL_URL_MISMATCH — distinct from the new TARBALL_URL_FETCH_FAILED. + const meta = { + name: 'present-pkg', + 'dist-tags': { latest: '1.0.0' }, + versions: { + '1.0.0': { + name: 'present-pkg', + version: '1.0.0', + dist: { tarball: 'https://registry.npmjs.org/present-pkg/-/present-pkg-1.0.0.tgz', shasum: 'aa' }, + }, + }, + modified: '2020-01-01T00:00:00.000Z', + } + const pool = getMockAgent().get('https://registry.npmjs.org') + pool.intercept({ path: '/present-pkg', method: 'GET' }).reply(200, meta).persist() + + const verifier = createNpmResolutionVerifier(makeVerifierOpts()) + const result = await verifier.verify( + makeTarballResolution('present-pkg', '2.0.0'), + { name: 'present-pkg', version: '2.0.0' } + ) + + expect(result).toMatchObject({ ok: false, code: 'TARBALL_URL_MISMATCH' }) +}) diff --git a/pnpr/crates/pnpr/src/resolver.rs b/pnpr/crates/pnpr/src/resolver.rs index fa24e20001..64c6284260 100644 --- a/pnpr/crates/pnpr/src/resolver.rs +++ b/pnpr/crates/pnpr/src/resolver.rs @@ -795,7 +795,15 @@ async fn verify_input_lockfile( return Ok(None); } - let violations = collect_resolution_policy_violations(lockfile, &verifiers, None).await; + // A transport failure verifying an entry (the upstream registry couldn't be + // reached/authorized) is a gateway error, not a policy violation — surface + // the registry's own (credential-redacted) message to the client. + let violations = match collect_resolution_policy_violations(lockfile, &verifiers, None).await { + Ok(violations) => violations, + Err(message) => { + return Err(VerifyFailure::Internal(json_error(StatusCode::BAD_GATEWAY, &message))); + } + }; let osv_violations = runtime .osv_index .as_ref()