From 64aaf4cb54c4eadd410133d72810b0124e818ce2 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 18 Dec 2019 09:31:42 -0800 Subject: [PATCH] Squashed commit of the following: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit commit d3e2f88c17dad2c7997e453d7ef518dd6e751ac8 Author: Aaron Patterson Date: Tue Dec 17 12:17:34 2019 -0800 making diff smaller commit 99a8a8776513839b5da4af393b67afe95a9412d8 Author: Aaron Patterson Date: Tue Dec 17 12:13:00 2019 -0800 fix memcache tests on 1.6 commit f2cb48e50e507e638973f331d4a62099fae567ec Author: Aaron Patterson Date: Tue Dec 17 11:48:45 2019 -0800 fix tests on 1.6 commit 7ff635c51d29f3e19377855f6010574fb2e8e593 Author: Rafael Mendonça França Date: Mon Oct 21 16:39:00 2019 -0400 Introduce a new base class to avoid breaking when upgrading Third-party session store would still need to be chaged to be more secure but only upgrading rack will not break any application. commit 3232f9370d099e784a16c01d32e8a2da4a953f18 Author: Rafael Mendonça França Date: Wed Oct 16 14:07:36 2019 -0400 Add a version prefix to the private id to make easier to migrate old values commit 15da2e5d95228d0b3fcdb38b2a562efc333402f0 Author: Rafael Mendonça França Date: Wed Oct 9 19:14:08 2019 -0400 Fallback to the public id when reading the session in the pool adapter commit 1a532d13eee9d5546349b5253a204187773de151 Author: Rafael Mendonça França Date: Wed Oct 9 18:06:23 2019 -0400 Also drop the session with the public id when destroying sessions commit 9fe40c68b514e0f4a947577e4b903a9ae477365e Author: Rafael Mendonça França Date: Wed Oct 9 17:50:45 2019 -0400 Fallback to the legacy id when the new id is not found This will avoid all session to be invalidated. commit b9565a90eea77960e552e3c86b0adb83ab034726 Author: Aaron Patterson Date: Tue Aug 13 17:16:23 2019 -0400 Add the private id commit 368effdbeca7955f4e46ea51bfb2d647bc79aa6b Author: Aaron Patterson Date: Tue Aug 13 16:48:41 2019 -0400 revert conditionals to master commit d49aa811d6c8fe109c7fc5ca9bddb3d9f7eba796 Author: Aaron Patterson Date: Tue Aug 13 16:45:04 2019 -0400 remove NullSession commit 3e9cb660cc8bf9543b134b6f3ca35aadfe4e0611 Author: Aaron Patterson Date: Tue Aug 13 16:38:01 2019 -0400 remove || raise and get closer to master commit 442dba2362558e4a7a3e39d437b95d81f2479b31 Author: Aaron Patterson Date: Tue Aug 13 16:31:06 2019 -0400 store hashed id, send public id commit 3ab0277cd129f15059662451718048bcf23cb5d1 Author: Aaron Patterson Date: Tue Aug 13 16:20:32 2019 -0400 use session id objects commit 7237b6661ad98c1dac6ad799192262697e1a3559 Author: Aaron Patterson Date: Tue Aug 13 15:43:58 2019 -0400 remove more nils commit 511f809e80c3264af3a26d485a5137ac28f08ebe Author: Aaron Patterson Date: Tue Aug 13 15:32:20 2019 -0400 try to ensure we always have some kind of object --- lib/rack/session/abstract/id.rb | 82 ++++++++++++++++++++++++++++++-- lib/rack/session/cookie.rb | 13 ++++- lib/rack/session/memcache.rb | 18 ++++--- lib/rack/session/pool.rb | 19 +++++--- test/spec_session_abstract_id.rb | 2 +- test/spec_session_memcache.rb | 43 +++++++++++++++-- test/spec_session_pool.rb | 43 +++++++++++++++-- 7 files changed, 196 insertions(+), 24 deletions(-) diff --git a/lib/rack/session/abstract/id.rb b/lib/rack/session/abstract/id.rb index 62bdb04..c13faf5 100644 --- a/lib/rack/session/abstract/id.rb +++ b/lib/rack/session/abstract/id.rb @@ -9,11 +9,38 @@ begin rescue LoadError # We just won't get securerandom end +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 ENV_SESSION_KEY = 'rack.session'.freeze ENV_SESSION_OPTIONS_KEY = 'rack.session.options'.freeze @@ -191,7 +218,7 @@ module Rack # Not included by default; you must require 'rack/session/abstract/id' # to use. - class ID + class Persisted DEFAULT_OPTIONS = { :key => 'rack.session', :path => '/', @@ -342,10 +369,10 @@ module Rack if not data = set_session(env, session_id, session_data, options) env["rack.errors"].puts("Warning! #{self.class.name} failed to save session. Content dropped.") elsif options[:defer] and not options[:renew] - env["rack.errors"].puts("Deferring cookie for #{session_id}") if $VERBOSE + env["rack.errors"].puts("Deferring cookie for #{session_id.public_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(env, headers, cookie.merge!(options)) @@ -354,6 +381,10 @@ module Rack [status, headers, body] end + 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. @@ -394,6 +425,51 @@ module Rack raise '#destroy_session not implemented' 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 } + unless k.instance_variable_defined?(:"@_rack_warned") + warn "#{klass} is inheriting from #{ID}. Inheriting from #{ID} is deprecated, please inherit from #{Persisted} instead" if $VERBOSE + k.instance_variable_set(:"@_rack_warned", true) + end + super + end + end end end end diff --git a/lib/rack/session/cookie.rb b/lib/rack/session/cookie.rb index 9bea586..9ca9df2 100644 --- a/lib/rack/session/cookie.rb +++ b/lib/rack/session/cookie.rb @@ -44,7 +44,7 @@ module Rack # }) # - class Cookie < Abstract::ID + class Cookie < Abstract::PersistedSecure # Encode session cookies as Base64 class Base64 def encode(str) @@ -151,6 +151,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 set_session(env, session_id, session, options) session = session.merge("session_id" => session_id) session_data = coder.encode(session) @@ -163,7 +172,7 @@ module Rack env["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/memcache.rb b/lib/rack/session/memcache.rb index c0e1f3e..1c6f256 100644 --- a/lib/rack/session/memcache.rb +++ b/lib/rack/session/memcache.rb @@ -19,7 +19,7 @@ module Rack # Note that memcache does drop data before it may be listed to expire. For # a full description of behaviour, please see memcache's documentation. - class Memcache < Abstract::ID + class Memcache < Abstract::PersistedSecure attr_reader :mutex, :pool DEFAULT_OPTIONS = Abstract::ID::DEFAULT_OPTIONS.merge \ @@ -42,15 +42,15 @@ module Rack def generate_sid loop do sid = super - break sid unless @pool.get(sid, true) + break sid unless @pool.get(sid.private_id, true) end end def get_session(env, sid) with_lock(env) do - unless sid and session = @pool.get(sid) + unless sid and session = get_session_with_fallback(sid) sid, session = generate_sid, {} - unless /^STORED/ =~ @pool.add(sid, session) + unless /^STORED/ =~ @pool.add(sid.private_id, session) raise "Session collision on '#{sid.inspect}'" end end @@ -63,14 +63,15 @@ module Rack expiry = expiry.nil? ? 0 : expiry + 1 with_lock(env) do - @pool.set session_id, new_session, expiry + @pool.set session_id.private_id, new_session, expiry session_id end end def destroy_session(env, session_id, options) with_lock(env) do - @pool.delete(session_id) + @pool.delete(session_id.public_id) + @pool.delete(session_id.private_id) generate_sid unless options[:drop] end end @@ -88,6 +89,11 @@ module Rack @mutex.unlock if @mutex.locked? end + private + + def get_session_with_fallback(sid) + @pool.get(sid.private_id) || @pool.get(sid.public_id) + end end end end diff --git a/lib/rack/session/pool.rb b/lib/rack/session/pool.rb index fcb34ec..e4afaaf 100644 --- a/lib/rack/session/pool.rb +++ b/lib/rack/session/pool.rb @@ -24,7 +24,7 @@ module Rack # ) # Rack::Handler::WEBrick.run sessioned - class Pool < Abstract::ID + class Pool < Abstract::PersistedSecure attr_reader :mutex, :pool DEFAULT_OPTIONS = Abstract::ID::DEFAULT_OPTIONS.merge :drop => false @@ -37,15 +37,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 get_session(env, sid) with_lock(env) 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 @@ -53,14 +53,15 @@ module Rack def set_session(env, session_id, new_session, options) with_lock(env) do - @pool.store session_id, new_session + @pool.store session_id.private_id, new_session session_id end end def destroy_session(env, session_id, options) with_lock(env) do - @pool.delete(session_id) + @pool.delete(session_id.public_id) + @pool.delete(session_id.private_id) generate_sid unless options[:drop] end end @@ -71,6 +72,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_abstract_id.rb b/test/spec_session_abstract_id.rb index 911f43b..99327a1 100644 --- a/test/spec_session_abstract_id.rb +++ b/test/spec_session_abstract_id.rb @@ -47,7 +47,7 @@ describe Rack::Session::Abstract::ID do end end id = Rack::Session::Abstract::ID.new nil, :secure_random => secure_random.new - id.send(:generate_sid).should.eql 'fake_hex' + id.send(:generate_sid).should.equal 'fake_hex' end end diff --git a/test/spec_session_memcache.rb b/test/spec_session_memcache.rb index 2b75980..5f50079 100644 --- a/test/spec_session_memcache.rb +++ b/test/spec_session_memcache.rb @@ -225,15 +225,52 @@ begin req = Rack::MockRequest.new(pool) res0 = req.get("/") - session_id = (cookie = res0["Set-Cookie"])[session_match, 1] - ses0 = pool.pool.get(session_id, true) + session_id = Rack::Session::SessionId.new (cookie = res0["Set-Cookie"])[session_match, 1] + ses0 = pool.pool.get(session_id.private_id, true) req.get("/", "HTTP_COOKIE" => cookie) - ses1 = pool.pool.get(session_id, true) + ses1 = pool.pool.get(session_id.private_id, true) ses1.should.not.equal ses0 end + it "can read the session with the legacy id" do + pool = Rack::Session::Memcache.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.get(session_id.private_id, true) + pool.pool.set(session_id.public_id, ses0, 0, true) + pool.pool.delete(session_id.private_id) + + res1 = req.get("/", "HTTP_COOKIE" => cookie) + res1["Set-Cookie"].should.be.nil + res1.body.should.equal '{"counter"=>2}' + pool.pool.get(session_id.private_id, true).should.not.be.nil + end + + it "drops the session in the legacy id as well" do + pool = Rack::Session::Memcache.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.get(session_id.private_id, true) + pool.pool.set(session_id.public_id, ses0, 0, true) + pool.pool.delete(session_id.private_id) + + res2 = dreq.get("/", "HTTP_COOKIE" => cookie) + res2["Set-Cookie"].should.be.nil + res2.body.should.equal '{"counter"=>2}' + pool.pool.get(session_id.private_id, true).should.be.nil + pool.pool.get(session_id.public_id, true).should.be.nil + end + # anyone know how to do this better? it "cleanly merges sessions when multithreaded" do unless $DEBUG diff --git a/test/spec_session_pool.rb b/test/spec_session_pool.rb index 984f55a..ec9239e 100644 --- a/test/spec_session_pool.rb +++ b/test/spec_session_pool.rb @@ -5,7 +5,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 @@ -13,7 +13,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) @@ -142,6 +142,43 @@ describe Rack::Session::Pool do pool.pool.size.should.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"].should.be.nil + res1.body.should.equal '{"counter"=>2}' + pool.pool[session_id.private_id].should.not.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"].should.be.nil + res2.body.should.equal '{"counter"=>2}' + pool.pool[session_id.private_id].should.be.nil + pool.pool[session_id.public_id].should.be.nil + end + # anyone know how to do this better? it "should merge sessions when multithreaded" do unless $DEBUG @@ -190,7 +227,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"].should.be.nil end -- 2.21.0