Repository: google/go-safeweb (go)

Issue:
XSRF Token Generation
In its current format, the XSRF plugin only supports verification of the XSRF token in  the `safehttp.IncomingRequest` body. We also need to generate tokens and add them to a `safehttp.IncomingRequest` context. These will then be injected  by the `htmlinject` plugin as hidden inputs to forms.

Pull request:
Added token generation to the XSRF plugin.
Fixes #122 

Extended the XSRF plugin to support token generation in the `xsrf.Before()` function. This will then be added to the context of a `safehttp.IncomingRequest` and can be retrieved using the `xsrf.Token()` getter.

Patch:
diff --git a/go.sum b/go.sum
index ec7170a..560b7a5 100644
--- a/go.sum
+++ b/go.sum
@@ -9,6 +9,7 @@ golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPh
 golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
 golang.org/x/net v0.0.0-20200707034311-ab3426394381 h1:VXak5I6aEWmAXeQjA+QSZzlgNrpq9mjcfDemuexIKsU=
 golang.org/x/net v0.0.0-20200707034311-ab3426394381/go.mod h1:/O7V0waA8r7cgGh81Ro3o1hOxt32SMVPicZroKQ2sZA=
+golang.org/x/net v0.0.0-20200822124328-c89045814202 h1:VvcQYSHwXgi7W+TpUR6A9g6Up98WAHf3f/ulnJ62IyA=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
diff --git a/safehttp/incoming_request.go b/safehttp/incoming_request.go
index 939673c..0be1bf0 100644
--- a/safehttp/incoming_request.go
+++ b/safehttp/incoming_request.go
@@ -49,20 +49,6 @@ func (r *IncomingRequest) Method() string {
 	return r.req.Method
 }
 
-// Host returns the host the request is targeted to.
-// TODO(@mihalimara22): Remove this after the safehttp.URL type has been implemented
-func (r *IncomingRequest) Host() string {
-	return r.req.Host
-}
-
-// Path returns the relative path of the URL in decoded format (e.g. %47%6f%2f
-// becomes /Go/).
-// TODO(@mihalimara22): Remove this after the safehttp.URL type has been
-// implemented.
-func (r *IncomingRequest) Path() string {
-	return r.req.URL.Path
-}
-
 // PostForm parses the form parameters provided in the body of a POST, PATCH or
 // PUT request that does not have Content-Type: multipart/form-data. It returns
 // the parsed form parameters as a Form object, if no error occurred. If a parsing
diff --git a/safehttp/plugins/xsrf/xsrf.go b/safehttp/plugins/xsrf/xsrf.go
index 7972d4c..a9f1587 100644
--- a/safehttp/plugins/xsrf/xsrf.go
+++ b/safehttp/plugins/xsrf/xsrf.go
@@ -15,6 +15,8 @@
 package xsrf
 
 import (
+	"context"
+	"errors"
 	"github.com/google/go-safeweb/safehttp"
 	"golang.org/x/net/xsrftoken"
 )
@@ -25,6 +27,12 @@ const (
 	TokenKey = "xsrf-token"
 )
 
+var statePreservingMethods = map[string]bool{
+	safehttp.MethodGet:     true,
+	safehttp.MethodHead:    true,
+	safehttp.MethodOptions: true,
+}
+
 // UserIdentifier provides the web application users' identifiers,
 // needed in generating the XSRF token.
 type UserIdentifier interface {
@@ -33,42 +41,70 @@ type UserIdentifier interface {
 	UserID(*safehttp.IncomingRequest) (string, error)
 }
 
-// Interceptor implements XSRF protection. It requires an application key and a
-// storage service. The appKey uniquely identifies each registered service and
-// should have high entropy. The storage service supports retrieving ID's of the
-// application's users. Both the appKey and user ID are used in the XSRF
-// token generation algorithm.
+// Interceptor implements XSRF protection.
 type Interceptor struct {
-	AppKey     string
+	// SecretAppKey uniquely identifies each registered service and should have high
+	// entropy as it is used for generating the XSRF token.
+	SecretAppKey string
+	// Identifier supports retrieving the user ID based on the incoming
+	// request. This is needed for generating the XSRF token.
 	Identifier UserIdentifier
 }
 
+type tokenCtxKey struct{}
+
+// Token extracts the XSRF token from the incoming request. If it is not
+// present, it returns a non-nil error.
+func Token(r *safehttp.IncomingRequest) (string, error) {
+	tok := r.Context().Value(tokenCtxKey{})
+	if tok == nil {
+		return "", errors.New("xsrf token not found")
+	}
+	return tok.(string), nil
+}
+
 // Before should be executed before directing the safehttp.IncomingRequest to
-// the handler to ensure it is not part of the Cross Site Request
-// Forgery. It checks for the presence of an xsrf-token in the request body and
-// validates it based on the userID associated with the request.
-func (p *Interceptor) Before(w *safehttp.ResponseWriter, r *safehttp.IncomingRequest, cfg interface{}) safehttp.Result {
-	userID, err := p.Identifier.UserID(r)
+// the handler to ensure it is not part of the Cross-Site Request
+// Forgery attack.
+//
+// In case of state changing requests (all except GET, HEAD and OPTIONS), it
+// checks for the presence of an XSRF token in the request and validates it
+// based on the user ID associated with the request.
+//
+// For authorized requests, it adds a cryptographically safe XSRF token to the
+// incoming request. It can be later extracted using Token.
+func (i *Interceptor) Before(w *safehttp.ResponseWriter, r *safehttp.IncomingRequest, cfg interface{}) safehttp.Result {
+	userID, err := i.Identifier.UserID(r)
 	if err != nil {
 		return w.ClientError(safehttp.StatusUnauthorized)
 	}
-	f, err := r.PostForm()
-	if err != nil {
-		mf, err := r.MultipartForm(32 << 20)
+
+	actionID := r.Method() + " " + r.URL.Path()
+	needsValidation := !statePreservingMethods[r.Method()]
+	if needsValidation {
+		f, err := r.PostForm()
 		if err != nil {
-			return w.ClientError(safehttp.StatusBadRequest)
+			// We fallback to checking whether the form is multipart. Both types
+			// are valid in an incoming request as long as the XSRF token is
+			// present.
+			mf, err := r.MultipartForm(32 << 20)
+			if err != nil {
+				return w.ClientError(safehttp.StatusBadRequest)
+			}
+			f = &mf.Form
 		}
-		f = &mf.Form
-	}
 
-	tok := f.String(TokenKey, "")
-	if f.Err() != nil || tok == "" {
-		return w.ClientError(safehttp.StatusUnauthorized)
-	}
+		tok := f.String(TokenKey, "")
+		if f.Err() != nil || tok == "" {
+			return w.ClientError(safehttp.StatusUnauthorized)
+		}
 
-	if ok := xsrftoken.Valid(tok, p.AppKey, userID, r.Host()+r.Path()); !ok {
-		return w.ClientError(safehttp.StatusForbidden)
+		if ok := xsrftoken.Valid(tok, i.SecretAppKey, userID, actionID); !ok {
+			return w.ClientError(safehttp.StatusForbidden)
+		}
 	}
 
+	tok := xsrftoken.Generate(i.SecretAppKey, userID, actionID)
+	r.SetContext(context.WithValue(r.Context(), tokenCtxKey{}, tok))
 	return safehttp.NotWritten()
 }


Test patch:
diff --git a/safehttp/plugins/xsrf/xsrf_test.go b/safehttp/plugins/xsrf/xsrf_test.go
index 7c42752..869c7f4 100644
--- a/safehttp/plugins/xsrf/xsrf_test.go
+++ b/safehttp/plugins/xsrf/xsrf_test.go
@@ -12,17 +12,16 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package xsrf_test
+package xsrf
 
 import (
-	"strings"
-	"testing"
-
+	"context"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-safeweb/safehttp"
-	"github.com/google/go-safeweb/safehttp/plugins/xsrf"
 	"github.com/google/go-safeweb/safehttp/safehttptest"
 	"golang.org/x/net/xsrftoken"
+	"strings"
+	"testing"
 )
 
 type userIdentifier struct{}
@@ -31,42 +30,24 @@ func (userIdentifier) UserID(r *safehttp.IncomingRequest) (string, error) {
 	return "1234", nil
 }
 
-func TestXSRFTokenPost(t *testing.T) {
-	tests := []struct {
-		name       string
-		host       string
-		path       string
-		userID     string
-		wantStatus safehttp.StatusCode
-		wantHeader map[string][]string
-		wantBody   string
+var (
+	formTokenTests = []struct {
+		name, userID, actionID, wantBody string
+		wantStatus                       safehttp.StatusCode
+		wantHeader                       map[string][]string
 	}{
 		{
 			name:       "Valid token",
 			userID:     "1234",
-			host:       "foo.com",
-			path:       "/pizza",
+			actionID:   "POST /pizza",
 			wantStatus: safehttp.StatusOK,
 			wantHeader: map[string][]string{},
 			wantBody:   "",
 		},
 		{
-			name:       "Invalid host in token generation",
-			userID:     "1234",
-			host:       "bar.com",
-			path:       "/pizza",
-			wantStatus: safehttp.StatusForbidden,
-			wantHeader: map[string][]string{
-				"Content-Type":           {"text/plain; charset=utf-8"},
-				"X-Content-Type-Options": {"nosniff"},
-			},
-			wantBody: "Forbidden\n",
-		},
-		{
-			name:       "Invalid path in token generation",
+			name:       "Invalid actionID in token generation",
 			userID:     "1234",
-			host:       "foo.com",
-			path:       "spaghetti",
+			actionID:   "HEAD /pizza",
 			wantStatus: safehttp.StatusForbidden,
 			wantHeader: map[string][]string{
 				"Content-Type":           {"text/plain; charset=utf-8"},
@@ -77,8 +58,7 @@ func TestXSRFTokenPost(t *testing.T) {
 		{
 			name:       "Invalid userID in token generation",
 			userID:     "5678",
-			host:       "foo.com",
-			path:       "/pizza",
+			actionID:   "POST /pizza",
 			wantStatus: safehttp.StatusForbidden,
 			wantHeader: map[string][]string{
 				"Content-Type":           {"text/plain; charset=utf-8"},
@@ -87,133 +67,84 @@ func TestXSRFTokenPost(t *testing.T) {
 			wantBody: "Forbidden\n",
 		},
 	}
-	for _, test := range tests {
-		i := xsrf.Interceptor{AppKey: "xsrf", Identifier: userIdentifier{}}
-		tok := xsrftoken.Generate("xsrf", test.userID, test.host+test.path)
-		req := safehttptest.NewRequest(safehttp.MethodPost, "http://foo.com/pizza", strings.NewReader(xsrf.TokenKey+"="+tok))
-		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+)
 
-		rec := safehttptest.NewResponseRecorder()
-		i.Before(rec.ResponseWriter, req, nil)
+func TestTokenPost(t *testing.T) {
+	for _, test := range formTokenTests {
+		t.Run(test.name, func(t *testing.T) {
+			rec := safehttptest.NewResponseRecorder()
+			tok := xsrftoken.Generate("testSecretAppKey", test.userID, test.actionID)
+			req := safehttptest.NewRequest(safehttp.MethodPost, "https://foo.com/pizza", strings.NewReader(TokenKey+"="+tok))
+			req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 
-		if rec.Status() != test.wantStatus {
-			t.Errorf("response status: got %v, want %v", rec.Status(), test.wantStatus)
-		}
-		if diff := cmp.Diff(test.wantHeader, map[string][]string(rec.Header())); diff != "" {
-			t.Errorf("rec.Header() mismatch (-want +got):\n%s", diff)
-		}
-		if got := rec.Body(); got != test.wantBody {
-			t.Errorf("response body: got %q want %q", got, test.wantBody)
-		}
+			i := Interceptor{SecretAppKey: "testSecretAppKey", Identifier: userIdentifier{}}
+			i.Before(rec.ResponseWriter, req, nil)
+
+			if got := rec.Status(); got != test.wantStatus {
+				t.Errorf("response status: got %v, want %v", got, test.wantStatus)
+			}
+			if diff := cmp.Diff(test.wantHeader, map[string][]string(rec.Header())); diff != "" {
+				t.Errorf("rec.Header() mismatch (-want +got):\n%s", diff)
+			}
+			if got := rec.Body(); got != test.wantBody {
+				t.Errorf("response body: got %q want %q", got, test.wantBody)
+			}
+		})
 	}
 }
 
-func TestXSRFTokenMultipart(t *testing.T) {
-	tests := []struct {
-		name       string
-		host       string
-		path       string
-		userID     string
-		wantStatus safehttp.StatusCode
-		wantHeader map[string][]string
-		wantBody   string
-	}{
-		{
-			name:       "Valid token",
-			userID:     "1234",
-			host:       "foo.com",
-			path:       "/pizza",
-			wantStatus: safehttp.StatusOK,
-			wantHeader: map[string][]string{},
-			wantBody:   "",
-		},
-		{
-			name:       "Invalid host in token generation",
-			userID:     "1234",
-			host:       "bar.com",
-			path:       "/pizza",
-			wantStatus: safehttp.StatusForbidden,
-			wantHeader: map[string][]string{
-				"Content-Type":           {"text/plain; charset=utf-8"},
-				"X-Content-Type-Options": {"nosniff"},
-			},
-			wantBody: "Forbidden\n",
-		},
-		{
-			name:       "Invalid path in token generation",
-			userID:     "1234",
-			host:       "foo.com",
-			path:       "spaghetti",
-			wantStatus: safehttp.StatusForbidden,
-			wantHeader: map[string][]string{
-				"Content-Type":           {"text/plain; charset=utf-8"},
-				"X-Content-Type-Options": {"nosniff"},
-			},
-			wantBody: "Forbidden\n",
-		},
-		{
-			name:       "Invalid userID in token generation",
-			userID:     "5678",
-			host:       "foo.com",
-			path:       "/pizza",
-			wantStatus: safehttp.StatusForbidden,
-			wantHeader: map[string][]string{
-				"Content-Type":           {"text/plain; charset=utf-8"},
-				"X-Content-Type-Options": {"nosniff"},
-			},
-			wantBody: "Forbidden\n",
-		},
-	}
-	for _, test := range tests {
-		i := xsrf.Interceptor{AppKey: "xsrf", Identifier: userIdentifier{}}
-		tok := xsrftoken.Generate("xsrf", test.userID, test.host+test.path)
-		b := "--123\r\n" +
-			"Content-Disposition: form-data; name=\"xsrf-token\"\r\n" +
-			"\r\n" +
-			tok + "\r\n" +
-			"--123--\r\n"
-		req := safehttptest.NewRequest(safehttp.MethodPost, "http://foo.com/pizza", strings.NewReader(b))
-		req.Header.Set("Content-Type", `multipart/form-data; boundary="123"`)
+func TestTokenMultipart(t *testing.T) {
+	for _, test := range formTokenTests {
+		t.Run(test.name, func(t *testing.T) {
+			rec := safehttptest.NewResponseRecorder()
+			tok := xsrftoken.Generate("testSecretAppKey", test.userID, test.actionID)
+			b := "--123\r\n" +
+				"Content-Disposition: form-data; name=\"xsrf-token\"\r\n" +
+				"\r\n" +
+				tok + "\r\n" +
+				"--123--\r\n"
+			req := safehttptest.NewRequest(safehttp.MethodPost, "https://foo.com/pizza", strings.NewReader(b))
+			req.Header.Set("Content-Type", `multipart/form-data; boundary="123"`)
 
-		rec := safehttptest.NewResponseRecorder()
-		i.Before(rec.ResponseWriter, req, nil)
+			i := Interceptor{SecretAppKey: "testSecretAppKey", Identifier: userIdentifier{}}
+			i.Before(rec.ResponseWriter, req, nil)
 
-		if rec.Status() != test.wantStatus {
-			t.Errorf("response status: got %v, want %v", rec.Status(), test.wantStatus)
-		}
-		if diff := cmp.Diff(test.wantHeader, map[string][]string(rec.Header())); diff != "" {
-			t.Errorf("rw.header mismatch (-want +got):\n%s", diff)
-		}
-		if got := rec.Body(); got != test.wantBody {
-			t.Errorf("response body: got %q want %q", got, test.wantBody)
-		}
+			if got := rec.Status(); got != test.wantStatus {
+				t.Errorf("response status: got %v, want %v", got, test.wantStatus)
+			}
+			if diff := cmp.Diff(test.wantHeader, map[string][]string(rec.Header())); diff != "" {
+				t.Errorf("rw.header mismatch (-want +got):\n%s", diff)
+			}
+			if got := rec.Body(); got != test.wantBody {
+				t.Errorf("response body: got %q want %q", got, test.wantBody)
+			}
+		})
 	}
 }
 
-func TestXSRFMissingToken(t *testing.T) {
+func TestMissingTokenInBody(t *testing.T) {
 	tests := []struct {
-		name       string
-		req        *safehttp.IncomingRequest
-		wantStatus safehttp.StatusCode
-		wantHeader map[string][]string
-		wantBody   string
+		name string
+		req  *safehttp.IncomingRequest
 	}{
 		{
-			name: "Missing token in POST request",
+			name: "Missing token in POST request with form",
 			req: func() *safehttp.IncomingRequest {
 				req := safehttptest.NewRequest(safehttp.MethodPost, "/", strings.NewReader("foo=bar"))
 				req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 				return req
 			}(),
-			wantStatus: safehttp.StatusUnauthorized,
-			wantHeader: map[string][]string{
-				"Content-Type":           {"text/plain; charset=utf-8"},
-				"X-Content-Type-Options": {"nosniff"},
-			},
-			wantBody: "Unauthorized\n",
 		},
 		{
-			name: "Missing token in multipart request",
+			name: "Missing token in PATCH request with form",
+			req: func() *safehttp.IncomingRequest {
+				req := safehttptest.NewRequest(safehttp.MethodPatch, "/", strings.NewReader("foo=bar"))
+				req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+				return req
+			}(),
+		},
+		{
+			name: "Missing token in POST request with multipart form",
 			req: func() *safehttp.IncomingRequest {
 				b := "--123\r\n" +
 					"Content-Disposition: form-data; name=\"foo\"\r\n" +
@@ -224,27 +155,92 @@ func TestXSRFMissingToken(t *testing.T) {
 				req.Header.Set("Content-Type", `multipart/form-data; boundary="123"`)
 				return req
 			}(),
-			wantStatus: safehttp.StatusUnauthorized,
-			wantHeader: map[string][]string{
-				"Content-Type":           {"text/plain; charset=utf-8"},
-				"X-Content-Type-Options": {"nosniff"},
-			},
-			wantBody: "Unauthorized\n",
+		},
+		{
+			name: "Missing token in PATCH request with multipart form",
+			req: func() *safehttp.IncomingRequest {
+				b := "--123\r\n" +
+					"Content-Disposition: form-data; name=\"foo\"\r\n" +
+					"\r\n" +
+					"bar\r\n" +
+					"--123--\r\n"
+				req := safehttptest.NewRequest(safehttp.MethodPatch, "/", strings.NewReader(b))
+				req.Header.Set("Content-Type", `multipart/form-data; boundary="123"`)
+				return req
+			}(),
 		},
 	}
 	for _, test := range tests {
-		i := xsrf.Interceptor{AppKey: "xsrf", Identifier: userIdentifier{}}
 		rec := safehttptest.NewResponseRecorder()
+
+		i := Interceptor{SecretAppKey: "testSecretAppKey", Identifier: userIdentifier{}}
 		i.Before(rec.ResponseWriter, test.req, nil)
 
-		if rec.Status() != test.wantStatus {
-			t.Errorf("response status: got %v, want %v", rec.Status(), test.wantStatus)
+		if want, got := safehttp.StatusUnauthorized, rec.Status(); got != want {
+			t.Errorf("response status: got %v, want %v", got, want)
+		}
+		wantHeaders := map[string][]string{
+			"Content-Type":           {"text/plain; charset=utf-8"},
+			"X-Content-Type-Options": {"nosniff"},
 		}
-		if diff := cmp.Diff(test.wantHeader, map[string][]string(rec.Header())); diff != "" {
+		if diff := cmp.Diff(wantHeaders, map[string][]string(rec.Header())); diff != "" {
 			t.Errorf("rw.header mismatch (-want +got):\n%s", diff)
 		}
-		if got := rec.Body(); got != test.wantBody {
-			t.Errorf("response body: got %q want %q", got, test.wantBody)
+		if want, got := "Unauthorized\n", rec.Body(); got != want {
+			t.Errorf("response body: got %q want %q", got, want)
 		}
 	}
 }
+
+func TestBeforeTokenInRequestContext(t *testing.T) {
+	rec := safehttptest.NewResponseRecorder()
+	req := safehttptest.NewRequest(safehttp.MethodGet, "https://foo.com/pizza", nil)
+
+	i := Interceptor{SecretAppKey: "testSecretAppKey", Identifier: userIdentifier{}}
+	i.Before(rec.ResponseWriter, req, nil)
+
+	tok, err := Token(req)
+	if tok == "" {
+		t.Error(`Token(req): got "", want token`)
+	}
+	if err != nil {
+		t.Errorf("Token(req): got %v, want nil", err)
+	}
+
+	if want, got := safehttp.StatusOK, safehttp.StatusCode(rec.Status()); want != got {
+		t.Errorf("response status: got %v, want %v", got, want)
+	}
+	if diff := cmp.Diff(map[string][]string{}, map[string][]string(rec.Header())); diff != "" {
+		t.Errorf("rec.Header() mismatch (-want +got):\n%s", diff)
+	}
+	if want, got := "", rec.Body(); got != want {
+		t.Errorf("response body: got %q want %q", got, want)
+	}
+
+}
+
+func TestTokenInRequestContext(t *testing.T) {
+	req := safehttptest.NewRequest(safehttp.MethodGet, "/", nil)
+	req.SetContext(context.WithValue(req.Context(), tokenCtxKey{}, "pizza"))
+
+	got, err := Token(req)
+	if want := "pizza"; want != got {
+		t.Errorf("Token(req): got %v, want %v", got, want)
+	}
+	if err != nil {
+		t.Errorf("Token(req): got %v, want nil", err)
+	}
+}
+
+func TestMissingTokenInRequestContext(t *testing.T) {
+	req := safehttptest.NewRequest(safehttp.MethodGet, "/", nil)
+	req.SetContext(context.Background())
+
+	got, err := Token(req)
+	if want := ""; want != got {
+		t.Errorf("Token(req): got %v, want %v", got, want)
+	}
+	if err == nil {
+		t.Error("Token(req): got nil, want error")
+	}
+}