Default cookie's SameSite attribute to lax #2495
Conversation
| @@ -166,14 +166,16 @@ public struct HTTPCookies: ExpressibleByDictionaryLiteral { | |||
| isHTTPOnly: Bool = false, | |||
| sameSite: SameSitePolicy? = nil | |||
| ) { | |||
| let sameSitePolicy = sameSite ?? .lax | |||
There was a problem hiding this comment.
We don't need this line, we can change the default parameter on the line above
| @@ -139,7 +139,7 @@ public struct HTTPCookies: ExpressibleByDictionaryLiteral { | |||
| /// A cookie which can only be sent in requests originating from the same origin as the target domain. | |||
| /// | |||
| /// This restriction mitigates attacks such as cross-site request forgery (XSRF). | |||
| public var sameSite: SameSitePolicy? | |||
| public var sameSite: SameSitePolicy | |||
There was a problem hiding this comment.
Making this non-optional is a breaking-change since anyone looking at this would have compiler errors after updating. However, I think we can still work around this. Leave this as optional but in the initialiser it will default to lax. We should also still give people the option to set it to nil as well
| self.string = string | ||
| self.expires = expires | ||
| self.maxAge = maxAge | ||
| self.domain = domain | ||
| self.path = path | ||
| self.isSecure = isSecure | ||
| self.isSecure = isSecure || sameSitePolicy == .none //samesite none requires secure attribute to be set |
There was a problem hiding this comment.
This should be noted in the comment block for the initialiser since it might catch some people out. If you could link to the same site spec where this is mandated in the new comment that would be great
Also, small nit, place the comment on the line above rather than at the end of the line.
There was a problem hiding this comment.
I read IETF draft but could not find description about the relation of SameSite=None and Secure
https://tools.ietf.org/id/draft-ietf-httpbis-rfc6265bis-03.html
That can be seen in Mozilla's documentation so I added link for this page.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite
| @@ -84,7 +84,7 @@ struct HTTPSetCookie { | |||
| guard let parameter = directive.parameter else { | |||
| return nil | |||
| } | |||
| self.value.sameSite = HTTPCookies.SameSitePolicy(rawValue: .init(parameter)) | |||
| self.value.sameSite = HTTPCookies.SameSitePolicy(rawValue: .init(parameter)) ?? .lax | |||
There was a problem hiding this comment.
Again, I think we should still give people the option to set it to nil, and if we default to lax in the initialiser that should fix the underlying issue
|
@0xTim
I added default |
|
PS I'll merge this as soon as that's done and CI is green |
|
@0xTim |
0xTim
changed the title
|
These changes are now available in 4.29.3 |
Set a cookie's default
SameSiteattribute tolax.This prevents warnings in browsers and stops functionality working when following redirects, cross site link or when browsers assume the attribute to be
None, which requiresSecureattribute(HTTPS only).#2495