Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add handling for invalid responses to Response.from_io #5630

Merged

Conversation

straight-shoota
Copy link
Member

Partly fixes #5621

@asterite
Copy link
Member

Careful with making HTTP more strict than it should.

With this, does HTTP::Client.get("https://neuralegionsite.wixsite.com/siteBackHtml/\n\n5") works? Because curl that URL does work.

Servers are not well implemented and browsers still accept them. The http server should tolerate these small errors and act accordingly without failing fast.

@straight-shoota
Copy link
Member Author

HTTP::Client.get("https://neuralegionsite.wixsite.com/siteBackHtml/\n\n5") doesn't work but that's because of the malformed URL. This needs another fix.

This PR just adds some sanity checks to make sure the io actually contains an HTTP response. This is very basic.
It should fail if the first line is not an HTTP status line or the HTTP version is not supported (can't handle something you don't even know about). Obviously, a status code must be a valid integer (otherwise to_i raises an ArgumentError). One could argue about restricting the range of the status code but I don't see any reason not to.

While HTTP:Client is solely an HTTP client, curl supports a number of protocols and applies some quirks if it encounters an error. If an HTTP request receives an invalid response, curl falls back to interpreting the response's body directly (thus eliminating the HTTP protocol).

I dont think a dedicated HTTP client API should have such quirky behaviour. At least not as default.
Browsers usually don't accept an invalid HTTP response either. Chrome and Edge fail with an appropriate error and Firefox displays the response as raw text.

@bararchy
Copy link
Contributor

@straight-shoota This looks great and will benefit me greatly when merged.
@sdogruyol @RX14 @asterite can one of you approve for merge?

@asterite
Copy link
Member

Before merging this, I'd like someone to test go's http client and see if it applies the same quirky things as curl. I'm pretty sure it does, and that this PR should not be merged. The internet is a jungle and not all servers are well implemented and http client must be super tolerant and flexible about this.

@straight-shoota
Copy link
Member Author

straight-shoota commented Apr 30, 2018

@asterite I haven't actually verified everything through running code, but from reading Go
s ReadResponse function, it validates the first line read as a HTTP response:

  • it consists of protocol and status, separated by one ore more spaces.
  • status contains a status code and optional message, separated by space
  • status code is a three digit integer.
  • protocol is in the format HTTP/\d+\.\d+ (just using regex grammar for illustration).

This PR ends up doing the same except enforcing protocol to be HTTP/0.9, HTTP/1.0 or HTTP/1.1. In my opinion this makes sense, because that are the only HTTP protocol versions currently supported by Crystal's HTTP implementation.

@ysbaddaden
Copy link
Contributor

Just for fun: we don't support HTTP 0.9! There are no headers, no status line in responses (only the body), no keep-alive, and it's not even supposed to support binary contents.

  • requests are "a line of ASCII characters terminated by a CR LF (...) pair. This request consists of the word 'GET', a space, the document address."
  • responses are "a message in hypertext mark-up language (HTML)."

See https://www.w3.org/Protocols/HTTP/AsImplemented.html

@asterite
Copy link
Member

So I guess we can merge this, except dropping support for HTTP 0.9 (or just checking that it's a digit-dot-digit).

@straight-shoota
Copy link
Member Author

@ysbaddaden Hm, I don't know why I was under the impression it was supported, but clearly it is not. It should be removed from supported versions then.

@straight-shoota
Copy link
Member Author

I removed HTTP/0.9 from supported versions.

@sdogruyol sdogruyol merged commit 9ddaf28 into crystal-lang:master Jun 6, 2018
@straight-shoota straight-shoota deleted the jm/fix/response-from-io branch June 6, 2018 13:16
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018
@RX14 RX14 added this to the 0.25.0 milestone Jun 7, 2018
@RX14 RX14 added the kind:bug label Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP::Client raise OOB error for certain responses
7 participants