Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Error with connect middleware for GET with contentType #1416

Closed
tonysherbondy opened this Issue Nov 14, 2012 · 22 comments

Comments

Projects
None yet
7 participants

Express 3.0.3
jQuery 1.7.2

Create default app with express. Perform $.ajax({url: '/users', contentType: "application/json; charset=utf-8", dataType: 'json'}). Get the following error:
Error: invalid json
at Object.exports.error (/Users/sherbond/git/temp/express3crash/node_modules/express/node_modules/connect/lib/utils.js:43:13)
at IncomingMessage.module.exports (/Users/sherbond/git/temp/express3crash/node_modules/express/node_modules/connect/lib/middleware/json.js:67:71)
at IncomingMessage.EventEmitter.emit (events.js:93:17)
at IncomingMessage._emitEnd (http.js:366:10)
at HTTPParser.parserOnMessageComplete as onMessageComplete
at Socket.socket.ondata (http.js:1704:22)
at TCP.onread (net.js:403:27)

This was working before express upgrade, last express 2.5.11. Works if you remove contentType, but some servers our front-end connect to need the contentType there for GET requests.

Owner

tj commented Nov 14, 2012

why do they need the content-type? that's not a valid request, this has been brought up lots in the connect issue queue, the fix is simple though just dont set a content-type with no body

@tj tj closed this Nov 14, 2012

tomdale commented Nov 14, 2012

It seems broken to throw an exception on an incoming GET request that contains an empty body, no matter what its content type is set to.

Owner

tj commented Nov 14, 2012

it's broken to send a request as json with no body, that's not valid json

Right, Content-Type identifies the media type of the entity body—which, at least according to Fielding, has no semantic meaning for a GET request (though, interestingly enough, HTTP doesn't disallow them).

But even if that were permitted, an empty body is, indeed, invalid JSON. See the JSON syntactic grammar—the JSONText symbol must contain exactly one JSONValue. RFC 4627 is even more strict—the goal JSON-text symbol must be either an object or an array.

I believe the correct header here is Accept, not Content-Type.

If something has "no meaning", shouldn't the logical behavior be to ignore, not enforce?

Owner

tj commented Nov 14, 2012

it responds with a 400 bad request, seems reasonable to me..

tomdale commented Nov 14, 2012

I'll concede that the empty body is not valid JSON. No argument there.

But if, per Fielding, if the body has no semantic meaning, isn't checking it for validity imbuing it with semantic meaning? Surely any other value for Content-Type would not cause similar behavior (i.e., it would not try to validate HTML.)

Anyway, there are obvious workarounds here, I'm just trying to better understand the correct behavior. It seems like it has/will bite many people and it's pretty easy to fix, Postel's Law, etc.

"A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request."

http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.3

Owner

tj commented Nov 14, 2012

I'm not sold personally, it's equally as easy to just not set the content-type, you could duct tape many problems in this industry but IMO it makes more sense to correct this sort of thing than just ignore it.

But if, per Fielding, if the body has no semantic meaning, isn't checking it for validity imbuing it with semantic meaning? Surely any other value for Content-Type would not cause similar behavior (i.e., it would not try to validate HTML.)

Ah, yes—I hadn't thought of it from that perspective. That makes sense; by validating a body—even if it's not used—you're implying that it has some sort of significance. I know libraries like Formidable will throw an error when given an improperly-formed (urlencoded, in that case) body...but, again, that suggests that the body is meaningful.

"A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request."

Me puts on dunce hat. I assumed that the message-body only applied to responses, completely missing the first sentence of that section—"used to carry the entity-body associated with the request or response."

I'm not sold personally, it's equally as easy to just not set the content-type, you could duct tape many problems in this industry but IMO it makes more sense to correct this sort of thing than just ignore it

Not sure what the right call is here. Like @tomdale, I'm more interested in what the correct behavior is, and why.

Contributor

gmethvin commented Nov 14, 2012

The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.3

jQuery isn't setting the Content-Length or Transfer-Encoding in the request, so I don't see why the server is trying to parse a body.

Owner

tj commented Nov 14, 2012

sure I mean we can argue with RFCs all day, but if jquery does not intent on sending a body, why the hell would you set the content-type? that's just lazy, and to me that signals intent, if you dont intend on sending JSON, dont set the content-type. If we did parse HTML or other data types I would apply similar validation, but we dont handle those types.

jQuery isn't setting the Content-Length or Transfer-Encoding in the request, so I don't see why the server is trying to parse a body.

No, but the browser does. Content-Length, among others, can't be programmatically set with setRequestHeader ("The above headers are controlled by the user agent...").

If we did parse HTML or other data types I would apply similar validation, but we dont handle those types.

Fair enough.

Contributor

gmethvin commented Nov 14, 2012

No, but the browser does.

Right. What I meant was that since no body is being sent, those headers aren't being set by the browser.

Sure, the Content-Type is useless if there is no body, but I'm just saying you shouldn't use it to signal the presence of a body.

Contributor

gmethvin commented Nov 14, 2012

I submitted a pull request (senchalabs/connect#693) which I believe fixes this issue.

We can definitely fix it on our end, and will, but I think this is an issue other people might run into as well.

Sure, the Content-Type is useless if there is no body, but I'm just saying you shouldn't use it to signal the presence of a body.

Oh, I see.

Owner

tj commented Nov 14, 2012

I'll cave, that's not unreasonable I suppose, personally I still find it really silly but it is what it is.

Owner

tj commented Nov 14, 2012

next release will have the patch

svasva commented Nov 15, 2012

is there any workaround for this?

Owner

tj commented Nov 15, 2012

@erundook im sure you can disable it with jquery, or use a less awful xhr lib

Perhaps I'm missing something, but how would one prevent the process from crashing on such requests?
Besides editing /lib/middleware/json.js ...

Owner

tj commented Dec 6, 2012

it doesn't crash, just prints the error (in the default connect error handler), anyway the next release of express will allow for these sort of ill-formed requests

@raitucarp raitucarp referenced this issue in benpickles/js-model Jul 26, 2013

Open

Persistent storage invalid json #33

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