Fix ActionController::TestSession#id to return Rack::Session::SessionId instance #38063
Conversation
There was a problem hiding this comment.
On 2a52a38#commitcomment-36521253, I commented exactly that ActionController::TestSession.initialize should be updated to use Rack::Session::SessionId for its @id assignment. Happy to see this PR 👍
| @@ -176,12 +176,12 @@ class LiveTestResponse < Live::Response | |||
|
|
|||
| # Methods #destroy and #load! are overridden to avoid calling methods on the | |||
| # @store object, which does not exist for the TestSession class. | |||
| class TestSession < Rack::Session::Abstract::SessionHash #:nodoc: | |||
| class TestSession < Rack::Session::Abstract::PersistedSecure::SecureSessionHash #:nodoc: | |||
There was a problem hiding this comment.
I am not sure why the superclass has to change. Can you please explain?
There was a problem hiding this comment.
Rack::Session::Abstract::PersistedSecure uses Rack::Session::Abstract::PersistedSecure::SecureSessionHash instead of Rack::Session::Abstract::SessionHash. Therefore, I changed the super class of TestSession to use SecureSessionHash as well.
The difference between SessionHash and SecureSessionHash is the behavior when accessing ["session_id"]. In the case of SecureSessionHash, the public_id of @id is returned.
|
Not relevant before merging, but: Don’t forget to backport this change to 5-2-stable too. |
Co-Authored-By: Benjamin Quorning <22333+bquorning@users.noreply.github.com>
The `ActionDispatch::Session::MemcacheStore` is still vulnerable given it requires the gem dalli to be updated as well. CVE-2019-16782
|
|
||
| def test_session_id | ||
| session = ActionController::TestSession.new | ||
| assert_instance_of String, session.id.public_id |
There was a problem hiding this comment.
Should we test session["session_id"]?
There was a problem hiding this comment.
I added test for session["session_id"]
…Id instance (#38063) * Fix ActionController::TestSession#id to return Rack::Session::SessionId instance * test SessionId#public_id * test session["session_id"] Co-authored-by: Benjamin Quorning <22333+bquorning@users.noreply.github.com>
…Id instance (#38063) * Fix ActionController::TestSession#id to return Rack::Session::SessionId instance * test SessionId#public_id * test session["session_id"] Co-authored-by: Benjamin Quorning <22333+bquorning@users.noreply.github.com>
|
Just ran into this in Rails 6.0.2.1. Can this be merged into |
|
It is in 6-0-stable. |
NOTE: The upgrade is backwards compatible with existing sessions, but the upgrade of redis-rack v2.1.2 changed Redis keys from `session:gitlab:<random hex value>` to `session:gitlab:2::<hash of hex value>`. If a session does not have a key in the new schema, it will be created transparently. The old session key will eventually be expired automatically. To upgrade to rack 2.0.9, we need to do the following: 1. Fix ActiveSession to use new Rack::Session::SessionId 2. Add a monkey patch for ActionController::TestSessionPatch Controller tests were failing without the changes in rails/rails#38063, which is available on the Rails `6-0-stable` branch but not in Rails 6.0.2.2. 3. Remove CGI escaping of ActiveSession keys. This was not needed because CGI escaping was already being done by Rails. 4. Fix deletion of Rack session keys with ActiveSession redis-rack v2.1.2 changed the session key from one based on the public ID to the private ID. We need to adapt ActiveSession to delete both versions of the key to clear out old data and to make it work with the redis-rack key name changes.
|
@rafaelfranca You merged/backported this into the Do you know why this might be? (Apologies if I'm missing something about the parts of For the avoidance of doubt It is present here: It is not present on any of:
|
|
@robotfelix Versions like 5.2.4.x contain security fixes only. The change in question would be included in 5.2.5, if that is ever released. For more information, see the Rails Maintenance Policy. If you need the change and cannot upgrade to Rails 6 at this time, you might consider pointing your app's |
|
@jonathanhefner Thanks for the clarification. I've now seen the same policy being applied to 892eab7 and am happily sitting on I've opened #41523 to try to help avoid other people running into the same problem in future, and #41524 to try making the Rails Maintenance Policy more explicit about this kind of bug-fix-to-security-patch scenario (I'm a little hesitant to call this a bug fix as that implies that leaving this change out of the security patch wasn't a conscious decision!) |
NOTE: The upgrade is backwards compatible with existing sessions, but the upgrade of redis-rack v2.1.2 changed Redis keys from `session:gitlab:<random hex value>` to `session:gitlab:2::<hash of hex value>`. If a session does not have a key in the new schema, it will be created transparently. The old session key will eventually be expired automatically. To upgrade to rack 2.0.9, we need to do the following: 1. Fix ActiveSession to use new Rack::Session::SessionId 2. Add a monkey patch for ActionController::TestSessionPatch Controller tests were failing without the changes in rails/rails#38063, which is available on the Rails `6-0-stable` branch but not in Rails 6.0.2.2. 3. Remove CGI escaping of ActiveSession keys. This was not needed because CGI escaping was already being done by Rails. 4. Fix deletion of Rack session keys with ActiveSession redis-rack v2.1.2 changed the session key from one based on the public ID to the private ID. We need to adapt ActiveSession to delete both versions of the key to clear out old data and to make it work with the redis-rack key name changes.
When testing with
ActionController::TestCase,session.idreturns a string value. With the update to rack v2.0.8,session.idnow returns an instance ofRack::Session::SessionId. Therefore, fixsession.idto return an instance ofRack::Session::SessionIdwhen testing withActionController::TestCase.