-
Notifications
You must be signed in to change notification settings - Fork 8k
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
Added support for SameSite cookie flag #1615
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1615 +/- ##
==========================================
+ Coverage 98.5% 98.51% +0.01%
==========================================
Files 40 41 +1
Lines 2267 2287 +20
==========================================
+ Hits 2233 2253 +20
Misses 18 18
Partials 16 16
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Samesite flag only support on Golang v1.11
Is there any chance that we could see this merged? There's several instances in codebases that I maintain where the ability to use this now that Go 1.11 is out would be massively useful. |
@anio sameSite mode only support on go 1.11 version. https://golang.org/pkg/net/http/#Cookie type Cookie struct {
Name string
Value string
Path string // optional
Domain string // optional
Expires time.Time // optional
RawExpires string // for reading cookies only
// MaxAge=0 means no 'Max-Age' attribute specified.
// MaxAge<0 means delete cookie now, equivalently 'Max-Age: 0'
// MaxAge>0 means Max-Age attribute present and given in seconds
MaxAge int
Secure bool
HttpOnly bool
SameSite SameSite // Go 1.11
Raw string
Unparsed []string // Raw text of unparsed attribute-value pairs
} |
Could this issue get some more attention? Chrome 80 with same-site cookies by default is shipping in less than a month: https://chromiumdash.appspot.com/schedule |
An earlier comment in this thread that is a request for changes blocking the merge:
The README already states that this project requires Go 1.11+. So I don't really see why this shouldn't be merged 🤔 ?
|
@appleboy I have fixed, please review, thanks! |
* Added support for SameSite cookie flag * fixed tests. * Update context.go * Update context_test.go * Update context_test.go Co-authored-by: thinkerou <[email protected]> Co-authored-by: Bo-Yi Wu <[email protected]>
do we have any strong reason to break back compatibility? |
* Added support for SameSite cookie flag * fixed tests. * Update context.go * Update context_test.go * Update context_test.go Co-authored-by: thinkerou <[email protected]> Co-authored-by: Bo-Yi Wu <[email protected]>
Read more at:
https://golang.org/pkg/net/http/#Cookie