Repository: google/go-safeweb (go)

Issue:
Come up with a better way of setting cookies.
Currently, to set cookies on a response you have to use the `safehttp.Header.SetCookie` method which takes a `http.Cookie`. All other ways to modify header values block the modification of the `Set-Cookie` header. 

I suggest that we remove the `safehttp.Header.SetCookie` method, implement a `safehttp.Cookie` type with safe defaults and add a `SetCookie` method to `safehttp.ResponseWriter`.

Pull request:
Implemented a safehttp.Cookie type, made it readable from IncomingRequest and settable from ResponseWriter.
Fixes #56 

This PR implements a cookie type with safe default values.

It also implements cookie reading methods on `IncomingRequest` and cookie setting methods on `ResponseWriter`. This mimics the setters and getters available for cookies in `net/http`.

The `safehttp.Header.SetCookie` method has been removed. Now, cookie interactions only happen with the `ResponseWriter` and the `IncomingRequest`  and never with the `Header` directly.

Patch:
diff --git a/safehttp/cookie.go b/safehttp/cookie.go
new file mode 100644
index 0000000..fefd8a5
--- /dev/null
+++ b/safehttp/cookie.go
@@ -0,0 +1,120 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package safehttp
+
+import (
+	"net/http"
+)
+
+// A Cookie represents an HTTP cookie as sent in the Set-Cookie header of an
+// HTTP response or the Cookie header of an HTTP request.
+//
+// See https://tools.ietf.org/html/rfc6265 for details.
+type Cookie struct {
+	wrapped *http.Cookie
+}
+
+// NewCookie creates a new Cookie with safe default settings.
+// Those safe defaults are:
+//  - Secure: true
+//  - HttpOnly: true
+//  - SameSite: Lax
+// For more info about all the options, see:
+// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
+//
+// TODO: What if name or value is invalid?
+func NewCookie(name, value string) *Cookie {
+	return &Cookie{
+		&http.Cookie{
+			Name:     name,
+			Value:    value,
+			Secure:   true,
+			HttpOnly: true,
+			SameSite: http.SameSiteLaxMode,
+		},
+	}
+}
+
+// SameSite allows a server to define a cookie attribute making it impossible for
+// the browser to send this cookie along with cross-site requests. The main
+// goal is to mitigate the risk of cross-origin information leakage, and provide
+// some protection against cross-site request forgery attacks.
+//
+// See https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00 for details.
+type SameSite int
+
+const (
+	SameSiteLaxMode SameSite = iota + 1
+	SameSiteStrictMode
+	SameSiteNoneMode
+)
+
+// SetSameSite sets the SameSite attribute.
+func (c *Cookie) SetSameSite(s SameSite) {
+	switch s {
+	case SameSiteLaxMode:
+		c.wrapped.SameSite = http.SameSiteLaxMode
+	case SameSiteStrictMode:
+		c.wrapped.SameSite = http.SameSiteStrictMode
+	case SameSiteNoneMode:
+		c.wrapped.SameSite = http.SameSiteNoneMode
+	}
+}
+
+// SetMaxAge sets the MaxAge attribute.
+//
+// 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
+func (c *Cookie) SetMaxAge(maxAge int) {
+	c.wrapped.MaxAge = maxAge
+}
+
+// SetPath sets the path attribute.
+func (c *Cookie) SetPath(path string) {
+	c.wrapped.Path = path
+}
+
+// SetDomain sets the domain attribute.
+func (c *Cookie) SetDomain(domain string) {
+	c.wrapped.Domain = domain
+}
+
+// DisableSecure disables the secure attribute.
+func (c *Cookie) DisableSecure() {
+	c.wrapped.Secure = false
+}
+
+// DisableHTTPOnly disables the HttpOnly attribute.
+func (c *Cookie) DisableHTTPOnly() {
+	c.wrapped.HttpOnly = false
+}
+
+// Name returns the name of the cookie.
+func (c *Cookie) Name() string {
+	return c.wrapped.Name
+}
+
+// Value returns the value of the cookie.
+func (c *Cookie) Value() string {
+	return c.wrapped.Value
+}
+
+// String returns the serialization of the cookie for use in a Set-Cookie
+// response header. If c is nil or c.Name() is invalid, the empty string is
+// returned.
+func (c *Cookie) String() string {
+	return c.wrapped.String()
+}
diff --git a/safehttp/header.go b/safehttp/header.go
index 4531085..d1b6c16 100644
--- a/safehttp/header.go
+++ b/safehttp/header.go
@@ -114,15 +114,18 @@ func (h Header) Values(name string) []string {
 	return clone
 }
 
-// SetCookie adds the cookie provided as a Set-Cookie header in the header
-// collection. If the cookie is nil or cookie.Name is invalid, no header is
-// added. This is the only method that can modify the Set-Cookie header.
-// If other methods try to modify the header they will return errors.
-// TODO: Replace http.Cookie with safehttp.Cookie.
-func (h Header) SetCookie(cookie *http.Cookie) {
-	if v := cookie.String(); v != "" {
-		h.wrapped.Add("Set-Cookie", v)
+// addCookie adds the cookie provided as a Set-Cookie header in the header
+// collection. If the cookie is nil or cookie.Name() is invalid, no header is
+// added and an error is returned. This is the only method that can modify the
+// Set-Cookie header. If other methods try to modify the header they will return
+// errors.
+func (h Header) addCookie(c *Cookie) error {
+	v := c.String()
+	if v == "" {
+		return errors.New("invalid cookie name")
 	}
+	h.wrapped.Add("Set-Cookie", v)
+	return nil
 }
 
 // TODO: Add Write, WriteSubset and Clone when needed.
diff --git a/safehttp/incoming_request.go b/safehttp/incoming_request.go
index cb8385a..db56919 100644
--- a/safehttp/incoming_request.go
+++ b/safehttp/incoming_request.go
@@ -124,3 +124,24 @@ func (r *IncomingRequest) MultipartForm(maxMemory int64) (*MultipartForm, error)
 			}},
 		nil
 }
+
+// Cookie returns the named cookie provided in the request or
+// net/http.ErrNoCookie if not found. If multiple cookies match the given name,
+// only one cookie will be returned.
+func (r *IncomingRequest) Cookie(name string) (*Cookie, error) {
+	c, err := r.req.Cookie(name)
+	if err != nil {
+		return nil, err
+	}
+	return &Cookie{wrapped: c}, nil
+}
+
+// Cookies parses and returns the HTTP cookies sent with the request.
+func (r *IncomingRequest) Cookies() []*Cookie {
+	cl := r.req.Cookies()
+	res := make([]*Cookie, 0, len(cl))
+	for _, c := range cl {
+		res = append(res, &Cookie{wrapped: c})
+	}
+	return res
+}
diff --git a/safehttp/response_writer.go b/safehttp/response_writer.go
index 2cc6d03..48f42f4 100644
--- a/safehttp/response_writer.go
+++ b/safehttp/response_writer.go
@@ -107,6 +107,13 @@ func (w ResponseWriter) Header() Header {
 	return w.header
 }
 
+// SetCookie adds a Set-Cookie header to the provided ResponseWriter's headers.
+// The provided cookie must have a valid Name. Otherwise an error will be
+// returned.
+func (w *ResponseWriter) SetCookie(c *Cookie) error {
+	return w.header.addCookie(c)
+}
+
 // Dispatcher TODO
 type Dispatcher interface {
 	Write(rw http.ResponseWriter, resp Response) error


Test patch:
diff --git a/safehttp/cookie_test.go b/safehttp/cookie_test.go
new file mode 100644
index 0000000..3f0165e
--- /dev/null
+++ b/safehttp/cookie_test.go
@@ -0,0 +1,135 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package safehttp
+
+import "testing"
+
+func TestCookie(t *testing.T) {
+	tests := []struct {
+		name   string
+		cookie *Cookie
+		want   string
+	}{
+		{
+			name:   "Default",
+			cookie: NewCookie("foo", "bar"),
+			want:   "foo=bar; HttpOnly; Secure; SameSite=Lax",
+		},
+		{
+			name: "SameSite Lax",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.SetSameSite(SameSiteNoneMode)
+				c.SetSameSite(SameSiteLaxMode)
+				return c
+			}(),
+			want: "foo=bar; HttpOnly; Secure; SameSite=Lax",
+		},
+		{
+			name: "SameSite strict",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.SetSameSite(SameSiteStrictMode)
+				return c
+			}(),
+			want: "foo=bar; HttpOnly; Secure; SameSite=Strict",
+		},
+		{
+			name: "SameSite none",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.SetSameSite(SameSiteNoneMode)
+				return c
+			}(),
+			want: "foo=bar; HttpOnly; Secure; SameSite=None",
+		},
+		{
+			name: "Maxage positive",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.SetMaxAge(10)
+				return c
+			}(),
+			want: "foo=bar; Max-Age=10; HttpOnly; Secure; SameSite=Lax",
+		},
+		{
+			name: "Maxage negative",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.SetMaxAge(-1)
+				return c
+			}(),
+			want: "foo=bar; Max-Age=0; HttpOnly; Secure; SameSite=Lax",
+		},
+		{
+			name: "Path",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.SetPath("/asdf")
+				return c
+			}(),
+			want: "foo=bar; Path=/asdf; HttpOnly; Secure; SameSite=Lax",
+		},
+		{
+			name: "Domain",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.SetDomain("example.com")
+				return c
+			}(),
+			want: "foo=bar; Domain=example.com; HttpOnly; Secure; SameSite=Lax",
+		},
+		{
+			name: "Not Secure",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.DisableSecure()
+				return c
+			}(),
+			want: "foo=bar; HttpOnly; SameSite=Lax",
+		},
+		{
+			name: "Not HttpOnly",
+			cookie: func() *Cookie {
+				c := NewCookie("foo", "bar")
+				c.DisableHTTPOnly()
+				return c
+			}(),
+			want: "foo=bar; Secure; SameSite=Lax",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := tt.cookie.String(); got != tt.want {
+				t.Errorf("tt.cookie.String() got: %q want: %q", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestCookieName(t *testing.T) {
+	c := NewCookie("foo", "bar")
+	if got, want := c.Name(), "foo"; got != want {
+		t.Errorf("c.Name() got: %v want: %v", got, want)
+	}
+}
+
+func TestCookieValue(t *testing.T) {
+	c := NewCookie("foo", "bar")
+	if got, want := c.Value(), "bar"; got != want {
+		t.Errorf("c.Value() got: %v want: %v", got, want)
+	}
+}
diff --git a/safehttp/header_test.go b/safehttp/header_test.go
index 42fbc5e..acdcd2e 100644
--- a/safehttp/header_test.go
+++ b/safehttp/header_test.go
@@ -46,17 +46,6 @@ func TestSetCanonicalization(t *testing.T) {
 	}
 }
 
-func TestSetSetCookie(t *testing.T) {
-	h := newHeader(http.Header{})
-	cookie := &http.Cookie{Name: "x", Value: "y"}
-	h.SetCookie(cookie)
-	if err := h.Set("Set-Cookie", "x=y"); err == nil {
-		t.Error(`h.Set("Set-Cookie", "x=y") got: nil want: error`)
-	}
-	if diff := cmp.Diff([]string{"x=y"}, h.Values("Set-Cookie")); diff != "" {
-		t.Errorf("h.Values(\"Set-Cookie\") mismatch (-want +got):\n%s", diff)
-	}
-}
 func TestSetEmptySetCookie(t *testing.T) {
 	h := newHeader(http.Header{})
 	if err := h.Set("Set-Cookie", "x=y"); err == nil {
@@ -127,17 +116,6 @@ func TestAddCanonicalization(t *testing.T) {
 	}
 }
 
-func TestAddSetCookie(t *testing.T) {
-	h := newHeader(http.Header{})
-	cookie := &http.Cookie{Name: "x", Value: "y"}
-	h.SetCookie(cookie)
-	if err := h.Add("Set-Cookie", "x=y"); err == nil {
-		t.Error(`h.Add("Set-Cookie", "x=y") got: nil want: error`)
-	}
-	if diff := cmp.Diff([]string{"x=y"}, h.Values("Set-Cookie")); diff != "" {
-		t.Errorf("h.Values(\"Set-Cookie\") mismatch (-want +got):\n%s", diff)
-	}
-}
 func TestAddEmptySetCookie(t *testing.T) {
 	h := newHeader(http.Header{})
 	if err := h.Add("Set-Cookie", "x=y"); err == nil {
@@ -208,18 +186,6 @@ func TestDelCanonicalization(t *testing.T) {
 	}
 }
 
-func TestDelSetCookie(t *testing.T) {
-	h := newHeader(http.Header{})
-	cookie := &http.Cookie{Name: "x", Value: "y"}
-	h.SetCookie(cookie)
-	if err := h.Del("Set-Cookie"); err == nil {
-		t.Error(`h.Del("Set-Cookie") got: nil want: error`)
-	}
-	if diff := cmp.Diff([]string{"x=y"}, h.Values("Set-Cookie")); diff != "" {
-		t.Errorf("h.Values(\"Set-Cookie\") mismatch (-want +got):\n%s", diff)
-	}
-}
-
 func TestDelEmptySetCookie(t *testing.T) {
 	h := newHeader(http.Header{})
 	if err := h.Del("Set-Cookie"); err == nil {
@@ -259,24 +225,6 @@ func TestDelEmptyClaimed(t *testing.T) {
 	}
 }
 
-func TestSetCookie(t *testing.T) {
-	h := newHeader(http.Header{})
-	c := &http.Cookie{Name: "x", Value: "y"}
-	h.SetCookie(c)
-	if got, want := h.Get("Set-Cookie"), "x=y"; got != want {
-		t.Errorf(`h.Get("Set-Cookie") got: %q want: %q`, got, want)
-	}
-}
-
-func TestSetCookieInvalidName(t *testing.T) {
-	h := newHeader(http.Header{})
-	c := &http.Cookie{Name: "x=", Value: "y"}
-	h.SetCookie(c)
-	if got, want := h.Get("Set-Cookie"), ""; got != want {
-		t.Errorf(`h.Get("Set-Cookie") got: %q want: %q`, got, want)
-	}
-}
-
 // TestValuesModifyClaimed verifies that modifying the
 // slice returned by Values() doesn't modify the underlying
 // slice. The test ensures that Values() returns a copy
diff --git a/safehttp/incoming_request_test.go b/safehttp/incoming_request_test.go
new file mode 100644
index 0000000..babfe42
--- /dev/null
+++ b/safehttp/incoming_request_test.go
@@ -0,0 +1,138 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package safehttp_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/google/go-safeweb/safehttp"
+)
+
+func TestIncomingRequestCookie(t *testing.T) {
+	var tests = []struct {
+		name      string
+		req       *http.Request
+		wantName  string
+		wantValue string
+	}{
+		{
+			name: "Basic",
+			req: func() *http.Request {
+				r := httptest.NewRequest(http.MethodGet, "/", nil)
+				r.Header.Set("Cookie", "foo=bar")
+				return r
+			}(),
+			wantName:  "foo",
+			wantValue: "bar",
+		},
+		{
+			name: "Multiple cookies with the same name",
+			req: func() *http.Request {
+				r := httptest.NewRequest(http.MethodGet, "/", nil)
+				r.Header.Add("Cookie", "foo=bar; foo=xyz")
+				r.Header.Add("Cookie", "foo=pizza")
+				return r
+			}(),
+			wantName:  "foo",
+			wantValue: "bar",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ir := safehttp.NewIncomingRequest(tt.req)
+			c, err := ir.Cookie(tt.wantName)
+			if err != nil {
+				t.Errorf(`ir.Cookie(tt.wantName) got: %v want: nil`, err)
+			}
+
+			if got := c.Name(); got != tt.wantName {
+				t.Errorf("c.Name() got: %v want: %v", got, tt.wantName)
+			}
+
+			if got := c.Value(); got != tt.wantValue {
+				t.Errorf(`c.Value() got: %v want: %v`, got, tt.wantValue)
+			}
+		})
+	}
+}
+
+func TestIncomingRequestCookieNotFound(t *testing.T) {
+	r := httptest.NewRequest(http.MethodGet, "/", nil)
+	ir := safehttp.NewIncomingRequest(r)
+	if _, err := ir.Cookie("foo"); err == nil {
+		t.Error(`ir.Cookie("foo") got: nil want: error`)
+	}
+}
+
+func TestIncomingRequestCookies(t *testing.T) {
+	var tests = []struct {
+		name       string
+		req        *http.Request
+		wantNames  []string
+		wantValues []string
+	}{
+		{
+			name: "One",
+			req: func() *http.Request {
+				r := httptest.NewRequest(http.MethodGet, "/", nil)
+				r.Header.Set("Cookie", "foo=bar")
+				return r
+			}(),
+			wantNames:  []string{"foo"},
+			wantValues: []string{"bar"},
+		},
+		{
+			name: "Multiple",
+			req: func() *http.Request {
+				r := httptest.NewRequest(http.MethodGet, "/", nil)
+				r.Header.Add("Cookie", "foo=bar; a=b")
+				r.Header.Add("Cookie", "pizza=hawaii")
+				return r
+			}(),
+			wantNames:  []string{"foo", "a", "pizza"},
+			wantValues: []string{"bar", "b", "hawaii"},
+		},
+		{
+			name:       "None",
+			req:        httptest.NewRequest(http.MethodGet, "/", nil),
+			wantNames:  []string{},
+			wantValues: []string{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ir := safehttp.NewIncomingRequest(tt.req)
+			cl := ir.Cookies()
+
+			if got, want := len(cl), len(tt.wantNames); got != want {
+				t.Errorf("len(cl) got: %v want: %v", got, want)
+			}
+
+			for i, c := range cl {
+				if got := c.Name(); got != tt.wantNames[i] {
+					t.Errorf("c.Name() got: %v want: %v", got, tt.wantNames[i])
+				}
+
+				if got := c.Value(); got != tt.wantValues[i] {
+					t.Errorf(`c.Value() got: %v want: %v`, got, tt.wantValues[i])
+				}
+			}
+		})
+	}
+}
diff --git a/safehttp/response_writer_test.go b/safehttp/response_writer_test.go
new file mode 100644
index 0000000..e4a1a7b
--- /dev/null
+++ b/safehttp/response_writer_test.go
@@ -0,0 +1,52 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package safehttp_test
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-safeweb/safehttp"
+)
+
+func TestResponseWriterSetCookie(t *testing.T) {
+	rr := newResponseRecorder(&strings.Builder{})
+	rw := safehttp.NewResponseWriter(testDispatcher{}, rr, nil)
+
+	c := safehttp.NewCookie("foo", "bar")
+	err := rw.SetCookie(c)
+	if err != nil {
+		t.Errorf("rw.SetCookie(c) got: %v want: nil", err)
+	}
+
+	wantHeaders := map[string][]string{
+		"Set-Cookie": {"foo=bar; HttpOnly; Secure; SameSite=Lax"},
+	}
+	if diff := cmp.Diff(wantHeaders, map[string][]string(rr.header)); diff != "" {
+		t.Errorf("rr.header mismatch (-want +got):\n%s", diff)
+	}
+}
+
+func TestResponseWriterSetInvalidCookie(t *testing.T) {
+	rr := newResponseRecorder(&strings.Builder{})
+	rw := safehttp.NewResponseWriter(testDispatcher{}, rr, nil)
+
+	c := safehttp.NewCookie("f=oo", "bar")
+	err := rw.SetCookie(c)
+	if err == nil {
+		t.Error("rw.SetCookie(c) got: nil want: error")
+	}
+}