Skip to content
Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tenderlove committed Dec 18, 2019
2 parents e82f06b + ef6d23d commit 7fecaee
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 12 deletions.
67 changes: 66 additions & 1 deletion lib/rack/session/abstract/id.rb
Expand Up @@ -8,11 +8,38 @@
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.

Expand Down Expand Up @@ -375,14 +402,18 @@ def commit_session(req, res)
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))
end
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.

Expand Down Expand Up @@ -424,6 +455,40 @@ def delete_session(req, sid, options)
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 }
Expand Down
13 changes: 11 additions & 2 deletions lib/rack/session/cookie.rb
Expand Up @@ -48,7 +48,7 @@ module Session
# })
#

class Cookie < Abstract::Persisted
class Cookie < Abstract::PersistedSecure
# Encode session cookies as Base64
class Base64
def encode(str)
Expand Down Expand Up @@ -154,6 +154,15 @@ def persistent_session_id!(data, sid = nil)
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)
Expand All @@ -166,7 +175,7 @@ def write_session(req, session_id, session, options)
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

Expand Down
19 changes: 13 additions & 6 deletions lib/rack/session/pool.rb
Expand Up @@ -26,7 +26,7 @@ module Session
# )
# 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

Expand All @@ -39,30 +39,31 @@ def initialize(app, options = {})
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
end

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
Expand All @@ -73,6 +74,12 @@ def with_lock(req)
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
43 changes: 40 additions & 3 deletions test/spec_session_pool.rb
Expand Up @@ -8,15 +8,15 @@

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
env["rack.session"]["counter"] += 1
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)

Expand Down Expand Up @@ -145,6 +145,43 @@
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
Expand Down Expand Up @@ -193,7 +230,7 @@
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
Expand Down

0 comments on commit 7fecaee

Please sign in to comment.