From 83ccad64a7b2c4d06f740a3d419e4acbae9033bf Mon Sep 17 00:00:00 2001 From: Niels Widger Date: Fri, 28 Sep 2018 07:53:49 -0400 Subject: [PATCH 1/2] Set http.Cookie's SameSite field in NewCookie for Go 1.11 or later Set the returned http.Cookie's SameSite field to the value of the SameSite field in the Options struct passed into NewCookie. This fixes an oversight made in PR #165 which was made to address issue Add a newCookieFromOptions function which takes a name, value and Options struct and returns an http.Cookie. There are two newCookieFromOptions implementations, one for Go 1.11 and later which sets SameSite and one for earlier Go versions which does not. Add new tests TestNewCookieFromOptions and TestNewCookieFromOptionsSameSite which ensure that the values passed to newCookieFromOptions are properly set in the returned cookie. The test TestNewCookieFromOptionsSameSite only runs if running Go 1.11 and later. --- cookie.go | 19 ++++++++++++++ cookie_go111.go | 20 +++++++++++++++ cookie_go111_test.go | 32 +++++++++++++++++++++++ cookie_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++++ sessions.go | 10 +------- 5 files changed, 132 insertions(+), 9 deletions(-) create mode 100644 cookie.go create mode 100644 cookie_go111.go create mode 100644 cookie_go111_test.go create mode 100644 cookie_test.go diff --git a/cookie.go b/cookie.go new file mode 100644 index 0000000..1928b04 --- /dev/null +++ b/cookie.go @@ -0,0 +1,19 @@ +// +build !go1.11 + +package sessions + +import "net/http" + +// newCookieFromOptions returns an http.Cookie with the options set. +func newCookieFromOptions(name, value string, options *Options) *http.Cookie { + return &http.Cookie{ + Name: name, + Value: value, + Path: options.Path, + Domain: options.Domain, + MaxAge: options.MaxAge, + Secure: options.Secure, + HttpOnly: options.HttpOnly, + } + +} diff --git a/cookie_go111.go b/cookie_go111.go new file mode 100644 index 0000000..173d1a3 --- /dev/null +++ b/cookie_go111.go @@ -0,0 +1,20 @@ +// +build go1.11 + +package sessions + +import "net/http" + +// newCookieFromOptions returns an http.Cookie with the options set. +func newCookieFromOptions(name, value string, options *Options) *http.Cookie { + return &http.Cookie{ + Name: name, + Value: value, + Path: options.Path, + Domain: options.Domain, + MaxAge: options.MaxAge, + Secure: options.Secure, + HttpOnly: options.HttpOnly, + SameSite: options.SameSite, + } + +} diff --git a/cookie_go111_test.go b/cookie_go111_test.go new file mode 100644 index 0000000..de3953f --- /dev/null +++ b/cookie_go111_test.go @@ -0,0 +1,32 @@ +// +build go1.11 + +package sessions + +import ( + "net/http" + "strconv" + "testing" +) + +// Test for setting SameSite field in new http.Cookie from name, value +// and options +func TestNewCookieFromOptionsSameSite(t *testing.T) { + tests := []struct { + sameSite http.SameSite + }{ + {http.SameSiteDefaultMode}, + {http.SameSiteLaxMode}, + {http.SameSiteStrictMode}, + } + for i, v := range tests { + t.Run(strconv.Itoa(i+1), func(t *testing.T) { + options := &Options{ + SameSite: v.sameSite, + } + cookie := newCookieFromOptions("", "", options) + if cookie.SameSite != v.sameSite { + t.Fatalf("bad cookie sameSite: got %v, want %v", cookie.SameSite, v.sameSite) + } + }) + } +} diff --git a/cookie_test.go b/cookie_test.go new file mode 100644 index 0000000..0e10ab3 --- /dev/null +++ b/cookie_test.go @@ -0,0 +1,60 @@ +package sessions + +import ( + "strconv" + "testing" +) + +// Test for creating new http.Cookie from name, value and options +func TestNewCookieFromOptions(t *testing.T) { + tests := []struct { + name string + value string + path string + domain string + maxAge int + secure bool + httpOnly bool + }{ + {"", "bar", "/foo/bar", "foo.example.com", 3600, true, true}, + {"foo", "", "/foo/bar", "foo.example.com", 3600, true, true}, + {"foo", "bar", "", "foo.example.com", 3600, true, true}, + {"foo", "bar", "/foo/bar", "", 3600, true, true}, + {"foo", "bar", "/foo/bar", "foo.example.com", 0, true, true}, + {"foo", "bar", "/foo/bar", "foo.example.com", 3600, false, true}, + {"foo", "bar", "/foo/bar", "foo.example.com", 3600, true, false}, + } + for i, v := range tests { + t.Run(strconv.Itoa(i+1), func(t *testing.T) { + options := &Options{ + Path: v.path, + Domain: v.domain, + MaxAge: v.maxAge, + Secure: v.secure, + HttpOnly: v.httpOnly, + } + cookie := newCookieFromOptions(v.name, v.value, options) + if cookie.Name != v.name { + t.Fatalf("bad cookie name: got %q, want %q", cookie.Name, v.name) + } + if cookie.Value != v.value { + t.Fatalf("bad cookie value: got %q, want %q", cookie.Value, v.value) + } + if cookie.Path != v.path { + t.Fatalf("bad cookie path: got %q, want %q", cookie.Path, v.path) + } + if cookie.Domain != v.domain { + t.Fatalf("bad cookie domain: got %q, want %q", cookie.Domain, v.domain) + } + if cookie.MaxAge != v.maxAge { + t.Fatalf("bad cookie maxAge: got %q, want %q", cookie.MaxAge, v.maxAge) + } + if cookie.Secure != v.secure { + t.Fatalf("bad cookie secure: got %v, want %v", cookie.Secure, v.secure) + } + if cookie.HttpOnly != v.httpOnly { + t.Fatalf("bad cookie httpOnly: got %v, want %v", cookie.HttpOnly, v.httpOnly) + } + }) + } +} diff --git a/sessions.go b/sessions.go index 2fcdf51..a821d31 100644 --- a/sessions.go +++ b/sessions.go @@ -178,15 +178,7 @@ func Save(r *http.Request, w http.ResponseWriter) error { // the Expires field calculated based on the MaxAge value, for Internet // Explorer compatibility. func NewCookie(name, value string, options *Options) *http.Cookie { - cookie := &http.Cookie{ - Name: name, - Value: value, - Path: options.Path, - Domain: options.Domain, - MaxAge: options.MaxAge, - Secure: options.Secure, - HttpOnly: options.HttpOnly, - } + cookie := newCookieFromOptions(name, value, options) if options.MaxAge > 0 { d := time.Duration(options.MaxAge) * time.Second cookie.Expires = time.Now().Add(d) From ec3daf30786806511087856741c2f9ffee76c731 Mon Sep 17 00:00:00 2001 From: Niels Widger Date: Fri, 28 Sep 2018 08:20:16 -0400 Subject: [PATCH 2/2] Don't use t.Run in tests, not supported in earlier Go versions This package is meant to work on Go versions going back to Go 1.3, which means tests can't use testing.T.Run which doesn't exists in Go 1.6 and earlier. --- cookie_go111_test.go | 17 +++++------- cookie_test.go | 61 +++++++++++++++++++++----------------------- 2 files changed, 36 insertions(+), 42 deletions(-) diff --git a/cookie_go111_test.go b/cookie_go111_test.go index de3953f..1aac273 100644 --- a/cookie_go111_test.go +++ b/cookie_go111_test.go @@ -4,7 +4,6 @@ package sessions import ( "net/http" - "strconv" "testing" ) @@ -19,14 +18,12 @@ func TestNewCookieFromOptionsSameSite(t *testing.T) { {http.SameSiteStrictMode}, } for i, v := range tests { - t.Run(strconv.Itoa(i+1), func(t *testing.T) { - options := &Options{ - SameSite: v.sameSite, - } - cookie := newCookieFromOptions("", "", options) - if cookie.SameSite != v.sameSite { - t.Fatalf("bad cookie sameSite: got %v, want %v", cookie.SameSite, v.sameSite) - } - }) + options := &Options{ + SameSite: v.sameSite, + } + cookie := newCookieFromOptions("", "", options) + if cookie.SameSite != v.sameSite { + t.Fatalf("%v: bad cookie sameSite: got %v, want %v", i+1, cookie.SameSite, v.sameSite) + } } } diff --git a/cookie_test.go b/cookie_test.go index 0e10ab3..acb4efb 100644 --- a/cookie_test.go +++ b/cookie_test.go @@ -1,7 +1,6 @@ package sessions import ( - "strconv" "testing" ) @@ -25,36 +24,34 @@ func TestNewCookieFromOptions(t *testing.T) { {"foo", "bar", "/foo/bar", "foo.example.com", 3600, true, false}, } for i, v := range tests { - t.Run(strconv.Itoa(i+1), func(t *testing.T) { - options := &Options{ - Path: v.path, - Domain: v.domain, - MaxAge: v.maxAge, - Secure: v.secure, - HttpOnly: v.httpOnly, - } - cookie := newCookieFromOptions(v.name, v.value, options) - if cookie.Name != v.name { - t.Fatalf("bad cookie name: got %q, want %q", cookie.Name, v.name) - } - if cookie.Value != v.value { - t.Fatalf("bad cookie value: got %q, want %q", cookie.Value, v.value) - } - if cookie.Path != v.path { - t.Fatalf("bad cookie path: got %q, want %q", cookie.Path, v.path) - } - if cookie.Domain != v.domain { - t.Fatalf("bad cookie domain: got %q, want %q", cookie.Domain, v.domain) - } - if cookie.MaxAge != v.maxAge { - t.Fatalf("bad cookie maxAge: got %q, want %q", cookie.MaxAge, v.maxAge) - } - if cookie.Secure != v.secure { - t.Fatalf("bad cookie secure: got %v, want %v", cookie.Secure, v.secure) - } - if cookie.HttpOnly != v.httpOnly { - t.Fatalf("bad cookie httpOnly: got %v, want %v", cookie.HttpOnly, v.httpOnly) - } - }) + options := &Options{ + Path: v.path, + Domain: v.domain, + MaxAge: v.maxAge, + Secure: v.secure, + HttpOnly: v.httpOnly, + } + cookie := newCookieFromOptions(v.name, v.value, options) + if cookie.Name != v.name { + t.Fatalf("%v: bad cookie name: got %q, want %q", i+1, cookie.Name, v.name) + } + if cookie.Value != v.value { + t.Fatalf("%v: bad cookie value: got %q, want %q", i+1, cookie.Value, v.value) + } + if cookie.Path != v.path { + t.Fatalf("%v: bad cookie path: got %q, want %q", i+1, cookie.Path, v.path) + } + if cookie.Domain != v.domain { + t.Fatalf("%v: bad cookie domain: got %q, want %q", i+1, cookie.Domain, v.domain) + } + if cookie.MaxAge != v.maxAge { + t.Fatalf("%v: bad cookie maxAge: got %q, want %q", i+1, cookie.MaxAge, v.maxAge) + } + if cookie.Secure != v.secure { + t.Fatalf("%v: bad cookie secure: got %v, want %v", i+1, cookie.Secure, v.secure) + } + if cookie.HttpOnly != v.httpOnly { + t.Fatalf("%v: bad cookie httpOnly: got %v, want %v", i+1, cookie.HttpOnly, v.httpOnly) + } } }