Skip to content

Commit

Permalink
Disable automatic handling of Sec-WebSocket-Protocol header (#493)
Browse files Browse the repository at this point in the history
See #490 for a detailed description of the problem this solves.

Previously, we had `ws`'s automatic `Sec-WebSocket-Protocol` handling
enabled, in addition to adding the header ourselves. This lead to
duplicate sub-protocols in the WebSocket handshake response which is
a protocol error.

Workers require users to include this header themselves in WebSocket
`Response`s, so ignoring this key outright when copying headers would
be incorrect. Instead, we just disable `ws`'s automatic handling.
Funnily enough, we had exactly the same issue in Miniflare 2 too
(#179), and fixed it in 34cc73a. Looks like the fix didn't make it
into Miniflare 3 though. :(

This PR also fixes an issue where `1006` closures were not propagated
correctly. This is an issue in Miniflare 2 too (#465), so we should
back-port this.

Supersedes #490
  • Loading branch information
mrbbot committed Nov 1, 2023
1 parent 8c107b1 commit 45367ba
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 3 deletions.
2 changes: 2 additions & 0 deletions packages/miniflare/src/http/websocket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,8 @@ export async function coupleWebSocket(
pair.addEventListener("close", (e) => {
if (e.code === 1005 /* No Status Received */) {
ws.close();
} else if (e.code === 1006 /* Abnormal Closure */) {
ws.terminate();
} else {
ws.close(e.code, e.reason);
}
Expand Down
8 changes: 7 additions & 1 deletion packages/miniflare/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,13 @@ export class Miniflare {
this.#log.debug(`Running workerd ${desc}...`);

this.#liveReloadServer = new WebSocketServer({ noServer: true });
this.#webSocketServer = new WebSocketServer({ noServer: true });
this.#webSocketServer = new WebSocketServer({
noServer: true,
// Disable automatic handling of `Sec-WebSocket-Protocol` header,
// Cloudflare Workers require users to include this header themselves in
// `Response`s: https://github.com/cloudflare/miniflare/issues/179
handleProtocols: () => false,
});
// Add custom headers included in response to WebSocket upgrade requests
this.#webSocketExtraHeaders = new WeakMap();
this.#webSocketServer.on("headers", (headers, req) => {
Expand Down
15 changes: 13 additions & 2 deletions packages/miniflare/test/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,13 @@ test("Miniflare: web socket kitchen sink", async (t) => {

// Create WebSocket origin server
const server = http.createServer();
const wss = new WebSocketServer({ server });
const wss = new WebSocketServer({
server,
handleProtocols(protocols) {
t.deepEqual(protocols, new Set(["protocol1", "protocol2"]));
return "protocol2";
},
});
wss.on("connection", (ws, req) => {
// Testing receiving additional headers sent from upgrade request
t.is(req.headers["user-agent"], "Test");
Expand Down Expand Up @@ -63,7 +69,11 @@ test("Miniflare: web socket kitchen sink", async (t) => {

// Testing dispatchFetch WebSocket coupling
const res = await mf.dispatchFetch("http://localhost", {
headers: { Upgrade: "websocket", "User-Agent": "Test" },
headers: {
Upgrade: "websocket",
"User-Agent": "Test",
"Sec-WebSocket-Protocol": "protocol1, protocol2",
},
cf: { country: "MF" },
});

Expand All @@ -74,6 +84,7 @@ test("Miniflare: web socket kitchen sink", async (t) => {
res.webSocket.close(1000, "Test Closure");
// Test receiving additional headers from upgrade response
t.is(res.headers.get("Set-Cookie"), "key=value");
t.is(res.headers.get("Sec-WebSocket-Protocol"), "protocol2");

// Check event results
const clientEvent = await clientEventPromise;
Expand Down

0 comments on commit 45367ba

Please sign in to comment.