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.

Sign up for GitHub

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

Fixed set-cookie regex to allow whitespace #5408

Merged
merged 5 commits into from
Jan 3, 2018
Merged

Fixed set-cookie regex to allow whitespace #5408

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea94774
Fixed set-cookie regex to allow whitespace
bararchy Dec 19, 2017
72fc8fa
Changed \s? to \s* as per request
bararchy Dec 19, 2017
28ddc59
added specs
bararchy Jan 3, 2018
1df80d2
fixed missing do after it
bararchy Jan 3, 2018
35b1789
formatted
bararchy Jan 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/http/cookie.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ module HTTP
end

CookieString = /(?:^|; )#{Regex::CookiePair}/
SetCookieString = /^#{Regex::CookiePair}(?:; #{Regex::CookieAV})*$/
SetCookieString = /^#{Regex::CookiePair}(?:;\s?#{Regex::CookieAV})*$/
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

@straight-shoota straight-shoota Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the regex addition should be changed to \s*.

Copy link
Member

@jhass jhass Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I would put a specific limit like I suggested above, * tends to be dangerous or at least slow when fed untrusted input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so \s{0,256} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@straight-shoota ? @RX14 ? @Sija ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

We enforce a 16KiB length limit on parsed HTTP headers, so we should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So... merge?

Copy link
Member

Choose a reason for hiding this comment

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

No, change to \s*


def parse_cookies(header)
header.scan(CookieString).each do |pair|
Expand Down