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.

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

Support SameSite value "None" cookie attribute #581

Merged
merged 3 commits into from
Jun 14, 2019

Conversation

RemiSirdata
Copy link
Contributor

As you may have heard Google recently announced changes coming to Chrome with the release of version 76 in July 2019. This change will directly impact the handling of 3rd party cookies.

For Chrome, set the cookie attribute “SameSite = None”, so that your cookie will be sent in the third party context. Leaving the attribute of “SameSite” unspecified would result in your cookie not being sent in case the user enables the “same-site-by-default-cookies” setting in their browser.
Set "SameSite = None" is required is this case alongside "Secure"

You can find more detailed information here:
https://tools.ietf.org/html/draft-west-cookie-incrementalism-00
https://blog.chromium.org/2019/05/improving-privacy-and-security-on-web.html

func (c *Cookie) SetSameSite(mode CookieSameSite) {
c.sameSite = mode
if mode == CookieSameSiteNoneMode {
c.SetSecure(true)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should do this by default. This might cause cookies to work if the server is HTTP only. Requiring Secure for SameSiteNone is a separate flag in Chrome that isn't enabled by default.

Copy link
Contributor Author

@RemiSirdata RemiSirdata May 28, 2019

Choose a reason for hiding this comment

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

In the specifications same-site with value "None" must have Secure parameter otherwise the cookie is ignored (https://tools.ietf.org/html/draft-west-cookie-incrementalism-00)
I hesitate between call SetSecure(true) in SetSameSite() and AppendBytes()

cookie.go L280
if c.secure {
to
if c.secure || c.sameSite == CookieSameSiteNoneMode {

I think we should force the value because the specification make it clear and developers could not be aware.

Copy link
Contributor

Choose a reason for hiding this comment

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

My two cents.
https://tools.ietf.org/html/draft-west-cookie-incrementalism-00#section-3.2 the specification is clear. What we can consider is create an extra flag in fasthttp configuration to refuse non secure SameSite None cookies. This will affect how we parse cookies, but I am not sure how it should affect writing cookies. It is really easy to force a non secure cookie with SameSite None.

Perhaps we can include a Valid() error method on cookie, who can verify the inner state of the cookie, like parse but to itself.

it is better than just refuse SetSecure(false) if SameSite=None IMHO

cookie.go Outdated Show resolved Hide resolved
@RemiSirdata
Copy link
Contributor Author

Mike West (Chrome security team) indicate default value to Lax have been suggest for 77 (September) for not approved yet, Secure bit either.

@erikdubbelboer
Copy link
Collaborator

Lets wait until Chrome changes this before we do.

@kirillDanshin
Copy link
Collaborator

Gecko going to default to Lax as well (w3ctag/design-reviews#373, mozilla.dev.platform list)

I'd like to @-mention one of primary contacts here: @mikewest, maybe the author of this idea got some comments or ideas here.

httpwg/http-extensions#788

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I agree we should set secure. There is just one more minor change and then this can get merged.

cookie.go Outdated Show resolved Hide resolved
@erikdubbelboer erikdubbelboer merged commit 9ba4cef into valyala:master Jun 14, 2019
@erikdubbelboer
Copy link
Collaborator

Thanks.

@RemiSirdata RemiSirdata deleted the add-same-site-value-none branch February 18, 2021 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants