Skip to content

Commit

Permalink
use session id objects
Browse files Browse the repository at this point in the history
  • Loading branch information
tenderlove committed Aug 13, 2019
1 parent a4f30d2 commit cc1d162
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 15 deletions.
28 changes: 24 additions & 4 deletions lib/rack/session/abstract/id.rb
Expand Up @@ -18,6 +18,20 @@ def empty?; true; end
def nil?; true; end
end

class SessionId
attr_reader :public_id

def initialize(public_id)
@public_id = public_id
end

alias :cookie_value :public_id

def empty?; false; end
def to_s; raise; end

This comment has been minimized.

Copy link
@naemcivic

naemcivic Dec 25, 2019

@tenderlove Hi, I know this commit has been a while. But why are we preventing this coercion to string here? It seems to be breaking things here. thanks

This comment has been minimized.

Copy link
@vincentwoo

vincentwoo Dec 28, 2019

Yeah, seeing breakage on latest rails due to passing a session id into model creation

This comment has been minimized.

Copy link
@jeremyevans

jeremyevans Jan 8, 2020

Contributor

This also breaks Roda's session plugin's automatic upgrading of Rack::Session::Cookie sessions. I suggest that if this is disabled for security reasons, using a specific error class and/or error message would be appreciated.

This comment has been minimized.

Copy link
@ioquatix

ioquatix Jan 9, 2020

Member

Can someone make a PR to fix?

This comment has been minimized.

Copy link
@jeremyevans

jeremyevans Jan 9, 2020

Contributor

Done: #1462

def inspect; public_id.inspect; end
end

module Abstract
# SessionHash is responsible to lazily load the session from store.

Expand Down Expand Up @@ -64,7 +78,11 @@ def each(&block)

def [](key)
load_for_read!
@data[key.to_s]
if key == "session_id"
id.public_id
else
@data[key.to_s]
end
end

def fetch(key, default = Unspecified, &block)
Expand Down Expand Up @@ -262,11 +280,13 @@ def initialize_sid
# Monkey patch this to use custom methods for session id generation.

def generate_sid(secure = @sid_secure)
if secure
public_id = if secure
secure.hex(@sid_length)
else
"%0#{@sid_length}x" % Kernel.rand(2**@sidbits - 1)
end

SessionId.new(public_id)
rescue NotImplementedError
generate_sid(false)
end
Expand Down Expand Up @@ -296,7 +316,7 @@ def load_session(req)
def extract_session_id(request)
sid = request.cookies[@key]
sid ||= request.params[@key] unless @cookie_only
sid || NullSessionId.new
(sid && SessionId.new(sid)) || NullSessionId.new
end

# Returns the current session id from the SessionHash.
Expand Down Expand Up @@ -367,7 +387,7 @@ 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] = data.cookie_value
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))
Expand Down
11 changes: 10 additions & 1 deletion lib/rack/session/cookie.rb
Expand Up @@ -153,6 +153,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 @@ -165,7 +174,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
10 changes: 5 additions & 5 deletions lib/rack/session/memcache.rb
Expand Up @@ -47,15 +47,15 @@ def initialize(app, options = {})
def generate_sid
loop do
sid = super
break sid unless @pool.get(sid, true)
break sid unless @pool.get(sid.public_id, true)
end
end

def get_session(env, sid)
with_lock(env) do
unless sid and session = @pool.get(sid)
unless !sid.nil? and session = @pool.get(sid.public_id)
sid, session = generate_sid, {}
unless /^STORED/.match?(@pool.add(sid, session))
unless /^STORED/.match?(@pool.add(sid.public_id, session))
raise "Session collision on '#{sid.inspect}'"
end
end
Expand All @@ -68,14 +68,14 @@ def set_session(env, session_id, new_session, options)
expiry = expiry.nil? ? 0 : expiry + 1

with_lock(env) do
@pool.set session_id, new_session, expiry
@pool.set session_id.public_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)
generate_sid unless options[:drop]
end
end
Expand Down
8 changes: 4 additions & 4 deletions lib/rack/session/pool.rb
Expand Up @@ -45,24 +45,24 @@ def generate_sid

def find_session(req, sid)
with_lock(req) do
unless sid and session = @pool[sid]
unless !sid.nil? and session = @pool[sid.public_id]
sid, session = generate_sid, {}
@pool.store sid, session
@pool.store sid.public_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.public_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)
if options[:drop]
NullSessionId.new
else
Expand Down
2 changes: 1 addition & 1 deletion test/spec_session_abstract_id.rb
Expand Up @@ -27,7 +27,7 @@ def hex(*args)
end
end
id = Rack::Session::Abstract::ID.new nil, secure_random: secure_random.new
id.send(:generate_sid).must_equal 'fake_hex'
id.send(:generate_sid).public_id.must_equal 'fake_hex'
end

end

0 comments on commit cc1d162

Please sign in to comment.