Crash with fallback GET route

[corruptmem]

If you run the following test app:

var express = require('express');
var app = express();

require('express-ws')(app);

app.ws('/test', function(ws, req) {
  console.log('incoming connection');
});

app.get('/*', function(req, res, next){
  res.sendFile(__dirname + '/test.html');
});

app.listen(3000);

When you make a WebSocket request to ws://localhost:3000/invalidroute, the process crashes with the following exception:

http_outgoing.js:135
      this.outputSize += this._header.length;
                                     ^

TypeError: Cannot read property 'length' of null
    at ServerResponse.OutgoingMessage._send (_http_outgoing.js:135:38)
    at ServerResponse.OutgoingMessage.write (_http_outgoing.js:477:16)
    at ReadStream.ondata (_stream_readable.js:536:20)
    at emitOne (events.js:90:13)
    at ReadStream.emit (events.js:182:7)
    at readableAddChunk (_stream_readable.js:153:18)
    at ReadStream.Readable.push (_stream_readable.js:111:10)
    at onread (fs.js:1818:12)
    at FSReqWrap.wrapper [as oncomplete] (fs.js:614:17)

[joepie91]

I'll have to look into this further to confirm, but I'm fairly sure that the issue is caused by the 'fake' GET route that's created for each .ws endpoint. If so, this might be a tricky one to fix...

[joepie91]

Another case from #46:

• app.get("*", (req, res, next) => { /* ... */ })

Two other cases from #65:

• app.use((req, res, next) => { /* ... */ })
• app.use("/", (req, res, next) => { /* ... */ })

And the culprit here would probably be:

• app.get("/*", (req, res, next) => { /* ... */ })

In other words, the culprit seems to be with with any kind of wildcard-y route or middleware that matches the fake GET route created by express-ws.

[xogeny]

Maybe this is a dumb question, but I still don't get why the /.websocket is necessary. It says here it is so that it can be distinguished from a normal GET request. But doesn't the protocol (i.e., ws or wss) in the URL distinguish it already?

[joepie91]

Express has no awareness of "protocol" - all it sees is HTTP requests and responses, with methods. Technically speaking, WebSockets aren't an independent protocol anyway; a WebSocket connection is established using an upgrade request, which is the same as a regular HTTP request from a protocol perspective.

Express also has no concept of WebSockets - it expects every HTTP request to be followed up by a HTTP response, after which the connection is closed. So what express-ws does is essentially creating a 'fake' GET route, and sending the upgrade request + a dummy response object through it. This makes Express think it's a regular HTTP request, executing all the applicable middleware, and so on.

However, you can't just create a regular GET route at the same path as every WebSocket path - if an application does the following:

router.get("/foo", (req, res) => {
    // ...
});

router.ws("/foo", (ws, req) => {
    // ...
});

... then you want to treat these two routes as entirely separate routes with entirely separate behaviour. If express-ws did the naive thing and created a fake router.get("/foo") for the router.ws("/foo"), this would conflict with the existing /foo route, and break things as a result.

That's why express-ws instead suffixes the path with .websocket. It's highly unlikely that somebody would manually specify a regular route that has the same path as a .ws route and ends in .websocket, so this prevents conflicts.

[xogeny]

Interesting. I was fully aware that websockets were just upgraded HTTP requests. But I didn't realize that there were no artifacts in the request that pointed to a websocket request.

I also understand the fact that you can't really have two handlers for the same HTTP request absent some way within the request itself to disambiguate them (what I thought the protocol would do, but apparently not). So in that sense, I understand the need now for the fake GET handler. However, I think the choice of appending .websocket clearly isn't the best solution. You say nobody would have a normal route that ended in .websocket. But, of course, we've seen in practice that it is entirely reasonable to have routes that end in *, so that is a bit problem.

I'm curious, wouldn't it be better to prefix the requests? It is seems quite easy for the developer of the application to carve out some part of the hierarchy for web socket handlers. In fact, a simple solution of prepending .websocket to the route has the same "unlikely to collide" with another route possibility (with the exception of /*, but that is easily worked around with a little regexp magic) but has the benefit over appending that you actually could handle *, i.e.,

/foo/* -> /.websocket/foo/* (no problem)

This rule about building the fake handlers is encapsulated in the library. Wouldn't it be simple to switch to this alternative strategy? I could even imagine allowing users to override this by providing their own transformation function that takes the original path as an input and returns the fake path.

Just some ideas.

[joepie91]

Prefixing was how it was implemented previously, but this breaks when using routers (see also this comment). Routers will 'chomp off' a part of the path when mounted as a prefix, which means that the .websocket/ prefix wouldn't be at the start of the path, but rather just at the start of the path within the router.

This is a problem because the 'fake' GET request is created in the server connection handler (here), and that code has no knowledge of which WebSocket routes are defined on what routers mounted at which paths, so it will just prefix the entire path with ./websocket.

This means that if you do the following:

const express = require("express");

let app = express();
let router = express.Router();

router.ws("/bar", (ws, req) => {
    // ...
});

app.use("/foo", router);
app.listen(3000);

... then when a WebSocket request for /foo/bar comes in, there's a disconnect between what the .ws route thinks the URL is, and what the server connection handler thinks the URL is:

• The .ws route thinks the fake URL is /foo/.websocket/bar
• The connection handler thinks the fake URL is /.websocket/foo/bar

That's why the .websocket bit is suffixed instead. No matter how many mountpoints you add into the mix, the fake URLs always match up:

• The .ws route thinks the fake URL is /foo/bar/.websocket
• The connection handler thinks the fake URL is /foo/bar/.websocket

[xogeny]

Actually, I was just reading RFC 6455. Why can't you simply differentiate the requests based on the presence of the header Sec-WebSocket-Key? If that is there, this is a websocket requests. If not, it is normal GET request. It seems like then you could avoid the fake handler and just implement this as middleware with that logic. I'm guessing you thought of that too. But I figured it is worth asking even if only to help me understand better.

[joepie91]

Because this would require changes to Express core, since it has no awareness of WebSockets whatsoever.

The reason it works how it does now, is so that express-ws doesn't have to replicate the middleware implementation; it can simply pass off the req to Express and let it worry about executing middleware correctly, as if it were a normal request (which is critical for integration with eg. express-session).

The problem isn't that express-ws can't detect WebSocket requests, it's that Express itself won't care about them, and wouldn't distinguish them from regular GET requests to the same route. So the fake route is used as a hack to isolate the WebSocket-specific logic while still taking advantage of the rest of Express' mechanisms.

[suhasdeshpande]

Is there any workaround on this?

[ryanph]

@suhasdeshpande just make sure any wildcard routes you have defined do not match the 'fake' route.

[select]

@ryanph how can I do this if I want to match all routes and render a default page for them?

[select]

Ok I solved it

the * route (everything except /websocket)

router.get(/^.(?!websocket).*$/, (req, res) => {
	res.render('angular/index', { environment: app.get('env') });
});

move the websocket to /websocket

app.ws('/websocket', (ws, req) => {
    ...
});
webSocketServer = expressWs.getWss('/websocket');

[dougwilson]

The root cause of the error is that the module is incorrectly overwriting the Node.js core method res.writeHead here: https://github.com/HenningM/express-ws/blob/master/src/index.js#L60-L65

The problem is that Node.js core is expecting that the call to res.writeHead sets a string to res._header, but the express-ws module does not do that. Ideally it should perform the same functions has the res.writeHead it is replacing or monkey patch more methods in Node.js core to achieve whatever it is trying to achieve (like perhaps it should be mokey-patching the res._send method instead of res.writeHead if it wants to block writes?).

I hope this helps. I'll funnel over the Express users who are encountering this issue to this thread.

[fyodor123]

Any progress?

[nicolasiensen]

I'm working on a project that has a middleware to catch all paths that don't match any route and return an HTML file.

To overcome the problem described in this issue, we added a check in the middleware to skip its action if the request contains the WebSocket header:

const frontendMiddleware = (req, res) => {
  if (req.header('sec-websocket-version') === undefined) {
    res.sendFile(path.join(FRONTEND_ROOT, 'build', 'index.html'))
  }
}

app.use(frontendMiddleware)