Repository navigation
Conversation
…ssion.origin()
- altsvc(alt, string): a string that does not parse as a URL now throws
ERR_INVALID_URL, like node's getURLOrigin(). It threw
ERR_HTTP2_ALTSVC_INVALID_ORIGIN.
- origin({ origin: '' }): node rejects only the origin 'null'. An empty
origin is sent as a zero-length ORIGIN entry. It threw
ERR_HTTP2_INVALID_ORIGIN.
- altsvc(alt) without an origin and without a stream id wrote an ALTSVC
frame with an empty origin on stream 0, which RFC 7838 section 4 makes
invalid. Node aborts the process on a CHECK in Http2Session::AltSvc, so
there is no error to match. It now throws
ERR_HTTP2_ALTSVC_INVALID_ORIGIN, the error node uses for an empty
object origin.
|
Warning Review limit reached
On-demand reviews are free for the next 9 days. After that, they cost $0.25 per reviewed file. Or wait 4 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 6:35 PM PT - Sep 11th, 2026
❌ @robobun, your commit 31b6c98 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 42397That installs a local version of the PR into your bun-42397 --bun |
|
Status: ready for review. The diff is green. The red CI jobs are tests that this PR does not touch. How it was reproduced: an h2c server calls USE_SYSTEM_BUN=1 bun test test/js/node/http2/node-http2.test.js -t "reject an origin with the same error as Node|zero-length ORIGIN entry" # 2 fail
bun bd test test/js/node/http2/node-http2.test.js -t "reject an origin with the same error as Node|zero-length ORIGIN entry" # 2 passCI: this PR changes
Both are reported for triage. Neither test uses PR: #42397 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node compat fix with the error contract matched per cited source lines.
What was reviewed:
getOrigin()now rethrowsERR_INVALID_URLfor both callers and only rejects"null"; checked thataltsvc's object-origin branch (line 4395) still rejects""independently, and that"file:///x"still maps to the"null"→ERR_HTTP2_*_INVALID_ORIGINpath viaURL#origin.- The new
stream === 0 && origin === ""guard is placed afteralt/length validation so check ordering matches Node up to the point Node aborts; the divergence is commented with thenode_http2.ccCHECK line. - Tests: matrix asserts exact
name + codeper call via onetoEqual, wireserrorto reject, usesport: 0/127.0.0.1; the wire test reuses the existingframesBeforePingAck/originEntrieshelpers in the same describe block.
Extended reasoning...
Overview
This PR adjusts origin validation in src/js/node/http2.ts for ServerHttp2Session#altsvc() and #origin() to match Node v26.3.0's error contract. Three changes: (1) getOrigin() no longer remaps a URL parse failure to ERR_HTTP2_ALTSVC_INVALID_ORIGIN for altsvc — it always throws ERR_INVALID_URL; (2) getOrigin() no longer rejects a falsy origin, only the literal "null", so origin({origin: ""}) sends a zero-length ORIGIN entry as Node does; (3) altsvc(alt) with neither an origin nor a stream id now throws ERR_HTTP2_ALTSVC_INVALID_ORIGIN instead of writing an RFC-7838-invalid frame (Node aborts on a native CHECK here — the divergence is deliberate and commented with a source permalink). Two tests are added to the existing test/js/node/http2/node-http2.test.js.
Security risks
None. This is error-code alignment for a server-side HTTP/2 API. The one behavioral loosening — allowing origin({origin: ""}) to write a zero-length ORIGIN entry — matches Node exactly and is a server-initiated advisory frame, not input validation on untrusted data. No auth, crypto, or parsing of untrusted bytes is touched.
Level of scrutiny
Low-to-moderate. The source change is ~15 lines, entirely within the JS built-in module, and each decision cites a nodejs/node source line. I verified that the object-origin branch of altsvc() (lines 4387-4397) still independently rejects "" and "null", so removing the !origin check from getOrigin() doesn't regress altsvc(alt, {origin: ""}). The "file:///x" cases still hit ERR_HTTP2_*_INVALID_ORIGIN because new URL("file:///x").origin === "null". The new guard's placement after validateString(alt) / kQuotedString / length checks preserves Node's check ordering for doubly-invalid input up to the abort point.
Other factors
The tests follow repo conventions closely: added to the existing file, port: 0 + 127.0.0.1, Promise.withResolvers(), error events wired to reject, a single expect(outcomes).toEqual({...}) asserting exact err.name + err.code for the full matrix, and a wire-level assertion reusing the file's existing framesBeforePingAck/originEntries helpers inside the same describe block. No CODEOWNERS cover these paths. The bug hunt exited on dry_streak with no findings and no ruled-out candidates.
![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 commits tightened the comments to one-liners with the Node permalink kept, and the runtime change itself is unchanged and correct.
What was reviewed:
getOriginpaths: string → URL parse throwsERR_INVALID_URLfor both callers; only the literal"null"origin is rejected post-parse, soorigin({origin: ""})now passes through — matches Node'score.jsloop.altsvc()object-origin branch (lines 4384-4396) still rejects""/"null", soaltsvc(alt, {origin: ""})still throwsERR_HTTP2_ALTSVC_INVALID_ORIGINas Node does; the newstream === 0 && origin === ""guard only affects theundefinedcase where Node aborts on a CHECK.- Tests: one
.toEqualon the full name+code matrix,Promise.withResolvers, error events wired to reject,port: 0on 127.0.0.1; the wire test reuses the existingframesBeforePingAck/originEntrieshelpers verbatim.
Extended reasoning...
Overview
This PR aligns ServerHttp2Session#altsvc() and #origin() invalid-origin error semantics with Node v26.3.0. In src/js/node/http2.ts, getOrigin() now always throws ERR_INVALID_URL when a string origin fails URL parsing (previously the altsvc path substituted ERR_HTTP2_ALTSVC_INVALID_ORIGIN), and the post-parse rejection is narrowed from any falsy origin to only the literal "null". altsvc() gains a guard that throws ERR_HTTP2_ALTSVC_INVALID_ORIGIN when neither an origin nor a stream id is supplied — the case where Node aborts on a CHECK in node_http2.cc. Two tests are added to test/js/node/http2/node-http2.test.js: a full error-code matrix asserted as one object, and an over-the-wire check that {origin: ""} produces zero-length ORIGIN entries.
Security risks
None. This is error-code alignment on a server-side HTTP/2 API. The one behavior loosening — origin({origin: ""}) no longer throws and instead sends a zero-length ORIGIN entry — matches Node exactly and is a valid RFC 8336 frame. The altsvc(alt) case is tightened (throws instead of writing an invalid frame that would cause peer session errors), which is strictly safer than before and safer than Node's process abort.
Level of scrutiny
Moderate, as a node:* compat change. The diff is ~15 lines of runtime code with Node source permalinks on both non-obvious choices, and the deliberate divergence (throw vs. abort) is documented at the guard and in the test. I traced every input class through both getOrigin() and the inline object-origin branch in altsvc(): string origins that parse to "null" (e.g. file:///x) still hit the right per-caller error; altsvc(alt, {origin: ""}) still throws via the unchanged inline check at lines 4394-4396, matching Node's origin.length === 0 check; valid string origins on stream 0 pass the new guard because origin !== "".
Other factors
Since the earlier COMMENTED review, the two follow-up commits only shortened the two source comments to single lines while keeping the Node permalinks — no logic changed. The github-actions inline comments landed on those exact lines and were followed by the shortening commit, so they appear addressed. Tests follow every harness convention called out in REVIEW.md: Promise.withResolvers, error events wired to reject, port: 0 bound to 127.0.0.1, one .toEqual on the whole outcomes object asserting err.name + err.code, and the wire test reuses the existing framesBeforePingAck/originEntries helpers from the same describe block. Exit reason was dry_streak.
Problem
ServerHttp2Sessiondiffer from node v26.3.0.altsvc(alt, origin)with a string that is not a URL ("","not a url","null","1") throwsERR_HTTP2_ALTSVC_INVALID_ORIGIN, node throwsERR_INVALID_URL.origin({ origin: "" })throwsERR_HTTP2_INVALID_ORIGIN, node sends a zero-length ORIGIN entry because it rejects only"null".altsvc(alt)with no origin and no stream id writes an ALTSVC frame with an empty origin on stream 0. RFC 7838 section 4 makes that frame invalid. A node client answers it with a session error.getOrigin(src/js/node/http2.ts:3925) replaces the URL error foraltsvcand rejects every falsy origin.altsvc()does not check the call without a second argument.Fix
getOriginrethrowsERR_INVALID_URLfor both callers and rejects only"null", likegetURLOrigin()and the loop in node'sorigin().altsvc(alt)with neither an origin nor a stream id throwsERR_HTTP2_ALTSVC_INVALID_ORIGIN, after every other argument check. Node has no error to match here: it aborts the process (CHECKinHttp2Session::AltSvc,node_http2.cc:3229). The code is the one node uses for an empty object origin.test/js/node/http2/node-http2.test.js, both fail on 1.4.3-canary. All ofnode-http2.test.jsand the upstreamtest-http2-altsvc.jsandtest-http2-origin.jspass.Background
ServerHttp2Session#altsvc(alt, originOrStream)sends an ALTSVC frame (RFC 7838): on stream 0 with an origin, or on a stream without one.ServerHttp2Session#origin(...origins)sends an ORIGIN frame (RFC 8336): a list of length-prefixed origins.originproperty is sent as is.Notes
Outcomes. Same script on node v26.3.0, bun 1.4.3-canary and this PR. The calls run in the
'session'listener of an h2c server. A raw client reads the ALTSVC and ORIGIN frames that arrive.altsvc(alt, ""),"not a url","null","1"altsvc(alt, "file:///x")origin({ origin: "" })00000000origin({ origin: "" }, "https://b.example")0000+ entry0000+ entryorigin({ origin: "null" })altsvc(alt),altsvc(alt, undefined)Assertion failed: (origin_len != 0 && id == 0) || (origin_len == 0 && id != 0)0000+ alt on stream 0The other 28 rows of the matrix are identical in all three: number arguments (
0,1.5,-1,2**32, a stream that does not exist),null,true,{},{ origin: 1 }, a non-stringalt, analtwith a character above U+00FF,origin()with no arguments,origin(""),origin("not a url"),origin("file:///x").Why throw for
altsvc(alt). The choices are: keep the invalid frame, drop the call in silence, or throw. A client that follows nghttp2 (node, and bun after #42391) destroys the session when it gets that frame. A throw tells the caller what is wrong at the call site. No program that runs on node can depend on the old behavior, because node aborts.Not matched.
ERR_INVALID_URLfromgetURLOrigin()comes from C++ and has noinputproperty. Bun's hasinput, asnew URL()does in both runtimes.origin()already behaved this way.header_value_bytes).