body-parser: Difficult to determine that an error is the result of bad JSON

Having SyntaxErrors consistently returned (#119) and a status code provided means we can fairly reliably detect errors in express middleware that are the result of bad JSON.

app.use(function(err, req, res, next) {
  if (err instanceof SyntaxError && err.status === 400) {
    console.error('Bad JSON');
  }
});

This test could theoretically give false positives though, if some other middleware were to throw something similar. Imagine we’re using body-parser first and then something else to validate the body has the structure we’re expecting, for example.

Is there any way to detect JSON parsing error with 100% reliability with the library as it is now? Is there something you could (perhaps in 2.0) to make this a little easier?

About this issue

  • Original URL
  • State: closed
  • Created 9 years ago
  • Reactions: 6
  • Comments: 26 (9 by maintainers)

Commits related to this issue

Most upvoted comments

All errors include a type property from 1.18.0 release onwards. For parse failure, err.type === 'entity.parse.failed'.

That’s fine, I trust your judgement 😃

I’ll stick with

app.use(function(err, req, res, next) {
  if (err instanceof SyntaxError && err.status === 400 && 'body' in err) {
    console.error('Bad JSON');
  }
});

Just adding another option here: instead of using the JSON body parser, you can parse the body as text:

app.use(bodyParser.text({ type: 'application/json' }));

And then parse the text as JSON in your request handlers. Then you can handle errors however you like:

app.get('/foo', (req, res) => {
  try {
    const parsed = JSON.parse(req.body);
  } catch (error) {
    res.status(400).send(error.message);
  }
})

Custom Error type

Probably the best, though without access, you cannot do instanceof against it.

Add a _isJsonParsingError property or something like that to the SyntaxErrors

We already do this, we just call the property body right now, and you didn’t like that above, so weird to propose it anyway 😃

Some sort of error callback passed into .json()

That wouldn’t make any sense, because it’s not how Express middleware works. You can easily wrap the body parsing middleware if you really want to know the error came from this middleware, and is the recommend way to determine error source:

app.use(errorFork(bodyParser.json(), function (err, req, res, next) {
  // do stuff with only body parser errors
}))

// this is an example; you can use any pattern you like.
function errorFork(middleware, errorHandler) {
  middleware(req, res, function (err) {
    if (err) return errorHandler(err, req, res, next)
    return next()
  })
}

Any other suggestions 😃 ? I don’t see anything to act on currently from that list.

True. Still seems messy though.

Is there any way to detect JSON parsing error with 100% reliability with the library as it is now?

AFAIK:

app.use(function(err, req, res, next) {
  if (err instanceof SyntaxError && err.status === 400 && 'body' in err) {
    console.error('Bad JSON');
  }
});

This should shut up the typescript 😃

app.use((err, req, res, next) => {
  if (err instanceof SyntaxError) {
    const error = {status: undefined, message: undefined, type: undefined, ...err}
    if (error.status === 400 && 'body' in err) {
      res.status(400).json({ error: "bad json", message: error.message, type: error.type })
    }
  }
});