JSON empty buffer/body #680

Closed
voortwis opened this Issue Oct 29, 2012 · 6 comments

Projects

None yet

3 participants

@voortwis

In case of a request application/json encoded with an empty body the JSON parser fails. Is it an option to return an undefined body instead of an error? See the additional if (buf.length === 0) in the 'end' handling.

Regards, Klaasjan

  return function json(req, res, next) {
    if (req._body) return next();
    req.body = req.body || {};

    // check Content-Type
    if ('application/json' != utils.mime(req)) return next();

    // flag as parsed
    req._body = true;

    // parse
    limit(req, res, function(err){
      if (err) return next(err);
      var buf = '';
      req.setEncoding('utf8');
      req.on('data', function(chunk){ buf += chunk });
      req.on('end', function(){
        if (buf.length === 0) {
            req.body = {};
            next();
        } else {
            if (strict && '{' != buf[0] && '[' != buf[0]) return next(utils.error(400, 'invalid json'));
            try {
              req.body = JSON.parse(buf, options.reviver);
              next();
            } catch (err){
              err.body = buf;
              err.status = 400;
              next(err);
            }
        }
      });
    });
  }
Member
tj commented Oct 29, 2012

and empty body is not valid json, dont set the content-type

@tj tj closed this Oct 29, 2012

When i debug the standard couchDB jquery plugin (also used in futon) and i access the _all_docs (GET) it will do a GET without data with a content-type 'application/json'. When proxied by nodejs with bodyparsing on it will fail.
Maybe you can reconsider, no data sent is indeed not valid json, it is no data -> undefined or no body at all (my code should then not add req.body = {}. But i think it should not fail either.

Regards, Klaasjan

Member
tj commented Oct 29, 2012

jquery and friends do this all the time, it's an invalid request it should respond with 400

Contributor
evdb commented Nov 29, 2012

Just spent some time debugging this. Would you accept a pull request that changes the error message to be more informative? For example:

invalid json - request content-type is 'application/json' but request body empty

This would provide a bit more information as to why the request is considered invalid.

@evdb evdb added a commit to evdb/connect that referenced this issue Nov 29, 2012
@evdb evdb Specific error message for content-type of JSON but empty body.
related to #680
fbe9e0c
Contributor
evdb commented Nov 30, 2012

OK - this issue is causing us more pain due to several libraries out there not being correct.

In the spirit of the robustness principle may I suggest that the JSON middleware not try to parse the body for the 'safe' HTTP methods - that is to say HEAD, GET, OPTIONS and TRACE - or at least just GET and HEAD.

I think that this would be in the spirit of the spec, as according to http://tech.groups.yahoo.com/group/rest-discuss/message/9962 these methods should not send a body, and if they do the server should ignore it. It would be nice if this was the letter of the spec as well.

This will allow the bad JSON sent to POST, PUT etc to be trapped (as it should be) but ignore bad JSON to GET or HEAD, which should never be used in an app anyway.

Ideally all the requesting libraries should be correct, but that seems unlikely anytime soon. I think that the above is a good pragmatic approach. Happy to code up and test a pull request if you'd like.

@evdb evdb added a commit to mysociety/popit that referenced this issue Nov 30, 2012
@evdb evdb Work around issue with GET requests with json content type and invali…
…d body

Related to senchalabs/connect#680
c19ec84
Member
tj commented Nov 30, 2012

we've already accepted a PR for empty json bodies, just no release yet, it's perfectly valid to use a body with GET, just not behind caches etc, so only really for internal use or databases with http interfaces etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment