Bug in HeaderHash

Bug in HeaderHash

[Alex Beregszaszi]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

Hi,

Currently the included Rack::Deflater will fail (and surely other
modules can fail too) if gzip encoding is used and some other conditions
are met. Like the ConditionalGet plugin is run first.

The appropriate code in Deflater:

mtime = headers.key?("Last-Modified") ?
Time.httpdate(headers("Last-Modified"]) : Time.now

Time.httpdate(nil) will cause an Exception. Based on the above code,
that shouldn't happen, but it does.

Turned out that in the ConditionalGet.modified_since function
headers['Last-Modified'] is read and that causes the error.

The real problem lies in HeaderHash.[]:

super(@names[k] ||= @names[k.downcase])

In the above situation where @names is a Hash, @names[k] (and
@names[k.downcase]) will return nil first and then a Hash entry will be
created in @names and set to nil. The above code will work fine
whatsoever, but afterwards HeaderHash.include? will erroneously report
that header key as present.

The attached patch fixes this issue. Note, you can still set a http
header to nil after the patch (the []= isn't involved).

Simple way to reproduce the error:
headers = Utils::HeaderHash.new(headers)
headers['wont-be-there']
puts "error" if headers.include?('wont-be-there')

--
Alex Beregszaszi


[-- Attachment #2: rack-headerhash-bug.diff --]
[-- Type: text/x-patch, Size: 348 bytes --]

diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb
index fab2c49..f07ec40 100644
--- a/lib/rack/utils.rb
+++ b/lib/rack/utils.rb
@@ -291,7 +291,8 @@ module Rack
       end
 
       def [](k)
-        super(@names[k] ||= @names[k.downcase])
+        super(@names[k]) if @names[k]
+        super(@names[k.downcase])
       end
 
       def []=(k, v)

Re: Bug in HeaderHash

[Alex Beregszaszi]

Hi,

On Sun, 2010-01-24 at 07:55 +0100, Alex Beregszaszi wrote:
> Hi,
> 
> Currently the included Rack::Deflater will fail (and surely other
> modules can fail too) if gzip encoding is used and some other conditions
> are met. Like the ConditionalGet plugin is run first.
> 
> The appropriate code in Deflater:
> 
> mtime = headers.key?("Last-Modified") ?
> Time.httpdate(headers("Last-Modified"]) : Time.now
> 
> Time.httpdate(nil) will cause an Exception. Based on the above code,
> that shouldn't happen, but it does.
> 
> Turned out that in the ConditionalGet.modified_since function
> headers['Last-Modified'] is read and that causes the error.
> 
> The real problem lies in HeaderHash.[]:
> 
> super(@names[k] ||= @names[k.downcase])
> 
> In the above situation where @names is a Hash, @names[k] (and
> @names[k.downcase]) will return nil first and then a Hash entry will be
> created in @names and set to nil. The above code will work fine
> whatsoever, but afterwards HeaderHash.include? will erroneously report
> that header key as present.
> 
> The attached patch fixes this issue. Note, you can still set a http
> header to nil after the patch (the []= isn't involved).
> 
> Simple way to reproduce the error:
> headers = Utils::HeaderHash.new(headers)
> headers['wont-be-there']
> puts "error" if headers.include?('wont-be-there')

Any comments on this?

--
Alex Beregszaszi

Re: Bug in HeaderHash

[Ryan Tomayko]

On Sat, Jan 23, 2010 at 10:55 PM, Alex Beregszaszi <alex@datira.com> wrote:
> Hi,
>
> Currently the included Rack::Deflater will fail (and surely other
> modules can fail too) if gzip encoding is used and some other conditions
> are met. Like the ConditionalGet plugin is run first.
>
> The appropriate code in Deflater:
>
> mtime = headers.key?("Last-Modified") ?
> Time.httpdate(headers("Last-Modified"]) : Time.now
>
> Time.httpdate(nil) will cause an Exception. Based on the above code,
> that shouldn't happen, but it does.
>
> Turned out that in the ConditionalGet.modified_since function
> headers['Last-Modified'] is read and that causes the error.
>
> The real problem lies in HeaderHash.[]:
>
> super(@names[k] ||= @names[k.downcase])
>
> In the above situation where @names is a Hash, @names[k] (and
> @names[k.downcase]) will return nil first and then a Hash entry will be
> created in @names and set to nil. The above code will work fine
> whatsoever, but afterwards HeaderHash.include? will erroneously report
> that header key as present.
>
> The attached patch fixes this issue. Note, you can still set a http
> header to nil after the patch (the []= isn't involved).
>
> Simple way to reproduce the error:
> headers = Utils::HeaderHash.new(headers)
> headers['wont-be-there']
> puts "error" if headers.include?('wont-be-there')

Applied and pushed to master in f6f3c60.

Thanks,
Ryan