Make sendErrorMessage response available on server

[mrazf]

Note: for support questions, please use one of these channels: stackoverflow or slack

You want to:

• [] report a bug
• request a feature

Current behaviour

Apologies if this is my misunderstanding of NodeJS streams.

The HTTP body of sendErrorMessage is readable on the client but does not appear to be accessible server side.

I'm currently investigating a number of 400 errors produced here but it looks like this could be 1 of 4 Server.errorMessages.

Are the response bodies available server side or would it be possible to add some debug logging that includes the body before sending the response?

Steps to reproduce (if the current behaviour is a bug)

Expected behaviour

Setup

• OS:
• browser:
• engine.io version:

Other information (e.g. stacktraces, related issues, suggestions how to fix)

[Khez]

We've noticed a very similar issue in production as well and came to the same conclusions.

We believe the root cause is a faulty proxy between a client and our socket.io implementation. But unfortunately we can't confirm it.

Flow

Delving through the logs, we found the same code in engine.io v3.x being the culprit:

• During server.attach
• A handler for server.on('request') is added
• Which handles the request directly
• Calls sendErrorMessage if it fails

Problem

This entire scenario is completely hidden from the underlying server.

Checking the code it seems there are a number of different errors that can cause this:

• An unknown transport
• An invalid origin
• An invalid SID
• An unexpected transport without upgrade
• An invalid HTTP Method
• Transport handshake error
• If using socket.io, there's also:
  • a default origin check
  • a userland origin check

So together there are 8 separate problems that lead to either 400 or 403 status code, the only difference being the json output, which itself has 5 distinct results

Solution

Since we're already using emit for connection, we could add an emit for these scenarios as well.

If there's no problem with a similar solution I can help with a PR (maybe on both 3.x and 4.x branches)

[darrachequesne]

@Khez Thanks for the detailed explanation. Do you have a suggestion for the API?

Related: socketio/socket.io#3819

[Khez]

It feels like emitting an event is the way to go, as it was proposed socketio/socket.io#3819 as well :)

To be honest, it kind of depends on what people need. Is it a hook that can change things or just a means to logging ?

@darrachequesne I'm going to try to make a PR over the weekend on the 3.4.x branch then we could port it to 3.5.x and master. It will give me more insight into how to properly tackle this issue. Unless you believe in a different approach.

Hooking into the error page

There might be a need to hook into and possibly even change the error page.

It doesn't seem that engine.io-client looks at what the underlying content of the error page is.
Meaning changes to the page should be backwards compatible with the client implementation.

I'd vote for this to be a separate issue / PR, since probably many users don't even know this scenario occurs.

Note on error pages

It seems there are two separate ways of causing an error page and they are different:

• abortConnection with a default text/html error reason
• sendErrorMessage with a default application/json error

Feels like it should always be application/json. Uncertain why it is not.

Hooking into the error codes

It seems @andrejleitner, @mrazf and myself just need a way to log this scenario.

With an update to make error codes unique, we could simply emit from sendErrorMessage:

• socket.io can then hook into this in its bind handler
• Emit the event from socket.io so we can do io.on('connection_dropped', (reason) => console.log(reason))
  • This similar to the already available io.on('connection', (socket) => {})

Path of least resistance

We already have an error list, some of which are unique with regard to the underlying issue.

We'd need to cover non-unique codes that are currently BAD_REQUEST:

• INVALID_ORIGIN
• BAD_TRANSPORT / TRANSPORT_WITHOUT_UPGRADE
• TRANSPORT_HANDSHAKE_ERROR
  • Probably the most beneficial PR would contain a way of accessing the underlying error

With a change to the error codes, we could simply emit from sendErrorMessage:

• Server.errorMessages[code] would represent all engine.io scenarios
• Server.errors.FORBIDDEN remains the result of allowRequest

This method would forces us to "skip" Server.errorMessages[5] which is used in branch 4.x by UNSUPPORTED_PROTOCOL_VERSION
Still wondering if a different parameter or a mild refactor of sendErrorMessage would be more beneficial.

[Khez]

@darrachequesne any chance you can take a look through the PR so we can move this issue forward ?

I'm open to any and all suggestions :)

[darrachequesne]

Closed by 7096e98, included in [email protected].

Syntax:

server.on("connection_error", (err) => {
  console.log(err.req);		// the request object
  console.log(err.code);	// the error code, for example 1
  console.log(err.message);	// the error message, for example "Session ID unknown"
  console.log(err.context);     // some additional error context
});