Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: cookie-av allows arbitrary casing #349

Closed

Conversation

gmalette
Copy link
Contributor

@gmalette gmalette commented Nov 26, 2024

According to RFC6265, cookie attributes are supposed to be ~PascalCase (Path, HttpOnly, Secure, etc). In practice, browsers are lax in their interpretation of cookie attributes and will allow arbitrary casing (path, Path, pAtH, etc).

Prior to this PR, Rack::Test::Cookie only supported lowercased cookie attributes, but this PR allows it to have any casing, making it behave closer to browsers and other cookie jars.

Python

Go

Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I agree that we should fix this. There is one issue in the lib change, and fixing this bug appears to break an existing test, which will need to be updated.

@@ -28,7 +28,7 @@ def initialize(raw, uri = nil, default_host = DEFAULT_HOST)
@raw, options = raw.split(/[;,] */n, 2)

@name, @value = parse_query(@raw, ';').to_a.first
@options = parse_query(options, ';')
@options = parse_query(options, ';').transform_keys(&:downcase)
Copy link
Contributor

Choose a reason for hiding this comment

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

rack-test supports back to Ruby 2.0, so you'll have to change this to use a backwards-compatible implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I introduced a correct fix but I'm currently installing 2.0.0p0 to confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not managed to install 2.0.0, I'm now going to hope CI runs for it

According to [RFC6265](https://httpwg.org/specs/rfc6265.html#sane-set-cookie), cookie attributes are supposed to be ~PascalCase (`Path`, `HttpOnly`, `Secure`, etc). In practice, browsers are lax in their interpretation of cookie attributes and will allow arbitrary casing (`path`, `Path`, `pAtH`, etc).

Prior to this PR, `Rack::Test::Cookie` only supported lowercased cookie attributes, but this PR allows it to have any casing, making it behave closer to browsers and other cookie jars.

https://github.com/python/cpython/blob/f0d3f10c43c9029378adba11a65b3d1287e4be32/Lib/http/cookiejar.py#L511-L512
https://cs.opensource.google/go/go/+/master:src/net/http/cookie.go;l=126-131;drc=592da0ba474b94b6eceee62b5613f1c9c1ed9c89?q=cookie&ss=go%2Fgo
@gmalette gmalette force-pushed the gm/cookie-av-allow-uppsercased-names branch from dfd71de to 1e4a44a Compare November 27, 2024 13:32
@@ -28,7 +28,7 @@ def initialize(raw, uri = nil, default_host = DEFAULT_HOST)
@raw, options = raw.split(/[;,] */n, 2)

@name, @value = parse_query(@raw, ';').to_a.first
@options = parse_query(options, ';')
@options = parse_query(options, ';').map { |k, v| [k.downcase, v] }.to_h
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess Array#to_h was added in 2.1 :(

@jeremyevans
Copy link
Contributor

Thanks for working on this and getting CI fixed. I squash merged this at 16a3c5c.

@gmalette
Copy link
Contributor Author

Thanks a lot for handling it this fast!

@gmalette gmalette deleted the gm/cookie-av-allow-uppsercased-names branch November 27, 2024 19:44
@gmalette
Copy link
Contributor Author

Would you be able to cut a release that includes this please?

@jeremyevans
Copy link
Contributor

It has been a while since the last release, so I can try to do that before the end of the year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants