Engine.io to not set CORS headers on error response

[jdolega]

I have an issue similar to #331 but related to the error situations which are eventually handled by the sendErrorMessage (https://github.com/socketio/engine.io/blob/master/lib/server.js#L236)
I have socket.io configured to allow only certain origins and in case of origin that is not passing validation I can see verify method failing (https://github.com/socketio/engine.io/blob/master/lib/server.js#L159) due to not allowed request. Unfortunately the error response generated by sendErrorMessage is having that unallowed origin set in Access-Control-Allow-Origin header which is
a) a bit confusing as my response about not allowed origin at the same time claims to allow it via Access-Control-Allow-Origin header
b) such responses are picked up by the security scans

Is there any reason sendErrorMessage explicitly sets the CORS headers? Could we add some way to somehow control their presence?

[darrachequesne]

How about adding a new error code, named REQUEST_DENIED (or something like that), which will be used when allowRequest fails and in that case the CORS headers would not be added? Would that make any sense?

[jdolega]

I believe that would require to expose error codes to the outside i.e. the allowRequest function is actually injected via the opts, and hence the the actual error code is set on the callback function, in the caller code (https://github.com/socketio/socket.io/blob/master/lib/index.js#L62).
Other option could be to add separate argument to the callback function

Server.prototype.handleRequest = function (req, res) {
(...)  
  var self = this;
  this.verify(req, false, function (err, success) {
    if (!success) {
      sendErrorMessage(req, res, err);
      return;
    }

or maybe (imho better) add separate callback function for handling 'allowance' to the verify method, which than can be passed to the external allowRequest handler (instead today's generic callback).

[darrachequesne]

You mean, something like the following?

Server.prototype.verify = function (req, upgrade, fn) {
(...)
  } else {
  // handshake is GET only
  if ('GET' !== req.method) return fn(Server.errors.BAD_HANDSHAKE_METHOD, false);
  if (!this.allowRequest) return fn(null, true);
  return this.allowRequest(req, function (err, success) {
    fn(err ? Server.errors.REQUEST_DENIED : null, success);
  });
);

[jdolega]

Yeap. That would do the trick.

[darrachequesne]

Actually, it seems the error code is used in the response (#259)

I'm wondering what the best way to handle that case is. Should every error message triggered by allowRequest be without CORS headers? Should we add yet another flag for that?

Related: #211 & #281