Hardening: input validation and bounds tightening across 27 subsystems (round 6)#31417
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSecurity and robustness hardening across Bun: installer path/symlink safety, tarball decompression limits, ArrayBuffer pinning for async I/O, parser recursion and merge-budget guards, NUL/control-character input validation, blob/Content-Type handling, crypto/serialization checks, HTTP/HTTP3/TLS guards, and related tests. ChangesPackage Installation Path & Tarball Safety
Lock File Validation & JSON Escaping
ArrayBuffer Pinning for Async I/O
Parser Recursion Limits & Performance
Input Validation & NUL Byte Handling
Webcore & Fetch Blob Safety
ANSI Control Character Sanitization
Crypto & Serialization Validation
Development Server & Debugger Safety
CLI & Shell Script Safety
Miscellaneous Path & Protocol Handling
|
Collaborator
|
Updated 6:20 PM PT - May 26th, 2026
✅ @Jarred-Sumner, your commit bb9c7cf54412ee509589427339160a682e09ece7 passed in 🧪 To try this PR locally: bunx bun-pr 31417That installs a local version of the PR into your bun-31417 --bun |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/npm.rs (1)
3148-3184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPre-existing logic issue: dist-tag key write occurs before value validation.
While the changed indexing at lines 3180-3181 is safe, there's a pre-existing bug in the dist-tags handling: line 3161 writes the tag key to
all_extern_stringsbefore line 3169 validates that the value is a string. If the value is not a string, the loop continues without incrementingdist_tag_i, causing the next iteration to overwrite the same slot.Example: For
{"latest": "1.0.0", "beta": null, "next": "2.0.0"}, the "beta" key is written then immediately overwritten by "next".The counting pass (line 2317) uses
.unwrap_or(b"")which counts all tags with string keys regardless of value type, while the building pass skips entries with non-string values. This creates a safe over-allocation but incorrect deduplication behavior.Proposed fix: move key write after value validation
for tag in tags { if let Some(key) = tag .key .as_ref() .expect("infallible: prop has key") .as_string(&bump) { - all_extern_strings[extern_strings_slice_start + dist_tag_i] = - string_builder.append::<ExternalString>(key); - let Some(version_name) = tag .value .as_ref() .expect("infallible: prop has value") .as_string(&bump) else { continue; }; + all_extern_strings[extern_strings_slice_start + dist_tag_i] = + string_builder.append::<ExternalString>(key); + let dist_tag_value_literal = string_builder.append::<ExternalString>(version_name);🤖 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 `@src/install/npm.rs` around lines 3148 - 3184, The code writes the dist-tag key into all_extern_strings before validating tag.value, which causes overwrites when a tag has a non-string value; in the loop in the dist-tags handling move the write to all_extern_strings (currently at extern_strings_slice_start + dist_tag_i) to after you confirm tag.value.as_string(&bump) succeeds and only then append the key and the value to string_builder and assign both all_extern_strings[...] and all_semver_versions[...] and increment dist_tag_i; ensure dist_tag_i is only incremented when both key and value are valid (leave the counting pass that uses unwrap_or(b"") unchanged).
🤖 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 `@src/install/bin.rs`:
- Around line 1340-1386: In resolved_target_parent_escapes_package_dir,
currently only the parent directory of abs_target is realpath'd which allows a
symlinked bin to escape; change the check to resolve the final target itself by
calling sys::realpath on abs_target (not just its dirname), treat errors as
escaping, and then use resolve_path::is_parent_or_equal(real_package_dir,
real_target) (where real_target is the realpath of abs_target) to determine
containment; keep the existing behavior of returning true on any resolution
error and reuse the existing package_dir/real_package_dir and resolve_path
helpers.
In `@src/install/extract_tarball.rs`:
- Around line 28-29: The libdeflate fast path currently skips the
MAX_DECOMPRESSED_TARBALL_SIZE check by calling
libdeflate::Decompressor::decompress_to_vec(...) and setting needs_to_decompress
= false on Status::Success; change this so the decompressed size is bounded:
either pass/enable a max-output-size variant (or check the returned Vec length)
against MAX_DECOMPRESSED_TARBALL_SIZE and treat any exceedance as an
error/failure, or disable the libdeflate fast path when you cannot guarantee the
bound; ensure the same MAX_DECOMPRESSED_TARBALL_SIZE constant used for
zlib_entry.max_output_size is applied before accepting success from libdeflate
to prevent underreported gzip ISIZE exploits.
In `@src/install/PackageInstaller.rs`:
- Around line 1385-1400: This branch checks
bin::bin_target_escapes_package_dir(folder) but still copies folder into a fixed
PathBuffer (folder_path_buf) later, so add a pre-copy length validation: before
any buffer write, verify folder.len() (or the number of bytes/slices used for
the copy) does not exceed folder_path_buf.capacity() (or the maximum allowed
buffer length), and if it does, mirror the existing failure handling (use
Output::pretty_errorln with a similar message, increment self.summary.fail, call
self.increment_tree_install_count(!IS_PENDING_PACKAGE_INSTALL,
self.current_tree_id, log_level), and return) to fail closed rather than allow a
panic; keep the same log_level/Options::LogLevel::Silent checks and reuse
pkg_name and folder in the error message.
In `@src/md/ansi_renderer.rs`:
- Around line 611-615: The pre-scan lookup key is inconsistent: render code sets
self.image_src = Some(Box::from(sanitize_source_text(detail.href,...))) but
ImageUrlCollector retains the raw detail.href bytes, so theme.remote_image_paths
misses sanitized keys. Fix by making the collector use the same sanitized key
(call sanitize_source_text on detail.href when storing in ImageUrlCollector) or
alternatively store both the raw detail.href and the sanitized value (e.g.,
image_src_raw and image_src_sanitized) and update any lookup code that queries
theme.remote_image_paths to use the sanitized key; ensure the
sanitize_source_text call and buffer usage match the render path so both
pre-scan and render use identical keys (refer to sanitize_source_text,
image_src, ImageUrlCollector, detail.href, and theme.remote_image_paths).
In `@src/semver/lib.rs`:
- Around line 1079-1082: The collision path can overrun the preallocated arena
because count_with_hash() reserves per hash not per distinct string, so when
string_entry.found_existing is true and you call append_without_pool(slice_,
hash) the precomputed cap/start may already be full; fix by making the
reservation collision-aware or by avoiding slicing the fixed buffer in the
collision path: update count_with_hash() (or the logic that sets cap/start) to
increment the reserved count when a new distinct string is detected even on
hash-collisions, or alter append_without_pool() (and the branch using self.ptr,
cap and start) to first check if start == cap and, if so, allocate a new
non-pooled buffer path instead of slicing self.ptr; reference functions/vars:
count_with_hash(), append_without_pool(), string_entry.found_existing, self.ptr,
cap, start.
In `@test/bake/dev/html.test.ts`:
- Around line 274-277: The response currently only asserts the valid frame is
present; add a negative assertion to ensure the oversized frame path is not
present by checking the response body string in the existing test variable text
(i.e., after const text = await res.text()). Add an assertion using
expect(text).not.toContain("oversized-frame") (or another stable prefix of the
oversized path used in your app) immediately after the positive
expect(text).toContain("script.ts") to fail if the oversized-frame entry
appears.
In `@test/cli/install/bun-install-lifecycle-scripts.test.ts`:
- Around line 3769-3773: The test currently checks filesystem side-effects
before asserting the install process exit code; move the exit code assertion for
the `exited` promise (i.e., `expect(await exited).toBe(0)`) to immediately after
the stdout/stderr checks and before any `exists(join(packageDir, ...))`
assertions so install failures aren't masked, and apply the same reordering to
the similar block at the other location mentioned (the assertions around lines
with `exists(... )` at 3801-3804).
In `@test/cli/install/bun-install-native-binlink.test.ts`:
- Around line 388-399: The test spawns a child via spawn (variable install) with
stdout: "pipe" but never reads install.stdout, which can hang if the pipe fills;
either stop piping stdout (e.g., set stdout: "ignore" or "inherit") or consume
it by awaiting install.stdout.text() (or include it in the Promise.all alongside
install.stderr.text() and install.exited) so the spawned process's stdout is
drained and cannot block; update the spawn call or add a read for install.stdout
in the test accordingly.
In `@test/cli/install/bun-install.test.ts`:
- Around line 9259-9261: The current assertions using err, out, and exitCode are
too broad; update the test to assert the specific alias traversal failure by
replacing expect(err).toContain("error:") with a check for the precise
invalid-alias message (e.g., expect(err).toContain("invalid-alias") or the exact
error string emitted by the alias validation path) and keep
expect(out).not.toContain("1 package installed") and
expect(exitCode).not.toBe(0); additionally add an assertion that the network
mock/spy used in this test (the fetch/http-get spy used to request package
tarballs) was never invoked for the tarball URL (e.g.,
expect(fetchSpy).not.toHaveBeenCalledWith(tarballUrl) or
expect(httpGetSpy).not.toHaveBeenCalled()) so the tarball was not fetched.
In `@test/cli/install/bun-lock.test.ts`:
- Around line 811-824: Stop reading the generated yarn.lock before confirming
the subprocess succeeded: add an early assertion that exitCode is 0 (and/or that
err contains "Saved yarn.lock") immediately after the install subprocess
completes, before calling file(join(packageDir, "yarn.lock")).text() and
splitting into lines; move or gate the yarnLock read/inspect logic (the yarnLock
variable and subsequent expects that inspect lines and contents) so it only runs
after the exitCode check to avoid masking CLI failures with file-read errors.
In `@test/cli/install/migration/migrate.test.ts`:
- Around line 279-284: The test currently reads installed artifacts via
Bun.file(join(testDir, "node_modules", ...)).json() before asserting
expect(exitCode).toBe(0), which can throw ENOENT and mask real install failures;
move the expect(exitCode).toBe(0) immediately after the stderr/stdout assertions
(i.e., right after any expect(stderr).toContain/... checks) and only then
perform filesystem checks that call Bun.file(...).json() or read lockfiles;
apply this reorder for the two occurrences referencing expect(exitCode).toBe(0)
and Bun.file(...) in this test file so exitCode is validated before any
filesystem reads.
In `@test/js/bun/json5/json5.test.ts`:
- Around line 1587-1612: The current probe only checks nesting depth via
arrayDepth but doesn't verify the array chain shape or final leaf; modify
probe(parse, depth) to, after parse(input) succeeds, walk the returned value
(use a local cur variable) depth times asserting at each level that
Array.isArray(cur) and cur.length === 1, then after the loop assert the final
cur === 1; keep the existing error handling and process.exit paths but replace
the loose arrayDepth check with this stricter single-element-chain and
leaf-value verification (refer to probe, arrayDepth, and makeInput to locate
relevant code).
In `@test/js/bun/shell/bunshell.test.ts`:
- Line 2909: Remove the explicit per-test timeout by deleting the trailing ",
60_000);" from the test closure (the literal "}, 60_000);") so the test uses the
suite default timeout; if you determine the extended timeout is truly necessary
(empirically for ASAN/debug lanes), instead keep the timeout but add a concise
comment above the test explaining why this test (buffer pinning/subprocess + 4MB
I/O) needs a 60s exception and reference the empirical evidence or CI flakes
that justify it.
In `@test/js/valkey/valkey.test.ts`:
- Around line 6906-7046: Extract the entire "Valkey incremental reply scanning"
describe block (including createMockValkeyServer, dripReply, parseCommandFrames,
and the contained test) out of the current Docker-backed test file and place it
into a new standalone test file alongside other non-Docker valkey tests; in the
new file keep the test name and logic intact and import any required symbols
(net, RedisClient, performance) so it runs independently, then remove the
original block from the Docker-backed file to avoid mixing non-Docker coverage
with Docker-based tests.
In `@test/js/web/fetch/body-clone.test.ts`:
- Around line 540-567: The test consumes each Response clone but never asserts
that clone.blob() yields a Blob with the overridden content-type, so add an
assertion that each consumed clone's Blob.type equals replacementType: after
clone.headers.set("content-type", replacementType) and await clone.blob(), check
the returned blob.type against replacementType (e.g., collect results from
clones.map/captured blob values) so the test fails if clone.blob() does not
honor the modified header; keep the existing churned logic and the final checks
for b1.type and churn-ok unchanged.
In `@test/js/web/fetch/fetch-http3-client.test.ts`:
- Around line 757-763: Replace the tempDir setup and running a temp file with
invoking Bun directly with -e: remove the tempDir("h3-tls-trust", {
"fixture.ts": fixture }) and the cwd:String(dir) usage and call Bun.spawn with
cmd: [bunExe(), "-e", fixture] (keeping the same env including
BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP3_CLIENT and stderr: "pipe") so the
single-file fixture runs via -e; update any references to dir/proc creation
accordingly while preserving await using proc and bunEnv usage.
---
Outside diff comments:
In `@src/install/npm.rs`:
- Around line 3148-3184: The code writes the dist-tag key into
all_extern_strings before validating tag.value, which causes overwrites when a
tag has a non-string value; in the loop in the dist-tags handling move the write
to all_extern_strings (currently at extern_strings_slice_start + dist_tag_i) to
after you confirm tag.value.as_string(&bump) succeeds and only then append the
key and the value to string_builder and assign both all_extern_strings[...] and
all_semver_versions[...] and increment dist_tag_i; ensure dist_tag_i is only
incremented when both key and value are valid (leave the counting pass that uses
unwrap_or(b"") unchanged).
🪄 Autofix (Beta)
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: dd125111-38f0-4bc3-ada8-dcc5a86463c2
📒 Files selected for processing (82)
packages/bun-debug-adapter-protocol/src/debugger/signal.tspackages/bun-usockets/src/crypto/openssl.cpackages/bun-vscode/src/features/diagnostics/diagnostics.tssrc/bun_core/string/immutable/escapeHTML.rssrc/cares_sys/c_ares.rssrc/css/css_parser.rssrc/glob/GlobWalker.rssrc/http/lib.rssrc/install/PackageInstaller.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/TarballStream.rssrc/install/bin.rssrc/install/extract_tarball.rssrc/install/lockfile.rssrc/install/lockfile/bun.lock.rssrc/install/lockfile/printer/Yarn.rssrc/install/migration.rssrc/install/npm.rssrc/js/node/_http_client.tssrc/js_parser_jsc/expr_jsc.rssrc/jsc/bindings/SQLClient.cppsrc/jsc/bindings/node/crypto/JSCipher.cppsrc/jsc/bindings/node/crypto/JSECDHPrototype.cppsrc/jsc/bindings/webcore/JSDOMFormData.cppsrc/jsc/bindings/webcore/SerializedScriptValue.cppsrc/md/ansi_renderer.rssrc/md/inlines.rssrc/parsers/yaml.rssrc/resolver/package_json.rssrc/runtime/api/JSON5Object.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/DevServer.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/run_command.rssrc/runtime/dns_jsc/dns.rssrc/runtime/node/node_fs.rssrc/runtime/server/RequestContext.rssrc/runtime/server/server_body.rssrc/runtime/shell/builtin/cd.rssrc/runtime/shell/states/Cmd.rssrc/runtime/shell/subproc.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/S3File.rssrc/semver/lib.rssrc/valkey/valkey_protocol.rstest/bake/dev/html.test.tstest/bundler/esbuild/packagejson.test.tstest/cli/install/bun-install-lifecycle-scripts.test.tstest/cli/install/bun-install-native-binlink.test.tstest/cli/install/bun-install-streaming-extract.test.tstest/cli/install/bun-install.test.tstest/cli/install/bun-lock.test.tstest/cli/install/migration/migrate.test.tstest/cli/run/run-shell.test.tstest/js/bun/css/doesnt_crash.test.tstest/js/bun/glob/scan.test.tstest/js/bun/http/bun-serve-file.test.tstest/js/bun/http/serve.test.tstest/js/bun/jsc/bun-jsc.test.tstest/js/bun/json5/json5.test.tstest/js/bun/md/md-edge-cases.test.tstest/js/bun/net/socket.test.tstest/js/bun/s3/s3-fd-validation.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/util/escapeHTML.test.jstest/js/bun/yaml/yaml.test.tstest/js/node/crypto/ecdh.test.tstest/js/node/dns/node-dns.test.jstest/js/node/fs/fs.test.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2-invalid-padding.test.tstest/js/sql/sql.test.tstest/js/valkey/valkey.test.tstest/js/web/fetch/blob.test.tstest/js/web/fetch/body-clone.test.tstest/js/web/fetch/fetch-http3-client.test.tstest/js/web/fetch/fetch.test.tstest/js/web/html/FormData.test.tstest/js/web/structured-clone-blob-file.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
bun/src/runtime/webcore/Blob.rs Line 2731 in cf62113 bun/src/runtime/webcore/Blob.rs Line 2959 in cf62113 We should use as_chunks instead chunks_exact here, see #31415 for details.
|
Collaborator
Author
|
Switched both misaligned UTF-16LE fallback sites in Blob.rs (the text() and json() paths) from |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/bun/http/serve.test.ts`:
- Around line 2333-2334: Replace the use of tmpdirSync() when creating the
unix-socket path with the test harness temporary directory helper: obtain the
harness tempDir and build the socket path from it (instead of join(tmpdirSync(),
"bun-info.sock")), so the unix variable uses the harness-provided temp
directory; keep the rest of the Bun.serve(...) usage unchanged and ensure
cleanup follows harness semantics.
🪄 Autofix (Beta)
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: a90e2ece-d511-471b-a515-3f0235c90557
📒 Files selected for processing (16)
src/install/PackageInstaller.rssrc/install/TarballStream.rssrc/install/extract_tarball.rssrc/install/lockfile/Package.rssrc/md/ansi_renderer.rssrc/runtime/server/server_body.rssrc/runtime/webcore/Blob.rstest/cli/install/bun-install-native-binlink.test.tstest/cli/install/bun-install.test.tstest/cli/install/bun-lock.test.tstest/cli/install/migration/migrate.test.tstest/js/bun/http/serve.test.tstest/js/bun/json5/json5.test.tstest/js/valkey/valkey-incremental-scan.test.tstest/js/web/fetch/body-clone.test.tstest/js/web/fetch/fetch-http3-client.test.ts
Contributor
There was a problem hiding this comment.
No issues found on this pass — both of my earlier inline points (unix-socket /bun:info and the valkey test Buffer.alloc idiom) are addressed. Deferring to a human reviewer given the breadth (83 files across install/TLS/crypto/serialization) and the security-sensitive nature of several changes.
Extended reasoning...
Overview
This is round 6 of the recurring hardening series (continuation of #31339). It touches 83 files across ~27 subsystems: package-manager path containment and lockfile escaping, TLS handshake state in openssl.c, crypto error paths (JSECDHPrototype.cpp, JSCipher.cpp, SerializedScriptValue.cpp non-extractable CryptoKey), HTTP framing (Transfer-Encoding, h2 padding zero-fill, h3 TLS-option exclusion), Blob/Body content-type ownership and structured-clone slice windowing, parser bounds (CSS tokenizer, YAML merge budget, markdown emphasis linearization, JSON5/JSONC stack checks), shell redirect buffer pinning, node_fs writeFile pinning and recursive-readdir path bounds, and assorted CLI/dev-server/vscode hardening. 34 regression tests accompany the changes.
Security risks
The PR is itself a security-hardening pass, so the relevant risk is regressions in the tightened paths rather than new attack surface. Areas where a subtle mistake would matter most: the openssl.c one-liner re-establishing per-loop BIO state before driving the handshake, the install bin-target realpath containment check (intentionally parent-only, with a clear rationale in-thread), the lockfile/yarn.lock JSON-escaping (must not break round-tripping of legitimate values), the Blob structured-clone offset rewrite (changes wire shape for memory-backed slices), and the has_trusted_dependency keying change (alias vs. resolved name for non-npm resolutions). All of these have targeted tests, and the author's in-thread responses to CodeRabbit's findings were detailed and correct where I spot-checked (libdeflate bound, semver pool collision, bin parent-only realpath).
Level of scrutiny
High. This is not a mechanical change — it modifies behaviour in production-critical paths (package installation trust/extraction, TLS handshake sequencing, crypto error handling, structured-clone wire format). Each individual fix is small and well-scoped, but the aggregate is large enough that a human should at minimum skim the install-path-containment, TLS, and Blob-serialization hunks. The pattern matches the previous five rounds, which lowers risk somewhat, but does not make it auto-approvable.
Other factors
- All 17 CodeRabbit inline threads and both of my prior inline comments are resolved, with several follow-up commits (a246cc7, cf62113, 8911f30, c550607) landing fixes.
- A drive-by review comment about
as_chunksvschunks_exactwas also applied. - CI shows one failure (
test-worker-message-port-transfer-terminate.jsSIGABRT on x64-asan) that is unrelated to any file in this diff. - CodeRabbit suggested
aliias reviewer; this matches the kind of PR that benefits from a maintainer pass rather than bot approval.
Jarred-Sumner
commented
May 26, 2026
Jarred-Sumner
commented
May 26, 2026
Jarred-Sumner
commented
May 26, 2026
Jarred-Sumner
commented
May 26, 2026
Jarred-Sumner
commented
May 26, 2026
Jarred-Sumner
commented
May 26, 2026
Jarred-Sumner
commented
May 26, 2026
Jarred-Sumner
commented
May 26, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/cli/install/migration/migrate.test.ts (1)
378-393:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the install-result guard before artifact reads, and capture stdout for diagnostics.
This new migration test reintroduces the same failure-masking pattern: it reads
node_modules/.../package.jsonandbun.lockbefore asserting process success, and it dropsstdoutentirely. If install fails, artifact reads can throw and hide the real subprocess failure context.Suggested fix
await using proc = Bun.spawn([bunExe(), "install"], { env: bunEnv, cwd: testDir, - stdout: "ignore", + stdout: "pipe", stderr: "pipe", }); - const [err, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + const [stdout, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(err).not.toContain("InvalidNPMLockfile"); expect(err).toContain("migrated lockfile from package-lock.json"); + expect({ stdout, exitCode }).toMatchObject({ exitCode: 0 }); expect(await Bun.file(join(testDir, "node_modules", "baz", "package.json")).json()).toHaveProperty( "version", "0.0.3", ); expect(fs.existsSync(join(testDir, "bun.lock"))).toBeTrue(); - expect(exitCode).toBe(0);Based on learnings: In
test/cli/install/**/*.test.ts, keep an earlyexpect({ stdout, exitCode }).toMatchObject({ exitCode: 0 })guard before reading generated artifacts, and avoid dropping stdout context.🤖 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/migration/migrate.test.ts` around lines 378 - 393, The test currently drops stdout and reads produced artifacts before asserting the subprocess succeeded; change Bun.spawn to capture stdout (use "pipe" instead of "ignore"), await both proc.stdout.text() and proc.exited (in addition to proc.stderr.text()), then add an early guard like expect({ stdout, exitCode }).toMatchObject({ exitCode: 0 }) (using the captured stdout and exitCode variables) before any Bun.file(...) or fs.existsSync(...) reads; keep the existing stderr assertions (err) and message checks but move artifact reads after the success guard so failures report process output instead of throwing.
🤖 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.
Duplicate comments:
In `@test/cli/install/migration/migrate.test.ts`:
- Around line 378-393: The test currently drops stdout and reads produced
artifacts before asserting the subprocess succeeded; change Bun.spawn to capture
stdout (use "pipe" instead of "ignore"), await both proc.stdout.text() and
proc.exited (in addition to proc.stderr.text()), then add an early guard like
expect({ stdout, exitCode }).toMatchObject({ exitCode: 0 }) (using the captured
stdout and exitCode variables) before any Bun.file(...) or fs.existsSync(...)
reads; keep the existing stderr assertions (err) and message checks but move
artifact reads after the success guard so failures report process output instead
of throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 576dddc3-be29-40b5-a978-2497e98d448e
📒 Files selected for processing (17)
src/install/bin.rssrc/install/extract_tarball.rssrc/jsc/array_buffer.rssrc/jsc/node_path.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/bun/subprocess/Readable.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rssrc/runtime/server/server_body.rssrc/runtime/shell/states/Cmd.rssrc/runtime/webcore/Body.rssrc/sys/lib.rssrc/sys/linux_syscall.rssrc/uws_sys/Response.rstest/cli/install/bun-install-native-binlink.test.tstest/cli/install/bun-install-streaming-extract.test.tstest/cli/install/migration/migrate.test.ts
This reverts commit cb0da77.
Jarred-Sumner
force-pushed
the
claude/security-round-6
branch
from
8fa2f1f to
05f0e92
Compare
Jarred-Sumner
added a commit
that referenced
this pull request
May 28, 2026
…ems (round 7) (#31495) Tightens input validation, bounds checking, protocol framing, and state handling across the package manager, HTTP stack, sockets/TLS, SQL clients, and parsers. Continuation of #31417 (round 6); same structure — small per-area commits with regression tests included. ### Package manager / CLI - install: validate extract folder names and dependency aliases for the remaining resolution shapes; bounds-check resolution lookups during lockfile loads - install: trusted-dependency entries for non-npm resolutions only apply when the dependency is declared by the root or a workspace; the security scanner is also told about packages reached through non-npm parents - install: registry auth tokens stay scoped to their configured host (and are not carried across an https→http redirect) - pm: `bun pm cache rm` resolves the cache directory from the process environment, ignoring project-local overrides for the destructive delete (`bun pm cache` prints the same path it would delete) - pack: validate the name/version used to form the tarball filename - compile: build artifacts are created exclusively with owner-only modes; repl: the history file is created 0600 (and an existing one is tightened) - yarn.lock migration behavior is unchanged; a comment now documents the intentionally lenient handling of entries without integrity ### HTTP / HTTP2 / WebSocket / S3 - fetch: never forwards a caller-supplied Content-Length that disagrees with the actual body (node:http behavior unchanged); drains interim 1xx bytes as they are parsed; never returns a connection with a framing violation to the keep-alive pool - node:http2: rejects requests carrying connection-specific headers, repeated single-value/pseudo headers, or content-length/DATA mismatches (verified against Node/nghttp2 behavior); copies queued DATA frames before re-entrant writes; keeps parsing safe when a socket chunk's buffer is transferred mid-frame - Bun.serve: absolute-form request targets are routed by path with `request.url` derived from Host; no message-body bytes are written for null-body statuses (101/103/204/205/304) — Response construction is unchanged - websocket: handshake buffering survives client teardown from the open handler - S3: object keys keep `?`/`#` characters end-to-end; list-response parsing is linear; the remote-address accessor copies into caller-owned storage ### Sockets / TLS / SQL / runtime - sql: an explicit `tls`/`ssl` option aborts the connection before credentials are sent if the server declines TLS; certificate-verification behavior is unchanged. Row construction is bounded by the declared structure; MySQL rebuilds cached row shapes when result columns change; postgres protocol counts are validated against the received bytes - node:tls: server sockets only report `authorized: true` after a client certificate was requested and verified (matches observed Node behavior) - sockets: handler reload and accepted-socket reuse re-validate ownership before freeing; the TLS read path restores shared dispatch state after re-entrant callbacks; pipe reads re-acquire the scratch buffer per use - sqlite: the cross-worker database registry and lazy library load are lock-protected; spawned children close descriptors across the full fd range - crypto: encoded-hash cost parameters are bounded before verification; errors: stack-frame callees stay GC-reachable while formatting ### Parsers / shell / node - markdown: reference-definition lookup, table column counts, and autolink scanning are linear/bounded - YAML: alias expansion is budgeted in the parser used by imports and the bundler - node:fs: watcher path joins surface ENAMETOOLONG instead of aborting; assert: diff output is bounded for large operands ### Tests 33 new regression tests across the touched areas (validated to fail on the released build and pass here), plus a handful of regression-only cases where the released build does not exhibit the behavior deterministically. A few fixes have no standalone test where the change is not observable from JS. `cargo check` and the cross-target checks pass on all 10 CI targets; the touched suites pass locally with no new failures versus main. The branch is rebased on current main. ### Changes tried and reverted during review (rationale) - **Lockfile lifecycle scripts** — an earlier commit re-derived dependency lifecycle script text from each installed package's `package.json` instead of the deserialized lockfile data; reverted because `bun.lockb` intentionally stores and uses the script text, and that design stays. - **yarn.lock migration** — an earlier commit rejected off-registry tarball URLs without integrity during migration; reverted because yarn v1 lockfiles legitimately contain such entries (workspace, `file:`, codeload) and migration must not fail closed. The lenient handling is now documented with a comment at the site. - **`Response` and null-body statuses** — both the body-dropping and the later spec-style `TypeError` for non-null bodies with 101/103/204/205/304 were reverted: existing code and frameworks (including Elysia) construct such responses and expect the body to be ignored, so changing construction is a breaking change nobody asked for. Only the wire-level behavior (no body bytes emitted) is kept. - **SQL client TLS** — an earlier commit made an explicit `tls`/`ssl` option imply certificate verification; narrowed to abort-on-decline only because the unverified default for these options is intentional and matches other JavaScript database clients. - **node:http `path` with an absolute URI** — an earlier commit rewrote such requests to a path-only target; removed because measured Node behavior does not do that, and Node compatibility takes precedence. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Jarred-Sumner
added a commit
that referenced
this pull request
May 29, 2026
…(round 8) (#31559) Tightens input validation, bounds checking, and pointer/lifetime handling across the package manager, node:http/http2, the debug adapter, the V8 shim, S3, and path/fs internals. Continuation of #31417 and #31495 (rounds 6–7); same structure — small per-area commits with regression tests included. ### Package manager / build - install: validate dependency-alias-derived names at the remaining cache-index call site; validate git/github resolved values when loading `bun.lockb` (same check the text lockfile applies) - compile: bounds-check Mach-O segment offsets/sizes with overflow-checked arithmetic before patching; a corrupted template now errors instead of crashing - webview: the default Chrome profile directory is created with an unpredictable name and owner-only permissions ### node:http / node:http2 / debugger / V8 shim - node:http: a `host` option containing URL-reserved delimiters is treated as an unresolvable hostname instead of being re-parsed into a different target - node:http2: the raw-socket TLS upgrade path forwards the server's `requestCert`/`rejectUnauthorized` configuration into the native TLS config explicitly - debug adapter: only allowlisted inspector event domains from the debug target are dispatched; the diagnostics signal listener binds to loopback - v8 shim: `String::Utf8Length`/`WriteUtf8` saturate at `INT_MAX` instead of wrapping for very large strings ### Runtime / fs / S3 / path - node:fs: the read path re-fetches the destination buffer after argument coercion can run user code; buffer-backed path arguments are pinned for the duration of async operations - S3: a malformed multipart upload ID from the server surfaces as a request error instead of aborting the process - node:path: `path.format` sizes its scratch buffer for the implicitly inserted extension separator, removing a reachable abort ### Not changed The bin-link containment behavior for dotless symlinked targets and the binary-lockfile trust policy are unchanged (both previously decided); the file-backed Blob deserialization finding is already covered by the open #31329. ### Tests 9 validated regression tests (each fails on the released build — two of them by crashing it — and passes here), plus 2 regression-locked tests for repo-TypeScript debugger changes. The Chrome profile-dir and h2-upgrade changes ship without standalone tests (no non-flaky observable in existing files; noted per commit). `cargo check` and the cross-target checks pass on all 10 CI targets; the touched suites pass locally with no new failures versus main. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This was referenced May 30, 2026
Jarred-Sumner
added a commit
that referenced
this pull request
Jun 2, 2026
…ms (round 9) (#31606) Tightens input validation, pointer/lifetime handling, and Node-compatibility across the package manager, bun-lambda, node:zlib/net/http, Glob, the transpiler cache, postgres, and HTMLRewriter. Continuation of #31417, #31495, and #31559; same structure — small per-area commits with regression tests included. ### Scaffolding / Lambda - create: dependency names detected from scanned source files are passed to the spawned installer as positionals (after `--`), so a specifier can never be interpreted as a flag - bun-lambda: event-to-Request conversion builds the URL from the request context's domain; client-supplied path bytes can no longer change the URL authority ### node runtime - node:zlib: native compression handles re-resolve the caller-supplied output array on every write completion instead of caching a raw pointer, and reject detached/resized buffers - node:net: BlockList subnet matching handles IPv4-mapped IPv6 rules the same way Node does (verified against Node's behavior) - node:http: duplicate request headers follow Node's IncomingMessage precedence — first value wins for singleton headers, others are joined, `set-cookie` is arrayed (verified against Node's behavior) - Glob: an oversized `cwd` option throws a catchable error instead of aborting the process ### Runtime internals - transpiler: cached module metadata records with out-of-range string indices are rejected at load (previously a corrupted cache record could crash the process) - postgres: unknown backend messages are skipped by exactly their declared length, and NotificationResponse is consumed through the protocol decoder, so the stream can no longer desynchronize - HTMLRewriter: `comment.replace()` removes the original comment and emits only the replacement (previously both appeared in the output) ### Not changed The bin-link containment behavior for symlinked targets is unchanged (previously decided); the recursive-delete traversal change is already covered by the open #31337. ### Tests 11 regression tests across the touched areas. Most are validated to fail on the released build and pass here — including one where the released build segfaults on a corrupted cache record and one where it hangs on an oversized Glob cwd. The zlib write-state tests are regression-only (the released build does not exhibit the failure deterministically without ASAN). `cargo check` and the cross-target checks pass on all 10 CI targets; the touched suites pass locally with no new failures versus main. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
This was referenced Jun 16, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jun 17, 2026
…ns (#32452) ## Repro In `test/`: ``` ❯ bun install error: refusing to install dependency react with unsafe folder path "../node_modules/react" error: refusing to install dependency react with unsafe folder path "../node_modules/react" ... ``` Minimal case: a root package.json with ```json { "dependencies": { "pkg-a": "file:./pkg-a", "shared": "file:../shared" }, "resolutions": { "shared": "file:../shared" } } ``` where `pkg-a` depends on `shared`. The `resolutions` entry applies the `../shared` path to pkg-a's transitive `shared` dep, and the escape check added in #31417 rejects it in both the enqueue path (`Could not find package.json for "file:../shared"`) and, when a lockfile already exists, the install path (`refusing to install dependency shared with unsafe folder path "../shared"`). ## Cause `PackageManagerEnqueue.rs` and `PackageInstaller.rs` gate the escape check on `is_workspace_dependency(id)` / `is_workspace_tree_id(tree)`. A `resolutions`/`overrides` entry is declared in the root package.json but is applied to dependencies owned by transitive packages and installed into nested trees, so both predicates are false and the user's own path is rejected. ## Fix `lockfile.overrides` is only ever populated from the root package.json (`Package.rs` guards `parse_append` behind `FEATURES.is_main`). Skip the escape check at both sites when the dependency name has an entry in the override map; the path it resolved to came from the root package.json and is trusted the same as a direct dependency. The overlong path check is kept unconditionally. ## Tests Six new cases in `test/cli/install/bun-install.test.ts`: - fresh install + install from existing lockfile, for both `resolutions` and `overrides` - two guard tests: a transitive package's own escaping `file:` dep is still rejected when a different name is in `overrides`/`resolutions` ``` bun bd test test/cli/install/bun-install.test.ts -t "pointing outside the project when it came from root" 4 pass, 0 fail bun bd test test/cli/install/bun-install.test.ts -t "still rejects transitive file" 2 pass, 0 fail bun bd test test/cli/install/bun-install.test.ts -t "does not install transitive file: dependencies" 2 pass, 0 fail bun bd test test/cli/install/overrides.test.ts 7 pass, 0 fail ``` The four positive tests fail on the released build with the exact errors above. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tightens input validation, bounds checking, and state handling across the runtime, package manager, and CLI. Continuation of #31339; same structure (small per-area commits, regression tests included).
Package manager / CLI
package-lock.json(yarn.lock migration unchanged — its v1 workspace/github entries legitimately omit integrity)file:dependency targets to their declaring packagebun.lock; escape quoted scalars in the yarn.lock printernode_modules/.binHTTP / TLS / networking
options.portin ClientRequest (ERR_SOCKET_BAD_PORT)/bun:inforoute to loopback clientsWeb / runtime APIs
typeoption with the same check other Blob constructors useParsers / shell / misc
cdarguments literallyTests
34 new regression tests across the touched areas, each verified to fail on the released build and pass here. A few fixes have no standalone test where the behavior difference is not observable from JS (noted per-commit in the shard history); CI exercises the affected suites.
cargo checkand cross-target checks pass on all 10 CI targets; the touched test suites pass locally with no new failures versus main.