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

net/http: support Cookie "SameSite" attribute #15867

Closed
ghost opened this issue May 28, 2016 · 21 comments
Closed

net/http: support Cookie "SameSite" attribute #15867

ghost opened this issue May 28, 2016 · 21 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ghost
Copy link

ghost commented May 28, 2016

https://tools.ietf.org/html/draft-west-first-party-cookies-07
https://www.chromestatus.com/feature/4672634709082112

@hkeide
Copy link

hkeide commented Jun 4, 2016

Since this is already supported in Chrome 51, and it won't be in Go for a while yet, here is a simple workaround (tested to work in Chrome 51):

cs := w.Header().Get("Set-Cookie")
cs += "; SameSite=lax"
w.Header().Set("Set-Cookie", cs)

@quentinmit
Copy link
Contributor

/cc @bradfitz @adg

@quentinmit quentinmit added this to the Unplanned milestone Jun 17, 2016
@bradfitz bradfitz changed the title http/cookie: support same-site attribute net/http: support Cookie "SameSite" attribute Jun 25, 2016
@bradfitz
Copy link
Contributor

This seems trivial, but it also seems like we should wait until there's more web consensus. Chrome can pull or modify support, but our Go 1 compatibility promise is stronger. It would be unfortunate if we added a SameSite bool field to net/http.Cookie and then they renamed it yet again before it became fully standardized.

@kardianos
Copy link
Contributor

SameSite would probably need to be an const (Strict, Lax).

Would it make sense to serialize Cookie.Unparsed into the cookie string? Then I can just set Unparsed: []string {"SameSite=Strict"},.

@JaredWilliams
Copy link

Alternative hack allowing for multiple cookies.

type SameSite string

const (
    SameSiteLax    SameSite = "lax"
    SameSiteStrict          = "strict"
)

func SetCookie(w http.ResponseWriter, cookie *http.Cookie, sameSite SameSite) {
    if v := cookie.String(); v != "" {
        switch sameSite {
        case SameSiteLax:
            v = v + "; SameSite=lax"
        case SameSiteStrict:
            v = v + "; SameSite=strict"
        }
        w.Header().Add("Set-Cookie", v)
    }
}

@reedloden
Copy link

Keep in mind there are actually three different possible values here. SameSite, SameSite=Lax, and SameSite=Strict. SameSite without a value means the same as SameSite=Strict.

Updated spec: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00

reedloden added a commit to reedloden/go that referenced this issue Apr 19, 2017
The same-site cookie attribute prevents a cookie from being sent along with
cross-site requests. The main goal is mitigate the risk of cross-origin
information leakage and provides some protection against cross-site request
forgery attacks.

Spec: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00

Fixes golang#15867

XXX: Write tests.
@akyoto
Copy link
Contributor

akyoto commented Jun 17, 2017

It's been a year now and the name hasn't changed.
I believe it should be added to the standard lib.

@mbyczkowski
Copy link
Contributor

It's still only supported by Chrome and Opera and the IETF draft (both, orignal and the updated spec) has expired last year. ¯\_(ツ)_/¯

srenatus added a commit to srenatus/scs that referenced this issue Nov 27, 2017
Note that upstream support is still pending [0] (and even when merged, this
will require the latest Go version). So this adds a workaround to allow
for setting SameSite on the cookies used, as per the draft [1].

Also changes the options_test TestCookieOptions calls to `t.Errorf`,
since this will run all checks even if one fails. (`t.Fatal[f]` would
stop test execution.)

[0]: golang/go#15867
[1]: https://tools.ietf.org/html/draft-west-first-party-cookies-07

Signed-off-by: Stephan Renatus <srenatus@chef.io>
@gopherbot
Copy link

Change https://golang.org/cl/79919 mentions this issue: net/http: Add support for SameSite cookie option

@kardianos
Copy link
Contributor

I had to remove the samesite cookie attribute even for chrome due to https://bugs.chromium.org/p/chromium/issues/detail?id=626245 .

Until bugs like this are resolved in chrome, I don't see any viable implementation. Not one, zero.

@srenatus
Copy link
Contributor

srenatus commented Nov 27, 2017

@kardianos oh that's a bummer. Maybe we should close the change then.

@kardianos
Copy link
Contributor

@srenatus You can leave it open and put R=go1.11 in a comment line so it won't show up on a dashboard until then. If it does become a standard, the bug will eventually get fixed. It just might not be ready now. Go can't accept the change for now anyway as it is in a freeze.

@srenatus
Copy link
Contributor

@kardianos done -- thanks 😃

@agnivade
Copy link
Contributor

ping on this.

OAWSP site says

As of November 2017 the SameSite attribute is implemented in Chrome, Firefox, and Opera.

caniuse.com is still pretty red but FF and Chrome support is there.

@andybons
Copy link
Member

60% usage globally is still pretty low given that Chrome is much more cavalier about removing features.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 14, 2018
@bradfitz
Copy link
Contributor

Still not in Edge or Safari, and still only a draft standard. I still think we should wait. There's no rush and people can't use string concatenation in the meantime.

@reedloden
Copy link

Firefox just announced support for this

https://blog.mozilla.org/security/2018/04/24/same-site-cookies-in-firefox-60/

@bradrydzewski
Copy link

FYI this shipped in Edge 18 and in the Edge 17 June security patch
https://developer.microsoft.com/en-us/microsoft-edge/platform/status/samesitecookies/

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 9, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 9, 2018
@bradfitz
Copy link
Contributor

bradfitz commented Jul 9, 2018

If somebody wants to send a change for this for Go 1.12, that's fine.

@agnivade
Copy link
Contributor

agnivade commented Jul 9, 2018

I believe CL 79919 is already there. Probably needs to be bumped to 1.12.

@golang golang deleted a comment from gopherbot Jul 9, 2018
@ptman
Copy link

ptman commented Aug 21, 2018

unless I'm interpreting something wrong the samesite code seems to be in 1.11, no need to wait for 1.12

@golang golang locked and limited conversation to collaborators Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests