diff options
author | Aaron Patterson <aaron.patterson@gmail.com> | 2019-12-18 10:07:23 -0800 |
---|---|---|
committer | Aaron Patterson <aaron.patterson@gmail.com> | 2019-12-18 10:07:23 -0800 |
commit | 7fecaee81f59926b6e1913511c90650e76673b38 (patch) | |
tree | 287ad9e7a49765b8d45cb334241d54c9df2721b0 | |
parent | e82f06b354fa74ef40740993bd87280bbf010227 (diff) | |
parent | ef6d23d36a1480980971a8ad81ed0b7621ab8101 (diff) | |
download | rack-7fecaee81f59926b6e1913511c90650e76673b38.tar.gz |
Merge branch 'advisory-fix-1'
* advisory-fix-1:
Introduce a new base class to avoid breaking when upgrading
Add a version prefix to the private id to make easier to migrate old values
Fallback to the public id when reading the session in the pool adapter
Also drop the session with the public id when destroying sessions
Fallback to the legacy id when the new id is not found
Add the private id
revert conditionals to master
remove NullSession
remove || raise and get closer to master
store hashed id, send public id
use session id objects
remove more nils
try to ensure we always have some kind of object
-rw-r--r-- | lib/rack/session/abstract/id.rb | 67 | ||||
-rw-r--r-- | lib/rack/session/cookie.rb | 13 | ||||
-rw-r--r-- | lib/rack/session/pool.rb | 19 | ||||
-rw-r--r-- | test/spec_session_pool.rb | 43 |
4 files changed, 130 insertions, 12 deletions
diff --git a/lib/rack/session/abstract/id.rb b/lib/rack/session/abstract/id.rb index b15ee3b8..633f2703 100644 --- a/lib/rack/session/abstract/id.rb +++ b/lib/rack/session/abstract/id.rb @@ -8,11 +8,38 @@ require 'time' require 'rack/request' require 'rack/response' require 'securerandom' +require 'digest/sha2' module Rack module Session + class SessionId + ID_VERSION = 2 + + attr_reader :public_id + + def initialize(public_id) + @public_id = public_id + end + + def private_id + "#{ID_VERSION}::#{hash_sid(public_id)}" + end + + alias :cookie_value :public_id + + def empty?; false; end + def to_s; raise; end + def inspect; public_id.inspect; end + + private + + def hash_sid(sid) + Digest::SHA256.hexdigest(sid) + end + end + module Abstract # SessionHash is responsible to lazily load the session from store. @@ -375,7 +402,7 @@ module Rack req.get_header(RACK_ERRORS).puts("Deferring cookie for #{session_id}") if $VERBOSE else cookie = Hash.new - cookie[:value] = data + cookie[:value] = cookie_value(data) cookie[:expires] = Time.now + options[:expire_after] if options[:expire_after] cookie[:expires] = Time.now + options[:max_age] if options[:max_age] set_cookie(req, res, cookie.merge!(options)) @@ -383,6 +410,10 @@ module Rack end public :commit_session + def cookie_value(data) + data + end + # Sets the cookie back to the client with session id. We skip the cookie # setting if the value didn't change (sid is the same) or expires was given. @@ -424,6 +455,40 @@ module Rack end end + class PersistedSecure < Persisted + class SecureSessionHash < SessionHash + def [](key) + if key == "session_id" + load_for_read! + id.public_id + else + super + end + end + end + + def generate_sid(*) + public_id = super + + SessionId.new(public_id) + end + + def extract_session_id(*) + public_id = super + public_id && SessionId.new(public_id) + end + + private + + def session_class + SecureSessionHash + end + + def cookie_value(data) + data.cookie_value + end + end + class ID < Persisted def self.inherited(klass) k = klass.ancestors.find { |kl| kl.respond_to?(:superclass) && kl.superclass == ID } diff --git a/lib/rack/session/cookie.rb b/lib/rack/session/cookie.rb index 70ddadd6..d78fa997 100644 --- a/lib/rack/session/cookie.rb +++ b/lib/rack/session/cookie.rb @@ -48,7 +48,7 @@ module Rack # }) # - class Cookie < Abstract::Persisted + class Cookie < Abstract::PersistedSecure # Encode session cookies as Base64 class Base64 def encode(str) @@ -154,6 +154,15 @@ module Rack data end + class SessionId < DelegateClass(Session::SessionId) + attr_reader :cookie_value + + def initialize(session_id, cookie_value) + super(session_id) + @cookie_value = cookie_value + end + end + def write_session(req, session_id, session, options) session = session.merge("session_id" => session_id) session_data = coder.encode(session) @@ -166,7 +175,7 @@ module Rack req.get_header(RACK_ERRORS).puts("Warning! Rack::Session::Cookie data size exceeds 4K.") nil else - session_data + SessionId.new(session_id, session_data) end end diff --git a/lib/rack/session/pool.rb b/lib/rack/session/pool.rb index 2e1f867f..f5b62650 100644 --- a/lib/rack/session/pool.rb +++ b/lib/rack/session/pool.rb @@ -26,7 +26,7 @@ module Rack # ) # Rack::Handler::WEBrick.run sessioned - class Pool < Abstract::Persisted + class Pool < Abstract::PersistedSecure attr_reader :mutex, :pool DEFAULT_OPTIONS = Abstract::ID::DEFAULT_OPTIONS.merge drop: false @@ -39,15 +39,15 @@ module Rack def generate_sid loop do sid = super - break sid unless @pool.key? sid + break sid unless @pool.key? sid.private_id end end def find_session(req, sid) with_lock(req) do - unless sid and session = @pool[sid] + unless sid and session = get_session_with_fallback(sid) sid, session = generate_sid, {} - @pool.store sid, session + @pool.store sid.private_id, session end [sid, session] end @@ -55,14 +55,15 @@ module Rack def write_session(req, session_id, new_session, options) with_lock(req) do - @pool.store session_id, new_session + @pool.store session_id.private_id, new_session session_id end end def delete_session(req, session_id, options) with_lock(req) do - @pool.delete(session_id) + @pool.delete(session_id.public_id) + @pool.delete(session_id.private_id) generate_sid unless options[:drop] end end @@ -73,6 +74,12 @@ module Rack ensure @mutex.unlock if @mutex.locked? end + + private + + def get_session_with_fallback(sid) + @pool[sid.private_id] || @pool[sid.public_id] + end end end end diff --git a/test/spec_session_pool.rb b/test/spec_session_pool.rb index fda5f56e..dd1b6573 100644 --- a/test/spec_session_pool.rb +++ b/test/spec_session_pool.rb @@ -8,7 +8,7 @@ require 'rack/session/pool' describe Rack::Session::Pool do session_key = Rack::Session::Pool::DEFAULT_OPTIONS[:key] - session_match = /#{session_key}=[0-9a-fA-F]+;/ + session_match = /#{session_key}=([0-9a-fA-F]+);/ incrementor = lambda do |env| env["rack.session"]["counter"] ||= 0 @@ -16,7 +16,7 @@ describe Rack::Session::Pool do Rack::Response.new(env["rack.session"].inspect).to_a end - session_id = Rack::Lint.new(lambda do |env| + get_session_id = Rack::Lint.new(lambda do |env| Rack::Response.new(env["rack.session"].inspect).to_a end) @@ -145,6 +145,43 @@ describe Rack::Session::Pool do pool.pool.size.must_equal 1 end + it "can read the session with the legacy id" do + pool = Rack::Session::Pool.new(incrementor) + req = Rack::MockRequest.new(pool) + + res0 = req.get("/") + cookie = res0["Set-Cookie"] + session_id = Rack::Session::SessionId.new cookie[session_match, 1] + ses0 = pool.pool[session_id.private_id] + pool.pool[session_id.public_id] = ses0 + pool.pool.delete(session_id.private_id) + + res1 = req.get("/", "HTTP_COOKIE" => cookie) + res1["Set-Cookie"].must_be_nil + res1.body.must_equal '{"counter"=>2}' + pool.pool[session_id.private_id].wont_be_nil + end + + it "drops the session in the legacy id as well" do + pool = Rack::Session::Pool.new(incrementor) + req = Rack::MockRequest.new(pool) + drop = Rack::Utils::Context.new(pool, drop_session) + dreq = Rack::MockRequest.new(drop) + + res0 = req.get("/") + cookie = res0["Set-Cookie"] + session_id = Rack::Session::SessionId.new cookie[session_match, 1] + ses0 = pool.pool[session_id.private_id] + pool.pool[session_id.public_id] = ses0 + pool.pool.delete(session_id.private_id) + + res2 = dreq.get("/", "HTTP_COOKIE" => cookie) + res2["Set-Cookie"].must_be_nil + res2.body.must_equal '{"counter"=>2}' + pool.pool[session_id.private_id].must_be_nil + pool.pool[session_id.public_id].must_be_nil + end + # anyone know how to do this better? it "should merge sessions when multithreaded" do unless $DEBUG @@ -193,7 +230,7 @@ describe Rack::Session::Pool do end it "does not return a cookie if cookie was not written (only read)" do - app = Rack::Session::Pool.new(session_id) + app = Rack::Session::Pool.new(get_session_id) res = Rack::MockRequest.new(app).get("/") res["Set-Cookie"].must_be_nil end |