Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/rack/session/abstract/id.rb
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ def cookie_value(data)

# 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.
# Allow subclasses to set the cookie value in a different way.

def set_cookie(request, response, cookie)
if request.cookies[@key] != cookie[:value] || cookie[:expires]
Expand Down
1 change: 1 addition & 0 deletions lib/rack/session/constants.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
module Rack
module Session
RACK_SESSION = 'rack.session'
RACK_SESSION_WAS = 'rack.session.was'
RACK_SESSION_OPTIONS = 'rack.session.options'
RACK_SESSION_UNPACKED_COOKIE_DATA = 'rack.session.unpacked_cookie_data'
end
Expand Down
16 changes: 15 additions & 1 deletion lib/rack/session/cookie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,13 @@ def unpacked_cookie_data(request)
end
end

request.set_header(k, session_data || {})
if session_data
request.set_header(RACK_SESSION_WAS, session_data.dup)
request.set_header(k, session_data)
else
request.set_header(RACK_SESSION_WAS, nil)
request.set_header(k, {})
end
end
end

Expand All @@ -253,6 +259,14 @@ def persistent_session_id!(data, sid = nil)
data
end

def set_cookie(request, response, cookie)
return if request.session.to_h == request.get_header(RACK_SESSION_WAS) &&
!request.get_header(RACK_SESSION_OPTIONS).fetch(:renew, false) &&
!cookie[:expires]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reverse the order of operations here. Only do the request.session conversion if the other two cheaper checks pass.

As mentioned in the other comment, instead of request.session.to_h, I would compare using marshal/json to avoid issues when modifying nested structures.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opt to compare using ruby object here to avoid ordering mismatch that could happen during marshal/json. I think we don't intend to modify RACK_SESSION_WAS, I hope that could work in this case. Please let me know what you think.


response.set_cookie(@key, cookie)
end

class SessionId < DelegateClass(Session::SessionId)
attr_reader :cookie_value

Expand Down
18 changes: 14 additions & 4 deletions test/spec_session_cookie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,15 @@ def decode(str); @calls << :decode; JSON.parse(str); end
response["Set-Cookie"].must_match /SameSite=None/i
end

it "does not create new cookies if cookies does not change" do
response = response_for(app: incrementor)
cookie = response["Set-Cookie"]
cookie.must_include "rack.session="
response = response_for(app: only_session_id, cookie: cookie)
cookie = response["Set-Cookie"]
cookie.must_be_nil
end

it "allows using a lambda to specify same_site option, because some browsers require different settings" do
# Details of why this might need to be set dynamically:
# https://www.chromium.org/updates/same-site/incompatible-clients
Expand All @@ -253,14 +262,16 @@ def decode(str); @calls << :decode; JSON.parse(str); end
response = response_for(app: incrementor)
cookie = response['Set-Cookie']
response = response_for(app: only_session_id, cookie: cookie)
cookie = response['Set-Cookie'] if response['Set-Cookie']
response['Set-Cookie'].must_be_nil

response.body.wont_equal ""
old_session_id = response.body

response = response_for(app: renewer, cookie: cookie)
cookie = response['Set-Cookie'] if response['Set-Cookie']
response = response_for(app: only_session_id, cookie: cookie)
new_cookie = response['Set-Cookie']
new_cookie.wont_equal cookie

response = response_for(app: only_session_id, cookie: new_cookie)

response.body.wont_equal ""
response.body.wont_equal old_session_id
Expand All @@ -276,7 +287,6 @@ def decode(str); @calls << :decode; JSON.parse(str); end
response = response_for(app: destroy_session, cookie: response)
response = response_for(app: only_session_id, cookie: response)

response.body.wont_equal ""
response.body.wont_equal old_session_id
end

Expand Down