upgrade hook runs even on non-websocket upgrade requests

[eltigerchino]

Describe the change

Currently, the upgrade hook runs for any upgrade request, even non-websocket ones, but returns 400 Invalid Upgrade header afterwards if Upgrade: websocket isn't present. This is because the Node adapter documentation suggests attaching the upgrade handler such as below:

// this handles all upgrade requests, not just websocket ones
server.on("upgrade", ws.handleUpgrade);

Ideally, the crossws upgrade hook only runs if the header Upgrade: websocket exists since the upgrade hook has probably been programmed with only websocket upgrade requests in mind.

Alternatively, we could change the documentation to suggest handling this case ourselves (not so ideal):

server.on("upgrade", (req, socket, head) => {
  if (req.headers.upgrade === 'websocket') {
    ws.handleUpgrade(req, socket, head);
    return;
  }
  // TODO: reject connection
});

URLs

No response

Additional information

• Would you be willing to help?

[pi0]

Thats a good pooint and idea! Indeed documentation should avice explicit value check.

I have to check implementation bug i guess for other adapters we also ask users to check header manually.

[LukeHagar]

This seems like a good thing to add to the docs as stated, since the usage is not the same across adapters, and the manual header check is required for other implementations