redstone-dart / redstone

A metadata driven microframework for Dart.
http://redstone-dart.github.io/redstone
MIT License
342 stars 42 forks source link

http_body_parser tries to parse body of GET Requests #131

Open davidmehren opened 9 years ago

davidmehren commented 9 years ago

I just found out that http_body_parser tries to parse the body of every request if the header 'Content-Type' is set to 'application/json'. If the request is GET this obviously fails because GET requests dont have a body.

Stack trace of the error: Failed to execute .getDays - FormatException: Unexpected end of input (at character 1)

^

dart:convert JsonCodec.decode package:redstone/src/http_body_parser.dart 101:60 _parseRequestBody.

Pacane commented 8 years ago

Does anyone know what the standard is for this? I know from that a GET request can actually have a body.

A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests. 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.

Although, nowhere in the GET specification do I see anything saying that it's forbidden to send an entity-body in such request. [As opposed to OPTIONS request where it's explicitly written.]

I wonder what others think about this. I feel something's wrong with ignoring the request, but I also feel like it's not so good to throw an error (for instance, Google throws a BAD REQUEST error when this happens.

Any thoughts? @kaendfinger @cgarciae

TheBosZ commented 8 years ago

:+1: Just ran into this problem myself.

I have an Angular2 (Ionic2) app talking to my server and when I try to http.get('server/users') my auth interceptor throws up.

According to the Angular2 docs get doesn't even have an option for a body.

Pacane commented 8 years ago

Can you show the code you use to initiate the query? It is likely to be invalid to provide a body for a GET.

TheBosZ commented 8 years ago
var headers = AuthInterceptor.generateHeaders(url, null, 'GET', contentType, 'bob', 'lskfjwefweljfk');
var result = this.http.get(url, {
    headers: headers
});

Notice that there's only 2 parameters for get: url and options.

Pacane commented 8 years ago

so this is JS right?

TheBosZ commented 8 years ago

That particular bit of code is Typescript, but a Javascript version would look exactly the same.

To me, it seems like a simple fix would be to change http_body_impl to check the body before trying to decode it. Any reason why that wouldn't work?

Pacane commented 8 years ago

No that would be fine I suppose. I just didn't know if we should throw an error or just ignore the body on a GET. There doesn't seem to be a standard for this AFAIK. If you know anything about such standard, let me know.

Pacane commented 8 years ago

Anyway, if you want to contribute it, I'd be glad to merge it, otherwise I'll add this to my todo list. :)

TheBosZ commented 8 years ago

Well, this response on that Stack Overflow post has this bit:

So, yes, you can send a body with GET, and no, it is never useful to do so.

And reading the comments and other thoughts, it sounds like the best idea is parse it if it's there, but otherwise ignore it.

Webstorm is being stupid about letting me edit imported libraries, but I'll try my idea and see if I like the results.

Pacane commented 8 years ago

So "parse it if it's there", and do what with it?

TheBosZ commented 8 years ago

Now that I've had a chance to look at what the code actually does, I don't really see an easy way to do what I was thinking. I was thinking he had more control in how it parses, but now I see that it's handled outside of this library.

Playing around with it, what if we did something like this in request_parser.dart, around line 37, essentially ignoring a body if it's not expected:

if (_body == null && validBodyMethods.contains(httpRequest.method)) {

Pacane commented 8 years ago

I think this could be something doable yes. I still wonder if we should parse it or not. I think we should just ignore the body when it's a GET.

TheBosZ commented 8 years ago

I agree. From the SO post, that seems to be an acceptable solution.

Pacane commented 8 years ago

@TheBosZ Fix deployed in the wild with v0.6.5. Thanks again!

fwgreen commented 8 years ago

This issue persists if you use Postman and don't explicitly disable application/json on the request body.