Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughHTTP/2 parent validation now accepts non-negative numeric values, including zero. Frame handling treats zero as the default dependency and removes self-dependencies. Tests cover validation errors, priority encoding, session integrity, and responses. ChangesHTTP/2 parent handling
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:38 PM PT - Sep 21st, 2026
✅ @robobun, your commit 20a7eeba30d0ec8768574d7d4d3bf99af380055f passed in 🧪 To try this PR locally: bunx bun-pr 43544That installs a local version of the PR into your bun-43544 --bun |
|
Status: closed in favor of #43599, which now contains the same change (no PRIORITY field on any HEADERS frame, and How I reproduced it. On bun 1.4.3-canary.1+367d939d9 and on main, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether an out-of-range parent (2**31, Infinity) could reach the u32::try_from(parent).expect(...) in the HEADERS/CONTINUATION writers as a negative after to_int32 — it cannot: JSValue::to_int32 in src/jsc/JSValue.rs saturates to i32::MAX rather than wrapping, so the parent < 0 guard at src/runtime/api/bun/h2_frame_parser.rs:6969 is the only negative path and it returns before the writer.
Extended reasoning...
The source diff is three lines (a 0 minimum on validateNumber, parent <= 0 || > MAX_STREAM_ID narrowed to parent < 0, and has_priority |= parent != 0). The one candidate I traced myself was the reviewer-noted worry that 2**31 might wrap to a negative int32 after JS-side validation passed; JSValue::to_int32 uses Rust as on the double, which saturates, so out-of-range positives become i32::MAX, never negative, and the expect("int cast") at h2_frame_parser.rs:7175/7223 is unreachable from both the client request() and server respond() paths. The inline findings (mostly pre-existing behaviour around the shared native request, padding/priority ordering, and weight) plus an unposted verified finding mean this still warrants a human look rather than approval.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/api/bun/h2_frame_parser.rs— pre-existing: a client session opened with paddingStrategy MAX or ALIGNED dies (GOAWAY PROTOCOL_ERROR or COMPRESSION_ERROR) on any request that still sets a PRIORITY field, same as on the base branch. The single-frame path at h2_frame_parser.rs:7173-7193 writes the 5 priority bytes, then the Pad Length octet, then the block; RFC 7540 6.2 puts Pad Length first, and Bun's own receiver (h2/connection.rs:891-910) reads it first. Fix: when PADDED and PRIORITY are both set, emit Pad Length, then the priority field, then the block (the CONTINUATION branch never pads, so only this site needs it). The PR narrows the trigger to parent != 0 or exclusive: true but lists this as found, not fixed.Extended reasoning...
http2.connect(url, { paddingStrategy: PADDING_STRATEGY_MAX }) reaches native via nativeSettings at http2.ts:5701 and h2_frame_parser.rs:7625, and every new Stream copies it at h2_frame_parser.rs:3395. request(headers, { exclusive: true }) or { parent: 3 } sets has_priority (h2_frame_parser.rs:6955 or 6981). get_padding at 1459 returns a non-zero pad, so flags gain PADDED at 7150. The writer then emits FrameHeader (7170), StreamPriority (7180), and finally the block with buffer[0] = padding (7192). A conforming peer parses byte 0 of the payload as Pad Length. For exclusive: true that byte is 0x80 = 128, larger than the payload, so the peer answers GOAWAY PROTOCOL_ERROR (Bun's own server: connection.rs:894-896). For parent: 3 the byte is 0x00, the peer skips 5 bytes landing at the real block start but keeps the trailing padding zeros as block bytes; 0x00 is a literal with name index 0, invalid in HPACK, so the peer answers COMPRESSION_ERROR and the session is torn down. Base branch behaves the same; this PR only removes parent: 0 from the set of inputs that reach this path.
Verification: pre-existing (the writer code is byte-identical on base 9b7c982 and HEAD; the PR only changes when
has_priorityis set, at h2_frame_parser.rs:6979has_priority |= parent != 0, which narrows rather than widens the inputs reaching this path). Triggering condition: a client session created withpaddingStrategy: PADDING_STRATEGY_MAX(or ALIGNED when the length is not already 8-aligned) issues…
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the follow-up commit addresses the earlier inline notes (connect now rejects on error/close, respond() with parent: 0 is covered, and a self-dependency is collapsed to 0 instead of going on the wire).
What was reviewed:
- Traced
parent_js.to_int32()(src/jsc/JSValue.rs:765): NaN maps to 0 and out-of-range/Infinity saturate to i32 bounds, so after theparent < 0check the value is in [0, i32::MAX] and the downstreamu32::try_from(parent).expect(...)cannot fire; dropping the> MAX_STREAM_IDclause is a no-op since MAX_STREAM_ID == i32::MAX. validateNumber(options.parent, "options.parent", 0)in request() matches node's minimum and the RangeError/ERR_OUT_OF_RANGE contract; the nativeparent < 0rejection remains as the guard for respond(), which passes options unvalidated.has_priority |= parent != 0preserves the existing exclusive/weight-driven PRIORITY emission; the raw-socket test pins exactly which requests carry the field.
Extended reasoning...
Overview
The PR changes three files: a one-argument change in src/js/node/http2.ts (validateNumber(options.parent, "options.parent", 0)), a ~10-line change in the native HEADERS writer in src/runtime/api/bun/h2_frame_parser.rs (reject only parent < 0, collapse parent == stream_id to 0, set has_priority only for a non-zero dependency), and ~216 lines of new tests in test/js/node/http2/node-http2.test.js covering request() with non-negative/negative/NaN parents, wire-level PRIORITY field presence via a raw net server, and server-side respond(headers, { parent: 0 }).
Security risks
None identified. The change relaxes a rejection of parent: 0 (which previously corrupted HPACK state by encoding a block and never sending it) and tightens JS-side validation to reject negatives/NaN synchronously. The remaining native path is bounded: to_int32 saturates so parent is always in [0, i32::MAX] once the < 0 check passes, which keeps the later u32::try_from(...).expect unreachable on user input. No new allocation, no new untrusted-length arithmetic, and no change to how peer-supplied frames are parsed.
Level of scrutiny
Moderate. It touches the native HTTP/2 frame writer, but the diff is small and each clause is directly exercised by a test: the raw-socket test asserts which HEADERS frames carry a PRIORITY field (including the self-dependency collapse on streams 13/15), the request() tests assert exact error class/code/message and that the HPACK encoder stays in sync afterwards, and the respond() test covers the sibling entry point. I checked that removing the > MAX_STREAM_ID bound is behavior-preserving given MAX_STREAM_ID == i32::MAX and saturating conversion. The changed files are not covered by CODEOWNERS.
Other factors
The prior review's nits (connect promise with no reject path; missing respond() coverage) and the pre-existing self-dependency observation were all addressed in the latest commit, and no new findings surfaced this run (the hunt ran dry). The known pre-existing weight-byte-0 issue is explicitly scoped out in the PR description and is not widened by this change. The PR author's description matches the code as merged.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
|
On the additional finding in the review summary (a HEADERS frame with both PADDED and PRIORITY writes the priority fields before the Pad Length octet): confirmed, and older than this PR. On main, a client session with |
|
@robobun Node v26.3.0 never puts a PRIORITY field in a request HEADERS frame: its nghttp2 1.69.0 discards the spec ( |
…N parent like node
request(headers, { parent: 0 }) reset the stream with
NGHTTP2_INTERNAL_ERROR. The native check rejected parent <= 0, and it
ran after the header block was in the session's HPACK encoder. The
HEADERS frame never went out, so the server failed the next header
block with GOAWAY(COMPRESSION_ERROR).
The native check now rejects only a negative parent. Stream 0 is the
dependency every stream starts with (RFC 7540 5.3.5), so parent: 0
alone adds no PRIORITY field to the HEADERS frame.
request() validates options.parent with a minimum of 0, like node's
setAndValidatePriorityOptions. A negative or NaN parent throws
ERR_OUT_OF_RANGE before the request reaches the encoder.
A request whose parent is its own stream id put a self-dependency in
the PRIORITY field. RFC 7540 5.3.1 forbids that, and a node server
answers it with a GOAWAY that ends the session. The native code now
treats such a parent as stream 0, the default dependency.
The tests also cover respond(headers, { parent: 0 }), which uses the
same native function. The wait for 'connect' now rejects when the
session fails.
node v26.3.0 validates the priority options of request() and sends none of them: nghttp2 1.69 discards the priority spec, so every request HEADERS frame goes out without the PRIORITY flag. Bun wrote a PRIORITY field for `exclusive: true`, a non-zero `parent` and a `weight`. The HEADERS writer no longer reads `exclusive`, `parent` or `weight` and no longer writes the field. request() still validates the options in JS. respond() shares the writer, so it ignores the three options like node. `StreamPriority`, `UInt31WithReserved::to_uint32`, `HeadersFrameFlags::PRIORITY` and the per-stream `weight` field had no other user and are removed. `state.weight` reports 16, as node does. This also removes three defects of the field: it went out before the Pad Length octet on a padded session, its weight byte was always 0, and a parent equal to the stream's own id made a node server end the session.
robobun
changed the title
e0b0aa5 to
20a7eeb
Compare
|
@cirospaciari Done in 20a7eeb. It was not for safety. The first version only kept what bun already sent, and matching node is simpler and better.
Three defects of the field go away with it, so they need no fix of their own: the field went out before the Pad Length octet on a padded session (the session ended), its weight byte was always 0, and a I rebased on main and updated the PR title and body. |
|
A review on #43599 asked that bun match node v26.3.0, which never sets the PRIORITY flag on a HEADERS frame (nghttp2 1.69.0 discards the priority spec): #43599 (comment) #43599 now removes the
|
|
Correction to my note above: after a second review comment, #43599 now includes |
|
Closing in favor of #43599. Both PRs got the same request (match node v26.3.0 and send no PRIORITY field), and both now make the same change. The #43599 covers everything this PR set out to fix:
Only one of the two needs to land. @cirospaciari there is nothing left to review here. |
Problem
request(headers, { parent: 0 })fails withERR_HTTP2_STREAM_ERROR: Stream closed with error code NGHTTP2_INTERNAL_ERROR. The next request fails withERR_HTTP2_SESSION_ERROR: Session closed with error code 9. node v26.3.0 answers both with 200.parent <= 0inH2FrameParser::request()(src/runtime/api/bun/h2_frame_parser.rs:6974). It runs after the header block is in the HPACK encoder, and no HEADERS frame follows. A negative or NaNparenttakes the same path, where node throwsRangeErrorERR_OUT_OF_RANGE(src/js/node/http2.ts:5962sets no minimum).exclusive: true, a non-zeroparentor aweight(h2_frame_parser.rs:7178). node v26.3.0 never does: its nghttp2 1.69.0 discards the priority spec, so every request HEADERS frame has flags 0x05.Fix
exclusive,parentorweight, and never writes a PRIORITY field.request()andrespond()share that writer, so both match node.request()callsvalidateNumber(options.parent, "options.parent", 0), like node'ssetAndValidatePriorityOptions.StreamPriority,UInt31WithReserved::to_uint32,HeadersFrameFlags::PRIORITYand the per-streamweightfield.state.weightreports 16, as node does.test/js/node/http2/node-http2.test.js(5 new tests, all fail on bun 1.4.3-canary), the rest oftest/js/node/http2/, and node's 261test-http2-*.jsfiles. The two wire-level scenarios pass under node v26.3.0 (Notes).Background
GOAWAY(COMPRESSION_ERROR), code 9.parent) and a weight.Notes
History of this PR. The first version kept the PRIORITY field and only made
parent: 0legal (no field for the default dependency, and no self-dependency on the wire). A maintainer asked to match node instead, which sends no PRIORITY field at all. The current version does that, and it removes the code the first version changed.Repro for
parent(bun file.mjsandnode file.mjs):node v26.3.0, and bun with this change (the output is the same, message text included):
bun 1.4.3-canary.1+367d939d9, for
-1,0,NaN,-Infinity,-0and-0.5:What goes on the wire. A raw TCP peer that records the flags of each request HEADERS frame (a GET, so END_STREAM | END_HEADERS = 0x05 with no PRIORITY):
{ parent: 0 }{ exclusive: true },{ parent: 0, exclusive: true }{ parent: 1 },{ exclusive: true, parent: 3 }{ parent: 2 ** 31 - 1, exclusive: true }The new tests, in
describe("http2 priority options"):request()withparent0, -0, 0 withexclusive: true, 1.5, 2 ** 31 and Infinity gets a 200 from anhttp2.createServer()peer each time. A plain request on the same session then gets a 200, and the session emits no error.request()withparent-1, -0.5, -Infinity and NaN throwsRangeErrorERR_OUT_OF_RANGEwith node's message, before and after'connect'. A plain request on the same session then gets a 200.paddingStrategyALIGNED or MAX, a request with{ exclusive: true, parent: 3 }and then a plain request both get a 200.respond()and{ parent: 0 },{ parent: -1 },{ parent: 3 },{ weight: 0 },{ weight: 16 },{ exclusive: true }. Every response HEADERS frame arrives, and none has the PRIORITY flag.Tests 3 and 5 as one standalone script (
node:assert, raw sockets) print the same under node v26.3.0 and under this branch:Defects of the PRIORITY field that go away with it. None of them needs its own fix now:
http2.connect(url, { paddingStrategy: PADDING_STRATEGY_MAX })followed byrequest(h, { exclusive: true })ended the session with error code 9, and ALIGNED ended it with error code 1. Test 4 covers this.request()deletesoptions.weight(DEP0194). The default weight is 16.parentequal to the stream's own id put a self-dependency on the wire (RFC 7540 5.3.1 forbids it). A node v26.3.0 server answeredclient.request(h, { parent: 1 }), as the first request of a session, with a GOAWAY.respond(headers, { parent: -1 }),{ weight: 0 }and{ weight: 300 }reset the stream with no frame on the wire, so the client waited forever.What
respond()still gets wrong, outside this PR:{ silent: 1 },{ endStream: 0 }and asignalstill reach the native type checks and throw. #43478 coversrespond()options.Relation to other PRs. #32995 (closed as stale, not rejected) fixed
parent: 0as one part of a larger rewrite ofrequest(). #41520 (open) moves the HPACK encode after the option checks. It touches thehas_priorityandpriority_overheadlines that this PR deletes, so the second of the two to land needs a rebase. #43536 (open) moves the JS option checks after the options copy, in node's order. It moves thevalidateNumber(options.parent, ...)line that this PR edits, and the second to land must keep the0. #43599 (open) corrects the weight octet of the PRIORITY field. This PR stops writing the field, so #43599 has nothing left to fix if this one lands. #43554 (open) fixes the padded frame length and also moves the priority fields behind the Pad Length octet inrequest(). Its padding fix is independent. The order of the two fields has no effect after this PR, and the second to land needs a rebase.One difference from node stays, and #43536 removes it.
request()validatesendStream,parent,exclusiveandsilenton the caller's object and copies the object later. node copies first and validates the copy. So an inherited or non-enumerable option is checked in bun and ignored by node. On main an inheritedparent: "x"throws for this reason. With this PR an inheritedparent: -1throws too.request(headers, 5)returns a stream, and node throwsERR_INVALID_ARG_TYPE. #43491 covers that.Suites run with the debug build:
test/js/node/http2/node-http2.test.js(395 pass, 6 skip, 0 fail),h2-conformance(70),node-http2-streams-rehash(5),node-http2-invalid-padding(7),node-http2-continuation(9),node-http2-client-close(69),node-http2-settings-ack-ordering(1), and the 261test-http2-*.jsfiles undertest/js/node/test/(all exit 0).[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file