install: do not resolve an exact version from an abbreviated manifest under minimum-release-age - #38659
install: do not resolve an exact version from an abbreviated manifest under minimum-release-age#38659robobun wants to merge 1 commit into
Conversation
… under minimum-release-age An install without minimum-release-age caches the abbreviated manifest, which the registry serves without publish times. A later install with minimum-release-age and an exact version in package.json resolved that version straight from the cached manifest: every version in it has a publish time of 0, so the age check passed and no request was made for the full manifest. Skip the exact-version shortcut when an age gate is set and the cached manifest is not the full one, so the install falls through to the network request that already asks for the full manifest.
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: fix pushed, waiting on CI. Reproduced on the released binary (1.4.0-canary.1) with a mock registry that, like the real one, only includes The new |
There was a problem hiding this comment.
LGTM — one-condition guard that closes an age-gate bypass in the exact-version shortcut, using the same needs_extended_manifest && !has_extended_manifest predicate already applied at PackageManifestMap.rs:186/217 and NetworkTask.rs:567.
What was reviewed
- Verified
loaded_manifestisSometwo lines above the added.unwrap()(line 1045), matching the existing.unwrap()s on 1062/1076/1108. - Traced the fall-through:
by_name_hash_allow_expiredsetsexpired = truefor an abbreviated manifest under an age gate, so the!expiredfast path at 1137 is skipped andfor_manifestat ~1170 drops conditional headers and requests the full document. - Confirmed
is_package_version_too_recent(npm.rs:1558) returns false forpublish_timestamp_ms == 0, which is why the abbreviated shortcut silently passed before. - Tests: shared
registryRequestsarray is safe because the new tests are serial and each run slices from a captured start index; each test uses an isolatedBUN_INSTALL_CACHE_DIR.
Extended reasoning...
Overview
Adds a single guard to the exact-version resolution shortcut in enqueue_dependency_with_main_and_success_fn (src/install/PackageManager/PackageManagerEnqueue.rs): when minimum-release-age is configured, the shortcut is skipped unless the cached manifest is the extended (full) variant with real publish timestamps. Five new tests in test/cli/install/minimum-release-age.test.ts cover the abbreviated-cache-then-gated matrix (install block/allow, bun add block) plus two pinning tests that the shortcut still fires with a stale full manifest. The mock registry gains a per-request log so tests can assert which manifest flavor was fetched.
Security risks
The feature under repair is itself a supply-chain guard. This change strictly tightens it — the only new behavior is refusing to resolve from a cache entry that lacks publish times and instead fetching the full manifest. There is no path where the change loosens a check. A bug here would degrade to an extra registry request, not a bypass.
Level of scrutiny
Medium. The touched function is on the package manager's hot enqueue path, but the change is a single boolean conjunction that mirrors the exact predicate used at three other sites in the same subsystem (PackageManifestMap.rs:186, PackageManifestMap.rs:217, NetworkTask.rs:567). No new state, no control-flow restructuring, no allocation. The .unwrap() on loaded_manifest is provably safe (assigned Some immediately above) and matches four existing .unwrap()s in the same block.
Other factors
- The PR description cites specific line numbers for every claim (
by_name_hash_allow_expireddemoting to expired,publish_timestamp_ms == 0for abbreviated manifests,for_manifestdropping ETag/If-Modified-Since when upgrading); I verified each against the current tree. - The fall-through path was already exercised by every non-exact specifier under an age gate, so the network-task machinery being reused is not new.
- Tests follow harness conventions:
tempDir,bunEnvspread,Promise.alldraining stdout/stderr/exited, per-test isolated cache dir viaBUN_INSTALL_CACHE_DIR,BUN_MANIFEST_CACHE=1to force the stale-shortcut path for the two pinning tests. - No CODEOWNERS entry covers
src/install/. No prior reviewer comments to address.
Problem
bun install --minimum-release-age N(orinstall.minimumReleaseAgein bunfig) installs a version younger than N when package.json pins an exact version and the package's abbreviated manifest is already in the install cache (for example from an earlierbun installwithout the setting, in any project sharing the cache).bun add pkg@x.y.ztakes the same path.enqueue_dependency_with_main_and_success_fn(src/install/PackageManager/PackageManagerEnqueue.rs:1035-1110on main):by_name_hash_allow_expiredhands back the cached manifest even when an age gate is set and the manifest is abbreviated; it only marks it expired (src/install/PackageManifestMap.rs:186and:217). That is deliberate: the manifest's ETag is needed for the network request further down.:1049) does not check which flavor it got. An abbreviated manifest haspublish_timestamp_ms == 0for every version (src/install/npm.rs:764), sois_package_version_too_recent(npm.rs:1558) is false and the pin is resolved from the cache.get_or_put_resolved_packageusesby_name_hash(PackageManagerEnqueue.rs:2424), which refuses an abbreviated manifest when an age gate is set. Only the exact-version shortcut bypasses it. The same structure existed before the Rust port, so this is not a regression.Fix
!needs_extended_manifest || manifest.pkg.has_extended_manifest. With an age gate and an abbreviated manifest, the code falls through to the network request it would have made for any non-exact specifier.NetworkTask::for_manifestasks for the full manifest and skips the conditional headers when the loaded manifest is abbreviated (src/install/NetworkTask.rs:567), so the registry returns the full document, and the re-enqueued dependency is then resolved (or rejected) by the regular path with real publish times. The condition is the same onePackageManifestMapandfor_manifestalready use to decide whether a manifest is good enough under an age gate.needs_extended_manifestis false and the shortcut behaves exactly as before. With an age gate and a full manifest in the cache (even a stale one), the shortcut is still taken and still applies its own age check; two of the new tests pin that down.minimumReleaseAgeExcludeswith an exact pin now also refetch the full manifest once, like they already did for non-exact specifiers.test/cli/install/minimum-release-age.test.ts, newexact version with a cached manifestblock (5 tests):bun installon a too-recent pin,bun installon an old enough pin,bun addon a too-recent pin) fail on the released binary: the too-recent pins install with zero registry requests, and the old enough pin resolves without the full-manifest request.BUN_MANIFEST_CACHE=1so the cached manifest counts as stale and the shortcut is the path taken (confirmed by its distinct error message).bun bd test.Background
Accept: application/vnd.npm.install-v1+jsonit returns the abbreviated one (versions and tarballs, notimefield); withAccept: application/jsonit returns the full one, includingtimewith a publish date per version. Bun asks for the full one only when a minimum release age is configured (needs_extended_manifest), and records which one it parsed inpkg.has_extended_manifest. Both are written to the same cache file, so a later process can load either flavor.pkg@1.2.3and a cached manifest (even one past its freshness window) contains that version, bun resolves it from the cache instead of re-requesting the manifest, since a published version does not change. The network request below the shortcut is what every other specifier uses when the cached manifest cannot be used.BUN_MANIFEST_CACHE=1(used by two tests): keep reading the manifest cache but treat every cached manifest as past its freshness window, which is the state the shortcut exists for.