install: compare stored scope name and folder path, not just their hash - #32745
Conversation
The scoped-registry map and the folder-resolution dedupe map are keyed by a fixed-seed non-cryptographic hash (Wyhash11 over scope names, seed-0 Wyhash over absolute package.json paths) without comparing the bytes the hash was computed from. A constructed hash collision then confuses two distinct inputs: - scope_for_package_name resolved @scopeA/pkg through any scope whose name hashed to the same value, so a request could be sent to a different scope's registry carrying that scope's token. The lookup now compares the stored scope name and falls back to the default registry on a mismatch. - folder resolution keyed only on the path hash, so two local file: dependencies whose absolute package.json paths collided shared one resolution and the second resolved to the first package's contents. The map value now carries the absolute path; the lookup compares it and resolves fresh on a collision instead of reusing the entry. This mirrors #31218, which applied the same store-the-bytes-and-compare invariant to trustedDependencies.
|
Updated 3:06 AM PT - Jun 26th, 2026
❌ @autofix-ci[bot], your commit 12ad7cb has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32745That installs a local version of the PR into your bun-32745 --bun |
|
Warning Review limit reached
More reviews will be available in 30 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs and the fix follows the #31218 pattern cleanly, but this is a security fix touching auth-token routing and package-identity resolution in the install core, so it warrants a human sign-off (also note the build-rust CI failures on f66a067).
Extended reasoning...
Overview
This PR hardens two install-path lookups against constructed Wyhash collisions: Options::scope_for_package_name now compares the stored scope.name bytes (not just the hash) before returning a scoped registry, and folder_resolver::get_or_put now stores the normalized absolute package.json path alongside each FolderResolution and compares it on lookup. The FolderResolutionMap value type changes from FolderResolution to a new Entry { abs_path, resolution }, with the one direct .put in PackageManager::init updated accordingly. Tests add a runtime Wyhash-collision constructor and two regression tests (scoped-registry token routing, file: dep identity).
Security risks
The change is itself a security fix for (1) a scoped-registry auth-token misrouting — a colliding scope name could cause one scope's request to be sent to another scope's registry carrying that registry's bearer token — and (2) local package confusion between two file: deps whose absolute paths collide. The fix is strictly defensive (extra byte-equality check, fall back to default registry / resolve-fresh on mismatch) and I don't see it introducing new attack surface. Behavior is unchanged in the no-collision case.
Level of scrutiny
High. This is production-critical install-path code that decides which registry receives credentials and which on-disk package is materialized for a dependency. Even though the diff is small and mechanical, the consequences of a subtle mistake here (e.g., scope-name normalization mismatch causing legitimate scoped registries to silently fall back to the default) are user-facing and security-relevant. A human reviewer who knows the Scope::get_name / Scope::name normalization invariants should confirm the byte comparison can't false-negative on a legitimately configured scope.
Other factors
- robobun reports build-rust failures across all platforms on f66a067; an autofix commit (12ad7cb) landed afterward but the CI status comment hasn't been updated to reflect it, so build health is currently unconfirmed.
- The PR explicitly leaves related hash-as-identity sites (
Task::Id,PackageNameHash) for follow-up — reasonable scoping, but worth a maintainer ack. - No CODEOWNERS cover
src/install/, and there are no prior human reviews on the thread. - Good test coverage: both new tests are constructed to fail pre-fix and self-verify their collision against
Bun.hash.wyhash.
|
Thanks for the review. Addressing the two points: The scope-name comparison can't false-negative on a legitimately configured scope. The change only adds a byte-equality check after the existing hash lookup, so it can only alter behavior when the hash matches but the names differ (the collision case). For any scope that resolved correctly before, the looked-up entry's name already equals the queried scope, so the check passes. Concretely, the stored
So Build health is confirmed. The Agreed this warrants a human sign-off given it touches credential routing and package identity. |
2 participants
Fixes two install-path sites from #32741 where a fixed-seed non-cryptographic hash is used as an identity key with no comparison of the bytes it was computed from, so a constructed hash collision confuses two distinct inputs. Both follow the store-the-bytes-and-compare invariant established by #31218 for
trustedDependencies.The headline of the report (trustedDependencies lifecycle-script RCE) is already fixed on main by #31218 and is not touched here.
1. Scoped-registry token leak
The registry map is keyed by
Scope::hash(scope_name)(Wyhash11) andscope_for_package_namereturned whatever scope sat in that slot without checking the name. Two scope names that collide overwrite each other, so a request for one scope was routed to the other scope's registry with the other scope's token.Before this change, installing
@scopeA/probesent the request to registry B withAuthorization: Bearer scope-B-SECRET-token.Fix: compare the stored
scope.nameagainst the requested scope, and fall back to the default registry on a mismatch.2. Folder-resolution package confusion
FolderResolution::get_or_putkeyed its dedupe map onhash(absolute_package_json_path)(seed-0 Wyhash) and stored only a package id. Two localfile:dependencies whose absolutepackage.jsonpaths collide shared one entry, so the second dependency resolved to the first package's contents:Fix: store the absolute path alongside the resolution and compare it on lookup; on a hash collision, resolve fresh without evicting the existing entry.
Scope
The same "hash as identity, no byte comparison" pattern exists in a few other install maps from the report (remote tarball URL dedup via
Task::Id,PackageNameHash). Those are lower severity (they need the attacker to control both colliding remote URLs, or they fail loudly with a duplicate-dependency error) and touch a shared identity type, so they are left for a focused follow-up rather than widening this change.Verification
Both tests fail on the unfixed build and pass with the fix.
test/cli/install/npmrc.test.tsstands up two in-process registries, configures the colliding scopes, and asserts the colliding scope's registry never receives the request or its token.test/cli/install/bun-install.test.tsconstructs the path collision at runtime (self-verified againstBun.hash.wyhash, which is the exact function keying the map) and asserts eachfile:dependency keeps its own identity. Gated to POSIX.