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

Does round-tripping cookie names work as expected? #1865

Open
ioquatix opened this issue Apr 9, 2022 · 2 comments
Open

Does round-tripping cookie names work as expected? #1865

ioquatix opened this issue Apr 9, 2022 · 2 comments

Comments

@ioquatix
Copy link
Member

ioquatix commented Apr 9, 2022

While working through the cookie utils methods adding documentation and examples, I noticed the following:

Rack::Utils.set_cookie_header("a&b", ":)")
# => "a%26b=%3A%29"

cookies = Rack::Utils.parse_cookies_header("a%26b=%3A%29")
# => {"a%26b"=>":)"}

Rack::Utils.set_cookie_header("a%26b", ":)")
# => "a%2526b=%3A%29"

URL encoding the cookie name on the way to the client but not on the way back from the client means that if a server application tries to set the same cookie based on the incoming name, it will have it's percent escapes encoded again, never reaching a fixed point/symmetry.

Not sure if this is the desired behaviour or not. I know there is a security issue with decoding cookies which are URL encoded. Just wondering if we've made the right trade off here.

Looking at the relevant RFCs:

         CTL            =  %x00-1F / %x7F
                                ; controls
       token          = 1*<any CHAR except CTLs or separators>
       separators     = "(" | ")" | "<" | ">" | "@"
                      | "," | ";" | ":" | "\" | <">
                      | "/" | "[" | "]" | "?" | "="
                      | "{" | "}" | SP | HT
 set-cookie-header = "Set-Cookie:" SP set-cookie-string
 set-cookie-string = cookie-pair *( ";" SP cookie-av )
 cookie-pair       = cookie-name "=" cookie-value
 cookie-name       = token

Maybe rather than general URL encoded cookie key names, we should either:

  1. Reject any cookie key that doesn't match the token pattern, or
  2. Have a token specific encoded form which encodes and decodes only CTLs or separators as defined by the parser.

Sorry to get pedantic about this but I'm a little concerned that URL encoding cookie keys is probably not the right behaviour and maybe we should follow the RFCs here more closely.

@ioquatix
Copy link
Member Author

ioquatix commented Apr 9, 2022

So thinking this out a bit more, the following are valid cookie key characters:

!#$%&'*+-.^_`|~

And should not be encoded, but they are:

Rack::Utils.set_cookie_header("!#$%&'*+-.^_`|~", ":)")
# => "%21%23%24%25%26%27*%2B-.%5E_%60%7C%7E=%3A%29"

This means that if someone writes:

headers = {'set-cookie' => '~=~'}
Rack::Utils.set_cookie_header!(headers, '~', '~')
=> ["~=~", "%7E=%7E"]

This will result in the browser storing two distinct cookies. Obviously an edge case but an edge case non-the-less.

@robinetmiller
Copy link

robinetmiller commented Apr 18, 2023

Just encountered this today in a situation wherein cookie keys included # as a separator for additional namespacing.
This was fine when only created & parsed in JS, but but they are overzealously escaped when coming from Rack, breaking compatibility.

Somewhat related (though maybe should file a different issue) is the use of Rack::Utils.escape to encode the value portion. It uses URI.encode_www_form_component under the hood and produces + for whitespace.

Would Rack::Utils.escape_path instead be better? I haven't verified, but I wonder if it would align with JS encodeURI() or encodeURIComponent().

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

No branches or pull requests

2 participants