Fixed set-cookie regex to allow whitespace #5408
Conversation
Add an optional whitespace to set-cookie regex
src/http/cookie.cr
Outdated
| @@ -81,7 +81,7 @@ module HTTP | |||
| end | |||
|
|
|||
| CookieString = /(?:^|; )#{Regex::CookiePair}/ | |||
| SetCookieString = /^#{Regex::CookiePair}(?:; #{Regex::CookieAV})*$/ | |||
| SetCookieString = /^#{Regex::CookiePair}(?:;\s?#{Regex::CookieAV})*$/ | |||
There was a problem hiding this comment.
Can anybody check the RFC whether zero or more or zero or one are allowed? In the case of zero or more this should probably use something like {0,256} or so rather than ?
There was a problem hiding this comment.
According to RFC 2965, white space is permitted between tokens. It is not explicit if it means "one" or "multiple" but I'd assume withour further specifictation it is to be understood as "any number".
Whitespaces are also allowed in other places, for example between attribute and = (this is explicitly mentioned in the RFC). Also the date formats like the one described by RFC 1123 actually allow arbitrary number of whitespace between the tokens.
So, there are several issues with this cookie string parser. This PR is probably good for a quick fix, but the entire parse will need to be refactored, probably using a custom parser instead of regular expressions.
There was a problem hiding this comment.
So the regex addition should be changed to \s*.
There was a problem hiding this comment.
I would put a specific limit like I suggested above, * tends to be dangerous or at least slow when fed untrusted input.
There was a problem hiding this comment.
@straight-shoota ? @RX14 ? @Sija ?
There was a problem hiding this comment.
I don't think it matters, really. If it was up to me, I'd choose \s* because it's simpler and the entire regex parser is not "safe" and needs an oberhaul anyway.
There was a problem hiding this comment.
We enforce a 16KiB length limit on parsed HTTP headers, so we should be fine.
|
@straight-shoota Is this fine from your side now? |
|
I'll admit to not looking, but isn't this feature already covered with specs? Of it is, they pass, if not, then we didn't lose anything and I can add some specs anytime. |
|
Why don't you just add a simple spec to Something like this should 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 |
|
@straight-shoota Will do, thanks |
|
@straight-shoota @jhass @RX14 @asterite |
|
@bararchy you are missing a |
|
@asterite good catch, never edit on github : |
|
Needs a format. |
|
@RX14 You mean run it with crystal format tool? |
|
@RX14 Done. |
|
Thanks! |
* Fixed set-cookie regex to allow whitespace Add an optional whitespace to set-cookie regex * Changed \s? to \s* as per request * added specs * fixed missing do after it * formatted
Add an optional whitespace to set-cookie regex
This is a fix for: #5407