Rack no longer unescapes cookie keys, so these tests no longer make sense. #2030
Conversation
ioquatix
force-pushed
the
fix-rack-protection-cookie
branch
from
27f5fbd to
fd0c323
Compare
ioquatix
force-pushed
the
fix-rack-protection-cookie
branch
from
fd0c323 to
40fb141
Compare
| if (k == session_key && Array(v).size > 1) || | ||
| (k != session_key && Rack::Utils.unescape(k) == session_key) | ||
| if k == session_key && Array(v).size > 1 |
There was a problem hiding this comment.
I think the point of this middleware is to deny requests with both rack.session and rack.%73ession cookie keys. If we don't unescape, we fail to detect that. Right?
There was a problem hiding this comment.
Those cookie keys don't collide in Rack either, as we don't unescape cookie keys so they are distinct keys.
There was a problem hiding this comment.
It looks okay to me, IOW, it can't collide.
- 3-1-stable: https://github.com/rack/rack/blob/3-1-stable/lib/rack/utils.rb#L238-L239
- 3-0-stable: https://github.com/rack/rack/blob/3-0-stable/lib/rack/utils.rb#L250-L251
- 2-2-stable: https://github.com/rack/rack/blob/2-2-stable/lib/rack/utils.rb#L232-L233
- 2-1-stable: https://github.com/rack/rack/blob/2-1-stable/lib/rack/utils.rb#L232-L233
This was a security bug, and was fixed in various stable branches, e.g.
- 2-1-stable fixed in rack/rack@a243510
- 2-2-stable fixed in rack/rack@5ccca47
- 3-0-stable: rack/rack@1f5763d
- 3-1-stable: same as above.
I understand this is an important issue, so feel free to do your own investigation and add appropriate tests to confirm the behaviour. From the logs, it was fixed about 4 years ago, so anything since then should be good.
There was a problem hiding this comment.
So what this change does it having the CookieTossing middleware accept more requests, but it should be safe to do so.
There was a problem hiding this comment.
Yes, the previous key collisions can no longer occur.
|
On the topic of unescaping, I guess this isn't great? |
|
That definitely looks like the same security issue. However, it's parsing |
|
I'm going with the more conservative approach to start with (9e07d4f) |
|
Would you like to rework this PR? Or should we close it? |
|
I hope to get to it |
Fixes #2017.