Difficult to determine that an error is the result of bad JSON

[carlbennettnz]

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?

[dougwilson]

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');
  }
});

[carlbennettnz]

True. Still seems messy though.

[dougwilson]

Is there something you could (perhaps in 2.0) to make this a little easier?

Suggestions :) ?

[carlbennettnz]

Off the top of my head:

• Custom Error type
• Add a _isJsonParsingError property or something like that to the SyntaxErrors
• Some sort of error callback passed into .json()

[dougwilson]

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.

[carlbennettnz]

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

True, hadn't thought of that. You could expose it though I guess.

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 :)

_isJsonParsingError is more specific to the problem than body. Less chance of a clash with other middleware.

That wouldn't make any sense

You're right, ignore me :)

Any other suggestions :) ? I don't see anything to act on currently from that list.

It's not a big issue, for reasons you've outlined. A combination of 1 & 2 might be useful though.

[dougwilson]

Probably the best, though without access, you cannot do instanceof against it.
True, hadn't thought of that. You could expose it though I guess.

I have done that in the past, but it never works; the only way it can be guaranteed to function is if you create your body parser and your body error handling in the same file. Otherwise, you are just hoping that require('body-parser').SomeError is actually the same instance between two files (and it is even more unlikely to be the same between two modules, since they can have different installs of body-parser.

The only way for this to be reliable is to add the class to the globals, which is really nasty (of course, JavaScript itself understand that problem, that's why there is a global Symbol registry, for this exact problem, just it only solves it for symbols).

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 :)
_isJsonParsingError is more specific to the problem than body. Less chance of a clash with other middleware.

I will think on this, but i.m.o this is unlikely to happen, mainly because of the changes this module would have to undergo just to differentiate between a general parsing error, a JSON parsing error, a fault in something else (for example, when v8 gets corrupted, which happens from time to time in Node.js 0.12, JSON.parse will return an illegal access error, similar to the issue 0.12 has with decodeURIComponent nodejs/node-v0.x-archive#25551).

A PR of you trying to implement this is probably the only way to get this proposal moving forward, unfortunately (and it needs to work for all the different parsing, not just JSON, and keep it open for when #22 lands).

[carlbennettnz]

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');
  }
});

[factoidforrest]

Came here for this issue. Not only is it not clear that the errors are parse errors, it also is hard to catch them cleanly, as you can see. I would take two steps to correct this:

1.) Improve the default error response to include something like {name: 'parseErr'} or anything that to makes it obvious and easy to handle programmatically. Including a property won't break anyone's existing code but it will make developers lives easier. My frontend devs had no idea what the JSON parse error meant.

2.)Optionally take a custom error handler as part of the options hash when defining the parser. This is becoming standard practice with middlewares. I shouldn't have to wait for your library to write something to the response object and then watch for it and clean it up in a middleware.

[hacker112]

I am also looking for a more readable and clear way of catching body-parser errors. But I guess I will stick to version 1.13.3 for a while longer.

I think that it is difficult to understand that it is body-parsers json error that are caught by looking at the example above.

[dougwilson]

Please feel free to propose solutions via a PR and then the discussion regarding the solution can be had there :)

@hacker112 the change in 1.14.0 was specifically asked for by @carlbennettnz as a proposal to assist this issue (in part). Are you saying that now that JSON parsing only those one type of error instead of two it's harder to handle? The SyntaxError is part of ES5 and is defined to what JSON.parse will thow, so any JSON.parse action in your app would produce these errors due to invalid JSON.

[hacker112]

@dougwilson What I was thinking that in 1.13.3 it was clear that the error was from body-parser even though it was very ugly to look at the message to know that it was an an json body-parser error, at least it was json and body-parser specific.

Now (even though the probability is low) a SyntaxError could be thrown from somewhere else and then presented as "Bad JSON" to the web client even though that was not the case and might have been an error that should have been logged instead.

I think it is good that the SyntaxError is available for those who needs it, but it should be easy know it's origin. Maybe an Exception that would look like this would be a better solution:

  Error({
  bodyParserError: true,
  jsonParseError: true,
  inner: SyntaxError(...)
})

[carlbennettnz]

@hacker112 +1 to that suggestion.

I'm not sure how 1.13 was any better though. In 1.14 you simply get SyntaxErrors consistently instead of a mixture of Errors and SyntaxErrors. What method were you using then that you can't use now?

[hacker112]

@carlbennettnz If that is true, then I am not handling all json errors correctly now. I did a very dirty check on the message. "if (err.message === 'invalid json')" which broke my tests when I tried to upgrade. And now that message has changed.

I can agree that SyntaxError instead of mix of errors are better :)

[dougwilson]

Hi @hacker112 yes, your code would not be handling all json errors correctly, even in 1.13. In 1.13 all JSON parse errors were already SyntaxErrors with a different message; only the strict check gave that message. If you sent an incomplete JSON body in 1.13, you would get a SyntaxError, for example.

[hacker112]

@dougwilson Thanks for the clarification :)

[ReinsBrain]

I'm uncomfortable with body-parser responding to requests (on errors) - I don't think that is its job and it steps in my way because i prefer to make that communication. Having to sniff out a potential error thrown by body-parser in a "catch-all" errors middleware doesn't leave me feeling warm and fuzzy either.

@light24bulbs suggested

2.)Optionally take a custom error handler as part of the options hash when defining the parser.

which would solve my problem but, ultimately you're back to the same problem if the optional "custom error handler" function is not supplied.

Ideally, I think that body-parser would not be middleware but rather, a helper module with asynchronous functions that send back errors in the traditional node way (via the first argument supplied to the callback). You'd only have to supply it the request for it to do its work on parsing the body in whichever context is required by the caller.

@dougwilson - is there any particular reason why body-parser is specifically middleware? would it or could it not function the same as I've suggested?

[dougwilson]

Hi @ReinsBrain, this module is in no way responding to the request on error--we call the given callback with the error and we leave responding to the error completely up to you; if you then leave that up to Express, then that would of course be your choice :) but still this module literally never writes a response, just so we're on the same page there.

Ideally, I think that body-parser would not be middleware but rather, a helper module with asynchronous functions that send back errors in the traditional node way (via the first argument supplied to the callback).

I agree and there is some progress on making independent modules for this. As one example, we made the module raw-body that may be exactly what you are looking for. You can even adopt middleware like this into what you desire, here is a simple example (this is a simple example, to get you started on something):

var bodyParser = require('body-parser');

// ... stuff
var parseJson = bodyPaser.json();

app.use(function (req, res, next) {
    req.getBody = function (callback) {
        parseJson(req, res,function (err) {
          callback(err, req.body);
        });
    };
    next();
});

is there any particular reason why body-parser is specifically middleware? would it or could it not function the same as I've suggested?

Yes, because this is a module, designed to be a middleware for Connect/Express. That's like asking is there any parciular reason a car is designed to be a car. There ahs been work underway to pull apart this module so "power users" can use parts in their own way (the raw-body was an example of this).

[ReinsBrain]

Hi @dougwilson, thanks for clarifying. I have taken up using raw-body just as you've suggested and I'm having success with it. Thanks again :)

[MeLlamoPablo]

Though this issue is old, I wanted to add that adding properties to SyntaxError (the way it's done now) doesn't seem ideal to me because it causes Typescript errors:

export function errorHandler(err, req, res, next) {
    if (err instanceof SyntaxError && err.status === 400) {
        return res.status(400).send(JSON.stringify({
            error: {
                code: "INVALID_JSON",
                message: "The body of your request is not valid JSON."
            }
        }))
    }

    console.error(err);
    res.status(500).send();
}

tsc output:

src/middleware/errorHandler.ts(2,43): error TS2339: Property 'status' does not exist on type 'SyntaxError'.

The obvious way to solve it would be the proposed custom error type, but then there's the exposure problem. Imo the best solution would be prefixing the message with body-parser: , so that you can do err.message.includes("body-parser: "). If the chances of getting a SyntaxError from a source other than body-parser were low, the chances of it also containing body-parser are negligible.

[dougwilson]

Sure, but a 400 can occur from our dependency raw-body. Should that prepend body-parser: ? Should it append raw-body: and so on, requiring you to add a check for the name of every dependency of this module? Also, since this message by default is getting exposed by Express, that would bring up the question of if too much information about your server is getting exposed, or that people are already matching on the message today (Node.js core has the policy that error message changes are semver major).

Node.js attaches the .code property to it's error objects (which are just plain Error). How does Typescript work with that at all?

[MeLlamoPablo]

Also, since this message by default is getting exposed by Express, that would bring up the question of if too much information about your server is getting exposed

I really hadn't thought of that.

Node.js attaches the .code property to it's error objects (which are just plain Error). How does Typescript work with that at all?

Checking for .code throws a similar error:

error TS2339: Property 'code' does not exist on type 'SyntaxError'.

Maybe the best solution is just ignoring the error; it's probably not worth the trouble. Dealing with typings would be a pain, since SyntaxError is not body-parser's type. But if we wanted an elegant solution maybe we could use polymorphism:

class JSONParseError extends SyntaxError {
    status: number;
}

if (err as JSONParseError instanceof SyntaxError && err.status === 400) // No error

JSONParseErrorwouldn't need to be exposed because err instanceof SyntaxError is true for JSONParseError, and typescript would stop complaining. You could also make sure that the error came from body parser by doing err.constructor.name === "JSONParseError". Would this also be semver major?

Maybe all of this is not worth the trouble, but there's an idea.

[OliverJAsh]

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);
  }
})

[dougwilson]

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

[mendeljacks]

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 })
    }
  }
});

[connorads]

This should shut up the typescript :)

Another way of pleasing TypeScript:

Create your own error type

export interface ExpressError extends Error {
  statusCode?: number;
}

And include it on your error handler

export const handleErrors = (
  err: ExpressError ,
  req: Request,
  res: Response,
  next: NextFunction
) => {
  const { statusCode, message } = err;
  if (statusCode && statusCode >= 400 && statusCode < 500) {
    console.log("Client error", { message });
    res.status(statusCode).json({
      error: message,
    });
  } else {
    console.error("Unhandled error", { statusCode, message, stack: err.stack });
    res.status(500).send({ error: "Internal server error" });
  }
};

Interestingly it seems that Express appears to only throw errors with a statusCode of 400 and 406