Hardening: input validation and compatibility fixes across 9 subsystems (round 9)#31606
Conversation
|
Updated 2:05 PM PT - Jun 2nd, 2026
❌ @robobun, your commit 054a9fc has some failures in 🧪 To try this PR locally: bunx bun-pr 31606That installs a local version of the PR into your bun-31606 --bun |
|
Warning Review limit reached
More reviews will be available in 28 minutes and 24 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughHardens request authority and input validation, adds duplicate-header semantics and Postgres notification handling, refactors zlib write-state caching to JS-backed properties, improves BlockList subnet matching, and expands tests for these edge cases. ChangesLambda Security, PostgreSQL, Node.js, and Input Validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/run/transpiler-cache.test.ts`:
- Around line 351-355: The test asserts first.exitCode before other expectations
which reduces failure context; update the run block using the variable first
(from run()) so that you check stderr/stdout and existsSync(cache_dir) first and
assert expect(first.exitCode).toBe(0) last, i.e., move the exit code assertion
for the 'first' run to the end of that block after the other expects.
In `@test/js/node/zlib/zlib-handle-bounds-check.test.ts`:
- Around line 143-145: The test currently only awaits proc.stdout.text() and
proc.exited and asserts stdout and exitCode separately, which can hide crash
diagnostics; update the await to capture stderr as well (e.g., await
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])) and replace
the separate expects with a single assertion on the combined result object
(include stdout.trim(), stderr.trim(), and exitCode) so stdout/stderr/exit are
all visible together when the subprocess fails; locate and change the variables
used in this test (proc, stdout, stderr, exitCode) and the two expect calls to
assert the combined object.
🪄 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: 172b0cc1-c23d-49a1-a29e-0f4feaeac6bf
⛔ Files ignored due to path filters (1)
test/cli/create/__snapshots__/create-jsx.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (24)
packages/bun-lambda/runtime.tssrc/bun_core/util.rssrc/bundler_jsc/analyze_jsc.rssrc/jsc/bindings/NodeHTTP.cppsrc/lolhtml_sys/lol_html.rssrc/runtime/api/glob.rssrc/runtime/api/zlib.classes.tssrc/runtime/cli/create/SourceFileProjectGenerator.rssrc/runtime/node/net/BlockList.rssrc/runtime/node/node_zlib_binding.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rssrc/sql_jsc/postgres/PostgresRequest.rssrc/sql_jsc/postgres/PostgresSQLConnection.rstest/cli/create/create-jsx.test.tstest/cli/run/transpiler-cache.test.tstest/integration/bun-lambda/bun-lambda.test.tstest/js/bun/glob/scan.test.tstest/js/node/http/node-http.test.tstest/js/node/net/node-net.test.tstest/js/node/zlib/zlib-handle-bounds-check.test.tstest/js/sql/postgres-multi-statement-fields.test.tstest/js/workerd/html-rewriter.test.js
![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 correctness issues, but this touches memory-safety and security-sensitive paths across nine subsystems (zlib pointer lifetime, NodeHTTP.cpp header handling, lambda URL authority, argv injection), so it warrants a human review.
Extended reasoning...
Overview
This PR makes hardening/compatibility changes across 9 distinct subsystems (25 files): bun-lambda URL construction, bun create argv building, node:zlib native handle write-state pointer handling, node:net BlockList IPv4-mapped-IPv6 subnet matching, node:http duplicate-header precedence (~110 lines of new C++ in NodeHTTP.cpp), Glob cwd length validation, transpiler-cache module-record index validation, Postgres protocol message framing, and HTMLRewriter comment.replace(). Each change ships with a regression test.
Security risks
Several changes are explicitly security-motivated: the lambda runtime now derives the request authority from requestContext.domainName and assigns the path via url.pathname so a //attacker.example/... path can't rewrite the origin; bun create inserts -- before scanned dependency names so a specifier can't be parsed as an installer flag; the zlib refactor removes a cached raw *mut u32 into a JS-owned ArrayBuffer in favor of re-resolving the typed array on each completion; and the transpiler-cache deserializer rejects out-of-range string indices that previously crashed the process. These look correct to me, but they sit on attack-surface boundaries and merit a second pair of eyes.
Level of scrutiny
High. The zlib change rewires pointer lifetime across three native handle types plus the codegen values: list; NodeHTTP.cpp adds a new slow-path buildHeadersObjectHandlingDuplicates with non-trivial JSC object/identifier handling (including the parseIndex numeric-header edge case); and the BlockList change alters IP-matching semantics. None of these are mechanical.
Other factors
The bug-hunting pass surfaced only a test-convention nit (explicit per-test timeouts), which doesn't affect correctness. CI shows a pre-existing -no-pie linker warning unrelated to this diff. Given the breadth and the security-adjacent nature of several fixes, I'm deferring rather than approving.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can you reach into the internals of PutPropertySlot to see where it finds out if there's an existing property there and then if so, we do the right thing - avoiding the extra property lookup in the rare case?
|
@robobun Can you reach into the internals of PutPropertySlot to see where it finds out if there's an existing property there and then if so, we do the right thing - avoiding the extra property lookup in the rare case? |
|
✅ Reworked per feedback in 054a9fc.
Tests (incl. triples, mixed-case, empty-first-value, CI context from the previous rounds: all Linux/Windows/ASAN lanes green across three builds; |
…ot offset When putDirect reports ExistingProperty, the slot has already recorded the property's offset, so the duplicate can be fixed up with putDirectOffset at that offset instead of flagging the request and rebuilding the whole headers object with a second pass of property lookups. The merged value is re-derived from the raw header list, which also handles names that differ only by case. Index-shaped names, which bypass PutPropertySlot, check getDirectIndex directly instead of forcing a rebuild.
There was a problem hiding this comment.
No issues found, but this touches nine subsystems including low-level JSC property-slot internals in NodeHTTP.cpp, memory-lifetime changes in the zlib write-state path, and security-relevant Lambda URL/argv handling — worth a human pass, and the macOS x64 sql.test.ts / sql-prepare-false.test.ts failures may be related to the Postgres framing changes.
Extended reasoning...
Overview
This PR makes hardening and Node-compat fixes across 26 files in nine independent subsystems: bun-lambda request-URL construction, bun create argv building, node:zlib write-state lifetime, node:net BlockList IPv4-mapped matching, node:http duplicate-header merging in NodeHTTP.cpp, Glob cwd validation, transpiler-cache module-record bounds checking, Postgres unknown-message framing + NotificationResponse handling, and HTMLRewriter comment.replace. Each area ships with a regression test.
Security risks
Several changes are explicitly security-relevant: the Lambda runtime now derives the request authority from requestContext.domainName and assigns pathname separately so a client-supplied path can't rewrite the origin; SourceFileProjectGenerator inserts -- before scanned dependency names so a specifier can't be parsed as a CLI flag; the zlib handles stop caching a raw *mut u32 into a JS-owned ArrayBuffer and re-resolve it per write; and the bundler rejects out-of-range string indices in cached module records. These all narrow attack surface rather than widen it, but they are exactly the kind of changes that benefit from a second set of eyes.
Level of scrutiny
High. The NodeHTTP.cpp change went through three iterations during review and now relies on PutPropertySlot::ExistingProperty + putDirectOffset / structure()->didReplaceProperty() to merge duplicate headers in place — subtle JSC internals on a hot path. The zlib change removes a native pointer field across three codegen-backed classes. The Postgres change alters wire-protocol framing for unknown messages. None of these are mechanical.
Other factors
The author has already self-reviewed and addressed all inline comments (CodeRabbit's two nits and my earlier note about per-test timeouts were fixed in e005625; the NodeHTTP.cpp structure concerns were resolved in 3e0ef3d and 1ffc4ca). The bug-hunting system found nothing. However, CI on the latest commit shows sql.test.ts and sql-prepare-false.test.ts failing on macOS 14 x64, which overlaps the Postgres protocol changes here and should be confirmed as pre-existing flakes before merge. Given the breadth, the JSC-internals work, and the open SQL failures, I'm deferring rather than approving.
|
Still not correct. It shouldn't be creating temporary JSString (no rope). It should not call putDirect before checking if it exists (using the code path the PutPropertySlot uses). |
…ings Replace the put-then-fixup pattern: each header name is looked up with getDirectOffset (the same structure lookup putDirect's replace path uses) before anything is stored. On a duplicate the first value is still intact at the returned offset, so singleton headers keep it with no write at all and joinable/cookie headers merge through putDirectOffset. The merged value is built with tryMakeString as a single flat string instead of a rope, and the temporary separator JSString in the index-name path is gone.
|
Addressed in 054a9fc. The header loops no longer put before checking: each name's own-property offset is looked up first via |
### What does this PR do? Fixes #23149: a response that declares a `Content-Encoding` but sends **zero body bytes** — e.g. an empty chunked gzip response — failed with `ZlibError`. The decompressor ran on zero input at stream end and reported a truncated stream; Node resolves these as an empty body. ```js // server: HTTP/1.1 200 OK // Content-Encoding: gzip // Transfer-Encoding: chunked // // 0\r\n\r\n const res = await fetch(url); await res.text(); // before: throws ZlibError — after: "" ``` The fix is an early-out in `decompress_bytes`: when the response delivered no body bytes at all (`buffer` empty and `total_body_received == 0`), there is nothing to decompress. An empty final flush *after* real data still flushes normally (`total_body_received` is non-zero on that path). ### Tests Two cases added to `test/js/web/fetch/fetch-gzip.test.ts` using a raw socket server: empty gzip body via chunked encoding (fails on released Bun with `ZlibError`, passes with this PR) and via `Content-Length: 0` (already worked; pinned). `fetch-gzip.test.ts` + `fetch.test.ts` suites pass locally (the only failures are pre-existing external-network tests that fail identically on released Bun in this environment). ### CI notes - The "multiple chunks, TCP server" test in the same file was pinned to `127.0.0.1`: binding `localhost` can pick `::1` while `fetch()` resolves `localhost` to `127.0.0.1`, which made it fail `ConnectionRefused` on some hosts. - `windows-x64-baseline-verify-baseline` failed with `strpbrk [CLDEMOTE]` — newer UCRT on the CI Windows agents emits `cldemote` (`NP 0f 1c /0`, hint-space, NOP where unsupported) in `strpbrk`. An interim per-symbol allowlist widening unblocked this branch; main then landed the proper global ignore (#31606), so this branch merged main and dropped the interim entry — no verifier changes remain in the diff. --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
2 participants
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
--), so a specifier can never be interpreted as a flagnode runtime
set-cookieis arrayed (verified against Node's behavior)cwdoption throws a catchable error instead of aborting the processRuntime internals
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 checkand the cross-target checks pass on all 10 CI targets; the touched suites pass locally with no new failures versus main.