search

seanmonstar / httparse

A push parser for the HTTP 1.x protocol in Rust.
https://docs.rs/httparse
Apache License 2.0
585 stars 114 forks source link

Failure to parse 200 status code not followed by SP #29

Closed donniebishop closed 7 years ago

donniebishop commented 7 years ago

I was trying to use hyper and reqwest on a project yesterday to communicate to a server internal to my workplace. When trying to create a client using either crate, it returns an HTTP(Status) error, stating "Invalid Status provided".

Here's the strace of the executable, changed only to remove private server details from the request and response headers:

sendto(3, "GET /{OMITTED} HTTP/1.1\r\nHost: {OMITTED}\r\nAccept: */*\r\nUser-Agent: reqwest/0.1.0\r\n\r\n", 103, 0, NULL, 0) = 103
read(3, "HTTP/1.1 200\r\nServer: nginx/1.4.6 (Ubuntu)\r\nDate: Fri, 02 Dec 2016 21:18:20 GMT\r\nContent-Type: text/html\r\nTransfer-Encoding: chunked\r\nConnection: keep-alive\r\nContent-Language: en\r\n\r\n1fba\r\n<html>\n<head>

{Output Truncated}

write(1, "Failed because: Invalid Status provided\n", 40Failed because: Invalid Status provided
) = 40
+++ exited with 0 +++

After discussing this in the rust IRC, we've suspect that httparse might be failing on the fact that the custom webserver is returning "HTTP/1.1 200\r\n", with no SP or Reason Phrase following the status code before the CRLF.

CCing @joshtriplett on this, as well, since he helped me find the issue

joshtriplett commented 7 years ago

Note that curl's HTTP parsing accepts this response. While such a response voilates the HTTP spec, curl seems to tolerate it, and I would suggest that httparse should as well.

seanmonstar commented 7 years ago

Considering the reason phrase is deprecated in HTTP2, this seems like a reasonable change.

On Sat, Dec 3, 2016, 1:25 PM Josh Triplett notifications@github.com wrote:

Note that curl's HTTP parsing accepts this response. While such a response voilates the HTTP spec, curl seems to tolerate it, and I would suggest that httparse should as well.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/seanmonstar/httparse/issues/29#issuecomment-264666513, or mute the thread https://github.com/notifications/unsubscribe-auth/AADJF-obf31KgJZEW_B7mnJlVHNnvdVjks5rEd5BgaJpZM4LDZri .

seanmonstar commented 7 years ago

This has been included in the newly published v1.2.1.

donniebishop commented 7 years ago

Great turnaround for this Sean! Thanks!

Will this functionality be ported downstream into hyper and subsequently reqwest?

seanmonstar commented 7 years ago

Both of those depend on essentially 1.x of httparse, so running a cargo update in your project should bring it up to date.

donniebishop commented 7 years ago

Awesome, will do. Thank you again for your help!

On Sun, Dec 4, 2016, 2:09 PM Sean McArthur notifications@github.com wrote:

Both of those depend on essentially 1.x of httparse, so running a cargo update in your project should bring it up to date.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/seanmonstar/httparse/issues/29#issuecomment-264723672, or mute the thread https://github.com/notifications/unsubscribe-auth/AHYkbByS5ajjfqJ9a0sYnOXSds4AaYZ3ks5rEw_ogaJpZM4LDZri .