Bun.serve: validate the WebSocket opening handshake in server.upgrade()

Merged
RO
4mo ago
+169-1

What does this PR do?

server.upgrade() accepted any GET request with a 24-byte Sec-WebSocket-Key as a WebSocket handshake. A request with Upgrade: h2c, a non-base64 key, or Sec-WebSocket-Version other than 13 (or absent) was answered with 101 Switching Protocols and the app's open() handler ran.

Repro:

import net from "node:net";
const server = Bun.serve({
  port: 0,
  fetch(req, srv) { if (srv.upgrade(req)) return; return new Response("no", { status: 400 }); },
  websocket: { open(ws) { ws.send("hi"); }, message() {} },
});
const sock = net.connect(server.port, "127.0.0.1", () => {
  sock.write(
    "GET / HTTP/1.1\r\nHost: x\r\nUpgrade: h2c\r\nConnection: Upgrade\r\n" +
    "Sec-WebSocket-Key: !!!!!!!!!!!!!!!!!!!!!!==\r\nSec-WebSocket-Version: 8\r\n\r\n"
  );
});
sock.on("data", d => console.log(d.toString().split("\r\n")[0])); // HTTP/1.1 101 Switching Protocols

RFC 6455 requires the server to refuse a handshake whose Upgrade token is not websocket or whose Sec-WebSocket-Key is not the base64 encoding of 16 bytes (section 4.2.1), and to answer an unsupported Sec-WebSocket-Version with an HTTP error carrying a Sec-WebSocket-Version header naming the supported versions (section 4.4).

server.upgrade() now:

  • returns false if the Upgrade header is not websocket (case-insensitive), letting the caller's fetch() respond;
  • returns false if Sec-WebSocket-Key does not match ^[A-Za-z0-9+/]{22}==$;
  • answers 426 Upgrade Required with Sec-WebSocket-Version: 13 and returns false if Sec-WebSocket-Version is missing or not 13.

How did you verify your code works?

New tests in test/js/bun/websocket/websocket-server.test.ts drive raw TCP handshakes for each invalid case (bad Upgrade token, non-base64 key, wrong padding, short key, version 8, missing version) plus a valid handshake and a mixed-case Upgrade: WebSocket, and assert the status and that open() is not called for the rejected cases. A second test covers the req.headers-materialized async path.


no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/websocket/websocket-server.test.ts