Bug #5022

WEBrick returns improper response for malformed HTTP Request

Added by Felix Jodoin almost 3 years ago. Updated over 2 years ago.

[ruby-core:38040]
Status:Closed
Priority:Normal
Assignee:Aaron Patterson
Category:lib
Target version:1.9.3
ruby -v:ruby 1.9.2p180 (2011-02-18 revision 30909) [x86_64-darwin10.7.0] Backport:

Description

=begin

When sending an improper HTTP request in the form of:

GET /\n

(with any valid or invalid HTTP verb), WEBrick returns:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN">

Internal Server Error

Internal Server Error
undefined method `each' for nil:NilClass


WEBrick/1.3.1 (Ruby/1.9.2/2011-02-18) at
localhost:3000


and then closes the connection, without sending any HTTP status headers or printing a correct '400 Bad Request' error page. This is because the @header variable wasn't set up properly, so @header is nil when a message is passed to it when handling the request. WEBrick's log on the server console will read similar to:

[2011-07-13 00:49:40] ERROR NoMethodError: undefined method each' for nil:NilClass
/Users/x/.rvm/rubies/ruby-1.9.2-p180/lib/ruby/1.9.1/webrick/httprequest.rb:154:in
each'
/Users/x/.rvm/rubies/ruby-1.9.2-p180/lib/ruby/1.9.1/webrick/httprequest.rb:231:in meta_vars'
/Users/x/.rvm/gems/ruby-1.9.2-p180/gems/rack-1.3.0/lib/rack/handler/webrick.rb:34:in
service'
/Users/x/.rvm/rubies/ruby-1.9.2-p180/lib/ruby/1.9.1/webrick/httpserver.rb:111:in service'
/Users/x/.rvm/rubies/ruby-1.9.2-p180/lib/ruby/1.9.1/webrick/httpserver.rb:70:in
run'
/Users/x/.rvm/rubies/ruby-1.9.2-p180/lib/ruby/1.9.1/webrick/server.rb:183:in `block in start_thread'

(Where line 34 of rack/handler/webrick.rb is a simple env = req.meta_vars)

This is reproducible in both 1.8.7 and 1.9.2.

A simple patch is attached to WEBrick's httprequest.rb that will allow the request to continue processing, fixing the 500 internal server error (and let the app decide how to handle the malformed request). In my testing with rack 1.3.0 & sinatra 1.2.6, this patch allowed the request complete normally.

=end

webrick_httprequest_header_fix.patch Magnifier - Patches lib/webrick/httprequest.rb (323 Bytes) Felix Jodoin, 07/13/2011 03:25 PM

webrick_test_httprequest_case.patch Magnifier - Test case to show error on req.meta_vars, patches test/webrick/test_httprequest.rb (676 Bytes) Felix Jodoin, 07/13/2011 04:56 PM

Associated revisions

Revision 32593
Added by Yui NARUSE over 2 years ago

  • lib/webrick/httprequest.rb (WEBrick::HTTPRequest#each): Allow HTTP/0.9 request which doesn't has any header or body. patched by Felix Jodoin. [Bug #5022]

History

#1 Updated by Aaron Patterson almost 3 years ago

Can you add a test?

Also, I'm not sure that this should be an error. In section 4.1 of RFC 1945, it says that simple requests are OK. Simple requests do not require an http version:

Simple-Request = "GET" SP Request-URI CRLF

#2 Updated by Felix Jodoin almost 3 years ago

Aaron Patterson wrote:

Can you add a test?

Also, I'm not sure that this should be an error. In section 4.1 of RFC 1945, it says that simple requests are OK. Simple requests do not require an http version:

Simple-Request = "GET" SP Request-URI CRLF

Hi, yes, this should not be a 400 bad request at all (I was incorrect in labeling it as a malformed HTTP request) - however, it is still broken without my patch since it returns a 500 instead of passing the request. Rather than erroring when the metavars are accessed, it should be prepared for a nil header (as RFC 1945 specifies headers should not exist with an HTTP/0.9 simple request). With my original patch applied, metavars will work as intended.

The attached test case will illustrate this (patches test/webrick/test_httprequest.rb).

#3 Updated by Nobuyoshi Nakada almost 3 years ago

  • Category set to lib
  • Assignee set to Aaron Patterson
  • Target version set to 1.9.3

assert(hash) does not seem nice.
assert_equal({}, hash) would be better.

#4 Updated by Felix Jodoin almost 3 years ago

Nobuyoshi Nakada wrote:

assert(hash) does not seem nice.
assert_equal({}, hash) would be better.

Has to be assertnothingraised { } at least, since meta_vars won't be empty with no headers.

Improved test case:

def testsimplerequest
msg = <<-endofmessage
GET /
endofmessage

req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP)
req.parse(StringIO.new(msg.gsub(/^ {6}/, '')))

# assertion fails if @header was not initialized and iteration is attempted on the nil reference
assert_nothing_raised('req.meta_vars should be available with HTTP/0.9 simple request') { req.meta_vars }

end

#5 Updated by Motohiro KOSAKI over 2 years ago

  • Status changed from Open to Assigned

#6 Updated by Yui NARUSE over 2 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r32593.
Felix, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • lib/webrick/httprequest.rb (WEBrick::HTTPRequest#each): Allow HTTP/0.9 request which doesn't has any header or body. patched by Felix Jodoin. [Bug #5022]

Also available in: Atom PDF