Skip to content

node:http2: match Node's unsupported-protocol check in connect() - #43505

Open
robobun wants to merge 4 commits into
mainfrom
robobun/d80742c6/http2-unsupported-protocol
Open

node:http2: match Node's unsupported-protocol check in connect()#43505
robobun wants to merge 4 commits into
mainfrom
robobun/d80742c6/http2-unsupported-protocol

Conversation

@robobun

@robobun robobun commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Problem

  • http2.connect("ftp://127.0.0.1:1") throws ERR_HTTP2_UNSUPPORTED_PROTOCOL with the message ftp:. Node.js v26.3.0 says protocol "ftp:" is unsupported. ErrorCode.cpp has no message text for this code, so the first argument becomes the message.
  • The ClientHttp2Session constructor (src/js/node/http2.ts) checks the protocol before it looks at options.createConnection. Node.js checks it only when it opens the socket itself (connect()). So http2.connect("ftp://host", { createConnection }) throws in Bun only.
  • request() takes the default :scheme from the URL, then from its own options. http2.connect({ port }, { protocol: "http:" }) sends :scheme: https. Node.js sends http.

Fix

  • Add the message row to the simpleErrorMessages table. Move the protocol switch into connectWithProtocol, which runs only when createConnection is not a function. This supersedes node:http2: add throw for ERR_HTTP2_UNSUPPORTED_PROTOCOL #17927, which has the same placement.
  • The constructor stores the default :scheme: the connect-time protocol without the trailing : (as in Node.js). request() reads it for both header forms (an object, a raw [name, value, ...] array). Without this, a custom protocol would send :scheme: ftp:.
  • A protocol in the request() options no longer changes :scheme. Node.js has no such option. The #url field had no other reader, so it is gone.
  • Verified: test/js/node/http2/node-http2-connect-protocol.test.ts (4 tests, a debug build of main fails all 4, Node.js passes the same scenarios). Also 14 upstream test-http2-* files. Self-reviewed: 5 concerns raised, 5 addressed (see Notes).

Background

  • http2.connect(authority, options) opens a client session. The protocol is authority.protocol, then options.protocol, then https:.
  • options.createConnection lets the caller supply the transport. The protocol then only names the :scheme.
  • $ERR_* calls in built-in JS go through jsFunctionMakeErrorWithCode. A code with no case there and no table row uses its first argument as the message.
Notes

Repro (run with bun file.cjs and node file.cjs):

const http2 = require("node:http2");
const net = require("node:net");
try { http2.connect("ftp://127.0.0.1:1"); } catch (e) { console.log(e.code, JSON.stringify(e.message)); }

const server = http2.createServer();
server.on("stream", (stream, headers) => { stream.respond(); stream.end(headers[":scheme"]); });
server.listen(0, "127.0.0.1", async () => {
  const port = server.address().port;
  const createConnection = () => net.connect(port, "127.0.0.1");
  const get = (client, requestOptions) => new Promise(resolve => {
    const req = client.request({ ":path": "/" }, requestOptions);
    let body = "";
    req.setEncoding("utf8").on("data", d => (body += d)).on("end", () => { client.close(); resolve(body); });
  });
  try {
    console.log(await get(http2.connect(`ftp://127.0.0.1:${port}`, { createConnection })));
  } catch (e) { console.log(e.code); }
  console.log(await get(http2.connect({ hostname: "127.0.0.1", port }, { protocol: "http:" })));
  console.log(await get(http2.connect({ hostname: "127.0.0.1", port }, { createConnection }), { protocol: "http:" }));
  server.close();
});
Node.js v26.3.0 bun 1.4.3-canary.1+367d939d9 this PR
message protocol "ftp:" is unsupported. ftp: protocol "ftp:" is unsupported.
ftp:// with createConnection ftp throws ERR_HTTP2_UNSUPPORTED_PROTOCOL ftp
{ protocol: "http:" } in the connect() options http https http
{ protocol: "http:" } in the request() options, default https: session https http https

[human-review] gate passed · iteration 1 · 4 files touched

fails on main (without fix) 
ASAN without fix: 4 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/node-http2-connect-protocol.test.ts"
bun test v1.4.3 (367d939d9)

test/js/node/http2/node-http2-connect-protocol.test.ts:
69 |       name: "Error",
70 |       code: "ERR_HTTP2_UNSUPPORTED_PROTOCOL",
71 |       message: `protocol "${protocol}" is unsupported.`,
72 |     });
73 | 
74 |     expect(thrownBy("ftp://127.0.0.1:1")).toEqual(unsupported("ftp:"));
                                               ^
error: expect(received).toEqual(expected)

  {
    "code": "ERR_HTTP2_UNSUPPORTED_PROTOCOL",
-   "message": "protocol "ftp:" is unsupported.",
+   "message": "ftp:",
    "name": "Error",
  }

- Expected  - 1
+ Received  + 1

      at <anonymous> (/workspace/bun/test/js/node/http2/node-http2-connect-protocol.test.ts:74:43)
(fail) http2.connect() authority protocol > throws ERR_HTTP2_UNSUPPORTED_PROTOCOL with Node's message [95.13ms]
84 |   test("is not checked when options.createConnection opens the socket", async () => {
85 |     const { server, clientSide } = echoServerOverDuplexPair();
86 |     const createConnection
... (truncated)

release without fix: 4 FAILED
bun test v1.4.3-canary.1 (367d939d9)

test/js/node/http2/node-http2-connect-protocol.test.ts:
69 |       name: "Error",
70 |       code: "ERR_HTTP2_UNSUPPORTED_PROTOCOL",
71 |       message: `protocol "${protocol}" is unsupported.`,
72 |     });
73 | 
74 |     expect(thrownBy("ftp://127.0.0.1:1")).toEqual(unsupported("ftp:"));
                                               ^
error: expect(received).toEqual(expected)

  {
    "code": "ERR_HTTP2_UNSUPPORTED_PROTOCOL",
-   "message": "protocol "ftp:" is unsupported.",
+   "message": "ftp:",
    "name": "Error",
  }

- Expected  - 1
+ Received  + 1

      at <anonymous> (/workspace/bun/test/js/node/http2/node-http2-connect-protocol.test.ts:74:43)
(fail) http2.connect() authority protocol > throws ERR_HTTP2_UNSUPPORTED_PROTOCOL with Node's message [1.21ms]
84 |   test("is not checked when options.createConnection opens the socket", async () => {
85 |     const { server, clientSide } = echoServerOverDuplexPair();
86 |     const createConnection = mock((_authority: URL) => clientSide);
87 |     let client: http2.ClientHttp2Session | undefined;
88 |     try {
89 |       client = connect("ftp://example.test", { createConnect
... (truncated)
passes on PR (with fix) 
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/node-http2-connect-protocol.test.ts"
bun test v1.4.3 (367d939d9)

test/js/node/http2/node-http2-connect-protocol.test.ts:
(pass) http2.connect() authority protocol > throws ERR_HTTP2_UNSUPPORTED_PROTOCOL with Node's message [109.63ms]
(pass) http2.connect() authority protocol > is not checked when options.createConnection opens the socket [865.33ms]
(pass) http2.connect() authority protocol > from options.protocol is the default :scheme of a request [628.41ms]
(pass) http2.connect() authority protocol > is not overridden by a protocol in the request() options [131.16ms]

 4 pass
 0 fail
 10 expect() calls
Ran 4 tests across 1 file. [6.21s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 1021ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/127] gen ErrorCode+*.h
[2/127] gen cpp.rs (cppbind)
[3/127] gen JS modules (bundle-modules)
Preprocess modules (13334ms)
Bundle modules (119ms)
Postprocesss modules (27ms)
Bundle Functions (661ms)
Generate Code (43ms)

[14.19s] Bundled "src/js" for production
  2606 kb
  197 internal modules
  13 native modules
  50 internal functions across 16 files
[3/10] cargo bun_runtime → libbun_runtime.a
�[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m
   �[1m�[94m|�[0m
�[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl
   �[1m�[94m|�[0m                                              �[1m�[33m^^^^^^^^^^^^^�[0m
   �[1m�[94m|�[0m
   �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default
�[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name`
  �[1m�[94m--> �[0msrc/install/windows-shim/Cargo.toml:41:8
   �[1m�[94m|�[0m
�[1m�[94m41�[0m �[91m- �[0mname = �[91m"bun_shim_impl"�[0m
�[1m�[94m
... (truncated)
diff hotspot 
src/js/builtins.d.ts                               |   1 +
 src/js/node/http2.ts                               |  43 +++----
 src/jsc/bindings/ErrorCode.cpp                     |   1 +
 .../node/http2/node-http2-connect-protocol.test.ts | 135 +++++++++++++++++++++
 4 files changed, 151 insertions(+), 29 deletions(-)

gate history · 2 passed · 1 rejected · iteration 1

evidence per changed file 
file                                                    reads  edits  tests
src/js/builtins.d.ts                                        1      1     11
src/js/node/http2.ts                                        4      7     14
src/jsc/bindings/ErrorCode.cpp                              1      1     11
test/js/node/http2/node-http2-connect-protocol.test.ts      0      2      9

@robobun @nektro
node:http2: match Node's unsupported-protocol check in connect()
3e1247c 
http2.connect() throws ERR_HTTP2_UNSUPPORTED_PROTOCOL with Node's message,
and only when it opens the socket itself. With options.createConnection the
protocol is not checked. request() takes its default :scheme from the
protocol the session was connected with, minus the trailing colon. A
protocol in the request() options no longer changes it.

Co-authored-by: Meghan Denny <hello@nektro.net>
@robobun
robobun requested a review from alii as a code owner September 19, 2026 18:47
@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: d3e6d6d9-6194-4427-8a5b-8b92b19db5ac

📥 Commits

Reviewing files that changed from the base of the PR and between 42cc173 and ae43c6a.

📒 Files selected for processing (4)
  • src/js/builtins.d.ts
  • src/js/node/http2.ts
  • src/jsc/bindings/ErrorCode.cpp
  • test/js/node/http2/node-http2-connect-protocol.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Walkthrough

The change adds HTTP/2 unsupported-protocol error handling, restricts client connections to http: and https:, stores the selected default scheme, and adds protocol behavior tests.

Changes

HTTP/2 protocol handling

Layer / File(s) Summary
Unsupported protocol error contract
src/js/builtins.d.ts, src/jsc/bindings/ErrorCode.cpp		 Adds the $ERR_HTTP2_UNSUPPORTED_PROTOCOL declaration and formats its runtime message as protocol "<argument>" is unsupported.
Client protocol selection
src/js/node/http2.ts		 Selects net.connect for http: and tls.connect for https:. Other protocols raise ERR_HTTP2_UNSUPPORTED_PROTOCOL. The client session stores the protocol-derived default scheme without its trailing colon.
Request scheme propagation
src/js/node/http2.ts, test/js/node/http2/node-http2-connect-protocol.test.ts		 Uses the stored default scheme for raw and object-form requests. Tests cover unsupported protocols, custom sockets, authority protocols, and request-level protocol options.

Suggested reviewers: cirospaciari

Priority: ⬇️ Low

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary change: matching Node.js unsupported-protocol behavior in node:http2 connect().
Description check ✅ Passed The description explains the problem, fix, scope, background, and verification results. It does not use the exact template headings, but it provides the required information in equivalent sections.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 12:18 AM PT - Sep 20th, 2026

✅ @robobun, your commit ae43c6a19bd0b5f36f2058cf00f8b32d2e1c5fc0 passed in Build #118855! 🎉

🧪   To try this PR locally:

bunx bun-pr 43505

That installs a local version of the PR into your bun-43505 executable, so you can run:

bun-43505 --bun

claude[bot]
claude Bot reviewed Sep 19, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it changes observable node:http2 behavior (the throw now happens only when http2 opens the socket itself, and a request-level protocol option no longer affects :scheme) and touches src/js/builtins.d.ts, which has a designated code owner, a human look is still worthwhile.

What was reviewed:

  • Compared the new connectWithProtocol switch and constructor ordering against Node v26.3.0 core.js connect(): the check sits in the non-createConnection branch there too, and the protocol "<p>" is unsupported. text matches Node's ERR_HTTP2_UNSUPPORTED_PROTOCOL message; the simpleErrorMessages row takes 1 argument, matching the single call site and the new .d.ts declaration.
  • Confirmed both request paths (object headers and raw [name, value, ...] array) now read the same #defaultScheme, and that #url had no remaining readers after the removal.
  • Checked that nothing between the old throw site and the new one (authority string, onConnect closure) acquires a socket or timer, so the later throw does not leak a resource; #defaultScheme without an initializer matches the neighboring #authority/#socket_proxy fields.
  • The only other suite assertion on this code (test/js/node/test/parallel/test-http2-connect.js:127) checks code and name only, so it is unaffected by the message change.
Extended reasoning...

Overview

The PR touches four files. src/js/node/http2.ts moves the unsupported-protocol throw from the top of the ClientHttp2Session constructor into connectWithProtocol (reached only when options.createConnection is not a function), replaces the stored #url with a precomputed #defaultScheme (the connect-time protocol minus its trailing colon, via a newly hoisted StringPrototypeSlice primordial), and makes both request() header paths use that field instead of re-deriving the scheme from this.#url.protocol || options?.protocol where options was accidentally the per-request options. src/jsc/bindings/ErrorCode.cpp adds a simpleErrorMessages row so $ERR_HTTP2_UNSUPPORTED_PROTOCOL(protocol) produces Node's message instead of echoing the argument; src/js/builtins.d.ts adds the matching typed declaration. The test file adds a describe block with four tests covering the throw (string/URL/object authority, options.protocol, non-function createConnection), the createConnection bypass with :scheme: ftp observed server-side over a duplexPair, options.protocol: "http:" becoming the default scheme, and a request-level protocol option not overriding the session scheme.

Security risks

None identified. The relaxed check only applies when the caller supplies their own transport via createConnection, in which case the protocol string is only used as the :scheme pseudo-header value; this mirrors Node exactly. No user input reaches native code differently than before, and the C++ change is a constexpr table entry with a fixed argument count of 1 that matches the single call site.

Level of scrutiny

Moderate. The diff is small and closely tracks Node's connect() in lib/internal/http2/core.js, which I compared against directly: Node computes protocol = authority.protocol || options.protocol || 'https:', then only reaches its switch (protocol) (with default: throw new ERR_HTTP2_UNSUPPORTED_PROTOCOL(protocol)) in the else branch of typeof options.createConnection === 'function', and request() sets :scheme from session[kProtocol].slice(0, -1). The refactor removed two divergent copy-pasted scheme derivations in favor of one field, which is the right direction. I verified that the code path between the old throw location and the new one allocates no socket or timer, so a throw from connectWithProtocol does not leak anything the earlier throw would have avoided. The mock/duplexPair helpers used by the tests are already imported in the file. Still, this is an observable behavior change in a node:* module (two inputs that previously threw or sent a different scheme now behave differently), and src/js/builtins.d.ts matches a *.d.ts CODEOWNERS entry, so a human sign-off is appropriate rather than an automated approve.

Other factors

The four candidate issues raised during the hunt were all Node-parity consequences rather than defects: the request-level protocol override and the createConnection bypass are exactly Node's semantics, the uninitialized #defaultScheme field matches sibling private fields in the same class, and non-string protocol values are coerced the same way Node's slice call coerces them. The only pre-existing suite assertion on this error (test-http2-connect.js) checks code and name, not message, so no existing test was weakened. One pre-existing divergence not introduced here: Bun defaults the port using the merged protocol, while Node uses only authority.protocol; the PR does not claim to address that.

@robobun

robobun commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on bun 1.4.3-canary.1+367d939d9 against Node.js v26.3.0 with the script in the description (bun file.cjs, then node file.cjs). The outputs differ in four places: the message of ERR_HTTP2_UNSUPPORTED_PROTOCOL, the throw with createConnection, the default :scheme from the connect() options, and the protocol request option.

The four tests in test/js/node/http2/node-http2-connect-protocol.test.ts fail on a debug build of main and pass on a debug build of this branch:

bun bd test test/js/node/http2/node-http2-connect-protocol.test.ts   # src/ at main:        0 pass, 4 fail
bun bd test test/js/node/http2/node-http2-connect-protocol.test.ts   # src/ at this branch: 4 pass, 0 fail

PR: #43505

@robobun
node:http2: move the connect() protocol tests to their own file
9399b26 
The tests do not depend on anything in node-http2.test.js. On their own they
run in about 5 seconds on a debug build.
github-actions[bot]
github-actions Bot reviewed Sep 19, 2026
View reviewed changes
Comment thread src/js/node/http2.ts Outdated
@robobun robobun mentioned this pull request Sep 19, 2026
Closed
claude[bot]
claude Bot reviewed Sep 19, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/js/node/http2/node-http2-connect-protocol.test.ts
claude[bot]
claude Bot reviewed Sep 19, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun robobun mentioned this pull request Sep 20, 2026
Closed
claude[bot]
claude Bot reviewed Sep 20, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
@claude claude[bot] claude[bot] left review comments
@alii alii Awaiting requested review from alii alii is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@robobun