Skip to content

fix(vite-plugin): don't mistake pipelined HTTP responses for claimed WebSockets #16516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dedsec-terminal wants to merge 1 commit into from
Closed

fix(vite-plugin): don't mistake pipelined HTTP responses for claimed WebSockets #16516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions packages/vite-plugin-cloudflare/src/__tests__/websockets.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,4 +545,59 @@ describe("handleWebSocket", () => {

await closed;
});

// A pipelined HTTP response can finish while `dispatchFetch` is still in
// flight. Its bytes must not be mistaken for another upgrade listener's
// handshake, or a valid Worker WebSocket is silently dropped.
test("does not treat an in-flight HTTP response as a claimed upgrade", async ({
expect,
}) => {
startMiniflare(`export default {
async fetch() {
await new Promise((resolve) => setTimeout(resolve, 100));
const [client, server] = Object.values(new WebSocketPair());
server.accept();
return new Response(null, { status: 101, webSocket: client });
}
}`);

httpServer.removeAllListeners("request");
httpServer.on("request", (request, response) => {
if (request.url === "/page") {
setTimeout(() => response.end("OK"), 20);
} else {
response.end("OK");
}
});

await listen();

const socket = await connect();
const chunks: Buffer[] = [];
const upgraded = new Promise<void>((resolve) => {
socket.on("data", (chunk) => {
chunks.push(chunk);
if (Buffer.concat(chunks).toString("utf8").includes("HTTP/1.1 101")) {
resolve();
}
});
});

// Pipeline a normal request and an upgrade on one connection so the
// normal response is written while the upgrade is still pending.
socket.write(
"GET /page HTTP/1.1\r\n" +
`Host: 127.0.0.1:${port}\r\n\r\n` +
"GET /ws HTTP/1.1\r\n" +
`Host: 127.0.0.1:${port}\r\n` +
"Upgrade: websocket\r\n" +
"Connection: Upgrade\r\n" +
"Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n" +
"Sec-WebSocket-Version: 13\r\n\r\n"
);

await upgraded;
expect(Buffer.concat(chunks).toString("utf8")).toContain("HTTP/1.1 101");
socket.destroy();
}, 5000);
});
11 changes: 4 additions & 7 deletions packages/vite-plugin-cloudflare/src/websockets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { UNKNOWN_HOST } from "./shared";
import { createHeadersForIncomingMessage, getForwardedProto } from "./utils";
import type { Headers, Miniflare } from "miniflare";
import type { IncomingMessage } from "node:http";
import type { Socket } from "node:net";
import type { Duplex } from "node:stream";
import type * as vite from "vite";

Expand Down Expand Up @@ -50,14 +49,12 @@ export function handleWebSocket(

// Another listener may already have completed the handshake (for
// example Vite DevTools at `/__devtools/__ws`, which the Worker does
// not route). `bytesWritten` is cumulative across keep-alive
// connections, so compare against a baseline rather than zero.
// not route). `ws` attaches a `data` listener when it takes ownership,
// while an in-flight HTTP response on the same keep-alive connection
// only writes bytes, so listener count distinguishes the two.
// See https://github.com/cloudflare/workers-sdk/issues/15654
const bytesWrittenAtStart =
(socket as unknown as Socket).bytesWritten ?? 0;
const isClaimed = () =>
socket.destroyed ||
((socket as unknown as Socket).bytesWritten ?? 0) > bytesWrittenAtStart;
socket.destroyed || socket.listenerCount("data") > 0;
Comment on lines 56 to +57

@devin-ai-integration devin-ai-integration Bot Oct 27, 2026

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.

🔴 Raw upgrade owners lose their sockets

When another listener writes an upgrade response but uses readable instead of data, isClaimed() reports false. The plugin destroys that connection or sends a second handshake, depending on the Worker response.

Learn more

The HTTP server sends each upgrade event to every registered listener. A listener can own the connection by writing an HTTP upgrade response and consuming subsequent input through readable, without ever installing a data listener. The new check then treats that socket as unclaimed in both the no-WebSocket and WebSocket branches, destroying an owned connection or issuing a second handshake. The previous write-count check caught these written handshakes but incorrectly counted pending pipelined HTTP responses.

Example: A Vite plugin writes HTTP/1.1 101 Switching Protocols and registers socket.on('readable', ...) for its protocol. If the Worker returns 404, this handler destroys the plugin's upgraded socket; if the Worker returns a WebSocket, it writes another 101.

Recommended fix: Track ownership using a mechanism that can distinguish the upgrade listener's response from preceding pipelined HTTP responses, without relying exclusively on data listeners. Test an owner that writes its handshake and reads via readable alongside the pipelined-response case.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 56 to +57

@devin-ai-integration devin-ai-integration Bot Oct 27, 2026

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.

🟡 Passive listeners block Worker WebSockets

When another upgrade listener observes socket data without answering, isClaimed() reports ownership. A valid Worker WebSocket then receives no handshake and remains pending.

Learn more

An upgrade listener can register a data listener to inspect incoming bytes without sending an HTTP response or claiming the socket. This check cannot distinguish that observer from a WebSocket owner. When the Worker supplies a WebSocket, the existing isClaimed() branch returns without upgrading or destroying the connection, leaving the client waiting. The former write-count check allowed the Worker handshake because no competing response was written.

Example: A Vite extension attaches socket.on('data', inspectBytes) during every upgrade event, but handles only /inspector. A request to /worker-ws reaches a Worker returning status 101, yet this plugin skips its handshake because the inspector callback is still registered.

Recommended fix: Base ownership on evidence of another handler taking responsibility for this specific upgrade, not on the mere presence of a socket reader. Preserve the case where an ordinary HTTP response to an earlier pipelined request is still in flight.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 56 to +57

@devin-ai-integration devin-ai-integration Bot Oct 27, 2026

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.

🔍 Changeset describes outdated ownership behavior

The existing patch changeset says a written response protects the socket. The new check uses data listeners instead; update the release note to match.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


try {
const rawHost = request.headers.host ?? UNKNOWN_HOST;
Expand Down
Loading