fix(http/cookies): allow spaces to be present in the cookie value #14455
Conversation
anton7c3
force-pushed
the
master
branch
2 times, most recently
from
eeb056a
to
ebce743
Compare
|
Looks good to me and fixes the current issue I'm seeing (#14435) |
| it "with space" do | ||
| cookie = parse_set_cookie("key=value; path=/test") | ||
| parse_set_cookie("key=value;path=/test").should eq cookie | ||
| parse_set_cookie("key=value; \t\npath=/test").should eq cookie |
There was a problem hiding this comment.
If I'm not mistaken, there's a test case missing here. A value with leading and trailing whitespace. From RFC 6265:
- Remove any leading or trailing WSP characters from the name
string and the value string.
parse_set_cookie("key= \tvalue \t; \t\npath=/test").should eq cookieNote that the test case is not currently passing.
There was a problem hiding this comment.
The solution I can propose is to add \s* around CookieName and CookieValue regexes in the CookiePair regex and then strip the name and the value before passing it to the Cookie constructor.
The changes are uploaded with your example added to the spec.
There was a problem hiding this comment.
Changed logic a little based on @straight-shoota's comment. Now only unquoted values are stripped.
anton7c3
force-pushed
the
master
branch
3 times, most recently
from
c748b0d
to
912f1c1
Compare
src/http/cookie.cr
Outdated
| # Unwrap quoted cookie value | ||
| value = value.byte_slice(1, value.bytesize - 2) | ||
| end | ||
| yield Cookie.new(pair["name"], value) | ||
| yield Cookie.new(pair["name"].strip, value.strip) |
There was a problem hiding this comment.
Stripping leading and trailing spaces should be done only for unquoted names/values.
ditto below
There was a problem hiding this comment.
The RFC 6265 mentioned above doesn't mention quotes, It states that the name and the value should be stripped, that's it.
I don't mind any solution, but let's come to a single decision.
There was a problem hiding this comment.
I wonder how other languages/frameworks do it.
There was a problem hiding this comment.
IIUC, leading or trailing whitespace can only exist when the value is quoted. On unquoted values, as well as name (which is never quoted) all extra whitespace should be matched by \s* and thus is not part of the capture group.
So I don't think stripping makes any sense here.
If the value is quote, it's probably intended to have leading or trailing whitespace (for whatever reason 🤷).
There was a problem hiding this comment.
Reverting the change in this line doesn't break any specs. So if it was preserved, we'd need to add some test for it.
But I think it should be removed.
There was a problem hiding this comment.
I suppose \s* can miss some whitespace on unquoted values because a blank also matches CookieOctet and the quantifiers are both greedy. So we might need to strip trailing whitespace off of an unquoted value, unless we can fix the quantifier greediness in regex.
There was a problem hiding this comment.
I've changed the logic to strip only unquoted values.This way the following works:
key= value ; path=/testshould be parsed as<HTTP::Cookie @name="key" @value="value">key=" value "; path=/testshould be parsed as<HTTP::Cookie @name="key" @value=" value ">
The specs for this case are also added.
Fixes crystal-lang#14435. Move parse_set_cookie example to the correct location.
Fixes #14435.
Move parse_set_cookie example to the correct location.