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.

Sign up for GitHub

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
Merged

Support SameSite value "None" cookie attribute #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dd0296
Support SameSite value "None" cookie attribute
molz May 28, 2019
1bd32b1
Fix typo in CookieSameSiteNoneMode comment
RemiSirdata May 28, 2019
6a8c504
fix comment for SameSite None
molz Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions cookie.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ const (
CookieSameSiteLaxMode
// CookieSameSiteStrictMode sets the SameSite flag with the "Strict" parameter
CookieSameSiteStrictMode
// CookieSameSiteNoneMode sets the SameSite flag with the "None" parameter
// see https://tools.ietf.org/html/draft-west-cookie-incrementalism-00
CookieSameSiteNoneMode
)

// AcquireCookie returns an empty Cookie object from the pool.
Expand Down Expand Up @@ -119,8 +122,12 @@ func (c *Cookie) SameSite() CookieSameSite {
}

// SetSameSite sets the cookie's SameSite flag to the given value.
// set value CookieSameSiteNoneMode will set Secure to true also to avoid browser rejection
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 •
edited
Loading

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

}

// Path returns cookie path.
Expand Down Expand Up @@ -288,6 +295,11 @@ func (c *Cookie) AppendBytes(dst []byte) []byte {
dst = append(dst, strCookieSameSite...)
dst = append(dst, '=')
dst = append(dst, strCookieSameSiteStrict...)
case CookieSameSiteNoneMode:
dst = append(dst, ';', ' ')
dst = append(dst, strCookieSameSite...)
dst = append(dst, '=')
dst = append(dst, strCookieSameSiteNone...)
}
return dst
}
Expand Down Expand Up @@ -386,6 +398,10 @@ func (c *Cookie) ParseBytes(src []byte) error {
if caseInsensitiveCompare(strCookieSameSiteStrict, kv.value) {
c.sameSite = CookieSameSiteStrictMode
}
case 'n': // "none"
if caseInsensitiveCompare(strCookieSameSiteNone, kv.value) {
c.sameSite = CookieSameSiteNoneMode
}
}
}
}
Expand Down
23 changes: 23 additions & 0 deletions cookie_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,29 @@ func TestCookieSameSite(t *testing.T) {
t.Fatalf("missing SameSite flag in cookie %q", s)
}

if err := c.Parse("foo=bar; samesite=none"); err != nil {
t.Fatalf("unexpected error: %s", err)
}
if c.SameSite() != CookieSameSiteNoneMode {
t.Fatalf("SameSite None Mode must be set")
}
s = c.String()
if !strings.Contains(s, "; SameSite=None") {
t.Fatalf("missing SameSite flag in cookie %q", s)
}

if err := c.Parse("foo=bar"); err != nil {
t.Fatalf("unexpected error: %s", err)
}
c.SetSameSite(CookieSameSiteNoneMode)
s = c.String()
if !strings.Contains(s, "; SameSite=None") {
t.Fatalf("missing SameSite flag in cookie %q", s)
}
if !strings.Contains(s, "; secure") {
t.Fatalf("missing Secure flag in cookie %q", s)
}

if err := c.Parse("foo=bar"); err != nil {
t.Fatalf("unexpected error: %s", err)
}
Expand Down
1 change: 1 addition & 0 deletions strings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ var (
strCookieSameSite = []byte("SameSite")
strCookieSameSiteLax = []byte("Lax")
strCookieSameSiteStrict = []byte("Strict")
strCookieSameSiteNone = []byte("None")

strClose = []byte("close")
strGzip = []byte("gzip")
Expand Down