tmm1 / http_parser.rb

simple callback-based HTTP request/response parser
MIT License
200 stars 43 forks source link

"post identity body world" fails with http-parser >= 2.9.3 #68

Open cbiedl opened 3 years ago

cbiedl commented 3 years ago

Little surprised nobody has reported this earlier ...

Re-building http_parser.rb with the latest version of http-parser (2.9.4) I noticed the "post identity body world" check fails:

  1) HTTP::Parser should parse request: post identity body world
     Failure/Error: @parser << test['raw']

     HTTP::Parser::Error:
       Could not parse data entirely (116 != 122)
     # ./spec/parser_spec.rb:317:in `<<'
     # ./spec/parser_spec.rb:317:in `block (4 levels) in <top (required)>'

After a lengthy research I think the test is indeed flawed, i.e. in violation of RFC 7320 3.3.1. ("Transfer-Encoding").

The check sets Transfer-Encoding: identity and also Content-Length: 5

About the first, the RFC states:

   If any transfer coding
   other than chunked is applied to a request payload body, the sender
   MUST apply chunked as the final transfer coding to ensure that the
   message is properly framed.

...so this is not acceptable.

According to 3.3.3. ("Message Body Length"), combining Transfer-Encoding: and Content-Length: indicate "request smuggling" which "ought to be handled as an error" - which is what the http-parser library now does: It implemented a stricter check in commit https://github.com/nodejs/http-parser/commit/7d5c99d09f6743b055d53fc3f642746d9801479b.

Reproducer (could possibly be shorter):

require "http/parser"

parser = Http::Parser.new

parser.on_headers_complete = proc do
  p parser.http_version

  p parser.http_method # for requests
  p parser.request_url

  p parser.status_code # for responses

  p parser.headers
end

parser <<"POST /post_identity_body_world?q=search#hey HTTP/1.1\r\nAccept: */*\r\nContent-Length: 5\r\nTransfer-Encoding: identity\r\n\r\nWorld"

Solution: Please rewrite or disable that test.

ashie commented 3 years ago

Solution: Please rewrite or disable that test.

I confirmed it too. I think the current implementation refers old RFC rfc2616 because

ashie commented 3 years ago

So we should increment feature version (0.x.0) of this gem when we upgrade to http-parser 2.9.3 or later.

BTW because http-parser isn't maintained anymore, I'll also not actively maintain this after resolving major remaining issues.

From https://github.com/nodejs/http-parser

http-parser is not actively maintained. New projects and projects looking to migrate should consider llhttp.