bodyParser should check for empty body #415

Closed
donpark opened this Issue Nov 19, 2011 · 20 comments

Projects

None yet

9 participants

@donpark
donpark commented Nov 19, 2011

Sometimes requests are made with Content-Type set but without body.

Connect 1.8.0 throws SyntaxError: Unexpected end of input error when HTTP POST with Content-Type set to 'application/json' but without body.

Master branch seems to be checking for empty body.

Workaround: send empty object ({}) as JSON body.

@tj
Member
tj commented Nov 19, 2011

hm.. im not convinced that shouldn't error, it is after-all invalid JSON, what lib is setting the content-type to json without data? I would consider that an error on their part

@donpark
donpark commented Nov 19, 2011

True but, unless I'm mistaken, Connect used to ignore body-less content-type request and 2.0 looks to be same so I would treat this issue as a possible legacy issue. Just reporting, your call. :-)

@tj
Member
tj commented Nov 19, 2011

might be right, i dont remember, i think we had issues with jquery or backbone sending content-type: application/json for a GET with no body haha, but i cant recall about this. i'll take a closer look when i have a minute

@timoxley

Workaround courtesy of @visionmedia:

  // insert after app.use(express.bodyParser())

  app.use(function(err, req, res, next) {
    if (err.message == 'Unexpected end of input')  {
      next()
    } else {
      next(err)
    }
  })
@georgesnelling

I hit this too. In Connect 1.x bodyParser there is this line:

if ('GET' == req.method || 'HEAD' == req.method) return next();

Not so in connect 2. The problem manifests if you send a GET and also set the content-type to application/json. True this is not technically valid combination, but its also not uncommon in real life. I'm no expert in the finer points of http, but it seems to me that bodyParser should simply skip DELETE requests too. Happy to whip up some tests and a fix if you like, but I don't want to go to the trouble unless you agree that its a bug.

Cheers TJ. Thanks for all your work.

-George

@m9dfukc
m9dfukc commented Sep 22, 2012

Oh ... I finally found somebody with the some problems.

It was driving me insane until I found the source of the problem. I'm using Spine.js as Frontend MVC framework and there it's common to use contenType:"application/json" even for a empty get request :(

@tj
Member
tj commented Sep 24, 2012

@m9dfukc yeah lots of client-side libs seem to do strange things like that, it's probably jquery if spine is using it

@HenrikJoreteg

I think it may be worth ignoring bodies on GETs and HEADs despite the fact that they're technically allowed to have them, just because it's so common for client libraries to do it. See request/request#332 for example.

@tj
Member
tj commented Oct 4, 2012

it shouldn't be common though, that's not correct

@HenrikJoreteg

I agree completely. Another option would be to have an ignoreEmpty option that defaults to false. But, I hear ya. It's the client being silly.

@cliffano
Contributor

How about using the existence of content-length header to determine whether the body exists or not and hence whether anything needs to be JSON.parsed or not?

From RFC 2616 - http://www.ietf.org/rfc/rfc2616.txt
"The presence of an entity body in a request is signaled by the inclusion of a Content-Length header field in the request message headers."

I'm not sure if it's the same for all clients, but my quick test using REST Console on Chrome showed that GET method does not have content-length header *, and by definition that means there is no body to parse.

I can't find any reference on RFC 2616 about ignoring body on certain request methods.

  • while POST, PUT, and DELETE includes content-length header
@tj
Member
tj commented Nov 12, 2012

there's also chunked requests with no content-length

@perki
perki commented Mar 27, 2013

Safari (6.0.2) does not respect RFC 2616...
it sends the header content-length: 0 on DELETE requests

This is not a big deal unless a content-type header is also sent.

In my case I sent a "json content-type header". Normally the body parser would eitheir have considered "no-body" as it has a zero length .. but in fact it

1- tests if there is body .. (utils.js)

utils.hasBody = function(req) {
    return 'transfer-encoding' in req.headers || 'content-length' in req.headers;
};

2- tests if it's json .. (json.js)

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

3- throw an error because it's zero lengthed .. (json.js)

if (0 == buf.length) {
  return next(400, 'invalid json, empty body');
}

-- I propose either that utils.hasBody consider a length of "0" has an empty body or that the json parser consider that a zero length returns a null object, such as JSON.parse(null) does

@tj
Member
tj commented Mar 28, 2013

JSON.parse of null is just a side-effect of the recursion, that's not valid json

@perki
perki commented Mar 28, 2013

yes. but shouldn't an advertised zero lengthed content should result in a "null" body?

@tj
Member
tj commented Mar 28, 2013

I think 400 is appropriate there, it's an invalid request

@perki
perki commented Mar 28, 2013

Ok, my mistake was to include a "content-type" header on a DELETE request.

But Safari (6.0.2) still send content-length: 0 even without content-type headers..
My point was to get connect more robust with a non-valid request request from Safari..

Would getting utils.hasBody return true only if content-length > 0 be a not-so-bad solution?

@mhart
mhart commented Jul 4, 2013

We're seeing these now after upgrading Express - and it's googlebot that's generating them.

   "req": {
     "method": "GET",
     "headers": {
       ...
       "accept": "application/json, text/javascript, */*; q=0.01, */*",
       "content-type": "application/json",
       "x-requested-with": "XMLHttpRequest",
       "from": "googlebot(at)googlebot.com",
       "user-agent": "Mozilla/5.0 (compatible; Googlebot/2.1; +http://www.google.com/bot.html)",
       ...
      }
    }

Very frustrating!

@tj
Member
tj commented Jul 4, 2013

not sure why we had length || type, it should be length && type

@tj
Member
tj commented Jul 4, 2013
@tj tj closed this Jul 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment