Deduplicate http client and h2 header-validation internals - #32020
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis PR consolidates HTTP/2 header field validation, refactors HTTP client completion and redirect handling, simplifies WebSocket initialization, extracts socket-stub infrastructure for reuse, and refactors HTTP/2 frame-parser stream ID handling and header encoding logic. ChangesHTTP/2 Validation Extraction and Client/Parser Consolidation
Socket Stub Infrastructure Extraction and Application
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:26 AM PT - Aug 11th, 2026
✅ @robobun, your commit 4e752b352118ecc6580c6d81b27401e61da3aafb passed in 🧪 To try this PR locally: bunx bun-pr 32020That installs a local version of the PR into your bun-32020 --bun |
|
@robobun adopt |
|
Adopted. Re-scoped to the four dedupes main has not absorbed (see the PR description); merged current main at 4e752b3, diff unchanged. Format, clippy and cargo miri pass, no open review threads, mergeable. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/lib.rs (1)
3671-3680:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep-alive reuse still misclassifies in-flight stream/sendfile uploads as drained.
request_side_drainedonly checks the unsent slice forHTTPRequestBody::Bytes(_); every other body kind returnstrue. That still lets an HTTP/1.1 socket be pooled after an early response while aStreamorSendfileupload is mid-flight, so the next request can reuse a connection whose previous request body is still being written/read.Suggested fix
- let request_side_drained = match &this.state.original_request_body { - HTTPRequestBody::Bytes(_) => this.state.request_body.is_empty(), - _ => true, - }; + let request_side_drained = match &this.state.original_request_body { + HTTPRequestBody::Bytes(_) => this.state.request_body.is_empty(), + HTTPRequestBody::Stream(_) | HTTPRequestBody::Sendfile(_) => { + this.state.request_stage == RequestStage::Done + } + };As per coding guidelines, "Fix the whole class in the same PR."
🤖 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/http/lib.rs` around lines 3671 - 3680, The pooling check incorrectly treats non-Bytes bodies as drained; change the request_side_drained logic so it returns true only when the original_request_body is a fully-sent Bytes (and request_body.is_empty()) or when it is explicitly an Empty/no-body variant, and return false for streaming/Sendfile variants so in-flight uploads block pooling; update the match on this.state.original_request_body (and any HTTPRequestBody variants like Stream, Sendfile, AsyncStream, etc.) to reflect that behavior so is_keep_alive_possible() && !socket.is_closed_or_has_error() && tunnel_poolable only proceeds when request_side_drained is truly drained.Source: Coding guidelines
🤖 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/js/internal/http.ts`:
- Around line 522-543: The setters (remoteAddress, remotePort, remoteFamily)
assume this.address() returns an object and crash when it returns
null/undefined; fix each setter to call const addr = this.address(); if (!addr)
create and assign a new backing object (e.g. this._address = {}) then use that
addr to set the property, so you initialize the address object on demand instead
of blindly writing into a possibly null value; apply this pattern in the
remoteAddress, remotePort, and remoteFamily setters.
---
Outside diff comments:
In `@src/http/lib.rs`:
- Around line 3671-3680: The pooling check incorrectly treats non-Bytes bodies
as drained; change the request_side_drained logic so it returns true only when
the original_request_body is a fully-sent Bytes (and request_body.is_empty()) or
when it is explicitly an Empty/no-body variant, and return false for
streaming/Sendfile variants so in-flight uploads block pooling; update the match
on this.state.original_request_body (and any HTTPRequestBody variants like
Stream, Sendfile, AsyncStream, etc.) to reflect that behavior so
is_keep_alive_possible() && !socket.is_closed_or_has_error() && tunnel_poolable
only proceeds when request_side_drained is truly drained.
🪄 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: 2bd442c1-2f2d-421c-a6c3-78c9e8436830
📒 Files selected for processing (8)
src/http/h2_client/dispatch.rssrc/http/lib.rssrc/http_jsc/websocket_client.rssrc/http_types/h2.rssrc/js/internal/http.tssrc/js/internal/http/FakeSocket.tssrc/js/node/_http_server.tssrc/runtime/api/bun/h2_frame_parser.rs
|
On the request_side_drained finding (outside the diff): that match and its comment exist verbatim on main (src/http/lib.rs, "Stream/Sendfile are left as-is, they do not track an unsent slice here"); this PR only renames self to this while moving the block into the closure. Changing the pooling predicate for Stream/Sendfile bodies would be a behavior change, which this PR deliberately avoids, and the proposed request_stage gate is unvalidated (the comment above it explains why Bytes cannot use request_stage: a fully sent small request parks at .body). If that predicate needs tightening it should be its own change with its own test. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/lib.rs (1)
4349-4358:⚠️ Potential issue | 🟠 MajorAdd validation of redirect URL scheme and length in
apply_redirect_url.The protocol-relative redirect path (lines 4690-4706) validates the URL length before normalization. The relative redirect path (else branch at 4709-4721) calls
bun_url::joinwithout a length check, then passes the result directly toapply_redirect_url. Sinceapply_redirect_urlperforms no validation, an adversarially long relative redirect can bypass the length limit that applies to protocol-relative redirects. Additionally, although WHATWG URL joining from an http/https base should preserve the scheme, explicit validation inapply_redirect_urlcloses the gap and ensures only http/https redirects are installed, matching the precondition enforcement on the protocol-relative path.Proposed fix
fn apply_redirect_url(&mut self, new_href: Vec<u8>) -> bool { + if new_href.len() > MAX_REDIRECT_URL_LENGTH { + return false; // or return Err if signature changes to Result + } // SAFETY: self-borrow — `new_href` is moved into `self.redirect` // below, which lives as long as `self` (≥ `'a`). let new_url: URL<'a> = unsafe { URL::parse(&new_href).erase_lifetime() }; + let protocol = new_url.display_protocol(); + if protocol != b"http" && protocol != b"https" { + return false; // or return Err if signature changes to Result + } let is_same_origin = strings::eql_case_insensitive_ascii( strings::without_trailing_slash(new_url.origin), strings::without_trailing_slash(self.url.origin), true, );🤖 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/http/lib.rs` around lines 4349 - 4358, The apply_redirect_url method needs to add two validations to prevent security issues: first, validate that the redirect URL scheme is either http or https to match the precondition enforcement in the protocol-relative redirect path, and second, validate the URL length before installation to prevent adversarially long relative redirects from bypassing the length limits that apply to protocol-relative redirects. Add these checks at the beginning of apply_redirect_url before the URL is assigned to self.url.Source: Coding guidelines
🤖 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/http/lib.rs`:
- Around line 1327-1335: The has_unsent_request_body method does not account for
sendfile uploads, which leave request_body() empty but still have data in
flight. When a sendfile is active in RequestStage::Body and a peer FIN arrives,
the function incorrectly returns false, allowing a graceful close instead of the
required reset. Add a check for active sendfile uploads (similar to the
is_streaming_request_body flag check) that returns true if HTTPRequestBody
contains a Sendfile variant, ensuring in-flight sendfile operations are treated
as unsent request bodies.
---
Outside diff comments:
In `@src/http/lib.rs`:
- Around line 4349-4358: The apply_redirect_url method needs to add two
validations to prevent security issues: first, validate that the redirect URL
scheme is either http or https to match the precondition enforcement in the
protocol-relative redirect path, and second, validate the URL length before
installation to prevent adversarially long relative redirects from bypassing the
length limits that apply to protocol-relative redirects. Add these checks at the
beginning of apply_redirect_url before the URL is assigned to self.url.
🪄 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: 6725f7a2-49e6-4035-a758-3f555701fd9a
📒 Files selected for processing (5)
src/http/lib.rssrc/http_jsc/websocket_client.rssrc/js/internal/http.tssrc/js/node/_http_server.tssrc/runtime/api/bun/h2_frame_parser.rs
💤 Files with no reviewable changes (1)
- src/runtime/api/bun/h2_frame_parser.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/lib.rs (1)
4349-4358:⚠️ Potential issue | 🟠 MajorAdd validation of redirect URL scheme and length in
apply_redirect_url.The protocol-relative redirect path (lines 4690-4706) validates the URL length before normalization. The relative redirect path (else branch at 4709-4721) calls
bun_url::joinwithout a length check, then passes the result directly toapply_redirect_url. Sinceapply_redirect_urlperforms no validation, an adversarially long relative redirect can bypass the length limit that applies to protocol-relative redirects. Additionally, although WHATWG URL joining from an http/https base should preserve the scheme, explicit validation inapply_redirect_urlcloses the gap and ensures only http/https redirects are installed, matching the precondition enforcement on the protocol-relative path.Proposed fix
fn apply_redirect_url(&mut self, new_href: Vec<u8>) -> bool { + if new_href.len() > MAX_REDIRECT_URL_LENGTH { + return false; // or return Err if signature changes to Result + } // SAFETY: self-borrow — `new_href` is moved into `self.redirect` // below, which lives as long as `self` (≥ `'a`). let new_url: URL<'a> = unsafe { URL::parse(&new_href).erase_lifetime() }; + let protocol = new_url.display_protocol(); + if protocol != b"http" && protocol != b"https" { + return false; // or return Err if signature changes to Result + } let is_same_origin = strings::eql_case_insensitive_ascii( strings::without_trailing_slash(new_url.origin), strings::without_trailing_slash(self.url.origin), true, );🤖 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/http/lib.rs` around lines 4349 - 4358, The apply_redirect_url method needs to add two validations to prevent security issues: first, validate that the redirect URL scheme is either http or https to match the precondition enforcement in the protocol-relative redirect path, and second, validate the URL length before installation to prevent adversarially long relative redirects from bypassing the length limits that apply to protocol-relative redirects. Add these checks at the beginning of apply_redirect_url before the URL is assigned to self.url.Source: Coding guidelines
🤖 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/http/lib.rs`:
- Around line 1327-1335: The has_unsent_request_body method does not account for
sendfile uploads, which leave request_body() empty but still have data in
flight. When a sendfile is active in RequestStage::Body and a peer FIN arrives,
the function incorrectly returns false, allowing a graceful close instead of the
required reset. Add a check for active sendfile uploads (similar to the
is_streaming_request_body flag check) that returns true if HTTPRequestBody
contains a Sendfile variant, ensuring in-flight sendfile operations are treated
as unsent request bodies.
---
Outside diff comments:
In `@src/http/lib.rs`:
- Around line 4349-4358: The apply_redirect_url method needs to add two
validations to prevent security issues: first, validate that the redirect URL
scheme is either http or https to match the precondition enforcement in the
protocol-relative redirect path, and second, validate the URL length before
installation to prevent adversarially long relative redirects from bypassing the
length limits that apply to protocol-relative redirects. Add these checks at the
beginning of apply_redirect_url before the URL is assigned to self.url.
🪄 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: 6725f7a2-49e6-4035-a758-3f555701fd9a
📒 Files selected for processing (5)
src/http/lib.rssrc/http_jsc/websocket_client.rssrc/js/internal/http.tssrc/js/node/_http_server.tssrc/runtime/api/bun/h2_frame_parser.rs
💤 Files with no reviewable changes (1)
- src/runtime/api/bun/h2_frame_parser.rs
🛑 Comments failed to post (1)
src/http/lib.rs (1)
1327-1335:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat in-flight sendfile uploads as unsent request bodies.
HTTPRequestBody::Sendfile(_)leavesrequest_body()empty, so a peer FIN while sendfile is still inRequestStage::Bodyreturnsfalsehere andHTTPContext::on_endcan graceful-close instead of resetting. That reintroduces the queued-FIN-behind-unsent-body hazard this helper is meant to prevent.Proposed fix
pub fn has_unsent_request_body(&self) -> bool { if self.state.request_stage == RequestStage::Done { return false; } - if self.flags.is_streaming_request_body { - // More body chunks may still be produced by JS. - return true; - } - !self.request_body().is_empty() + match &self.state.original_request_body { + HTTPRequestBody::Stream(_) => { + // More body chunks may still be produced by JS. + true + } + HTTPRequestBody::Sendfile(_) => { + // Sendfile progress is tracked by the sendfile object, not + // `request_body`; until RequestStage::Done, bytes may remain. + true + } + HTTPRequestBody::Bytes(_) => !self.request_body().is_empty(), + } }🤖 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/http/lib.rs` around lines 1327 - 1335, The has_unsent_request_body method does not account for sendfile uploads, which leave request_body() empty but still have data in flight. When a sendfile is active in RequestStage::Body and a peer FIN arrives, the function incorrectly returns false, allowing a graceful close instead of the required reset. Add a check for active sendfile uploads (similar to the is_streaming_request_body flag check) that returns true if HTTPRequestBody contains a Sendfile variant, ensuring in-flight sendfile operations are treated as unsent request bodies.
|
On the two new findings: has_unsent_request_body (line 1327): this function was added to main by #32462 and merged in here unchanged; this PR does not touch it ( apply_redirect_url validation (line 4349): the helper preserves main's behavior exactly. On main the relative else-arm at 4776-4798 (the |
![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 is a ~700-line refactor across the core HTTP client (keep-alive pooling, redirect handling), WebSocket init (unsafe FFI/ref-counting), and the HTTP/2 frame parser — worth a pass from someone who owns these subsystems to confirm the behavioral-equivalence claim.
Extended reasoning...
Overview
This PR deduplicates internals across four networking subsystems with a claimed net of −700 lines and zero behavior change: (1) src/http/lib.rs — extracts send_progress_update_inner (shared between HTTP/1.1 and multiplexed paths) and apply_redirect_url/normalize_and_apply_redirect_url (shared across the three Location-header arms); (2) src/http_jsc/websocket_client.rs — extracts new_ws and finish_init shared between init and init_with_tunnel; (3) src/http_types/h2.rs + src/http/h2_client/dispatch.rs + src/runtime/api/bun/h2_frame_parser.rs — centralizes is_lower_tchar/is_malformed_field_value, adds a stream_from_js_arg helper for ~9 host fns, and folds the array/single-value header-encode arms into one closure; (4) src/js/internal/http.ts + FakeSocket.ts + _http_server.ts — extracts installSocketStubs for the duplicated socket-compat prototype members. Two new test files pin the consolidated surface.
Security risks
The consolidated HTTP/2 header-name/value validators are byte-identical to the originals (same tchar set, same NUL/CR/LF rejection), so no relaxation of header-injection guards. The redirect helper preserves the erase_lifetime self-borrow pattern and the prev_redirect swap. The WebSocket finish_init keeps the same ref-count discipline (initial I/O ref from new_ws, C++ ref taken at the tail) and the mimalloc buffer-adoption contract. I don't see new attack surface, but the unsafe blocks moved around enough that a second pair of eyes on the soundness invariants is warranted.
Level of scrutiny
High. This is production-critical networking code: the fetch() keep-alive pool release path, redirect origin comparison, WebSocket FFI init with raw-pointer ref-counting, and the node:http2 frame parser. The PR is explicitly a no-behavior-change refactor, but verifying that requires checking each consolidated path field-by-field against the original — e.g., the encode_value closure threads err_name/encode_err_return to preserve per-arm error messages and return values, and reorders stream.state = CLOSED relative to set_context in the single-value arm to match the array arm; the write_stream host fn now reads close_arg.to_boolean() after the stream lookup instead of before (commented as side-effect-free). These look correct to me but are exactly the kind of subtle equivalence a domain owner should sign off on.
Other factors
The bug-hunting system found nothing. CodeRabbit's one concern (setter null-deref) was correctly identified as a verbatim move of pre-existing code and withdrawn. The PR adds tests that pass on main too (by design — they pin invariants, not new behavior). No CODEOWNERS entries cover these paths. The PR has not yet been reviewed by a human; cirospaciari is the suggested reviewer.
|
Agreed. The two subtle points called out in the extended reasoning (the |
# Conflicts: # src/http/lib.rs
# Conflicts: # src/http/lib.rs
# Conflicts: # src/http_jsc/websocket_client.rs
robobun
changed the title
# Conflicts: # src/http/lib.rs # src/runtime/api/bun/h2_frame_parser.rs
|
@robobun get this mergeable — or close it if the changes are already covered on main / too stale to salvage. |
# Conflicts: # src/http/h2_client/dispatch.rs # src/http/lib.rs # src/js/node/_http_server.ts # src/runtime/api/bun/h2_frame_parser.rs
|
On it. Checked each consolidation against current main: the h2 header-validation, h2 host-fn stream lookup, redirect-tail, and JS socket-stub dedupes are all still unabsorbed upstream, so this is worth salvaging rather than closing. The one piece main has since made moot is the progress-update dedupe (main reworked body delivery in that path, and the two paths no longer share a tail), so I am dropping that part and taking main's code there. Merging the 863 commits now; will report back once the build and tests are green. |
|
@alii this is caught up and mergeable now. What survived the re-scope is in the description: four dedupes still unabsorbed on main (h2 header validation, h2 host-fn stream lookup plus the shared encode closure, redirect tails, and the JS socket stubs), net about 470 lines removed against 457 added with most of the additions being the two tests and the shared helpers. The WebSocket init and progress-update halves came out because main overtook both. One test needed adapting because main now reports bad header values on the request error event instead of throwing from request(); confirmed that is main's behavior, not something this diff changed. Fetch redirect, node-http2, h2-conformance, h3 client, and node-http suites are green locally; CI is running. If you would rather not carry a refactor of this size, closing is also reasonable, but it is no longer stale. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it consolidates security-sensitive HTTP client redirect handling and h2 frame-parser host functions across six documented merge resolutions (and the latest 863-commit merge hasn't yet had a green-CI confirmation posted), a human pass from someone who owns these paths would still be worthwhile before landing.
Checked the apply_redirect_url extraction against all three original arms — the added has_http_like_protocol() guard is unreachable for the absolute/protocol-relative arms (scheme is fixed before building) and matches main for the relative arm; dropping the original_url clone is safe since URL::clone copies slices, not bytes.
Checked each stream_from_js_arg call site's const-generic flags against the original inline validation (zero/max/neither) and error message; the close_arg.to_boolean() reorder is side-effect free.
Checked the encode_value closure's per-arm encode_err_return and the handle_received_stream_id-None path against both original inline copies.
Checked installSocketStubs installs the same descriptor shape (non-enumerable class members) as the removed inline definitions on both FakeSocket and NodeHTTPServerSocket.
Extended reasoning...
Overview
This PR deduplicates repeated code across the HTTP stack: it lifts is_lower_tchar/is_malformed_field_value into src/http_types/h2.rs (shared by the fetch h2 client and the node:http2 frame parser), extracts the three redirect-URL tails in src/http/lib.rs into apply_redirect_url/normalize_and_apply_redirect_url, factors the repeated stream-id-argument prologue in h2_frame_parser.rs into a const-generic stream_from_js_arg helper, folds the two header-encoding arms into a shared encode_value closure, and moves the ~60 lines of duplicated net.Socket-compat stub members from FakeSocket and NodeHTTPServerSocket into a shared installSocketStubs helper. Two new test files pin the consolidated header-validation and socket-surface behavior.
Security risks
Redirect handling (src/http/lib.rs) is security-sensitive: same-origin computation gates credential forwarding across redirects, and the has_http_like_protocol() guard prevents redirecting to non-http schemes. The extraction preserves both — the same-origin comparison reads self.url.origin before overwriting it (equivalent to the old clone-then-compare), and the protocol guard now runs on all three arms (a no-op on the two that already fix the scheme). H2 header-field validation is also security-relevant (CR/LF/NUL rejection prevents downstream header injection); the shared is_malformed_field_value is byte-identical to both originals.
Level of scrutiny
High. This touches production-critical networking code (fetch redirect following, node:http2 request encoding, node:http server socket surface) and has been rebased across six documented merge conflicts plus a fresh 863-commit merge. Each merge resolution is a place where the "zero behavior change" contract could silently break. The PR body's per-resolution audit is thorough and the bug-hunting pass found nothing, but the sheer surface area and the explicit earlier note that this "is waiting on a pass from someone who owns these paths" both point to human review.
Other factors
- The most recent timeline entry is robobun starting the 863-commit merge and promising to report back once build/tests are green; no follow-up confirmation is posted yet (the head commit adapts a test to async error delivery, suggesting at least one post-merge fix was needed).
- My only prior inline comment (on
websocket_client.rs) was addressed and that file has since been dropped from the diff entirely. - The new tests are behavior-pinning (pass on main by design), which is correct for a pure dedupe but means they don't independently prove the merge resolutions are right — that rests on the arm-by-arm audit.
|
Post-merge confirmation, since the review noted it was missing: on the 863-commit merge, fetch redirect + client + keepalive pass 77/77, node-http2 + h2-conformance 408 pass / 0 fail, the h3 client and adversarial suites pass 80/80 (these exercise the re-exported value check through h3_client/callbacks.rs), and node-http.test.ts passes 143 with its one failure (issue#4295, needs outbound network) reproducing identically on main. The test adapted in the head commit was tracking a main-side change (header value errors now arrive on the request error event rather than throwing from request()); I confirmed that behavior on pristine main before touching the test. The Format check caught rustfmt drift from the conflict resolution and autofix has already landed it; the miri entry showing as failed is a cancelled superseded run. Buildkite is in progress on the formatted head. Agree a human pass from an owner of these paths is the right bar for landing. |
|
@robobun fix the Format and cargo miri failures. |
|
@alii both are green now. What was going on: the Format job caught rustfmt drift from the conflict resolution and autofix pushed the fix, but a bot push does not trigger workflows, so the PR kept showing the stale Format failure from the previous head, and that same push cancelled the in-flight miri run, which GitHub surfaces as a failure. The tree itself was already clean, so I pushed a small real change to get fresh runs: unit tests for the two shared validators in bun_http_types, which is one of the crates the miri job covers, so miri now exercises the new helpers directly (the value check is gated to miri because a native cargo test of that leaf crate does not link the highway objects). On 8e0f4b4, autofix.ci (Format) and Miri have both completed successfully, and Lint, source-lints, and Clippy are green; Buildkite is the only thing still running. |
|
Verified your merge commit (33089f7): it touches none of the nine files in this diff, the effective change against main is unchanged, and it picks up the darwin lane fix from #37364. On that exact commit, cargo check and rustfmt are clean and the relevant suites pass locally (the two PR tests plus fetch-redirect and h2-conformance 97/0, node-http2 plus the h3 client 400/0). CI on it has no failures so far with the remaining lanes still running. Nothing further from my side unless CI turns something up. |
There was a problem hiding this comment.
I reviewed the current (post-rescope) diff and didn't find any bugs. Given this is a ~470-line behavior-preserving refactor across HTTP/2 header validation, redirect origin handling (with unsafe erase_lifetime), and the h2 frame parser's JS-facing host fns, a pass from someone who owns these paths is still the right bar — as already noted in the thread.
What was reviewed:
apply_redirect_url: the addedhas_http_like_protocol()guard is a no-op for the two normalized arms (both build from an http/https prefix before WHATWG parse), matching main's per-arm behavior.stream_from_js_arg: per-site error strings and the zero/MAX_STREAM_ID gates line up with each original prologue; theclose_arg.to_boolean()reorder is side-effect-free.encode_valueclosure: the per-arm compression-error return (UNDEFINEDvsstream_id) is threaded correctly; captured state matches the inlined originals.installSocketStubs: no stub name collides with a retained member on either prototype; descriptors stay non-enumerable.
Extended reasoning...
Overview
This PR consolidates four pieces of duplicated code across the HTTP stack: (1) the RFC 9113 §8.2.1 lowercase-tchar and NUL/CR/LF field validators, moved to bun_http_types::h2 and re-exported into h2_client/dispatch.rs and h2_frame_parser.rs; (2) a const-generic stream_from_js_arg helper replacing eight copies of the stream-id validation prologue in h2 host fns; (3) an encode_value closure shared by the array and single-value arms of send_headers; (4) apply_redirect_url / normalize_and_apply_redirect_url replacing three copies of the redirect-tail sequence in handle_response_metadata; (5) installSocketStubs copying twelve net.Socket compatibility stubs onto FakeSocket and NodeHTTPServerSocket prototypes. Two new test files pin the consolidated surfaces, plus miri-run unit tests for the shared validators.
Security risks
The touched code is security-adjacent: h2 header-value validation guards against header injection when values are forwarded downstream, and the redirect same-origin comparison gates whether credentials are carried across a hop. The refactor does not weaken either — the validator bodies are byte-identical moves, and the origin comparison in apply_redirect_url reads self.url.origin before assigning self.url = new_url, preserving the old-vs-new comparison the third arm previously did via a cloned original_url. The added has_http_like_protocol() guard in the shared helper is strictly no less restrictive than main on every arm.
Level of scrutiny
High. This is a hand-verified behavior-preserving refactor across hot, protocol-correctness-critical paths (HTTP/2 frame parsing, redirect handling with unsafe lifetime erasure, Node compat surfaces). REVIEW.md's "treat every refactor as guilty until proven behavior-preserving" applies directly. The PR description documents an arm-by-arm equivalence audit and the author's own tooling states a human owner pass is the right bar; I agree.
Other factors
My only prior comment on this PR (the stale # Safety doc on finish_init) was addressed and that file has since been dropped from the diff entirely when the WebSocket dedupe was removed. All prior review threads are resolved. The bug-hunting system found nothing on the current head. Tests are reported green locally across fetch redirect, node-http2, h2-conformance, h3, and node-http suites; Format/Miri/Lint/Clippy pass. The change is well-tested and carefully documented, but its scope and the subtlety of the equivalence claims (const-generic gating, closure capture of mutable encoder state, self-borrow lifetime erasure) put it outside what I'd approve without a human owner's sign-off.
|
Thanks, that matches the audit in the description point for point. For whoever does the owner pass, the spots worth the most attention are the ones named above: the const-generic flags on each stream_from_js_arg site (the original prologues differed only in zero/MAX gating and the not-a-number message), the encode_err_return threading in the shared closure, and the ordering in apply_redirect_url, which reads self.url.origin for the comparison before assigning self.url. Everything else is a literal move. Current head is the maintainer merge; 42 checks green, 3 still running, nothing failing. |
|
@robobun fix the alpine-x64 test failure in build #91764. |
|
Looking at the alpine x64 failure in build 91764 now (test/cli/run/require-cache.test.ts, the import() leak loop timing out). Will report back with what is behind it. |
|
The alpine x64 failure in build 91764 was Fix is in #37562: the fixture now warms up to steady state before taking its baseline, which lets it measure 40k loads against a 20 MB bound instead of 100k against 100 MB, so it runs in about a third of the time and is no longer the last process in the file to finish. Numbers are in that PR. I also merged current main into this branch (4e752b3, clean auto-merge, the diff against main is unchanged at the same 9 files), which re-runs CI here. |
2 participants
What this does
Removes four pieces of duplicated code in the HTTP/2 and HTTP client internals. Pure dedupe: every consolidated path was checked arm-by-arm against main for identical behavior (same error codes and messages per call site, same return values, same ordering of side effects).
h2 header validation (
src/http_types/h2.rs,h2_client/dispatch.rs,h2_frame_parser.rs): the lowercase-tchar name check and the NUL/CR/LF value check were each written out three times (client dispatch, frame parser name check, frame parser value check). They now live once inbun_http_types::h2asis_lower_tchar/is_malformed_field_value; dispatch re-exports the value check for its h3 caller.h2 host-fn stream lookup (
h2_frame_parser.rs): nine host fns repeated the same "is it a number, is it zero, is it aboveMAX_STREAM_ID, is it in the map" prologue with per-site error strings.stream_from_js_argconsolidates it, parameterized on which checks apply and the not-a-number message, so each site's user-visible errors are unchanged. Functions whose lookup main has since given distinct semantics (rst_stream,set_stream_context) keep their inline code; the three that main wrapped inenter_stream_dispatchfeed the helper's result into that guard.h2 header encoding (
h2_frame_parser.rs): the array-value and single-value arms ofsend_headersencoding were two copies of the same ~60-line block. One closure now serves both; the only remaining per-arm difference (the return value on a compression error) is a parameter.redirect tails (
src/http/lib.rs): the threeLocationarms (absolute, protocol-relative, relative) ended in the same parse / same-origin compare / swap-into-self.redirectsequence.apply_redirect_url(plusnormalize_and_apply_redirect_urlfor the two arms that go through the WHATWG normalizer) replaces them. Thehas_http_like_protocol()guard main added to the relative arm lives in the helper; for the other two arms it is a no-op since they reject non-http schemes before building the URL.net.Socketstub members (src/js/internal/http.ts,FakeSocket.ts,_http_server.ts):FakeSocketandNodeHTTPServerSocketcarried identical copies of twelve compatibility stubs (readyState,remoteAddress/Port/Familyaccessors,ref/unref,setNoDelay, etc.). They are defined once and installed onto both prototypes viaObject.defineProperties, preserving the non-enumerable class-member descriptors and leaving each class's real members (including the TLS getters andsetTimeoutmain has since added to the server socket) untouched.Dropped since the PR was opened
Two parts of the original PR were overtaken by main and are no longer in the diff:
new_raw+finish_init).Tests
test/js/node/http2/node-http2-header-validation.test.tspins the consolidated validators and both encoding arms: exactERR_HTTP2_INVALID_HEADER_VALUE/ERR_INVALID_HTTP_TOKEN/ERR_HTTP2_HEADER_SINGLE_VALUEerrors for single and array values, name lowercasing, and the full tchar set. (Updated in this round: value violations now surface on the request's'error'event rather than throwing fromrequest(), a change main made independently; verified identical on main.)test/js/node/http/node-http-server-socket-surface.test.tspins the installed stub surface on a live server socket; every assertion also holds under Node.Both pass on main as well, which is the intended property of a behavior-preserving refactor: they exist to keep the shared helpers from drifting, not to demonstrate a fix.
Verified on the current merge: fetch redirect/client/keepalive 77/77,
node-http2+h2-conformance408/0, h3 client + adversarial 80/0 (exercises the dispatch re-export),node-http.test.ts143 pass with the one failure (issue#4295, needs network) reproducing identically on main in this environment.