Permalink
Browse files

Add missing spec coverage for when secrets do not match (ACHTUNG!)

Fix Rack::Session::Cookie when old_secret or secret are not supplied.
Always encode cookies with one of the two secret options, if provided, preferencing :secret
Based on contribution from ciberch. Closes #304. Closes #299.
  • Loading branch information...
1 parent affdc3b commit 08e0eb07ae555ab08db8cbf82236dec573839d2d @raggi raggi committed Jan 7, 2012
Showing with 28 additions and 8 deletions.
  1. +9 −7 lib/rack/session/cookie.rb
  2. +19 −1 test/spec_session_cookie.rb
View
@@ -81,8 +81,7 @@ def decode(str); str.reverse; end
attr_reader :coder
def initialize(app, options={})
- @secret = options[:secret]
- @old_secret = options[:old_secret]
+ @secrets = options.values_at(:secret, :old_secret).compact
@coder = options[:coder] ||= Base64::Marshal.new
super(app, options.merge!(:cookie_only => true))
end
@@ -104,11 +103,14 @@ def unpacked_cookie_data(env)
request = Rack::Request.new(env)
session_data = request.cookies[@key]
- if (@secret || @old_secret) && session_data
+ if @secrets.size > 0 && session_data
session_data, digest = session_data.split("--")
- if (digest != generate_hmac(session_data, @secret)) && (digest != generate_hmac(session_data, @old_secret))
- session_data = nil
+
+ ok = @secrets.any? do |secret|
+ secret && digest == generate_hmac(session_data, secret)
end
+
+ session_data = nil unless ok
end
coder.decode(session_data) || {}
@@ -131,8 +133,8 @@ def set_session(env, session_id, session, options)
session = session.merge("session_id" => session_id)
session_data = coder.encode(session)
- if @secret
- session_data = "#{session_data}--#{generate_hmac(session_data, @secret)}"
+ if @secrets.first
+ session_data = "#{session_data}--#{generate_hmac(session_data, @secrets.first)}"
end
if session_data.size > (4096 - @key.size)
@@ -137,7 +137,7 @@ def decode(str); @calls << :decode; str; end
}.should.raise(Rack::MockRequest::FatalWarning)
end
- it "loads from a cookie wih integrity hash" do
+ it "loads from a cookie with integrity hash" do
res = Rack::MockRequest.new(Rack::Session::Cookie.new(incrementor, :secret => 'test')).get("/")
cookie = res["Set-Cookie"]
res = Rack::MockRequest.new(Rack::Session::Cookie.new(incrementor, :secret => 'test')).
@@ -147,6 +147,9 @@ def decode(str); @calls << :decode; str; end
res = Rack::MockRequest.new(Rack::Session::Cookie.new(incrementor, :secret => 'test')).
get("/", "HTTP_COOKIE" => cookie)
res.body.should.equal '{"counter"=>3}'
+ res = Rack::MockRequest.new(Rack::Session::Cookie.new(incrementor, :secret => 'other')).
+ get("/", "HTTP_COOKIE" => cookie)
+ res.body.should.equal '{"counter"=>1}'
end
it "loads from a cookie wih accept-only integrity hash for graceful key rotation" do
@@ -165,6 +168,8 @@ def decode(str); @calls << :decode; str; end
app = Rack::Session::Cookie.new(incrementor, :secret => 'test')
response1 = Rack::MockRequest.new(app).get("/")
response1.body.should.equal '{"counter"=>1}'
+ response1 = Rack::MockRequest.new(app).get("/", "HTTP_COOKIE" => response1["Set-Cookie"])
+ response1.body.should.equal '{"counter"=>2}'
_, digest = response1["Set-Cookie"].split("--")
tampered_with_cookie = "hackerman-was-here" + "--" + digest
@@ -175,6 +180,19 @@ def decode(str); @calls << :decode; str; end
response2.body.should.equal '{"counter"=>1}'
end
+ it "supports either of secret or old_secret" do
+ app = Rack::Session::Cookie.new(incrementor, :secret => 'test')
+ res = Rack::MockRequest.new(app).get("/")
+ res.body.should.equal '{"counter"=>1}'
+ res = Rack::MockRequest.new(app).get("/", "HTTP_COOKIE" => res["Set-Cookie"])
+ res.body.should.equal '{"counter"=>2}'
+ app = Rack::Session::Cookie.new(incrementor, :old_secret => 'test')
+ res = Rack::MockRequest.new(app).get("/")
+ res.body.should.equal '{"counter"=>1}'
+ res = Rack::MockRequest.new(app).get("/", "HTTP_COOKIE" => res["Set-Cookie"])
+ res.body.should.equal '{"counter"=>2}'
+ end
+
describe "1.9 bugs relating to inspecting yet-to-be-loaded from cookie data: Rack::Session::Abstract::SessionHash" do
it "can handle Rack::Lint middleware" do

0 comments on commit 08e0eb0

Please sign in to comment.