Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 71 additions & 1 deletion test/cli/install/bun-install-streaming-extract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { bunEnv, bunExe, readdirSorted, tempDir } from "harness";
import { createHash } from "node:crypto";
import { createWriteStream, existsSync, mkdirSync, readdirSync, readFileSync, statSync, writeFileSync } from "node:fs";
import { createServer, type Server } from "node:http";
import { join } from "node:path";
import { join, relative } from "node:path";
import { createGzip, gzipSync } from "node:zlib";

setDefaultTimeout(1000 * 60 * 5);
Expand Down Expand Up @@ -378,6 +378,76 @@ describe("streaming tarball extraction", () => {
expect(exitCode).toBe(0);
});

// Both extractors strip the leading `package/` and hand the rest to
// normalize_buf_t, which drops every `..` that would climb above the root
// of a relative path. The buffered extractor (Archiver::extract_to_dir) has
// nothing else between an entry name and openat(extraction_dir, ...); the
// streaming one (TarballStream) has a leading-`..` check behind it that the
// clamp makes unreachable. Pin the clamp for both so a normalizer change
// cannot quietly turn these entries into writes outside the extraction dir.
test.each([
["streaming", {}],
["buffered", { BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALL: "1" }],
] as const)("entries that climb above the package root with .. are clamped to it (%s)", async (label, env) => {
Comment on lines +388 to +391

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
bun bd test test/cli/install/bun-install-streaming-extract.test.ts

Repository: oven-sh/bun

Length of output: 190


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="test/cli/install/bun-install-streaming-extract.test.ts"

printf '%s\n' '--- imports and target test ---'
sed -n '1,35p' "$file"
sed -n '350,465p' "$file"

printf '%s\n' '--- test.concurrent.each usage in repository ---'
rg -n --glob '*.{ts,tsx,js,jsx}' 'test\.concurrent\.each|describe\.concurrent|\.concurrent\.' test src packages 2>/dev/null | head -200 || true

printf '%s\n' '--- test.each usage near install tests ---'
rg -n --glob 'test/**/*.test.{ts,tsx,js,jsx}' 'test\.each\(' test/cli/install | head -100 || true

printf '%s\n' '--- local review guidance ---'
if [ -f REVIEW.md ]; then sed -n '1,240p' REVIEW.md; fi

Repository: oven-sh/bun

Length of output: 50367


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="test/cli/install/bun-install-streaming-extract.test.ts"

printf '%s\n' '--- helper definitions and shared fixtures ---'
rg -n '^(async )?function (makeRegistry|runInstall|buildTarball)|^const (entries|tgz|shasum|integrity|chunkBytes)|^let |^var ' "$file"
sed -n '120,290p' "$file"

printf '%s\n' '--- all process-global mutations in the file ---'
rg -n 'process\.env|BUN_TMPDIR|TMPDIR|setDefaultTimeout|mkdirSync|tempDir|makeRegistry|runInstall' "$file"

printf '%s\n' '--- exact concurrent.each examples and their isolation patterns ---'
sed -n '1,105p' test/bundler/transpiler/assign-to-import.test.ts
sed -n '1,75p' test/cli/test/test-timeout-behavior.test.ts
sed -n '1,95p' test/cli/install/GHSA-pfwx-36v6-832x.test.ts

printf '%s\n' '--- test runner concurrency guidance ---'
rg -n -C 3 'test\.concurrent|concurrent tests|max_concurrency|concurrency' src test/harness.ts test/README.md .claude/docs 2>/dev/null | head -240 || true

Repository: oven-sh/bun

Length of output: 36648


Run the independent extraction modes concurrently.

Each case has isolated registry, cache, temporary directory, and subprocess state. Use test.concurrent.each to avoid serial execution.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/cli/install/bun-install-streaming-extract.test.ts` around lines 388 -
391, Update the parameterized test around the “entries that climb above the
package root” case to use test.concurrent.each instead of test.each, preserving
the existing cases, environment values, and test body unchanged.

Source: Coding guidelines

const climbing = [
"../climb-1.txt",
"../../../climb-3.txt",
"nested/../../climb-nested.txt",
"a/b/../../../climb-ab.txt",
];
const built = buildTarball([...entries, ...climbing.map(path => ({ path, body: Buffer.from(`${path}\n`) }))]);
expect(built.tgz.length).toBeGreaterThan(2 * 1024 * 1024);

await using reg = await makeRegistry(built.tgz, built.shasum, built.integrity, chunkBytes);
const registry = reg.url;

using dir = tempDir("streaming-extract-dotdot", {
"package.json": JSON.stringify({
name: "app",
version: "1.0.0",
dependencies: { "stream-pkg": "1.0.0" },
}),
"bunfig.toml": Bun.TOML.stringify({ install: { registry } }),
});
// Entries are extracted into a directory under BUN_TMPDIR and the tree is
// then renamed into the cache (which runInstall keeps under `dir`). Bury
// the temp dir deeper than the longest `..` chain above climbs, so an
// entry that did escape still lands somewhere the walk below can see.
const tmp = join(String(dir), "t1", "t2", "t3", "bun-tmp");
mkdirSync(tmp, { recursive: true });

const { stderr, exitCode } = await runInstall(String(dir), { ...env, BUN_TMPDIR: tmp, TMPDIR: tmp });
expect(stderr).not.toContain("error:");
if (label === "streaming") {
expect(stderr).toContain("] Streamed ");
} else {
expect(stderr).toContain("] Extracted to ");
}
expect(reg.tarballHits).toBe(1);

// The clamp keeps the basename: every climbing entry ends up directly in
// the package root, next to the regular entries.
const pkgRoot = join(String(dir), "node_modules", "stream-pkg");
const climbed = (await readdirSorted(pkgRoot)).filter(name => name.startsWith("climb-"));
expect(climbed).toEqual(["climb-1.txt", "climb-3.txt", "climb-ab.txt", "climb-nested.txt"]);
expect(readFileSync(join(pkgRoot, "climb-ab.txt"), "utf8")).toBe("a/b/../../../climb-ab.txt\n");
expect(readFileSync(join(pkgRoot, "index.js"), "utf8")).toBe("module.exports = 'ok';\n");
Comment on lines +431 to +434

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Verify the payload of every traversal variant.

The test checks content only for climb-ab.txt. Assert the body for all four paths so each normalization form proves both placement and extraction correctness.

Proposed change
-import { join, relative } from "node:path";
+import { basename, join, relative } from "node:path";
@@
-    expect(readFileSync(join(pkgRoot, "climb-ab.txt"), "utf8")).toBe("a/b/../../../climb-ab.txt\n");
+    for (const entryPath of climbing) {
+      expect(readFileSync(join(pkgRoot, basename(entryPath)), "utf8")).toBe(`${entryPath}\n`);
+    }

As per coding guidelines, every assertion must assert the strongest meaningful invariant.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const climbed = (await readdirSorted(pkgRoot)).filter(name => name.startsWith("climb-"));
expect(climbed).toEqual(["climb-1.txt", "climb-3.txt", "climb-ab.txt", "climb-nested.txt"]);
expect(readFileSync(join(pkgRoot, "climb-ab.txt"), "utf8")).toBe("a/b/../../../climb-ab.txt\n");
expect(readFileSync(join(pkgRoot, "index.js"), "utf8")).toBe("module.exports = 'ok';\n");
const climbed = (await readdirSorted(pkgRoot)).filter(name => name.startsWith("climb-"));
expect(climbed).toEqual(["climb-1.txt", "climb-3.txt", "climb-ab.txt", "climb-nested.txt"]);
for (const entryPath of climbing) {
expect(readFileSync(join(pkgRoot, basename(entryPath)), "utf8")).toBe(`${entryPath}\n`);
}
expect(readFileSync(join(pkgRoot, "index.js"), "utf8")).toBe("module.exports = 'ok';\n");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/cli/install/bun-install-streaming-extract.test.ts` around lines 431 -
434, Extend the traversal test around readdirSorted(pkgRoot) to read and assert
the expected file contents for all four entries: climb-1.txt, climb-3.txt,
climb-ab.txt, and climb-nested.txt. Keep the existing placement assertion and
index.js check unchanged, using each payload’s corresponding traversal-path
content.

Source: Coding guidelines


// The property that matters: every copy of a climbing entry anywhere under
// `dir` (node_modules, the cache, the temp dir) sits in a directory that
// is a stream-pkg package root. An escaped entry would have bun-tmp, one
// of the t* directories, .cache or `dir` itself as its parent.
const isStreamPkgRoot = (directory: string) => {
const manifest = join(directory, "package.json");
return existsSync(manifest) && JSON.parse(readFileSync(manifest, "utf8")).name === "stream-pkg";
};
const strays = readdirSync(String(dir), { recursive: true, withFileTypes: true })
.filter(entry => entry.name.startsWith("climb-") && !isStreamPkgRoot(entry.parentPath))
.map(entry => relative(String(dir), join(entry.parentPath, entry.name)));
expect(strays).toEqual([]);
expect(exitCode).toBe(0);
Comment on lines +419 to +448

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert install success before reading generated files.

Lines 430-446 read installation output before the test confirms success. If bun install fails, a missing path can hide the process failure with an ENOENT error. Assert exitCode after the stderr checks and before accessing pkgRoot or walking dir.

Proposed change
     expect(reg.tarballHits).toBe(1);
+    expect(exitCode).toBe(0);
 
     // The clamp keeps the basename: every climbing entry ends up directly in
@@
     expect(strays).toEqual([]);
-    expect(exitCode).toBe(0);

Based on learnings, install tests need an early exit-code guard before reading generated artifacts.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { stderr, exitCode } = await runInstall(String(dir), { ...env, BUN_TMPDIR: tmp, TMPDIR: tmp });
expect(stderr).not.toContain("error:");
if (label === "streaming") {
expect(stderr).toContain("] Streamed ");
} else {
expect(stderr).toContain("] Extracted to ");
}
expect(reg.tarballHits).toBe(1);
// The clamp keeps the basename: every climbing entry ends up directly in
// the package root, next to the regular entries.
const pkgRoot = join(String(dir), "node_modules", "stream-pkg");
const climbed = (await readdirSorted(pkgRoot)).filter(name => name.startsWith("climb-"));
expect(climbed).toEqual(["climb-1.txt", "climb-3.txt", "climb-ab.txt", "climb-nested.txt"]);
expect(readFileSync(join(pkgRoot, "climb-ab.txt"), "utf8")).toBe("a/b/../../../climb-ab.txt\n");
expect(readFileSync(join(pkgRoot, "index.js"), "utf8")).toBe("module.exports = 'ok';\n");
// The property that matters: every copy of a climbing entry anywhere under
// `dir` (node_modules, the cache, the temp dir) sits in a directory that
// is a stream-pkg package root. An escaped entry would have bun-tmp, one
// of the t* directories, .cache or `dir` itself as its parent.
const isStreamPkgRoot = (directory: string) => {
const manifest = join(directory, "package.json");
return existsSync(manifest) && JSON.parse(readFileSync(manifest, "utf8")).name === "stream-pkg";
};
const strays = readdirSync(String(dir), { recursive: true, withFileTypes: true })
.filter(entry => entry.name.startsWith("climb-") && !isStreamPkgRoot(entry.parentPath))
.map(entry => relative(String(dir), join(entry.parentPath, entry.name)));
expect(strays).toEqual([]);
expect(exitCode).toBe(0);
const { stderr, exitCode } = await runInstall(String(dir), { ...env, BUN_TMPDIR: tmp, TMPDIR: tmp });
expect(stderr).not.toContain("error:");
if (label === "streaming") {
expect(stderr).toContain("] Streamed ");
} else {
expect(stderr).toContain("] Extracted to ");
}
expect(reg.tarballHits).toBe(1);
expect(exitCode).toBe(0);
// The clamp keeps the basename: every climbing entry ends up directly in
// the package root, next to the regular entries.
const pkgRoot = join(String(dir), "node_modules", "stream-pkg");
const climbed = (await readdirSorted(pkgRoot)).filter(name => name.startsWith("climb-"));
expect(climbed).toEqual(["climb-1.txt", "climb-3.txt", "climb-ab.txt", "climb-nested.txt"]);
expect(readFileSync(join(pkgRoot, "climb-ab.txt"), "utf8")).toBe("a/b/../../../climb-ab.txt\n");
expect(readFileSync(join(pkgRoot, "index.js"), "utf8")).toBe("module.exports = 'ok';\n");
// The property that matters: every copy of a climbing entry anywhere under
// `dir` (node_modules, the cache, the temp dir) sits in a directory that
// is a stream-pkg package root. An escaped entry would have bun-tmp, one
// of the t* directories, .cache or `dir` itself as its parent.
const isStreamPkgRoot = (directory: string) => {
const manifest = join(directory, "package.json");
return existsSync(manifest) && JSON.parse(readFileSync(manifest, "utf8")).name === "stream-pkg";
};
const strays = readdirSync(String(dir), { recursive: true, withFileTypes: true })
.filter(entry => entry.name.startsWith("climb-") && !isStreamPkgRoot(entry.parentPath))
.map(entry => relative(String(dir), join(entry.parentPath, entry.name)));
expect(strays).toEqual([]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/cli/install/bun-install-streaming-extract.test.ts` around lines 419 -
448, Move the exitCode success assertion in the install test to immediately
after the stderr expectations and before constructing or reading pkgRoot and
recursively scanning dir. Keep the existing artifact and stray-file assertions
unchanged, ensuring install failures report the process failure instead of being
masked by filesystem errors.

Source: Learnings

});

test("tarballs below BUN_INSTALL_STREAMING_MIN_SIZE take the buffered path", async () => {
// Reuse the same large tarball but raise the threshold above it.
// The server sends Content-Length, so `notify()` sees a body_size
Expand Down