Hardening: input validation and bounds checking across 12 subsystems (round 8)#31559
Conversation
|
Updated 11:35 AM PT - May 29th, 2026
✅ @Jarred-Sumner, your commit bd18e22cb70c5dd35429add0d5d273e0780c982d passed in 🧪 To try this PR locally: bunx bun-pr 31559That installs a local version of the PR into your bun-31559 --bun |
|
Caution Review failedPull request was closed or merged during review WalkthroughThis PR hardens Bun's security and correctness across debug adapters, executable handling, package management, filesystem APIs, and input validation. It adds event filtering, bounds checking, path traversal prevention, buffer lifecycle management, host/upload-id validation, numeric saturation, and loopback/temp-directory isolation, with 12 new test suites covering the changes. ChangesSecurity and Safety Improvements
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this hardening round spans 12 security-sensitive subsystems (unsafe Mach-O pointer arithmetic, TLS auth forwarding, path-traversal guards, buffer lifetime/pinning in fs) and CI is currently red on most build-rust targets, so it's worth a human look before merging.
Extended reasoning...
Overview
This PR applies input-validation and bounds-checking hardening across 23 files in 12 distinct subsystems: the package manager (cache-index alias validation, binary lockfile git-tag validation), bun build --compile Mach-O patching, node:http host-option validation, node:http2 raw-socket TLS upgrade, the debug adapter (inspector event allowlisting, loopback-only signal listener), the V8 shim (Utf8Length/WriteUtf8 saturation), node:fs (buffer re-fetch after coercion, async path pinning), S3 multipart upload-ID validation, node:path scratch-buffer sizing, and the Chrome webview profile-dir creation. Each change ships with a regression test.
Security risks
Nearly every change here is security-relevant by design: path-traversal mitigation in extract_tarball.rs/lockfile.rs, host/SSRF confusion mitigation in _http_client.ts, mTLS config propagation in _http2_upgrade.ts, OOB memmove prevention in macho.rs (unsafe Rust with raw pointer arithmetic), use-after-detach prevention in node_fs.rs/types.rs/node_path.rs, and reducing local-network attack surface in signal.ts. The changes are defensive (tightening, not loosening), but several touch unsafe blocks and GC-lifetime invariants where a subtle mistake would itself be a memory-safety bug. The WriteUtf8 change from SIZE_MAX to INT_MAX for negative length is a behavior change worth a second look.
Level of scrutiny
High. This is exactly the category of PR — security hardening across many production-critical paths, with unsafe Rust and TLS/auth handling — where the approval criteria say not to auto-approve. The individual patches are small and well-commented, and the regression tests are thorough, but the breadth and the memory-safety/auth surface argue for human review.
Other factors
robobun reports build-rust failures on 9+ CI targets for the merge commit (the autofix commit landed afterward; the description claims all 10 targets pass, so there's a discrepancy worth confirming). No prior human or claude[bot] review exists on the timeline. The bug-hunting system found nothing, which is reassuring, but does not change the scrutiny bar for a 12-subsystem security PR.
| Valid::path_null_bytes(buffer.slice(), ctx)?; | ||
|
|
||
| let buffer = if arguments.will_be_async { | ||
| Buffer::from_js_pinned(ctx, arg).unwrap_or(buffer) |
There was a problem hiding this comment.
This makes it slower. One pass. Not 2.
There was a problem hiding this comment.
Restructured to one pass. When the call will be async, the single conversion is now Buffer::from_js_pinned (pin + one extraction), with the plain conversion as the fallback — same shape as the existing async StringOrBuffer path earlier in this file — and the sync path is a single from_typed_array/from_array_buffer exactly as on main. The second walk over the argument is gone. Validation runs on the pinned buffer and the throw paths release the pin before propagating, so the Drop balance in PathLike stays correct. String path and sync path do the same work as main; the async buffer path only adds the pin itself. The fs.promises.writeFile buffer-path test on the branch still passes and still fails on released Bun.
| } else { | ||
| 0.0 | ||
| }; | ||
| let buffer = if length_value.is_some_and(|arg| !arg.is_number()) { |
There was a problem hiding this comment.
Do not make it slower. One pass. Not two.
There was a problem hiding this comment.
Reordered to a single pass — the re-fetch is gone. The buffer pointer/length is now captured exactly once, immediately after the offset/length arguments are processed (length's ToNumber is the only step there that can re-enter JS), so nothing captured before that point is used after it. The all-numbers fast path does exactly the same operations as before this change set: fd, offset validation, length ToNumber, then one Buffer::from_js — no added conversions or branches, just a different order. The readSync detach test still fails on the released build and passes here, test-fs-read* parallel tests pass, and a readSync loop microbench is at parity with main.
|
The Windows failures on the Utf8Length bounds test were not an allocation/string-limit problem: in all three Windows job logs the addon built and the oversized string correctly reported "Utf8Length = 2147483647". The only mismatch was a trailing \r on the first output line (printf emits \r\n on Windows and the test split stdout on "\n"). Fixed in 4e2b03a by splitting on /\r?\n/ instead, which keeps the Windows coverage intact rather than skipping it; linux/macos validation and the C++ change are unchanged. |
![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 `@packages/bun-debug-adapter-protocol/src/debugger/sourcemap.test.ts`:
- Around line 72-74: Replace the pre-allocated port flow that uses
getAvailablePort() with binding the socket to port 0: construct TCPSocketSignal
with port: 0, await signal.ready, then read the assigned port from the signal
instance (use the property/method on TCPSocketSignal that exposes the actual
listening port) instead of using the previously created port variable; remove
getAvailablePort() usage and any reliance on a precomputed port when invoking
TCPSocketSignal and subsequent test code.
In `@test/cli/install/bun-lockb.test.ts`:
- Around line 346-367: The test reads the process exit status into the variable
code (via exited) but never asserts it; add a final assertion for the expected
non-zero exit code at the end of the tampered-lockfile test block (after the
exists() assertion). Specifically, after the existing expectations referencing
spawn, stderrForInstall, out, and exists(join(packageDir, "node_modules",
"dep")), add an assertion on code (e.g., expect(code).not.toBe(0) or the
concrete non-zero exit value your suite uses) so the exit status is checked last
and failures remain diagnosable.
- Around line 269-270: Update the test's hardcoded git URL to use port 0 instead
of 1: change the string assigned to gitUrl (currently
"git+ssh://git@127.0.0.1:1/example/repo.git") to use ":0" so it becomes
"git+ssh://git@127.0.0.1:0/example/repo.git" (leave sha untouched); this ensures
the test follows the harness rule to always use port: 0 and avoids
environment-dependent failures when running the tests.
🪄 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: 8f90be47-add7-4797-bcd6-8c0b83add65b
📒 Files selected for processing (23)
packages/bun-debug-adapter-protocol/src/debugger/adapter.tspackages/bun-debug-adapter-protocol/src/debugger/signal.tspackages/bun-debug-adapter-protocol/src/debugger/sourcemap.test.tssrc/exe_format/macho.rssrc/install/extract_tarball.rssrc/install/lockfile/bun.lockb.rssrc/js/node/_http2_upgrade.tssrc/js/node/_http_client.tssrc/jsc/bindings/v8/V8String.cppsrc/jsc/node_path.rssrc/runtime/node/node_fs.rssrc/runtime/node/path.rssrc/runtime/node/types.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webview/ChromeProcess.rstest/bundler/bundler_compile.test.tstest/cli/install/bun-install.test.tstest/cli/install/bun-lockb.test.tstest/js/bun/s3/s3.test.tstest/js/node/fs/fs.test.tstest/js/node/http/node-http.test.tstest/js/node/path/parse-format.test.jstest/v8/v8.test.ts
|
Looked into the darwin-14-x64 timeout for "rejects a binary lockfile whose git resolved tag contains path separators" — classifying it as runner congestion, not a hang in the test:
So nothing in the test can block indefinitely on macOS; the 90s was spent on a congested runner running three sequential debug-build installs plus the shared Verdaccio fixture. No code change. |
|
Investigated with the build 58946 darwin-14-x64 job logs rather than guessing: every attempt of the new test spends its full 90s with the last output being an interactive ssh prompt Root cause: the "unreachable" loopback port in the ssh URL ( Fix in 7a35ffa (no assertion weakened, test not skipped): the spawned installs now pass GIT_SSH_COMMAND="ssh -oBatchMode=yes -oStrictHostKeyChecking=accept-new -oConnectTimeout=5" and GIT_TERMINAL_PROMPT=0, so the fallback git clone fails fast everywhere instead of prompting. The same one-line env override is applied to the v1 block of lockfile-version-2.test.ts since it hits the identical hang on these jobs and a rebuild would not clear it. No rebuild triggered — the failure is in the test fixtures, not the infrastructure. |
…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>
2 participants
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
bun.lockb(same check the text lockfile applies)node:http / node:http2 / debugger / V8 shim
hostoption containing URL-reserved delimiters is treated as an unresolvable hostname instead of being re-parsed into a different targetrequestCert/rejectUnauthorizedconfiguration into the native TLS config explicitlyString::Utf8Length/WriteUtf8saturate atINT_MAXinstead of wrapping for very large stringsRuntime / fs / S3 / path
path.formatsizes its scratch buffer for the implicitly inserted extension separator, removing a reachable abortNot 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 checkand the cross-target checks pass on all 10 CI targets; the touched suites pass locally with no new failures versus main.