handleUpgrade() and undefined upgrade header

[FranckFreiburger]

ws/lib/websocket-server.js

Line 193 in 2789887

req.headers.upgrade.toLowerCase() !== 'websocket' ||

req.headers.upgrade may be undefined, that will raise TypeError: Cannot read property 'toLowerCase' of undefined

[lpinca]

It can only happen if the req argument is not an upgrade request. wss.handleUpgrade() must be called from the listener of the 'upgrade' event.

httpServer.on('upgrade', function (req, socket, head) {
  // Here `req.headers.upgrade` cannot be `undefined`, otherwise there is a bug in Node.js.
  wss.handleUpgrade(req, socket, head, callback);
});

[lpinca]

Please fill the issue template, it's there for a reason. Also, if you can, please write a test case using only ws to reproduce the issue.

[FranckFreiburger]

Sorry for that, I created the issue from the code view (Reference in new issue), no model was requested.

[FranckFreiburger]

In fact I am using handleUpgrade() in a get() handler of express :

app.get('/myWs', (req, res) => {
  wss.handleUpgrade(req, req.socket, [], handler);
})

Everything seems to work properly except for non-ws requests (that is expected of course)

[lpinca]

Yes, you can't do that, it doesn't make sense. The HTTP parser is still attached to the http.IncomingMessage object (req) in your example.

I would even say that the thrown error is good in that case.

[FranckFreiburger]

Thanks, however, I don't understand properly why it can't work (at least with upgrade requests only)
eg.

app.get('/myWs', (req, res) => {
	if ( req.get('upgrade')?.toLowerCase() !== 'websocket' )
		return void res.status(400).end();
	wss.handleUpgrade(req, req.socket, [], handler);
})

[lpinca]

Because upgrade requests are used to upgrade to another protocol not HTTP and are handled differently by Node.js. When you use app.get() you are handling a request received via

httpServer.on('request', function (req, res) {});

Upgrade requests are handled with

httpServer.on('upgrade', function (req, socket, head) {});

and the socket in this case is freed.

In your example the socket is still piped to the HTTP parser so it might not work if the client writes something using a different protocol. Also there is no reason to keep the HTTP parser and the http.IncomingMessage objects alive for the new protocol.

[FranckFreiburger]

ok I see, thanks !

[lpinca]

I'm closing this as answered. Discussion can continue if needed.