Do not return new cookies when session isn’t changed#53
Do not return new cookies when session isn’t changed#53
Conversation
hieuk09
force-pushed
the
bug/fix-encryption
branch
from
9ef8469 to
06ff52c
Compare
|
This looks okay to me. It seems robust to compare with the original value. @jeremyevans do you have any opinion on this? |
|
For reference, I did something like this in the past: https://github.com/socketry/utopia/blob/main/lib/utopia/session/lazy_hash.rb Tracking whether any changes were made. But if you changed it to a new value and then back to the original, my code would not pick that up, but this PR would. |
ioquatix
force-pushed
the
bug/fix-encryption
branch
from
06ff52c to
b086a5b
Compare
jeremyevans
left a comment
jeremyevans
left a comment
There was a problem hiding this comment.
Here are my thoughts from a brief review. Note that I don't follow rack-session development, so this is not a particularly informed review.
lib/rack/session/cookie.rb
Outdated
| return if request.session.to_h == request.get_header(RACK_SESSION_WAS) && | ||
| !request.get_header(RACK_SESSION_OPTIONS).fetch(:renew, false) && | ||
| !cookie[:expires] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@jeremyevans Thanks for the review. I have updated the code in 445c2d1 |
3 participants
Close #52