Setting Sec-WebSocket-Protocol header on response to ws request fails

[aboodman]

The standard (and Cloudflare) require that if Sec-WebSocket-Protocol request header is sent in a websocket connection request, the server must acknowledge it by setting a response header with the same value.

If you do this in miniflare, however, the response fails:

  // While Sec-WebSocket-Protocol is just being used as a mechanism for
  // sending `auth` since custom headers are not supported by the browser
  // WebSocket API, the Sec-WebSocket-Protocol semantics must be followed.
  // Send a Sec-WebSocket-Protocol response header with a value
  // matching the Sec-WebSocket-Protocol request header, to indicate
  // support for the protocol, otherwise the client will close the connection.
  //
  // THIS MAKES MINIFLARE SAD
  responseHeaders.set("Sec-WebSocket-Protocol", encodedAuth);

  const response = new Response(body, {
    status,
    statusText,
    webSocket,
    headers: responseHeaders,
  });
  return response;

[mrbbot]

Hey! 👋 Thanks for raising this and apologies for the delayed response. I might be misunderstanding this, but I would've thought this code that copys headers from the Response to the WebSocket response added in 2.1.0 would solve this:

miniflare/packages/http-server/src/index.ts

Lines 353 to 363 in b887877

	webSocketServer.on("headers", (headers, req) => {
	const extra = extraHeaders.get(req);
	extraHeaders.delete(req);
	if (extra) {
	for (const [key, value] of extra) {
	if (!restrictedWebSocketUpgradeHeaders.includes(key.toLowerCase())) {
	headers.push(`${key}: ${value}`);
	}
	}
	}
	});

Which version of Miniflare are you using?

[aboodman]

2.3.0

…

On Fri, Mar 4, 2022 at 10:10 AM MrBBot ***@***.***> wrote: Hey! 👋 Thanks for raising this and apologies for the delayed response. I might be misunderstanding this, but I would've thought this code that copys headers from the Response to the WebSocket response added in 2.1.0 would solve this: https://github.com/cloudflare/miniflare/blob/b887877810a745f2a458d20da0a3dbd9f8a58cf4/packages/http-server/src/index.ts#L353-L363 Which version of Miniflare are you using? — Reply to this email directly, view it on GitHub <#179 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAATUBGDAKCUZU2IC7ZJEWDU6JU25ANCNFSM5OWVZZIQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[aboodman]

I realize this isn't a reduced test case and I apologize. It's possible we are doing something wrong.

What we found was that in order for a web socket connection to succeed in CF's environment, if the request included the header Sec-WebSocket-Protocol: foo then we needed to manually add the response header Sec-WebSocket-Protocol: foo as well -- otherwise the connection would not suceed.

But that if we added that response header to Miniflare then ... something bad happened. I can't remember what the bad thing was, maybe an error, or maybe the connection just failed in some way.

It actually wasn't me who found this on our team. I'll poke the person and see if he can remember what the failure mode was.

[grgbkr]

Miniflare appears to automatically set the Sec-WebSocket-Protocol response header to match the Sec-WebSocket-Protocol request header (while CF Worker does not). This test appears to assert this behavior

miniflare/packages/web-sockets/test/fetch.spec.ts

Line 86 in 92d98c9

test("upgradingFetch: performs web socket upgrade with Sec-WebSocket-Protocol header", async (t) => {

When we set the Sec-WebSocket-Protocol response header on miniflare, we end up with two headers like:

Sec-WebSocket-Protocol: foo
sec-websocket-protocol: foo

The browser does not like receiving these duplicate headers and fails the connection upgrade.

Chrome won't show the response headers with this type of failure, but you can see them in Firefox:

[mrbbot]

Ah ok, that makes more sense! Thanks for the extra details. Will get this fixed. 🙂

For now (you might already be doing this), you could skip adding the Sec-WebSocket-Protocol header yourself if globalThis.MINIFLARE is true.

[grgbkr]

Thanks @mrbbot. Thanks for the suggested workaround, that is exactly what we are doing for now.