pdmlab / express-http-problem-details Goto Github PK

I just bumped into a problem that when a mapper is not registered, the handler will break and express dies.

The problem is scattered across multiple places. In this end, the mapping strategy is inhonest about the return value from map. The typing says that it will be a ProblemDocument yet an undefined is also possible.

https://github.com/PDMLab/express-http-problem-details/blob/master/src/ExpressMappingStrategy.ts#L16

I suggest strict: true in tsconfig.json to prevent such subtle errors.

Second, it actually falls to the registry that it does not find a mapper even when the default mapper is used.

I propose to always return the default mapper when a more specific cannot be found. I think it could also be possible to walk up the prototype chain to sort of emulate polymorphism.

I'm not sure how specific the ExpressMappingStrategy is to express actually?

I think right now that maybe it belongs to the http-problem-details-mapper package.

I'm curious if decision to use .send instead of .json was intentional for some reason:

The issue being, Expresses Response.json use app settings for the replacer, escape, and spaces settings of JSON.stringify and stringifying the payload directly bypasses these options.

If it doesn't make a difference to the functionality of this lib, I'd be happy to open a PR.

Requires PDMLab/http-problem-details-mapper#3

By the way, I wanted to join your Slack but the invitation link expired.

That said, maybe it's more hassle-free to set up a gitter community? It is more readily accessible to GitHub users than yet another Slack channel.

There are cases where I would like to set additional headers on a problem details response. For example:

• After authentication failure, we should set the WWW-Authenticate header (https://datatracker.ietf.org/doc/html/rfc6750#section-3)
• For rate-limiting, it would be nice to set the Retry-After header (which is mentioned at https://www.rfc-editor.org/rfc/rfc7807#section-4) and also X-RateLimit-Remaining, etc.

For now this is my workaround, which relies on the errors thrown or passed to next having a valid headers property, and feels a bit hacky:

app.use((err, req, res, next) => {
  if (err.headers && (typeof err.headers === 'object')) {
    for (let [key, val] of Object.entries(err.headers)) {
      res.header(key, val);
    }
  }
  next(err);
});

app.use(HttpProblemResponse({strategy}));

I think it would be better if we could somehow explicitly include response headers when mapping specific error types, such that those headers are then set on the response sent by HttpProblemDetailsMiddleware().

Current

Express has a built-in way for returning errors from any middleware with the next(error) call. It appears that errors returned this way do not get converted into problem+json because the handler is an ordinary middleware callback.

Desired

Any error returned in the standard way should be intercepted and converted into a mapped problem document.

Proposal

I think that following best practices of express, the 4-parameter error callback could be used instead. Thus the code can actually be simplified. Here's my implementation:

import { IMappingStrategy } from 'http-problem-details-mapper'

class HttpProblemResponseOptions {
  public strategy: IMappingStrategy;
}

export function HttpProblemResponse (options: HttpProblemResponseOptions) {
  const { strategy } = options

  return (err, req, res, next) => {
    let problem = strategy.map(err)

    res.statusCode = problem.status
    res.setHeader('Content-Type', 'application/problem+json')
    res.send(JSON.stringify(problem))
  }
}

The only difference is that it must be the last app.use call when setting up express

const server = express()
-server.use(HttpProblemResponse({strategy}))

server.get('/', async (req: express.Request, res: express.Response): Promise<any> => {
  return res.send(new NotFoundError({type: 'customer', id: '123' }))
})
+server.use(HttpProblemResponse({strategy}))