From 000adcc2d47c991e85a72101667b54aa3832b01b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:31:40 +0000 Subject: [PATCH 1/4] install: drop the query string and fragment from a tarball URL's extraction folder label `bun add ` names the dependency after the URL until the tarball's package.json has been read, and ExtractTarball::name_and_basename derives the temp folder label from that URL's basename. The query string and fragment were kept, so `?` broke mkdir on Windows and a `:` in the query tripped the install folder name check everywhere. Cut the URL at the first `?` or `#` before taking the basename, and fall back to "package" when the remaining label is still not a usable folder name, since a placeholder URL is not an alias to validate. --- src/install/extract_tarball.rs | 41 ++++++++---- test/cli/install/bun-add.test.ts | 110 +++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 14 deletions(-) diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index b8d3a8b091b5..87d9464ad388 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -198,30 +198,43 @@ impl ExtractTarball { debug_assert!(false); b"unnamed-package" }; - let basename: &[u8] = 'brk: { - let mut tmp = name; - if strings::has_prefix(tmp, b"https://") || strings::has_prefix(tmp, b"http://") { + let basename: &[u8] = + if strings::has_prefix(name, b"https://") || strings::has_prefix(name, b"http://") { + // `bun add ` names the dependency after the URL until the + // tarball's package.json has been read, so the name is not an + // alias to validate; the basename only labels the temp dir. + let mut tmp = name; + if let Some(i) = strings::index_of_any(tmp, b"?#") { + tmp = &tmp[0..i]; + } tmp = bun_paths::basename(tmp); if strings::ends_with(tmp, b".tgz") { tmp = &tmp[0..tmp.len() - 4]; } else if strings::ends_with(tmp, b".tar.gz") { tmp = &tmp[0..tmp.len() - 7]; } - } else if tmp[0] == b'@' { - if let Some(i) = strings::index_of_char(tmp, b'/') { - tmp = &tmp[i as usize + 1..]; + if bun_install::dependency::is_safe_install_folder_name(tmp) { + tmp + } else { + b"package" + } + } else { + let mut tmp = name; + if tmp[0] == b'@' { + if let Some(i) = strings::index_of_char(tmp, b'/') { + tmp = &tmp[i as usize + 1..]; + } } - } - #[cfg(windows)] - { - if let Some(i) = strings::last_index_of_char(tmp, b':') { - tmp = &tmp[i + 1..]; + #[cfg(windows)] + { + if let Some(i) = strings::last_index_of_char(tmp, b':') { + tmp = &tmp[i + 1..]; + } } - } - break 'brk tmp; - }; + tmp + }; (name, basename) } diff --git a/test/cli/install/bun-add.test.ts b/test/cli/install/bun-add.test.ts index 423a7fbb8e4a..64e729b488b4 100644 --- a/test/cli/install/bun-add.test.ts +++ b/test/cli/install/bun-add.test.ts @@ -1,8 +1,11 @@ import type { BunLockFile } from "bun"; import { file, spawn } from "bun"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, setDefaultTimeout, test } from "bun:test"; +import { randomBytes } from "crypto"; import { access, appendFile, copyFile, mkdir, readlink, rm, writeFile } from "fs/promises"; import { bunExe, bunEnv as env, readdirSorted, tmpdirSync, toBeValidBin, toBeWorkspaceLink, toHaveBins } from "harness"; +import { createServer } from "http"; +import type { AddressInfo } from "net"; import { join, relative, resolve } from "path"; import { check_npm_auth_type, @@ -2665,6 +2668,113 @@ it("should not add duplicate package.json entries when installing the same tarba }); }); +// `bun add ` names the dependency after the URL until the tarball's package.json has been read, +// and the URL's basename labels the directory the tarball is extracted into. The query string and +// fragment used to end up in that label: `?` cannot appear in a directory name on Windows, and a `:` +// within the first 32 bytes of the label (it is truncated to that) fails the install folder name +// check on every platform with "Refusing to install package with invalid name". +describe("should add a tarball URL with a query string or fragment", () => { + const suffixes = [ + ["query string", "?token=abc"], + ["query string containing a colon", "?expires=12:00"], + ["fragment containing a colon", "#ref:main"], + ] as const; + + let tarball: Uint8Array; + beforeAll(async () => { + tarball = await new Bun.Archive( + { + "package/package.json": JSON.stringify({ name: "qs-pkg", version: "1.0.0" }), + // Incompressible padding so the drip-fed response below arrives in many socket reads, which + // is what commits the install to the streaming extractor. + "package/pad.bin": randomBytes(256 * 1024), + }, + { compress: "gzip" }, + ).bytes(); + }); + + // The tarball is extracted either from the fully buffered response body or, when the body is + // large enough and arrives in several reads, by the streaming extractor; both pick the extraction + // directory name the same way. + async function serveTarball(mode: "buffered" | "streaming") { + if (mode === "buffered") { + const server = Bun.serve({ + port: 0, + fetch: () => new Response(tarball), + }); + return { + origin: server.url.origin, + [Symbol.asyncDispose]: () => server.stop(true), + }; + } + + // node:http so the response carries a Content-Length and can still be written 1 KiB at a time. + const server = createServer((req, res) => { + res.setHeader("Content-Type", "application/gzip"); + res.setHeader("Content-Length", String(tarball.length)); + req.socket.setNoDelay(true); + let offset = 0; + const step = () => { + if (offset >= tarball.length) { + res.end(); + return; + } + res.write(tarball.subarray(offset, offset + 1024)); + offset += 1024; + setImmediate(step); + }; + step(); + }); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + return { + origin: `http://127.0.0.1:${port}`, + [Symbol.asyncDispose]: () => { + server.closeAllConnections(); + return new Promise(resolve => server.close(() => resolve())); + }, + }; + } + + describe.each(["buffered", "streaming"] as const)("%s extraction", mode => { + test.each(suffixes)("%s", async (_, suffix) => { + await using server = await serveTarball(mode); + const url = `${server.origin}/qs-pkg-1.0.0.tgz${suffix}`; + await writeFile(join(package_dir, "package.json"), JSON.stringify({ name: "foo", version: "0.0.1" })); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "add", url, "--verbose"], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env: mode === "streaming" ? { ...env, BUN_INSTALL_STREAMING_MIN_SIZE: "1024" } : env, + }); + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); + expect(err).not.toContain("error:"); + // Printed by the streaming extractor only, so each mode is known to have taken its own path. + if (mode === "streaming") { + expect(err).toContain("Streamed "); + } else { + expect(err).not.toContain("Streamed "); + } + expect(out).toContain(`+ qs-pkg@${url}`); + expect(exitCode).toBe(0); + expect(await file(join(package_dir, "package.json")).json()).toStrictEqual({ + name: "foo", + version: "0.0.1", + dependencies: { + "qs-pkg": url, + }, + }); + expect(await file(join(package_dir, "node_modules", "qs-pkg", "package.json")).json()).toEqual({ + name: "qs-pkg", + version: "1.0.0", + }); + }); + }); +}); + it("should add multiple dependencies specified on command line", async () => { expect(check_npm_auth_type.check).toBe(true); using server = Bun.serve({ From e40f560b0ac88b376ca3abd0da3cd396dbbf02a7 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:38:18 +0000 Subject: [PATCH 2/4] test: cover the "package" fallback with a tarball URL that has no path --- test/cli/install/bun-add.test.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/test/cli/install/bun-add.test.ts b/test/cli/install/bun-add.test.ts index 64e729b488b4..a399f6b8f6d5 100644 --- a/test/cli/install/bun-add.test.ts +++ b/test/cli/install/bun-add.test.ts @@ -2672,12 +2672,15 @@ it("should not add duplicate package.json entries when installing the same tarba // and the URL's basename labels the directory the tarball is extracted into. The query string and // fragment used to end up in that label: `?` cannot appear in a directory name on Windows, and a `:` // within the first 32 bytes of the label (it is truncated to that) fails the install folder name -// check on every platform with "Refusing to install package with invalid name". +// check on every platform with "Refusing to install package with invalid name". A URL without a path +// has nothing usable left once the query is gone (its basename is the `host:port`), so the label +// falls back to "package" instead. describe("should add a tarball URL with a query string or fragment", () => { - const suffixes = [ - ["query string", "?token=abc"], - ["query string containing a colon", "?expires=12:00"], - ["fragment containing a colon", "#ref:main"], + const paths = [ + ["query string", "/qs-pkg-1.0.0.tgz?token=abc"], + ["query string containing a colon", "/qs-pkg-1.0.0.tgz?expires=12:00"], + ["fragment containing a colon", "/qs-pkg-1.0.0.tgz#ref:main"], + ["query string on a URL without a path", "/?file=qs-pkg-1.0.0.tgz"], ] as const; let tarball: Uint8Array; @@ -2737,9 +2740,9 @@ describe("should add a tarball URL with a query string or fragment", () => { } describe.each(["buffered", "streaming"] as const)("%s extraction", mode => { - test.each(suffixes)("%s", async (_, suffix) => { + test.each(paths)("%s", async (_, path) => { await using server = await serveTarball(mode); - const url = `${server.origin}/qs-pkg-1.0.0.tgz${suffix}`; + const url = `${server.origin}${path}`; await writeFile(join(package_dir, "package.json"), JSON.stringify({ name: "foo", version: "0.0.1" })); const { stdout, stderr, exited } = spawn({ From 261f521bc937fa99f8c42ed0b71a0d53a79bb559 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:42:55 +0000 Subject: [PATCH 3/4] install: shorten the placeholder name comment in name_and_basename --- src/install/extract_tarball.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index 87d9464ad388..ca3c1691b6f9 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -200,9 +200,8 @@ impl ExtractTarball { }; let basename: &[u8] = if strings::has_prefix(name, b"https://") || strings::has_prefix(name, b"http://") { - // `bun add ` names the dependency after the URL until the - // tarball's package.json has been read, so the name is not an - // alias to validate; the basename only labels the temp dir. + // The URL is the placeholder name `bun add ` gives a dependency until + // its package.json has been read; the basename only labels the temp dir. let mut tmp = name; if let Some(i) = strings::index_of_any(tmp, b"?#") { tmp = &tmp[0..i]; From 5fef7c39339af7c1d7198945347bca06dcea2f6e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:44:56 +0000 Subject: [PATCH 4/4] install: one-line comment for the URL placeholder branch --- src/install/extract_tarball.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index ca3c1691b6f9..e3cec500e2ed 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -200,8 +200,7 @@ impl ExtractTarball { }; let basename: &[u8] = if strings::has_prefix(name, b"https://") || strings::has_prefix(name, b"http://") { - // The URL is the placeholder name `bun add ` gives a dependency until - // its package.json has been read; the basename only labels the temp dir. + // A URL name is the placeholder `bun add ` uses until package.json is read. let mut tmp = name; if let Some(i) = strings::index_of_any(tmp, b"?#") { tmp = &tmp[0..i];