Ensure HTTPCookie domain matching conforms to RFC6265 #1630
Conversation
Fix an issue when HTTPCookie domain starts with ".", cookies are not being sent with HTTP requests. Also, force domain matching to be in all lower case.
Foundation/HTTPCookieStorage.swift
Outdated
| // domain string is a %x2E (".") character. | ||
| // * The string is a host name (i.e., not an IP address). | ||
|
|
||
| let dotlessDomain = domain.first == "." ? String(domain.suffix(domain.count - 1)) : domain |
There was a problem hiding this comment.
Perhaps domain.hasPrefix(".") ? String(domain.dropFirst()) : domain?
You may also wish to save the result of testing for the prefix since you do use it twice, or refactor to use it once (this can save you from allocating another String too):
guard domain.hasPrefix(".") else { return host == domain }
return host == domain.dropFirst() || host.hasSuffix(domain)There was a problem hiding this comment.
That's a great suggestion. I have updated the code and will push shortly. I also updated unit tests a bit as part of this change.
Thanks!
- Also update unit tests to be more accurate in testing domain matching
|
@swift-ci please test |
|
Hi. Could this please be merged in? I'd really like to have this in the next release (4.1.3?). Thanks! |
|
Cc'ing a few more people... @ianpartridge @pushkarnk |
|
LGTM, @mamabusi ? |
|
@mamabusi could you please review? I would really like to have this fix in 4.2 Swift release. Thanks! |
|
@maksimorlovich Sorry for the delay. |
|
@swift-ci please test and merge |
|
@maksimorlovich can you open a PR against the 4.2 branch so we can get the fix in there too? Thanks! |
|
@ianpartridge what is your process for PR's in 4.2 branch... ie, should I make fresh changes in 4.2 branch and create a PR from my fork, or do a cherry pick from master to 4.2 branch? Thanks! |
|
You should be able to open a new PR that cherry-picks 8ebf49c into |
Fix an issue when HTTPCookie domain starts with ".", cookies are
not being sent with HTTP requests. Also, force domain matching to
be in all lower case.