install: pin that .. tarball entries are clamped to the package root in both extractors - #37431
install: pin that .. tarball entries are clamped to the package root in both extractors#37431robobun wants to merge 1 commit into
Conversation
WalkthroughChangesThe installation test suite adds regression coverage for tar entries containing Extraction traversal regression
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — comment correction + test-only PR pinning existing behavior.
What was reviewed:
- Verified
normalize_buf_tcallsnormalize_string_buf_twithALLOW_ABOVE_ROOT=falsefor relative inputs (resolve_path.rs:1302), so the new comment is accurate and the old one was wrong. - Confirmed
BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALLgatesuses_streaming_extraction()and the buffered path prints[name] Extracted to ...under--verbose, so bothtest.eacharms assert the extractor they claim to exercise. - Checked the test is hermetic (local registry,
tempDir,BUN_TMPDIRnested underdir), the stray-file walk'sisStreamPkgRootpredicate handles both node_modules and the cache layout, andexitCodeis asserted last per harness convention.
Extended reasoning...
Overview
Two files touched: a 5-line comment rewrite in src/install/TarballStream.rs (no code change — the leading-.. guard is kept verbatim) and a new test.each in test/cli/install/bun-install-streaming-extract.test.ts covering both the streaming and buffered tarball extractors. The test builds a tarball with four ..-climbing entries, installs it via a local drip-feed registry, and asserts every climb file lands directly in the package root (both in node_modules/stream-pkg and, via a recursive walk, everywhere under the test's temp tree).
Security risks
None introduced. The PR adds coverage for an existing path-traversal defense rather than changing it. I verified the claim underpinning the comment rewrite: normalize_buf_t on a relative input dispatches to normalize_string_buf_t::<_, false, _, _> (resolve_path.rs:1301-1304), i.e. ALLOW_ABOVE_ROOT = false, so leading .. segments are indeed dropped and the guard in TarballStream is a backstop today. The new comment states this correctly; the old one was factually wrong.
Level of scrutiny
Low-to-moderate. There is no production logic change — the Rust diff is comment-only and the guard bytes are unchanged. The test is additive and self-contained in an existing test file. The area (tarball extraction / traversal) is security-sensitive, which is precisely why pinning the clamp with a test is valuable, but since nothing in the extractor's control flow changed there is no new attack surface to reason about.
Other factors
The test is well-constructed against the repo's review checklist: it uses test.each over the variant matrix, spreads bunEnv, uses tempDir/using, drains stdout/stderr/exited concurrently via runInstall, keeps the registry local, buries BUN_TMPDIR four levels deep so an escaping write would still land inside the walked tree, asserts a positive property (exact sorted file list + content) and the negative containment property, and asserts exitCode last. The ] Streamed / ] Extracted to assertions tie each case to the extractor it claims to exercise (verified against TarballStream::populate_result and extract_tarball.rs:433). No prior human or bot review comments are outstanding.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Updated 11:30 PM PT - Aug 10th, 2026
✅ @robobun, your commit ac4b7334fbdbad6931dced812e1e9d4c883d7483 passed in 🧪 To try this PR locally: bunx bun-pr 37431That installs a local version of the PR into your bun-37431 --bun |
|
Not a duplicate of #31160. That PR is about the overlong PAX path crash, and main already has the bounds checks it adds ( |
…in both extractors Both extractors strip the first path component of a tarball entry and normalize the rest with normalize_buf_t, which drops any .. that would climb above the root of a relative path. Archiver::extract_to_dir relies on that alone; TarballStream has a leading-.. check behind it that the clamp makes unreachable. Neither had a test for .. entry names, so a normalizer change would have reopened traversal in the buffered extractor without anything failing. Install a tarball with climbing entries through both the streaming and buffered extractors, with the temp dir and cache kept inside the test directory, and assert the entries land in the package root and nowhere else.
b0adcbd to
ac4b733
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/cli/install/bun-install-streaming-extract.test.ts`:
- Around line 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.
- Around line 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.
- Around line 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1987a826-ef85-408f-81e7-f01674999c20
📒 Files selected for processing (1)
test/cli/install/bun-install-streaming-extract.test.ts
| 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) => { |
There was a problem hiding this comment.
🚀 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.tsRepository: 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; fiRepository: 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 || trueRepository: 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 { 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); |
There was a problem hiding this comment.
🎯 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.
| 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
| 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"); |
There was a problem hiding this comment.
🎯 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.
| 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
There was a problem hiding this comment.
LGTM — test-only addition that pins existing ..-clamping behavior in both tarball extractors.
What was reviewed:
- Verified
BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALLand the] Streamed/] Extracted toverbose markers exist insrc/install/so each case proves which extractor ran. - Checked the containment walk:
Dirent.parentPathis supported (NodeDirent.cpp), and keying on the package manifest rather than directory name correctly handles the cache layout. - Confirmed the
BUN_TMPDIRnesting (4 levels + extraction subdir) is deeper than the longest 3-level..chain, so an escape would land inside the walked tree and fail thestraysassertion rather than vanish outside it.
Extended reasoning...
Overview
This PR adds a single test.each (2 cases: streaming + buffered) to test/cli/install/bun-install-streaming-extract.test.ts. It builds a tarball containing four entries with .. components, installs it via a local drip-feed registry, and asserts that all four files land directly in the package root with correct contents — plus a recursive walk over the entire test directory to prove no copy escaped to a non-package-root parent. No runtime code is touched; the only other diff hunk is adding relative to the node:path import.
Security risks
None introduced. The change is test-only and adds regression coverage for a path-traversal-adjacent invariant (normalize_buf_t clamping .. on relative inputs) that previously had no direct test. The test itself is hermetic: local node:http registry, tempDir, BUN_TMPDIR/TMPDIR and the install cache all confined under the temp dir.
Level of scrutiny
Low-to-medium. It's a test file following the exact patterns already established in the same file (same buildTarball/makeRegistry/runInstall helpers, same test.each streaming/buffered matrix as the neighboring test). The security-sensitive extraction code in src/install/TarballStream.rs and src/libarchive/ is unchanged. The main things worth checking were whether the assertions can actually fail (they can — the PR description documents planting a decoy to prove the detector is live, and the tmpdir depth is sized so an escape stays observable) and whether the env knobs / verbose lines the test relies on exist in src/ (they do).
Other factors
The test avoids the pitfalls the repo review guide calls out: it drains stdout/stderr/exited concurrently via the shared runInstall helper, uses tempDir with using, asserts exact file contents (not just existence), asserts exitCode last, and covers both extractor variants in one test.each. The readdirSorted result order (climb-1, climb-3, climb-ab, climb-nested) is correct alphabetically. Dirent.parentPath is supported by Bun's NodeDirent.cpp. The one thing left unfixed — the misleading comment in TarballStream.rs — is explicitly called out in the PR description as intentionally deferred to keep this test-only.
2 participants
What
Both tarball extractors strip the first path component of an entry (
package/, or the repo directory for GitHub tarballs) and run the remainder throughnormalize_buf_t. For a relative input that normalizes withALLOW_ABOVE_ROOT = false(resolve_path.rs, the!is_absolutearms ofnormalize_buf_t), so../x,../../../xanda/b/../../../xall come out asxand are written into the package root.Archiver::extract_to_dir(buffered path) relies on that clamp alone: nothing else sits between the entry name andopenat(extraction_dir, ...).TarballStream(streaming path) has an additional leading-..check, but its comment said the normalizer "leaves a leading..on a relative input", which is not what happens; the check is unreachable today.Neither extractor had a test for
..entry names (the existing traversal tests cover symlink targets), so a normalizer change would have reopened traversal in the buffered extractor without anything failing.Change
Test only.
bun-install-streaming-extract.test.tsgets atest.eachover the streaming and buffered extractors (BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALLfor the latter, with the] Streamed/] Extracted toverbose lines asserted so each case proves which extractor ran). Each case installs a tarball containing../climb-1.txt,../../../climb-3.txt,nested/../../climb-nested.txtanda/b/../../../climb-ab.txt.BUN_TMPDIRis pointed atdir/t1/t2/t3/bun-tmp, deeper than the longest chain climbs, and the cache already lives underdir, so an escaping entry has to land inside the test directory. The test asserts the four files are innode_modules/stream-pkg/with their contents, and that everyclimb-*file anywhere underdirhas a directory whosepackage.jsonis stream-pkg's as its parent.Verification
Both new cases pass. Planting a stray
t1/climb-decoy.txtbefore the walk makes the containment assertion fail with that path, so the detector is live; an earlier version that keyed on the parent directory name failed on the cache layout (.cache/stream-pkg/1.0.0@@127.0.0.1@@@1/), which is why it checks for the package manifest instead.No runtime change. The comment above the streaming extractor's check in
TarballStream.rsstill describes the normalizer incorrectly; I left it alone here so this PR stays test only, and the test's own comment records what actually happens. It is a one-line fix to fold into whatever next touches that file.[stamp-90s] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file