Skip to content

fix(websocket): fail the connection on a non-200 h2 extended CONNECT response - #5751

Merged
mcollina merged 1 commit into
nodejs:mainfrom
kjsik11:fix/websocket-h2-non-200
Sep 3, 2026
Merged

fix(websocket): fail the connection on a non-200 h2 extended CONNECT response#5751
mcollina merged 1 commit into
nodejs:mainfrom
kjsik11:fix/websocket-h2-non-200

Conversation

@kjsik11

@kjsik11 kjsik11 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

This relates to...

Refs #5063: fixed for client.upgrade() in #5072, but the fetch/WebSocket path was left out — with no assertion to crash, it hangs instead of failing.

Rationale

When the server enables the extended CONNECT protocol and answers the handshake with anything but 200 (e.g. 401), the client gets no error and no close: it just hangs in CONNECTING forever, with nothing for the caller to await or catch.

Standalone repro — hangs forever on main, fails cleanly with this patch

Save as repro.mjs in the repo root and run node repro.mjs:

import { readFileSync } from "node:fs";
import { once } from "node:events";
import { createSecureServer } from "node:http2";
import { WebSocket, Agent } from "./index.js";

// A WebSocket endpoint that refuses the handshake with 401.
const server = createSecureServer({
  key: readFileSync("./test/fixtures/key.pem"),
  cert: readFileSync("./test/fixtures/cert.pem"),
  settings: { enableConnectProtocol: true },
});

server.on("stream", (stream) => {
  stream.respond({ ":status": 401 });
  stream.end();
});

await once(server.listen(0), "listening");

const dispatcher = new Agent({
  allowH2: true,
  connect: { rejectUnauthorized: false },
});
const ws = new WebSocket(`wss://localhost:${server.address().port}`, {
  dispatcher,
});

const stop = () => {
  clearTimeout(timer);
  dispatcher.destroy();
  server.close();
};

ws.onopen = () => {
  console.log("open");
  stop();
};
ws.onerror = ({ error }) => console.log("error:", error?.constructor.name);
ws.onclose = ({ code, wasClean }) => {
  console.log("close:", code, "wasClean:", wasClean);
  stop();
};

const timer = setTimeout(() => {
  console.log(
    "5s later: no event, readyState =",
    ws.readyState,
    "(0 = CONNECTING)",
  );
  stop();
}, 5000);

On main nothing is ever emitted:

5s later: no event, readyState = 0 (0 = CONNECTING)

With this patch the connection fails as it should:

error: TypeError
close: 1006 wasClean: false

onRequestUpgrade in lib/web/fetch/index.js returns false for a non-200 h2 response, but nothing honors that return value: onUpgradeResponse in client-h2.js finalizes the request regardless, so neither resolve nor reject ever runs and the WebSocket is stranded.

Changes

  • lib/web/fetch/index.js: abort with the existing SocketError('bad upgrade') when the h2 extended CONNECT response is not 200, so the handshake fails instead of hanging.
  • test/websocket/opening-handshake.js: regression test — the server answers 401, asserts error + close, and times out when nothing is emitted (as on main); passes with this patch.

Features

N/A

Bug Fixes

  • WebSocket over h2 now fails the handshake instead of hanging in CONNECTING forever when the extended CONNECT response is not 200

Breaking Changes and Deprecations

N/A

Assisted-by: Claude Code

Status

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (ede2a74) to head (da62877).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5751      +/-   ##
==========================================
+ Coverage   93.46%   93.47%   +0.01%     
==========================================
  Files         110      110              
  Lines       38939    38945       +6     
==========================================
+ Hits        36394    36404      +10     
+ Misses       2545     2541       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina
mcollina merged commit 9ceae23 into nodejs:main Sep 3, 2026
38 checks passed
@kjsik11
kjsik11 deleted the fix/websocket-h2-non-200 branch September 3, 2026 14:12
@github-actions github-actions Bot mentioned this pull request Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants