Errors from middleware swallowed by promise

[jamesskinner]

The authorisation middleware that is added by this module here looks like this:

function authorize(req, res, next) {
    return Promise.all(...)
    .then(() => next())
    .catch(e => next(e))
  }

This causes errors that are thrown from middleware further down the chain to be swallowed due to this: restify/node-restify/issues/962

Essentially the .then(() => next()) means that the execution carries on inside the promise scope, so any errors then thrown will land in .catch(e => next(e)), but by this time the next function has already been called once and calling it again has no effect.

Resolving this I think means removing the promises or potentially something like:

function authorize(req, res, next) {
    return Promise.all(...)
    .then(() => setImmediate(next))
    .catch(e => next(e))
  }

[mikestead]

@jamesskinner Thanks for digging this one out, good find.

What if we move the exception catching to only target the direct middleware.

function authorize(req, res, next) {
    return Promise.all(...)
    .then(() => next(), e => next(e))
}

I think that was my original intention. There's no reason this middleware should be special in being responsible for capturing errors down the chain in other middleware.

[jamesskinner]

@mikestead I think (and just tested) the problem then is then is that nothing is handling the rejection. In this scenario, (i.e. next() throws an error), the promise that is returned from authorize will be rejected, but nothing is handling this promise so the rejection will go unnoticed.

In a way it would be better as you get the unandledRejection warning

I am actually not sure what the solution here is beyond dropping support for promise based "authorizers" and removing the promises from this function. Looking at this, it does seem there might be a bug there anyway

form https://github.com/mikestead/swagger-routes/blob/master/src/routeSecurity.js#L63:

          try {
            const result = authorizer(req, res, err => err ? reject(err) : resolve())
            if (result && result.catch) result.catch(e => reject(e))
          } catch(e) {
            reject(e)
          }

Looks like there should be a result.then(resolve) line somewhere. If that is correct then technically removing support will not break anything :-)