install: bound isolated store entry names; tarball URL credentials; file: tarballs relative to their folder package - #38867
install: bound isolated store entry names; tarball URL credentials; file: tarballs relative to their folder package#38867robobun wants to merge 17 commits into
Conversation
With the isolated linker, a store entry is named <name>@<resolution>, and for folder, tarball, git and github dependencies the resolution embeds the whole path or URL. On Windows the entry's absolute path then passes MAX_PATH, which CreateProcess rejects as the cwd of the package's lifecycle scripts (bun install fails with "Failed to run script ... due to error ENOENT"), and a resolution longer than NAME_MAX cannot be created on any platform. Write at most 80 bytes of the resolution: a longer one is cut to 63 bytes and suffixed with + and the 16 hex digit wyhash of the full text. Every consumer of the name (project store, global store, dependency symlinks, entry hash, bun pm licenses, bun pm prune) formats it through StoreKeyFormatter, so they all agree on the cut name. Names of 80 bytes or less are unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesThe isolated-install store now bounds long resolution names to 80 bytes, preserves UTF-8 boundaries, and appends a hash. Store-key paths and installation tests cover long URLs, peer hashes, symlinks, and lifecycle scripts. Documentation describes the format and Windows limitation. Isolated-install store names
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced, fix and tests pushed, CI green on 474da9d, #37470 closed in favor of this PR; needs a maintainer to decide on the shape and the constant.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces a design decision — a fixed 80-byte cap with a hash-suffix scheme for isolated store directory names — that changes persistent on-disk layout, is documented as a public contract, and supersedes an alternative approach open in #37470, a human sign-off on the chosen shape and constant would still be worthwhile.
What was reviewed:
- Verified
Wyhash::update/final_is chunk-invariant (src/wyhash/lib.rstest_iterative_chunked_matches_oneshot), so hashing viaResolutionSink::write_strmatchesBun.hash(text)regardless of how the formatter splits its writes. - Traced the
write_strtruncation:buf[..sink.len]in the ≤80 branch is never mid-chunk, and the cut branch backscutto a char boundary beforestr_utf8, so nofmt::Erroron valid input. - Checked that
entryStoreNameandgitExecutableused by the new tests already exist in the file, and that the tests follow harness conventions (Buffer.alloc,port: 0, boundary at exactly 80/81 bytes).
Extended reasoning...
Overview
The PR bounds the resolution portion of isolated-linker store entry names at 80 bytes: longer resolutions are truncated to 63 bytes (backed to a UTF-8 char boundary) and suffixed with +<16-hex wyhash>. It touches src/install/isolated_install/Store.rs (the StoreKeyFormatter Display impl plus a new ResolutionSink/write_resolution helper), adds a long store entry names test suite to test/cli/install/isolated-install.test.ts, and documents the naming rule in docs/pm/isolated-installs.mdx.
Security risks
None identified. The change is confined to formatting a directory name derived from the lockfile's resolution string; no new untrusted input is parsed, and the hash is non-cryptographic wyhash used purely for disambiguation between long resolutions of the same package. It does not affect integrity checks, tarball extraction, or path validation.
Level of scrutiny
High. This alters the on-disk naming convention of the isolated store, which is persistent state consumed by bun install (re-detection of existing entries), bun pm prune (split_store_key), bun pm licenses (BunStore::lookup), and the global virtual store. The 80-byte constant is a tunable with a documented derivation, and the PR explicitly overlaps with the alternative in #37470 (200-byte cap on the whole key). A maintainer should confirm the chosen constant and scheme before it becomes a documented public contract.
Other factors
I confirmed the streaming-hash correctness concern is not an issue: bun_wyhash::Wyhash buffers to 48-byte rounds and its unit tests (test_iterative_chunked_matches_oneshot, test_iterative_maintains_last_sixteen) prove init(0).update(a).update(b).final_() == hash(0, a++b), so the per-write_str hasher.update calls in ResolutionSink produce the same value as Bun.hash(full_text) regardless of how fmt::write chunks the formatter output. I also checked that the ≤80-byte fast path can never observe a truncated chunk in buf (truncation implies sink.len > 80), and that the char-boundary loop guarantees str_utf8 succeeds on the cut prefix. The new tests cover the exact boundary, hash disambiguation of a shared prefix, multi-byte cut, peer-hash suffix ordering, global store link naming, and the reported git lifecycle-script case; entryStoreName and gitExecutable are pre-existing helpers in the file. Given the design decision and the overlap with #37470, deferring rather than approving.
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit cd82495 has some failures in 🧪 To try this PR locally: bunx bun-pr 38867That installs a local version of the PR into your bun-38867 --bun |
|
Updated 12:41 AM PT - Aug 16th, 2026
✅ @Jarred-Sumner, your commit 5fa0679ecba795e45f9b200008eb346df68a27e6 passed in 🧪 To try this PR locally: bunx bun-pr 38867That installs a local version of the PR into your bun-38867 --bun |
There was a problem hiding this comment.
Thanks for addressing the gitEnv nit — the git test now isolates bun's internal clone too. I re-reviewed and found no bugs. Because this changes the on-disk store entry naming scheme (a design decision with a tunable constant) and is positioned as superseding #37470's alternative approach, a maintainer should sign off on the 80-byte bound and the hash-suffix shape.
What was reviewed
write_resolution/ResolutionSink: buffer fill can't split a char in the ≤80 path (all bytes fit); the >80 path backs up to a UTF-8 boundary andcutcan't underflow (is_on_char_boundaryis true at 0).- Confirmed all store-name consumers route through
StoreKeyFormatterper the description;prune.rssplits at@so the cut-inside-resolution shape doesn't break it. - Tests cover the exact-limit / one-past / shared-prefix / multi-byte-boundary / peer-hash / global-store / git-lifecycle cases and compute expected names via
Bun.hash.
Extended reasoning...
Overview
The PR bounds the resolution portion of isolated-install store entry names to 80 bytes: resolutions ≤80 bytes are written verbatim (so registry versions and github+owner+repo+<sha> are unchanged); longer ones are cut to ≤63 bytes at a UTF-8 char boundary and suffixed with +<16-hex wyhash>. This fixes Windows CreateProcessW MAX_PATH failures for lifecycle-script cwds and cross-platform ENAMETOOLONG for very long tarball/folder/git resolutions. Changes: src/install/isolated_install/Store.rs (new ResolutionSink fmt::Write sink + write_resolution, wired into StoreKeyFormatter's Folder and catch-all arms), a 6-test describe block in test/cli/install/isolated-install.test.ts, and a paragraph in docs/pm/isolated-installs.mdx.
Security risks
None identified. The change shortens filesystem path components; no new external input is parsed. The wyhash is used for disambiguation, not security. Truncation preserves valid UTF-8 (verified is_on_char_boundary and str_utf8 in src/bun_core/string/immutable.rs), so no invalid-UTF-8 path is written.
Level of scrutiny
Medium-high. The mechanism is small and self-contained (one fmt::Write sink and one helper), but it changes the on-disk naming scheme for a subset of store entries — a format decision that every consumer of store entry names (installer paths, global-store links, bun pm prune, bun pm licenses, entry-hash seeding) depends on. The description enumerates those consumers and argues each still works, and names ≤80 bytes are byte-identical so existing installs aren't re-linked. That reasoning looks correct, but the choice of 80 and the hash-suffix shape are tunable design calls (the description compares against pnpm's 120-byte whole-name default and against #37470's 200-byte whole-name approach). Per the review guidelines, design decisions of this kind warrant a maintainer's sign-off rather than auto-approval.
Other factors
- My earlier nit (pass
gitEnvtorunBunInstallso bun's internal git clone is isolated from runner config) was addressed in 39bc380. - The comment-cop bot has repeatedly flagged the doc comment on
MAX_RESOLUTION_LEN; after the two shortening commits it's now a 5-line doc comment explaining a magic number and its Windows constraint, which reads as necessary context rather than a workaround justification — likely a heuristic false positive, but a maintainer can judge. - Test coverage is thorough: exact-limit vs one-past, two long resolutions sharing a prefix (hash keeps them distinct), peer-hash suffix after the cut name, multi-byte char at the cut point, tarball >NAME_MAX with global store enabled, and the reported git+file:// lifecycle-script case. Tests derive expected names via
Bun.hash(matchingWyhash::init(0)). - This PR explicitly supersedes #37470 if it lands and interacts with #36973 and #38810 — coordination another reason for a human look.
…t declares them A local tarball dependency declared in the package.json of a file: folder dependency was read relative to the project directory instead of the folder, so the install failed with ENOENT, or installed a same-named tarball from the project directory when one existed. Only workspace declarers had their directory joined in. enqueue_local_tarball now asks local_tarball_base_dir for the base: the directory of the declaring workspace or file: folder package when that package's own specifier is the tarball path, and the top-level dir otherwise. The second case covers paths substituted from the root package.json (overrides, resolutions, catalogs), which were previously read relative to the workspace when applied to a workspace dependency. Both the resolve pass and the install from bun.lock go through the same function, so they read the same file.
…ation A dependency declared as http://user:pass@host/pkg.tgz (or a registry dist.tarball URL of that shape) was requested without any Authorization header. NetworkTask::for_tarball now splits the userinfo off the request URL and sends it as Authorization: Basic base64(user:pass), the header npm sends for such URLs. Credentials configured for the registry scope still take precedence when they apply to the tarball. The request URL no longer carries the userinfo, so the HTTP client's same-origin check keeps the header across a redirect within the host and error output prints the URL without the credentials.
There was a problem hiding this comment.
I re-reviewed after the gitEnv nit was applied (39bc380) and the two later commits — no bugs found. Deferring to a maintainer because this picks the on-disk store-name format (80-byte resolution bound, wyhash-suffix truncation) and would supersede #37470's competing 200-byte approach; that's a design call the author also flagged for sign-off.
Checked: ResolutionSink::write_str handles writes past the buffer via get_mut(len..) returning None; the char-boundary loop terminates (index 0 is always a boundary) and the buffer holds ≥80 bytes when it runs; prune.rs split_store_key/store_link_target split on the first @, so the cut +<hash> suffix stays on the resolution side; the wyhash chunk-invariance test the description cites exists.
Extended reasoning...
Overview
Bounds the resolution part of isolated-install store entry names (<name>@<resolution>) to 80 bytes. Resolutions longer than that are cut to ≤63 bytes (backed up to a UTF-8 char boundary) and suffixed with +<16-hex wyhash>. Implemented as a ResolutionSink: fmt::Write that captures the first 80 bytes plus a running hash, wired into StoreKeyFormatter::fmt for the Folder and catch-all arms. Six new tests in isolated-install.test.ts cover the exact-limit boundary, hash-distinguished shared prefixes, peer-hash suffix ordering, multi-byte char boundary backup, a >NAME_MAX local tarball/folder, a >NAME_MAX tarball URL with the global store, and the reported git+lifecycle case. One docs paragraph added.
Security risks
None identified. The change shortens filesystem path components; no untrusted input reaches new parsing or allocation. The hash is wyhash (non-cryptographic) but is used only for uniqueness within a single user's store, not integrity.
Level of scrutiny
Medium-high. The implementation is small (~50 lines of Rust) and mechanically sound, but it establishes a persistent on-disk naming convention that bun pm prune, bun pm licenses, and the global store all read, and that #36973 will document. The 80-byte constant is a tunable with tradeoffs the PR description derives in detail (MAX_PATH headroom vs keeping github+owner+repo+<sha> verbatim), and #37470 proposes a different bound (200 bytes on the whole name) for the NAME_MAX-only case. The author's own status comment says a maintainer should sign off on the shape and the constant.
Other factors
- My earlier nit (pass
gitEnvtorunBunInstallin the git test) was applied in 39bc380; the comment-cop findings on the constant's doc comment were addressed by shortening it across e1ac7d0/3affb5c. All threads are resolved. - I verified the consumers named in the description:
prune.rssplit_store_keysplits at the first@after byte 0 andstore_link_targetonly checks for@in the component, so the cut format parses unchanged;bun_core::strings::is_on_char_boundaryandbun_core::str_utf8exist with the expected semantics;src/wyhash/lib.rshastest_iterative_chunked_matches_oneshotbacking the chunk-invariance claim that lets the tests useBun.hash. - Test coverage is thorough and follows harness conventions (
tempDirviacreateTestDir, localBun.serve({port:0})for the tarball URL,test.skipIf(!gitExecutable), drains all pipes,Buffer.alloc(n, fill)for long strings). CI on the latest commit (474da9d) is still building.
|
#37470 (the 200 byte whole-name variant of this) is closed in favor of this PR: its repro installs with this branch, and the one case of its tests not covered here, a local tarball and a folder three 85 byte directories deep (unbounded entry names of 277 bytes, the Two observations from comparing the two, for whoever settles the constant (the description is updated to say the same):
|
) ### Problem - With `--linker isolated`, a dependency declared as a tarball URL with credentials, e.g. `"direct": "http://carol:s3cret@127.0.0.1:PORT/cdn/direct-1.0.0.tgz?token=npm_a1b2..."`, is installed into a store directory literally named `node_modules/.bun/no-deps@http+++carol+s3cret@127.0.0.1+PORT+cdn+direct-1.0.0.tgz+token=npm_a1b2...` (reproduced on 1.4.0-canary.1 and current main). A git dependency such as `git+https://carol:s3cret@host/org/repo.git` gets `repo@git+https+++carol+s3cret@host+org+repo.git+<commit>`. - That directory name is part of the realpath of every file in the package, so the password and the token show up in stack traces, `import.meta.url`, `bun pm licenses` paths, the `--verbose` link failure messages, and `ls node_modules/.bun`. The output redaction in #38977 cannot cover this: these paths really exist on disk. - Cause: the entry name is `<name>@<resolution store path>` (`src/install/isolated_install/Store.rs` `StoreKeyFormatter`). For a remote tarball the store path was the whole URL with `/ \ : # ?` turned into `+` (`src/install/resolution.rs` `StorePathFormatter`, via `bun_semver`'s `String::fmt_store_path` in `src/semver/lib.rs`), and for git it was the whole repository URL plus the commit (`src/install/repository.rs` `StorePathFormatter`). Nothing removed the userinfo or the query string. ### Fix - `src/install/resolution.rs`: new `fmt_store_url`, used for the remote tarball store path and, in `src/install/repository.rs`, for the repository part of the git store path. It writes the URL without its userinfo and without its query string, and when either of them was present it appends `+<16 hex wyhash of the complete URL>`. The git commit suffix is unchanged and still follows. Examples: `http://carol:s3cret@h/p.tgz?token=x` becomes `no-deps@http+++h+p.tgz+<url hash>`; the same URL without credentials stays `no-deps@http+++h+p.tgz`; a credentialed git URL becomes `repo@git+https+++host+org+repo.git+<url hash>+<commit>`. - `src/semver/lib.rs`: `String::fmt_store_path` now delegates to a byte-slice `fmt_store_path`, so the two kept pieces of the URL are spelled exactly the way the whole URL was before (one character mapping, no copy of it). - Why this is correct: - The userinfo and the query string are the two places a URL carries credentials (`user:password@`, a bare token as the username, `?token=` / signed URLs); host, path and scheme are not secret, so they stay readable. The userinfo is delimited with RFC 3986's rule, the same one `find_url_password` in `bun_core` uses: the authority runs from `scheme://` (or from the start of an scp-like `user@host:path`) to the first `/`, `?` or `#`, and the userinfo is everything in it up to the last `@`. The whole userinfo goes, not only the password, because `https://TOKEN@host/...` is a documented way of passing tokens. `bun_url::URL::parse` was not used for this because it does not recognize the userinfo of `user@host:port` at all. - The name stays a function of the resolution alone, so a second install (whose resolution comes from the lockfile, which still stores the full URL) derives the same name and finds the same entry; the tests check this. - Whenever something is removed, the hash of the complete URL is appended, so two resolutions that used to get different names still get different names: `pkg.tgz?v=1` and `pkg.tgz?v=2` are different packages and must not share a directory, and even for git, where the commit usually disambiguates, `resolved` can be empty for packages migrated from pnpm/yarn lockfiles. Without either part there is nothing to disambiguate, so no hash is appended and every existing entry for a plain tarball, `git+file://`, `github:` or registry package keeps its name byte for byte. - Entries whose URL has a userinfo or a query string are renamed once by this change (that includes the common `git+ssh://git@host/...` form, whose `git@` is not a secret but cannot be told apart from a token); the next `bun install` links them under the new name and `bun pm prune` removes the old directories, since it removes every store entry the lockfile does not produce. The lockfile itself is untouched. - The hash sits inside the resolution part, so the consumers that read names back keep working: `bun pm prune` splits at `@` (`src/install/prune.rs` `split_store_key`; the names now also contain no second `@`), `bun pm licenses` re-derives the name through the same formatter (`src/runtime/cli/pm_licenses_command.rs`), and the global store derives its `links/<name>-<entry hash>` directory from the same name, so it is fixed as well. #38867 (bounding long names) would apply on top of this name. - Verification, `test/cli/install/isolated-install.test.ts`, describe `store entry names of URL dependencies`: - tarball dependencies with a plain URL (name unchanged), a password, a token in the query string, and both plus a fragment: exact entry name computed with `Bun.hash`, the `node_modules` link points into it, `bun.lock` still contains the full URL, the package imports at runtime, and a second install keeps the name - two tarball URLs differing only in `?v=` get two entries and each alias resolves to its own version - with `install.globalStore`, the `links/` directory name is built from the credential-free name - git dependencies served over git's dumb HTTP protocol from a local bare repository: plain URL (name unchanged), password, and a token as the username, each with the exact name including the commit, the lockfile resolution, and a second install - the username-only tarball form (`http://token@host:port/x.tgz`) is exercised through the git cases only: the tarball downloader currently sends that URL with the userinfo still in the Host header and gets a 400, which is tracked separately (as is the fact that the downloader never sends URL credentials at all); neither affects this change - the existing #36987 test in the same file asserted the old `+x=y` name and now asserts the hashed one; its point (no literal `?` in the name, package resolves at runtime) is unchanged - On the released build, 8 of these 10 tests fail with the old names (the two plain URL cases pass by design); all pass with `bun bd test`. Also run with the change: the rest of `isolated-install.test.ts` (75 pass), `bun-pm-licenses.test.ts` (79 pass, it asserts the unchanged name of a plain tarball entry), `bun-prune.test.ts` (109 pass), `bun-install-git-deps.test.ts` (7 pass), `cargo clippy` and `cargo fmt --check` on `bun_install` and `bun_semver`, and `test/internal/source-lints`. ### Background - Isolated linker: every package is materialized once under `node_modules/.bun/<entry>/node_modules/<name>` and everything that depends on it gets a symlink to that directory. `<entry>` is `<package name>@<store path of the resolution>`, optionally followed by `+<peer hash>`; with `install.globalStore` the entry is itself a symlink into `<cache>/links/<entry>-<entry hash>`. - Resolution: the lockfile's record of where a package came from. For registry packages it is the version, which is why their entries read `name@1.2.3`; for tarball and git dependencies it is the URL as written in package.json (git: plus the resolved commit), which is what became the directory name here. - Store path: the spelling of a resolution as one path component, done by replacing `/`, `\`, `:`, `#` and `?` with `+` (`bun_semver`'s `StorePathFormatter`); `http://h/p.tgz` reads `http+++h+p.tgz`. - Userinfo: the `user:password@` part of a URL's authority (`scheme://userinfo@host:port/path?query#fragment`). - wyhash: bun's default 64-bit hash (`bun_wyhash::hash`, what `Bun.hash()` computes), which is how the tests compute the expected names; the store already uses it for the peer hash suffix. <details> <summary>Reproduction on the released build</summary> `package.json` with `{"dependencies": {"direct": "http://carol:s3cret@127.0.0.1:PORT/cdn/direct-1.0.0.tgz?token=npm_a1b2c3d4e5f6"}}`, a `Bun.serve` answering that path with a tarball, `bunfig.toml` with `install.linker = "isolated"`: ``` $ bun install # 1.4.0-canary.1 + direct@http://carol:s3cret@127.0.0.1:35767/cdn/direct-1.0.0.tgz?token=npm_a1b2c3d4e5f6 $ ls node_modules/.bun no-deps@http+++carol+s3cret@127.0.0.1+35767+cdn+direct-1.0.0.tgz+token=npm_a1b2c3d4e5f6 node_modules ``` With this change the entry is `no-deps@http+++127.0.0.1+35767+cdn+direct-1.0.0.tgz+<16 hex>`. The cache folder for the same tarball was already credential-free (`@T@<hash>`). </details>
…tials' into farm/31a216b6/isolated-store-name-length
…arball-base' into farm/31a216b6/isolated-store-name-length
There was a problem hiding this comment.
I reviewed the expanded PR (now bundling the tarball-URL credential and folder-relative file: tarball fixes on top of the original store-name bound) and didn't find bugs. A maintainer should still look at this one: the author has flagged the 80-byte MAX_RESOLUTION_LEN constant and the cut-vs-threshold shape as needing a maintainer decision, and the new for_tarball credential path (userinfo → Authorization: Basic, precedence vs. registry credentials, redirect handling) is security-sensitive.
What was reviewed:
write_resolution/ResolutionSink: UTF-8 boundary backup, the ≤80 verbatim / >80 hash-suffix split, and that every store-name consumer goes throughStoreKeyFormatter.split_url_userinfo/basic_authorization_from_userinfo: authority-only@detection (scoped-path@excluded), empty-userinfo handling, and the count/append two-passHeaderBuilderstaying balanced.local_tarball_base_dir: thedeclared_by_parentguard so overrides/catalogs stay root-relative, and thatget_parent_pkg_of_dependencyreturningNonefalls back to the top-level dir.
Also: the comment-cop bot's four open flags on NetworkTask.rs look like the same false positive that was already answered for Store.rs (why-comments on constants/behavior, not workaround justifications).
Extended reasoning...
Overview
This PR bundles three independent install fixes across 10 files: (1) bounding the resolution part of isolated store entry names to 80 bytes with a wyhash suffix (Store.rs, docs, ~300 lines of new tests in isolated-install.test.ts), (2) sending tarball-URL userinfo as Authorization: Basic and requesting the URL without it (NetworkTask.rs, ~220 lines of new tests in bun-install.test.ts), and (3) resolving file: tarball paths relative to the file: folder package that declares them (PackageManagerEnqueue.rs, new get_parent_pkg_of_dependency in lockfile.rs, tests in bun-install.test.ts and bun-workspaces.test.ts). Since my last comment (2026-08-15, a nit on gitEnv that was applied), fixes 2 and 3 were merged in from #39025 and #39017, so the PR is substantially larger than when I last looked at it.
Security risks
Fix 2 is security-relevant: it derives an Authorization header from user-controlled URL userinfo, decides precedence vs. configured registry credentials, and depends on the HTTP client's origin comparison for whether the header follows redirects. The description documents the npm-compat rationale and the tests cover same-host redirect (keeps header), cross-host redirect (drops header), scoped-path @ not treated as userinfo, and registry-credential precedence. I didn't find a bypass, but this is exactly the class of code (auth, credentials on the wire) the review guidelines say a human should sign off on. Fix 1 changes on-disk directory names on upgrade (git entries over 80 bytes get re-created), which is a one-time compat effect the description calls out. Fix 3 changes which file a relative path resolves to; the declared_by_parent guard keeps overrides/catalogs root-relative and both are covered by decoy-tarball tests.
Level of scrutiny
High. The author's own status comment says "needs a maintainer to decide on the shape and the constant" for the 80-byte bound, and a follow-up comment lays out two open trade-offs (unbounded name part; most git+https entries over 80 bytes get renamed on upgrade). That is an explicit design decision left for a maintainer. The credential handling is new auth-header logic. The new get_parent_pkg_of_dependency is an O(packages) scan per local-tarball enqueue — likely fine given local tarballs are rare, but it is new lockfile surface. Three independent fixes in one PR also means a maintainer should confirm they want them landed together.
Other factors
CI is green on the latest commit (Build #98588). All three fixes have thorough tests (boundary at exactly 80 bytes, one past, shared-prefix collision, multi-byte cut, deep directory, global store, git postinstall; a 12-case credential table including redirects and precedence; decoy tarballs at the wrong-base path for both linkers, plus override/catalog guards). My earlier gitEnv nit was applied. The four unresolved comment-cop flags on NetworkTask.rs are the same automated heuristic that fired on Store.rs and was answered there; the flagged comments are why-comments (npm-compat citation, redirect-origin rationale, precedence rule), not workaround justifications.
This PR bundles three independent install fixes (each was reviewed on its own PR; folded here so they land together):
file:tarballs relative to thefile:folder package that declares them (from install: read file: tarballs relative to the file: folder package that declares them #39017)Rebased on main after #39014: the isolated-store test file now computes expected git entry names through the same resolution cut (
storeEntryName), since agit+http+++host+port+repo.git+<url hash>+<sha>resolution passes 80 bytes.1. install: bound the resolution part of isolated store entry names
Problem
linker = "isolated", a trusted git dependency with any lifecycle script makesbun installfail on Windows witherror: Failed to run script prepare due to error ENOENT(exit 1). The same project installs fine with the hoisted linker. This is the Windows failure of the isolated case in install: install a git dependency's devDependencies before running its prepare scripts #38810's test, which that PR currently skips on Windows.<name>@<resolution>(StoreKeyFormatter,src/install/isolated_install/Store.rs). For git, github, tarball and folder dependencies the resolution is the whole URL or path with separators turned into+(src/install/repository.rsStorePathFormatter,src/install/resolution.rsStorePathFormatter), plus the commit for git: a git dependency checked out from a temp directory gets an entry likedep@git+file++++C++Users+AZUREU~1+AppData+Local+Temp+lc-repro+dep-repo+3c6955c70dbe1ff186a126c93c5e77c3ae7bd6b4(111 bytes here, 170+ in install: install a git dependency's devDependencies before running its prepare scripts #38810's test), and the name grows with the repository path.<project>\node_modules\.bun\<entry>\node_modules\<name>, is the cwd of the package's lifecycle scripts (Scripts::get_list->lifecycle_script_runner.rsSpawnOptions.cwd). bun's own file operations use long-path-capable NT paths, so the entry is created and linked, butCreateProcessWdoes not accept a current directory longer thanMAX_PATH, and the spawn (libuv'suv_spawnon Windows) reports that as ENOENT. Measured on Windows x64 with the released1.4.0-canary.1: a package directory of 238 characters runs the script, one of 268 characters fails as above.NAME_MAX: a tarball URL or folder path of 250+ bytes makes the install fail withENAMETOOLONG: File name too long: failed to link package(the case of install: keep isolated store entry names within NAME_MAX #37470).Fix
StoreKeyFormatternow writes the resolution throughwrite_resolution: a resolution of at most 80 bytes (MAX_RESOLUTION_LEN) is written unchanged; a longer one is written as its first 63 bytes (backed up to a UTF-8 character boundary) followed by+and the 16 hex digit wyhash of the full text (seed 0, soBun.hash(text)reproduces it), which is 80 bytes again. Thename@prefix and the+<peer hash>suffix are unchanged. A cut git entry looks likedep@git+file++++C++Users+AZUREU~1+AppData+Local+Temp+lc-repro-aaaaa+ef7ad81a431ad661(the 268 character case from the measurement above; its package directory is now 206 characters).Displayimpl: the project store directory and the dependency symlinks into it (Installer.rsappend_store_path/append_store_node_modules_path/link_to_hidden_node_modules), the lifecycle script cwd (sameappend_store_path), the global store directory name and the entry hash seeded from the name (Installer.rsappend_global_store_entry_path,isolated_install.rsentry hash),bun pm prune's set of expected directories (prune.rspush_store_entry_names) andbun pm licenses's lookup (pm_licenses_command.rsBunStore::lookup, viafmt_store_key). They all see the same cut name; nothing persists or parses the old one (the lockfile stores resolutions, and an existing entry is detected by re-deriving its name).Wyhash's streaming form is chunk-invariant (test_iterative_chunked_matches_oneshotinsrc/wyhash/lib.rs), which is what lets the tests compute the expected names withBun.hash.prune.rs(split_store_key,store_has_entries,store_link_target, which split the name at@) andpm licenses(which matches<key>or<key>+<peer hash>) keep working without changes.github:shorthands (github+owner+repo+<sha>is about 60 to 75 bytes) are unaffected on upgrade. Most full git URLs are over 80 bytes (git+https+++github.com+is 23 bytes and the commit adds 41), so those entries are re-created once under the cut name on the first install after upgrading, and the old directories stay untilbun pm prune, as for any re-resolved entry. The cut keeps the URL itself readable:git-pkg@git+file++++tmp+licenses-git-repo_eXITMt+491eb29762d8d19b570b43+f87e35b31f207dd4.<project> + 34 + 2 * <name> + <resolution>(+17 with peers) characters, so with 80 it fitsMAX_PATHwhenever<project> + 2 * <name>is at most 145 (128 with peers); install: install a git dependency's devDependencies before running its prepare scripts #38810's case (61 character temp dir, 25 character name) ends up at about 225 instead of about 290. It also makes the entry directory name fitNAME_MAXtogether with every suffix the installers append (+<peer hash>,-<entry hash>,.tmp-<hex>) for names up to 119 bytes, and with the peer suffix alone for names up to 157 bytes. For comparison, pnpm'svirtual-store-dir-max-lengthdefaults to 120 bytes for the whole directory name; 80 bytes of resolution plus a typical name lands in the same range.name@resolution, for theNAME_MAXcase only) is closed in favor of this PR: that limit does not help theMAX_PATHcase (the failing names above are 130 to 180 bytes), and a cut throughname@would no longer work withbun pm prune/pm licenses(see theprune.rsbullet above). Its repro installs with this branch and its deep-directory case is carried over below. What it covered and this PR does not is the name part: a name over 119 bytes (157 in the project store) can still produce an entry overNAME_MAXonce its resolution is cut; if that should be covered too, it would be the same sink applied to the name separately, keeping the@. install: document and pin the isolated linker's store layout #36973 (documenting the layout) would need a sentence about this rule; this PR adds a short one todocs/pm/isolated-installs.mdx. install: install a git dependency's devDependencies before running its prepare scripts #38810's Windows skip of its isolated case can be removed once both land. install: leave URL credentials out of isolated store entry names #39014 changes the same text (it removes URL credentials, and the hash is of the text as formatted), so landing it in the same release as this avoids renaming those entries twice.long store entry namesintest/cli/install/isolated-install.test.ts:Bun.hash; two resolutions of one package that only differ after the cut point get separate entries that each resolve to their own package; a second install finds the same entries+<peer hash>appended after the cut name and links the peera formatting trait implementation returned an error; the test only asserts the shape because the spelling of non-ASCII bytes in these names is install: preserve non-ASCII UTF-8 bytes in isolated store path names #32304's subject).bun/node_modulesfallback link point at the cut names, and a second install finds the same entriesinstall.globalStoreenabled: the local entry has the cut name, links tolinks/<cut name>-<entry hash>, and the package imports at runtimegit+file://dependency whose repository directory name alone is 60 characters runs its postinstall script with the isolated linker; the entry has the cut nameStore.rschange the first three tests and the git one fail on the entry name (on Windows the git one fails witherror: Failed to run script postinstall due to error ENOENT, the reported message) and the deep-directory and tarball ones exit 1 withENAMETOOLONG; all of them pass withbun bd teston Linux (the five original ones also on a local Windows x64 build; the deep-directory one on the Windows lanes in CI). install: keep isolated store entry names within NAME_MAX #37470's repro from its description (./x/../x130 tarball spec) installs with this branch with an 84 byte entry.isolated-install.test.ts,bun-pm-licenses.test.ts(itsgit dependency is listed (isolated)case now goes through a cut name on every platform, since the repo lives in a temp dir),bun-prune.test.ts,public-hoist-pattern.test.ts,isolated-relink.test.ts,bun-install-native-binlink.test.ts,config-version.test.ts, and the PowerShell reproduction from the report on Windows x64 (package directory of 268 characters before, 206 after, script runs).Background
node_modules/.bun/<entry>/node_modules/<name>and everything else symlinks to it.<entry>is<name>@<resolution>, plus+<16 hex>when the package was installed for a specific set of peer dependencies. Withinstall.globalStore,node_modules/.bun/<entry>is itself a symlink to<cache>/links/<entry>-<16 hex entry hash>.MAX_PATH: Windows' 260 character limit for paths passed to Win32 APIs. Applications can opt out of it for file operations (bun does, by using\\?\/ NT paths), but the current directory handed toCreateProcessis still limited to it, and libuv maps the resulting Win32 error to ENOENT.NAME_MAX: the 255 byte limit on one path component on Linux and macOS (255 UTF-16 units on NTFS); exceeding it fails withENAMETOOLONGregardless of how long the whole path is.Windows measurement with the released build (1.4.0-canary.1)
Git repo and project created under
%TEMP%withlinker = "isolated",trustedDependencies: ["dep"]and"prepare": "echo prepared > prepared.txt"; the temp directory name was lengthened to move the package directory across the limit.Same second layout with this PR's build:
no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/isolated-install.test.ts
2. install: send credentials embedded in a tarball URL as Basic authorization (from #39025)
Problem
"no-deps": "http://carol:s3cret@127.0.0.1:PORT/cdn/no-deps-1.0.0.tgz", is downloaded without anAuthorizationheader. A server that needs the credentials answers 401 and the install fails witherror: GET http://carol:s3cret@127.0.0.1:PORT/cdn/no-deps-1.0.0.tgz - 401(bun 1.4.0 andmain, hoisted and isolated linker). npm 11 installs the same package.json and sendsAuthorization: Basic Y2Fyb2w6czNjcmV0(base64("carol:s3cret")).http://token@127.0.0.1:PORT/x.tgz) does not reach the server at all: the request goes to the hostnametoken@127.0.0.1.NetworkTask::for_tarball(src/install/NetworkTask.rs) only ever attaches the registry scope's configured token or_auth, and only for npm packages whose tarball is on the registry's origin. Nothing reads the URL's userinfo, and the HTTP client does not either (it only turns the userinfo of a proxy URL intoProxy-Authorization). The misrouted username-only form comes frombun_url::URL::parse, which takestoken@127.0.0.1for the hostname when the userinfo has no:and a port follows (tracked separately; this change no longer depends on how that parser splits the authority).Fix
for_tarballsplits the userinfo off the request URL before anything else looks at it (split_url_userinfo: the authority runs from://to the first/,?or#, the userinfo is everything in it up to the last@, so the@of/@scope/pkg/-/pkg.tgzis not one) and sends it asAuthorization: Basic base64(userinfo)(basic_authorization_from_userinfo, which appends:when the userinfo has no password). The request URL is the URL without the userinfo._auth), they are still sent and the URL's are not. This is npm's order as well: npm-registry-fetch sets the header from the config, and node only derivesAuthorizationfrom the URL'sauthwhen the request has no such header.user:pass,user(sent asuser:),:passand a percent-encoded password (sent undecoded); transcript below. It is also how bun already treats credentials written into a registry URL (install: send credentials embedded in --registry and registry env var URLs #38796 stores the username and password bytes as given). pnpm was not available offline. Two spellings are deliberately not npm's: a second:inside the password is sent as written where npm percent-encodes it, and:token@in a tarball URL is Basic like npm rather than the Bearer that bun's registry URLs make of it; both are called out in the test table and in the code comment.bun_url::URL::originincludes the userinfo and the HTTP client compares origins to decide whetherAuthorizationfollows a redirect, so with the userinfo left in, a redirect to the same host would drop the header (verified: with only the header added, the redirect test below fails withauthorization: nullon the second hop). It also fixes the username-only form without touchingbun_url, and theGET <url> - 401line now prints the URL without the credentials. The cross-host rule is unchanged: the HTTP client strips the header on a redirect to another origin, same as for the registry token (test included).package.json, the lockfile, the task id and the cache key still use the URL as written. Documented indocs/pm/cli/add.mdx.test/cli/install/bun-install.test.ts,describe("credentials embedded in a tarball URL"): 12 tests, 10 of which fail on the unfixed build (the two guards, scoped path and registry credentials taking precedence, pass on both). The rest of the file is unchanged: the remaining failures locally are the tests that need the public internet, identical with the unmodified binary.cargo check -p bun_install,cargo clippy -p bun_install, rustfmt and prettier are clean.Background
package.jsonentry whose version is anhttp(s)://URL ending in.tgz/.tar.gz/.tar. Bun downloads it directly; no registry manifest is involved, so the registry's configured credentials never applied to it (Authorization::NoAuthorizationat the call sites inPackageManagerEnqueue.rs). Registry packages reach the samefor_tarballthrough their manifest'sdist.tarballURL withAllowAuthorization, which is the case where the registry scope's credentials can apply.user:password@part of a URL's authority (RFC 3986 section 3.2.1). HTTP never puts it on the wire; clients that honor it (npm through minipass-fetch, curl, browsers) convert it intoAuthorization: Basic base64(user:password).NetworkTask::for_tarballbuilds one HTTP request per tarball download:url_buf(the request URL) andheader_buf(the headers, built withHeaderBuilderin two passes,countthenappend, because the buffer is allocated exactly once in between). Retries reuse the same request, so the header is sent on every attempt.bun_url::URL::parseis the allocation-free splitter the HTTP client andfor_tarballuse; it is not a WHATWG parser. Itsoriginis a prefix of the input string, which is why it still contains the userinfo.npm 11.16 against a local server logging the Authorization header (same tarball, same package.json shape)
bun 1.4.0-canary.1 (eabb96d) for the first row: the server logs
auth=nulland bun printserror: GET http://carol:s3cret@127.0.0.1:PORT/cdn/no-deps-1.0.0.tgz - 401.3. install: read
file:tarballs relative to thefile:folder package that declares them (from #39017)Problem
file:folder dependency whose own package.json declares a local tarball fails to install (released 1.4.0 canary and main, both linkers): project package.json{"dependencies":{"lib":"file:./vendor/lib"}},vendor/lib/package.jsondeclaring"tool": "file:./tool.tgz",vendor/lib/tool.tgzpresent:<project>/tool.tgz. If that file happens to exist it is installed astoolinstead of the folder's copy, with no error. The directory form of the same declaration ("tool": "file:./tool") is already resolved relative tovendor/lib.enqueue_local_tarball(src/install/PackageManager/PackageManagerEnqueue.rs) picks the directory a local tarball path is relative to, and the only declarer it looked at was a workspace (get_workspace_pkg_if_workspace_dep). Every other declarer, including afile:folder package, fell through to the top-level dir.overrides/resolutionsentry or catalog entry pointing a dependency atfile:./x.tgzwas read relative to the workspace when the dependency it applied to was declared by a workspace member, sooverrides: { bar: "file:./bar.tgz" }withbar.tgzin the project root failed with the same ENOENT as soon as a workspace depended onbar. This is Feature: Workspaceoverrideswithfile:paths should resolve relative to workspace root's package.json #25835 (overrides) and Catalog entries withfile:relative paths fail when referenced from workspace packages #25752 (catalogs); both reproduce as reported on the released build and install with this change. The directory form of an override is already resolved relative to the project (Folderarm ofget_or_put_resolved_package).Fixes #25835
Fixes #25752
Fix
enqueue_local_tarballnow takes the base directory fromlocal_tarball_base_dir: the directory of the declaring package when it is a workspace or afile:folder package and that package's own specifier is the tarball path being read; the top-level dir in every other case.file:and what bun already does for workspace declarers and for the directory form. Afile:folder package is read from the project like a workspace is, and itsResolution::Folderpayload is its directory relative to the top-level dir (folder_resolver.rs,NewResolver { folder_path: rel }), the same shape as a workspace'sResolution::Workspacepayload, so both are joined the same way. The only otherResolution::Folderpackages are the stubs created forfile:directories declared by something other than the root or a workspace (Folderarm ofget_or_put_resolved_package); those carry no dependency list, so they are never the declarer of an edge.overrides,resolutionsand catalogs are only parsed from the root package.json (Package.rs,FEATURES.is_main), and applying one leaves the declaring package's stored edge untouched (the replacement is local toenqueue_dependency_with_main_and_success_fn). So when the stored edge's specifier is not the path being read, the root wrote the path and the top-level dir is the only directory it can mean. Without this condition, root overrides applied to a folder-declared dependency, which work today only because of the bug, would start being read from the folder.version_was_replaced:enqueue_local_tarballis also reached fromenqueue_tarball_for_readingwhen a project with abun.lockis installed into an empty cache. The lockfile row keeps the path as declared ("tool": ["bar@./tool.tgz", ...]under"lib": [..., { "dependencies": { "tool": "file:./tool.tgz" } }]), and the edge's declarer and specifier are available there too, so both passes compute the same directory. Lockfile format is unchanged and existing lockfiles keep working; the path in the row is joined onto a different directory only for declarers that previously failed or installed the wrong file.Lockfile::get_parent_pkg_of_dependencyis the same helper that PR adds.bar@./tool.tgz), so two project packages declaring the same relative path to two different files still share one row and one read, the limitation workspaces already have. A package.json inside a folder dependency that worked around this bug by writing a project-relative path (file:./vendor/lib/tool.tgz) will now need the path relative to itself, which is what npm requires for it as well.test/cli/install/bun-install.test.ts,describe("file: tarball declared by a file: folder dependency"): the folder's tarball is installed with the hoisted and with the isolated linker, and a root override supplying the path is read from the project. Each test plants a different tarball at the other candidate path and runs a fresh install followed by--frozen-lockfileinto an emptied cache, so both the resolve pass and the install frombun.lockhave to read the right file.test/cli/install/bun-workspaces.test.ts,relative tarballs > from a root override / catalog entry applied to a workspace dependency, covers Feature: Workspaceoverrideswithfile:paths should resolve relative to workspace root's package.json #25835 and Catalog entries withfile:relative paths fail when referenced from workspace packages #25752 the same way. The four folder/workspace tests install the wrong tarball without thesrc/change (checked against a debug build of main and against the released build); the override-on-folder test passes before and after and pins that case.bun-workspaces.test.ts(74 pass),overrides.test.ts+nested-overrides.test.ts(151 pass),bun-lock.test.ts(40 pass),bun-install.test.ts -t "tarball|tgz|file:|folder|override|resolutions"(51 pass;should treat non-GitHub http(s) URLs as tarballsfails identically on the released build, it needs network),isolated-install.test.ts,bun-add.test.tsandbun-install-registry.test.tsfiltered to tarball/file tests (all pass).cargo clippy -p bun_installis clean.Background
file:dependency on a directory is aTag::Folderdependency; one on a.tgzis aTag::Tarballdependency with a local URI. The latter resolves toResolution::LocalTarball(<path as written>); the path string is both the task id for reading it and the package's identity inbun.lock.file:folder dependencies from disk (Package::parse), and stores for each of the latter two aResolution::Workspace/Resolution::Folderwhose payload is the package directory relative to the top-level dir. Packages from the registry, git or a tarball get their dependency lists from the manifest or the extracted archive instead and have no directory in the project while they resolve.enqueue_local_tarballis called from two places: theTarballarm of the resolve pass (no lockfile row yet, the tarball is read to learn the package's name and dependencies) andenqueue_tarball_for_readingduring install (a row exists inbun.lock, but the extracted package is missing from the cache). It computes the on-disk path on the main thread and hands it to a thread pool task, which only reads the file.Closes #39025
Closes #39017