@jeremyevans I'm not sure I completely follow your feedback.

#2188 which is not merged, implements symmetric escape/unescape except where it's a known security issue.

This PR adds a 2nd commit on top of that, which removes escape/unescape entirely, for the reasons outlined. I don't think the compatibility issue will be huge (no more than others we accept in minor releases), but I am okay with merging #2188 and as a follow up we can consider merging this PR.

I guess I expect more compatibility issues than you do. My expectation is there are users relying on the current behavior, and rejecting cookie keys previously not rejected will break their apps. When we made the change to not to unescape cookies due to the security issues, we broke a significant amount of apps, and this change seems as likely to cause issues.

Maybe the PR description makes sense as a commit message to the second commit. But in terms of the whole PR, this doesn't deal with unescaping.

I guess I expect more compatibility issues than you do. My expectation is there are users relying on the current behavior, and rejecting cookie keys previously not rejected will break their apps. When we made the change to not to unescape cookies due to the security issues, we broke a significant amount of apps, and this change seems as likely to cause issues.

I agree, it may. Do you have any reference to the previous issues we encountered?

Looks like cookie keys have always been escaped, dating to the initial import (7ed819a).

Unescaping of keys was the cause of CVE-2020-8184 (1f5763d).

I think that's why we are in the situation we are currently in, where keys are escaped when serializing but not unescaped when deserializing. Users do try to use cookie key names that require escaping (e.g. #1674). Since Rack's introduction, that worked fine because Rack did the escaping. If Rack stops doing escaping, and instead raises an exception, such code will break.

Note that the validation regexp used here I don't think is complete. It doesn't validate % escapes, for example, so it will treat invalid escapes in cookie keys as valid.

Escaping cookie keys is a rack invention, it's not standard and there is no standard interpretation of percent escapes for the cookie key except for the raw strings. The escaped cookie keys will show up in document.cookies in the escaped form. According to TOKEN, % is not a special character, so there is no reason it can't be part of the cookie key.

Example server:

run do |env|
	[200, {'set-cookie' => '%66oo=bar'}, ["Hello World!"]]
end

Browser shows:

And on subsequent requests, we see:

 "HTTP_COOKIE"=>"%66oo=bar",

Rack should never have escaped cookie keys - it's non-standard and the un-escaped name won't match subsequent cookie header keys.

Finally, without symmetry, I simply don't understand how anyone is supposed to use this API...

e.g. for a cookie key that will be escaped, how can this interface work???

KEY = 'my cookie'

run do |env|
	request = Rack::Request.new(env)
	puts "My Cookie: #{request.cookies[KEY]}" # always empty!
	puts "All Cookies: #{request.cookies}"
	
	headers = {}
	Rack::Utils.set_cookie_header!(headers, KEY, 'bar')
	
	[200, headers, ["Hello World!"]]
end

Thanks for that information. I'm OK with a deprecation warning for keys that need escaping in Rack 3.1, and removal in Rack 3.2.

That seems reasonable to me.

Closing in favour of #2191.