Walkthrough

Tarball URL rewriting is refactored to preserve the upstream dist.tarball basename instead of reconstructing filenames from version strings. A new tarball_basename helper extracts path segments. In serve_tarball, version resolution now matches the requested filename against each packument version's dist.tarball basename via expected_tarball_dist, with a second OSV authorization check when the resolved version differs from the filename-derived one.

Changes

Tarball Basename Preservation and Version Resolution

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant serve_tarball
  participant ensure_osv_allowed
  participant expected_tarball_dist
  participant packument

  Client->>serve_tarball: GET /pkg/-/foo-1.0.0.tgz
  serve_tarball->>serve_tarball: parse_tarball_name → name_version (e.g. "1.0.0")
  serve_tarball->>ensure_osv_allowed: check name_version
  serve_tarball->>packument: load packument
  serve_tarball->>expected_tarball_dist: match "foo-1.0.0.tgz" vs dist.tarball basenames
  expected_tarball_dist->>packument: iterate versions, extract tarball_basename
  expected_tarball_dist-->>serve_tarball: TarballDist { version: "2.0.0", integrity } or error
  alt resolved version ≠ name_version
    serve_tarball->>ensure_osv_allowed: re-check resolved version
  end
  serve_tarball-->>Client: serve tarball bytes or not_found / bad_gateway

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• pnpm/pnpm#12566: Both PRs modify serve_tarball to parse the tarball filename and perform OSV-related authorization, with this PR further refining when OSV is re-checked based on the resolved packument tarball version after dist.tarball basename matching.
• pnpm/pnpm#12570: Directly related — both PRs modify serve_tarball's dist.integrity resolution from packument data and the tarball_integrity_error handling path in the same server module.

• X
• Mastodon
• Reddit
• LinkedIn

• Preserve upstream tarball basenames when rewriting dist.tarball URLs.
• Resolve tarball requests by matching basename to packument versions and integrity.
• Update/expand tests and fix pnpr server test setup compile break.

graph TD
  client(["Client (pnpm)"]) --> pack["pnpr: packument/manifest\n(rewrite dist.tarball basename)"] --> client
  client --> tar["pnpr: serve_tarball"] --> osv{"OSV policy screen"} --> resolve["Resolve by dist.tarball\nbasename + dist.integrity"] --> up(("Upstream registry")) --> cache[("Tarball cache")] --> client

  subgraph Legend
    direction LR
    _a(["Actor"]) ~~~ _s["Service/handler"] ~~~ _d{"Policy/decision"} ~~~ _e(("External")) ~~~ _db[("Storage")]
  end

Loading

The following are alternative approaches to this PR:

1. Canonicalize filenames but add redirect/alias mapping

• ➕ Keeps a single canonical URL shape for clients/lockfiles
• ➕ Can preserve previous security model without packument basename reliance
• ➖ Requires storing and maintaining a mapping of canonical↔upstream basenames
• ➖ Adds extra request/redirect hop or more complex routing rules
• ➖ Still needs careful integrity binding to avoid cross-version confusion

2. Resolve tarball version by integrity instead of basename

• ➕ Avoids reliance on tarball filename conventions entirely
• ➕ Could handle edge cases where different versions share basenames
• ➖ Requires reading packument first and possibly more CPU/work per request
• ➖ Integrity lookup can be ambiguous if duplicates exist; still needs tie-break rules
• ➖ More invasive change than basename match and less aligned with upstream URL reality

Recommendation: The chosen approach (preserve upstream basenames in rewritten URLs and resolve the declaring version by matching that basename in the packument) is the most direct fix with minimal surface-area impact. It restores correctness for non-canonical upstream tarball names while keeping the GHSA protection intact by continuing to verify served bytes against the declaring version’s dist.integrity, and it keeps caching keyed to the actual upstream-served basename.

Bug fix (2) +75 / -30

server.rsResolve tarball integrity by dist.tarball basename, with OSV re-screening +49/-17

Resolve tarball integrity by dist.tarball basename, with OSV re-screening

• Changes tarball serving to treat the filename-derived version as a best-effort hint, then resolve the authoritative declaring version by matching the requested filename against each version’s 'dist.tarball' basename. Returns both resolved version and 'dist.integrity' and re-runs OSV screening when the resolved version differs.

pnpr/crates/pnpr/src/server.rs

upstream.rsPreserve upstream tarball basename when rewriting dist.tarball URLs +26/-13

Preserve upstream tarball basename when rewriting dist.tarball URLs

• Updates 'rewrite_tarball_urls' to rewrite URLs as '{public_url}/{pkg}/-/{basename}' using the original tarball URL’s basename rather than reconstructing '<name>-<version>.tgz'. Applies the rewrite helper to both packument and single-manifest shapes.

pnpr/crates/pnpr/src/upstream.rs

Tests (3) +46 / -10

tests.rsFix auth test setup after UserStore::in_memory signature change +2/-2

Fix auth test setup after UserStore::in_memory signature change

• Updates tests to pass 'MaxUsers::Unlimited' into 'UserStore::in_memory', fixing a compile break in the pnpr server test target.

pnpr/crates/pnpr/src/server/tests.rs

tests.rsAdjust tarball rewrite expectations and add non-canonical basename coverage +28/-2

Adjust tarball rewrite expectations and add non-canonical basename coverage

• Fixes scoped verdaccio tarball expectations to reflect basename-only rewriting and adds a new test ensuring non-canonical upstream basenames (e.g., zero-padded versions) are preserved in rewritten URLs.

pnpr/crates/pnpr/src/upstream/tests.rs

server.rsUpdate end-to-end tarball routing test to assert basename preservation and binding +16/-6

Update end-to-end tarball routing test to assert basename preservation and binding

• Renames and updates the tarball route/cache key test to assert that pnpr fetches/caches under the declaring version’s 'dist.tarball' basename and still verifies bytes against that version’s integrity, even when basenames are swapped between versions in the packument.

pnpr/crates/pnpr/tests/server.rs

Description

rewrite_dist_tarball() now returns without rewriting when tarball_basename() is None (e.g.,
upstream dist.tarball ends with /), so pnpr can serve a packument that still contains the
upstream tarball URL. That allows an upstream-controlled packument to direct clients to fetch
tarballs from arbitrary URLs instead of pnpr’s /pkg/-/file.tgz endpoint where integrity and OSV
enforcement occur.

Code

pnpr/crates/pnpr/src/upstream.rs[R394-399]

let Some(tarball_value) = dist.get_mut("tarball") else { return };
-    if !tarball_value.is_string() {
+    let Some(basename) = tarball_value.as_str().and_then(tarball_basename) else {
    return;
-    }
-    let filename = pkg.tarball_name_for_version(version);
-    *tarball_value = Value::String(format!("{public_url}/{}/-/{filename}", pkg.as_str()));
+    };
+    *tarball_value = Value::String(format!("{public_url}/{}/-/{basename}", pkg.as_str()));
+}

Evidence

The rewrite helper early-returns when it can’t compute a basename, which means dist.tarball can
remain an upstream URL in the JSON pnpr serves. packument_response() always calls
rewrite_tarball_urls() before serializing the packument, so any skipped rewrite is observable by
clients and can route them away from pnpr’s tarball endpoint (where pnpr enforces integrity/OSV).

pnpr/crates/pnpr/src/upstream.rs[380-411]
pnpr/crates/pnpr/src/server.rs[2542-2552]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`rewrite_tarball_urls()` is intended to ensure all `dist.tarball` URLs served to clients route through pnpr. After this PR, `rewrite_dist_tarball()` skips rewriting when `tarball_basename()` returns `None` (notably when the URL’s path ends in `/`), which can leave the upstream URL in the served packument. This bypasses pnpr’s tarball-serving path where integrity verification and OSV policy are enforced.
## Issue Context
This is a security control-plane regression: a malicious or malformed upstream packument can set `dist.tarball` to an arbitrary URL that triggers `tarball_basename(url) == None` and thus gets passed through to clients.
## Fix Focus Areas
- pnpr/crates/pnpr/src/upstream.rs[380-411]
- pnpr/crates/pnpr/src/server.rs[2542-2552]
### Suggested implementation direction
- Ensure `dist.tarball` is *always* rewritten to the pnpr public URL when `dist.tarball` is a string.
- If a basename cannot be derived (e.g., trailing `/`), do not leave the upstream URL intact. Options (pick one):
- **Fail closed**: make `rewrite_tarball_urls` return `Result<()>` and have `packument_response()` fail (502) when a tarball URL cannot be rewritten.
- **Safe fallback**: when iterating `versions`, keep access to the version key and fall back to `pkg.tarball_name_for_version(version_key)` for rewrite if `tarball_basename()` returns `None` (preserve-basename only for well-formed URLs).
- At minimum, replace unrewritable tarballs with a pnpr URL (or remove the affected version) rather than passing through the upstream URL.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

Because tarball URLs now preserve the upstream dist.tarball basename, the tarball cache is keyed
by an upstream-controlled filename rather than a stable version-derived name. If an upstream changes
the basename for the same version over time, pnpr will miss existing cache entries and store
duplicate tarballs under different filenames, increasing bandwidth/disk usage and creating a
cache-bloat amplification vector.

Code

pnpr/crates/pnpr/src/upstream.rs[R390-415]

+fn rewrite_dist_tarball(value: &mut Value, pkg: &PackageName, public_url: &str) {
+    // Every string `dist.tarball` must be rewritten to a route on *this*
+    // server, where integrity and OSV are enforced — never passed through.
+    // When the upstream URL has no usable basename (e.g. it ends in `/`), fall
+    // back to the version-derived canonical name (the manifest carries its own
+    // `version`) so a malformed URL still points at pnpr (and 404s there)
+    // rather than directing the client at an arbitrary upstream host.
+    let fallback = value
+        .get("version")
+        .and_then(Value::as_str)
+        .map(|version| pkg.tarball_name_for_version(version));
   let Some(dist) = value.get_mut("dist").and_then(Value::as_object_mut) else {
       return;
   };
   let Some(tarball_value) = dist.get_mut("tarball") else { return };
   if !tarball_value.is_string() {
       return;
   }
-    let filename = pkg.tarball_name_for_version(version);
+    let filename = tarball_value
+        .as_str()
+        .and_then(tarball_basename)
+        .map(str::to_owned)
+        .or(fallback)
+        .unwrap_or_default();
   *tarball_value = Value::String(format!("{public_url}/{}/-/{filename}", pkg.as_str()));
}

Evidence

The PR changes tarball rewrite to preserve the upstream basename, and the tarball-serving path uses
the request filename as the on-disk cache key via Storage::tarball_path(name, filename), so
upstream basename churn directly translates into new cache entries and cache misses.

pnpr/crates/pnpr/src/upstream.rs[360-415]
pnpr/crates/pnpr/src/server.rs[828-955]
pnpr/crates/pnpr/src/storage.rs[723-729]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`rewrite_dist_tarball()` now rewrites `dist.tarball` to `{public_url}/{pkg}/-/{basename}` where `basename` is preserved from the upstream URL. Downstream, `serve_tarball()` uses the requested `filename` as the storage/cache key (`open_cached_tarball*` and integrity marker I/O). This makes the cache identity effectively upstream-basename-driven, so an upstream that changes tarball basenames for the same version can cause repeated cache misses and duplicated cache entries.
### Issue Context
This is a resource-amplification risk: even if bytes/integrity are unchanged, varying basenames force extra upstream fetches and extra disk entries because the storage path is `package_dir.join(filename)`.
### Fix Focus Areas
- pnpr/crates/pnpr/src/server.rs[872-966]
- pnpr/crates/pnpr/src/storage.rs[723-729]
- pnpr/crates/pnpr/src/upstream.rs[380-415]
### Suggested fix
- After `expected_tarball_dist()` resolves the declaring `version`, derive a **stable internal cache filename** (e.g. `let cache_filename = name.tarball_name_for_version(&version);`).
- Use `cache_filename` (not the request `filename`) for:
- `open_cached_tarball`, `open_cached_tarball_tmp`
- cached integrity marker read/write/remove
- Keep using the request `filename` for routing and for the upstream fetch path (`fetch_tarball_response(&name, &filename)`), so non-canonical upstream basenames are still fetched correctly.
- (Optional) If you need offline replay for the preserved-basename URL, store a small alias mapping `{requested filename} -> {cache_filename}` (or compute it from cached packument), so requests can still find the stable cache entry without duplicating bytes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

In serve_tarball, OSV is enforced against the filename-derived name_version before resolving the
authoritative declaring version from the packument; when these differ (the PR explicitly allows this
for non-canonical upstream basenames), a vulnerable name_version will reject the request even if
the resolved version is allowed. This creates incorrect FORBIDDEN responses (availability/compat
regression) for some proxied tarballs with non-canonical naming.

Code

pnpr/crates/pnpr/src/server.rs[R838-852]

+    // `name_version` is the version segment carried by the filename. It is
+    // canonical for hosted tarballs (the publish handler enforces it) and a
+    // best-effort screen here; the authoritative version a proxied tarball
+    // resolves to is the `version` matched below, which may differ for a
+    // non-canonical upstream tarball name.
+    let (filename, name_version) = match name.parse_tarball_name(filename) {
      Ok(parsed) => parsed,
      Err(err) => return error_response(&err),
  };
  if let Err(err) = authorize(state, identity, name.as_str(), Action::Access) {
      return error_response(&err);
  }
-    if let Err(err) = ensure_osv_allowed(state, &name, &version) {
+    if let Err(err) = ensure_osv_allowed(state, &name, &name_version) {
      return error_response(&err);
  }

Evidence

The code comment states name_version is only a best-effort screen and the authoritative version
may differ, but the OSV check is still enforced (and can return early) before that authoritative
version is determined.

pnpr/crates/pnpr/src/server.rs[828-885]
pnpr/crates/pnpr/src/server.rs[2651-2668]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`serve_tarball()` performs `ensure_osv_allowed(..., name_version)` before it resolves the authoritative `(version, integrity)` owner of the requested tarball from the packument. The PR explicitly documents that `name_version` may differ from the resolved `version` for non-canonical upstream tarball names, so denying early can incorrectly block an otherwise-allowed tarball.
### Issue Context
This is a behavioral regression introduced by allowing non-canonical basenames while still using the filename-derived version as an enforcement gate.
### Fix Focus Areas
- Adjust the OSV gating so a mismatch can’t cause a false-deny.
- Prefer enforcing OSV on the resolved declaring version; keep any pre-check strictly as an optimization that cannot reject if resolution would allow.
### Fix Focus Areas (code pointers)
- pnpr/crates/pnpr/src/server.rs[838-885]
### Suggested implementation sketch
- Move the initial `ensure_osv_allowed(state, &name, &name_version)` to after `expected_tarball_dist(...)` so the decision is made using the resolved `version`.
- OR: keep the early check, but if it returns an error, continue to resolve via `expected_tarball_dist` and only return FORBIDDEN if the resolved `version` is also disallowed (and ensure you still block when resolved version is vulnerable).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

rewrite_dist_tarball preserves the upstream basename verbatim, but axum percent-decodes the
requested filename path param before serve_tarball/expected_tarball_dist compare it to the
packument’s raw dist.tarball basename. If the upstream basename contains percent-encoded bytes
(e.g. %2B), the decoded request (+) won’t match the stored basename (%2B), causing proxied
tarball requests to 404 (and the upstream fetch URL to be constructed from the decoded form).

Code

pnpr/crates/pnpr/src/server.rs[R988-995]

+    let Some((version, manifest)) = versions.iter().find(|(_, manifest)| {
+        manifest
+            .get("dist")
+            .and_then(|dist| dist.get("tarball"))
+            .and_then(Value::as_str)
+            .and_then(|url| url.rsplit('/').next())
+            .is_some_and(|basename| basename == filename)
+    }) else {

Evidence

The rewrite preserves the raw trailing substring after / from the upstream tarball URL, while the
server’s routing stack percent-decodes path parameters before handler code runs; the tarball
resolver then compares the decoded request filename against the still-encoded basename extracted
from the raw packument URL, so percent-encoded basenames can never match.

pnpr/crates/pnpr/src/upstream.rs[380-399]
pnpr/crates/pnpr/src/server.rs[979-1013]
pnpr/crates/pnpr/src/server.rs[1074-1076]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tarball basenames that contain percent-encoded bytes in the upstream packument (e.g. `foo-1.0.0%2Bbuild.tgz`) can break after this PR:
- `rewrite_tarball_urls` emits proxy URLs containing the raw `%2B` basename.
- axum’s `Path` extraction percent-decodes path params before `serve_tarball` sees them.
- `expected_tarball_dist` matches the request’s (decoded) `filename` against the packument’s (still encoded) basename via plain string equality.
This can lead to `404 Not Found` for valid upstream tarballs and/or incorrect upstream request URL construction.
### Issue Context
This is introduced by switching tarball URL rewriting from canonical reconstruction to “preserve basename verbatim”. The server still treats route params as decoded strings.
### Fix Focus Areas
- pnpr/crates/pnpr/src/upstream.rs[390-399]
- pnpr/crates/pnpr/src/server.rs[979-1013]
- pnpr/crates/pnpr/src/server.rs[1074-1076]
### Suggested fix approach
- Decode the upstream `dist.tarball` basename to a normalized filename string before storing/using it for comparisons, or decode during comparison:
- In `expected_tarball_dist`, percent-decode the extracted `basename` (from packument) before comparing to `filename`.
- When constructing rewritten `dist.tarball` URLs, ensure the basename is re-encoded as a URL path segment (so the URL is valid and round-trips safely).
- When constructing the upstream fetch URL, build it with a URL builder (or path-segment encoding) so decoded characters are correctly percent-encoded for the outbound request.
The key invariant: **the same canonical representation** of the basename must be used across (1) rewritten metadata, (2) route matching, (3) packument lookup, and (4) upstream fetch URL construction.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.