Only check Cookie paths when sending requests

[christhesoul]

Hey wonderful Rack::Test folks 👋

I come with little open source contribution experience, but plenty of gratitude for the folks that do and positive intent.

This PR aims to resolve a cookie issue where I believe the Rack::Test implementation differs from the cookie specification.

I've done some testing and checked the spec, in these cases it's possible to set a cookie with a path parameter different to the current path of the browser. The cookie will only be sent to the server when the browser makes a request to that path.

Even though the Set-Cookie header supports the Path attribute, the Path attribute does not provide any integrity protection because the user agent will accept an arbitrary Path attribute in a Set-Cookie header. For example, an HTTP response to a request for http://example.com/foo/bar can set a cookie with a Path attribute of "/qux".

The above is taken from https://httpwg.org/specs/rfc6265.html#rfc.section.8.6

In the current implementation of Rack::Test the same valid? method is performed when setting and sending cookies. The validity rules should differ; the path validity can be ignored when setting cookies.

This PR is an attempt to demonstrate the problem, double check on my interpretation of the spec, and propose a solution.

Thanks in advance for your time.

[jeremyevans]

Thanks for the patch! I agree with the principle of only checking paths when sending and not when receiving. Please see inline comment for a possible change to the implementation.

I'm currently traveling to RubyKaigi, but I should be able to merge and test this next week.

[christhesoul]

Hey @jeremyevans – thank you for taking a look ... but I can't see the suggested change anywhere. 🤔

I know Github has been hiccuping a bit today, so maybe it just got lost?! Happy to make some changes – I just don't know what they are!

Safe travels.

[jeremyevans]

@christhesoul Sorry about that, not sure why the inline comment didn't get submitted. I think it is best to not introduce new methods for this, and just move the path check from valid? to matches?. What are your thoughts on that? In any case, we cannot move valid? from public to private visibility, as that breaks the public API.

[christhesoul]

@jeremyevans Good idea! And the change seems much simpler as a result. I like it. Let me know what you think.

[jeremyevans]

Looks good, thanks!

[christhesoul]

My pleasure!