Merge pull request #52 from rmm5t/strip_set_cookie

Rack::Cache caches Set-Cookie response headers yielding potential security holes in apps

Author: rtomayko

lib/rack/cache/context.rb

@@ -260,6 +260,7 @@ def fetch
 
     # Write the response to the cache.
     def store(response)
+      strip_ignore_headers(response)
       metastore.store(@request, response, entitystore)
       response.headers['Age'] = response.age.to_s
     rescue Exception => e
@@ -269,6 +270,12 @@ def store(response)
       record :store
     end
 
+    # Remove all ignored response headers before writing to the cache.
+    def strip_ignore_headers(response)
+      stripped_values = ignore_headers.map { |name| response.headers.delete(name) }
+      record :ignore if stripped_values.any?
+    end
+
     def log_error(exception)
       @env['rack.errors'].write("cache error: #{exception.message}\n#{exception.backtrace.join("\n")}\n")
     end

lib/rack/cache/options.rb

@@ -78,6 +78,14 @@ def option_name(key)
     # Default: 0
     option_accessor :default_ttl
 
+    # Set of response headers that are removed before storing them in the
+    # cache. These headers are only removed for cacheable responses.  For
+    # example, in most cases, it makes sense to prevent cookies from being
+    # stored in the cache.
+    #
+    # Default: ['Set-Cookie']
+    option_accessor :ignore_headers
+
     # Set of request headers that trigger "private" cache-control behavior
     # on responses that don't explicitly state whether the response is
     # public or private via a Cache-Control directive. Applications that use
@@ -138,6 +146,7 @@ def initialize_options(options={})
         'rack-cache.metastore'        => 'heap:/',
         'rack-cache.entitystore'      => 'heap:/',
         'rack-cache.default_ttl'      => 0,
+        'rack-cache.ignore_headers'   => ['Set-Cookie'],
         'rack-cache.private_headers'  => ['Authorization', 'Cookie'],
         'rack-cache.allow_reload'     => false,
         'rack-cache.allow_revalidate' => false,

test/context_test.rb

@@ -57,6 +57,7 @@
     response.should.be.ok
     cache.trace.should.include :miss
     cache.trace.should.include :store
+    cache.trace.should.not.include :ignore
     response.headers.should.include 'Age'
     response.headers['Cache-Control'].should.equal 'public'
   end
@@ -85,6 +86,40 @@
     response.headers['Cache-Control'].should.equal 'private'
   end
 
+  it 'does remove Set-Cookie response header from a cacheable response' do
+    respond_with 200, 'Cache-Control' => 'public', 'ETag' => '"FOO"', 'Set-Cookie' => 'TestCookie=OK'
+    get '/'
+
+    app.should.be.called
+    response.should.be.ok
+    cache.trace.should.include :store
+    cache.trace.should.include :ignore
+    response.headers['Set-Cookie'].should.be.nil
+  end
+
+  it 'does remove all configured ignore_headers from a cacheable response' do
+    respond_with 200, 'Cache-Control' => 'public', 'ETag' => '"FOO"', 'SET-COOKIE' => 'TestCookie=OK', 'X-Strip-Me' => 'Secret'
+    get '/', 'rack-cache.ignore_headers' => ['set-cookie', 'x-strip-me']
+
+    app.should.be.called
+    response.should.be.ok
+    cache.trace.should.include :store
+    cache.trace.should.include :ignore
+    response.headers['Set-Cookie'].should.be.nil
+    response.headers['x-strip-me'].should.be.nil
+  end
+
+  it 'does not remove Set-Cookie response header from a private response' do
+    respond_with 200, 'Cache-Control' => 'private', 'Set-Cookie' => 'TestCookie=OK'
+    get '/'
+
+    app.should.be.called
+    response.should.be.ok
+    cache.trace.should.not.include :store
+    cache.trace.should.not.include :ignore
+    response.headers['Set-Cookie'].should.equal 'TestCookie=OK'
+  end
+
   it 'responds with 304 when If-Modified-Since matches Last-Modified' do
     timestamp = Time.now.httpdate
     respond_with do |req,res|