Repository: google/go-safeweb
Pull request #288 (2021-05-12 11:29:49)

Turn redirects into responses.
This fixes a bug that caused Redirect to not run Commit phase.


Patch:
diff --git a/safehttp/default_dispatcher.go b/safehttp/default_dispatcher.go
index 73a4688..87fbfd9 100644
--- a/safehttp/default_dispatcher.go
+++ b/safehttp/default_dispatcher.go
@@ -77,6 +77,9 @@ func (DefaultDispatcher) Write(rw http.ResponseWriter, resp Response) error {
 		rw.Header().Set("Content-Type", x.ContentType())
 		// The http package will take care of writing the file body.
 		return nil
+	case RedirectResponse:
+		http.Redirect(rw, x.Request.req, x.Location, int(x.Code))
+		return nil
 	default:
 		return fmt.Errorf("%T is not a safe response type and it cannot be written", resp)
 	}
diff --git a/safehttp/flight.go b/safehttp/flight.go
index 081a1d7..3011ad1 100644
--- a/safehttp/flight.go
+++ b/safehttp/flight.go
@@ -15,7 +15,6 @@
 package safehttp
 
 import (
-	"fmt"
 	"net/http"
 )
 
@@ -121,21 +120,6 @@ func (f *flight) WriteError(resp ErrorResponse) Result {
 	return Result{}
 }
 
-// Redirect responds with a redirect to the given url, using code as the status code.
-//
-// If the ResponseWriter has already been written to, then this method will panic.
-func (f *flight) Redirect(r *IncomingRequest, url string, code StatusCode) Result {
-	if code < 300 || code >= 400 {
-		panic(fmt.Sprintf("wrong method called: redirect with status %d", code))
-	}
-	if f.written {
-		panic("ResponseWriter was already written to")
-	}
-	f.written = true
-	http.Redirect(f.rw, r.req, url, int(code))
-	return Result{}
-}
-
 // Header returns the collection of headers that will be set on the response.
 // Headers must be set before writing a response.
 func (f *flight) Header() Header {
diff --git a/safehttp/plugins/fetchmetadata/fetchmetadata.go b/safehttp/plugins/fetchmetadata/fetchmetadata.go
index 9bb0b5a..a27ca14 100644
--- a/safehttp/plugins/fetchmetadata/fetchmetadata.go
+++ b/safehttp/plugins/fetchmetadata/fetchmetadata.go
@@ -152,7 +152,7 @@ func (p *Plugin) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequest,
 	if !p.checkNavigationIsolationPolicy(r) {
 		rejected = true
 		if p.RedirectURL != nil {
-			return w.Redirect(r, p.RedirectURL.String(), safehttp.StatusMovedPermanently)
+			return safehttp.Redirect(w, r, p.RedirectURL.String(), safehttp.StatusMovedPermanently)
 		}
 	}
 
diff --git a/safehttp/plugins/hsts/hsts.go b/safehttp/plugins/hsts/hsts.go
index 679a449..7e9f77f 100644
--- a/safehttp/plugins/hsts/hsts.go
+++ b/safehttp/plugins/hsts/hsts.go
@@ -100,7 +100,7 @@ func (it Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequ
 			return w.WriteError(safehttp.StatusInternalServerError)
 		}
 		u.Scheme = "https"
-		return w.Redirect(r, u.String(), safehttp.StatusMovedPermanently)
+		return safehttp.Redirect(w, r, u.String(), safehttp.StatusMovedPermanently)
 	}
 
 	var value strings.Builder
diff --git a/safehttp/response.go b/safehttp/response.go
index 09ae990..f24d004 100644
--- a/safehttp/response.go
+++ b/safehttp/response.go
@@ -15,6 +15,7 @@
 package safehttp
 
 import (
+	"fmt"
 	"io"
 )
 
@@ -104,3 +105,23 @@ func ExecuteNamedTemplateWithFuncs(w ResponseWriter, t Template, name string, da
 // NoContentResponse is sent to the commit phase when it's initiated from
 // ResponseWriter.NoContent.
 type NoContentResponse struct{}
+
+// RedirectResponse is used to generate redirect responses.
+type RedirectResponse struct {
+	// Code is the status to use for the redirect.
+	Code StatusCode
+	// Location is the value to use for the redirect Location.
+	Location string
+	// Request is the matching request for which this response is being written.
+	// It is used to build the redirect response.
+	Request *IncomingRequest
+}
+
+// Redirect creates a RedirectResponse and writes it to w.
+// If the given code is not a valid Redirect code this function will panic.
+func Redirect(w ResponseWriter, r *IncomingRequest, location string, code StatusCode) Result {
+	if code < 300 || code >= 400 {
+		panic(fmt.Sprintf("wrong method called: redirect with status %d", code))
+	}
+	return w.Write(RedirectResponse{Code: code, Location: location, Request: r})
+}
diff --git a/safehttp/response_writer.go b/safehttp/response_writer.go
index fb3f9c5..1dc0f71 100644
--- a/safehttp/response_writer.go
+++ b/safehttp/response_writer.go
@@ -35,11 +35,6 @@ type ResponseWriter interface {
 	//
 	// If the ResponseWriter has already been written to, then this method panics.
 	WriteError(resp ErrorResponse) Result
-
-	// Redirect responds with a redirect to the given url, using code as the status code.
-	//
-	// If the ResponseWriter has already been written to, then this method panics.
-	Redirect(r *IncomingRequest, url string, code StatusCode) Result
 }
 
 // ResponseHeadersWriter is used to alter the HTTP response headers.


Test patch:
diff --git a/safehttp/default_dispatcher_test.go b/safehttp/default_dispatcher_test.go
index 5b48c22..b474e07 100644
--- a/safehttp/default_dispatcher_test.go
+++ b/safehttp/default_dispatcher_test.go
@@ -21,6 +21,7 @@ import (
 	"net/http/httptest"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-safeweb/safehttp"
 	"github.com/google/safehtml"
 	safetemplate "github.com/google/safehtml/template"
@@ -116,6 +117,17 @@ func TestDefaultDispatcherValidResponse(t *testing.T) {
 			},
 			wantBody: ")]}',\n{\"field\":\"myField\"}\n",
 		},
+		{
+			name: "Redirect Response",
+			write: func(w http.ResponseWriter) error {
+				d := &safehttp.DefaultDispatcher{}
+				req := httptest.NewRequest("GET", "/path", nil)
+				r := safehttp.NewIncomingRequest(req)
+				return d.Write(w, safehttp.RedirectResponse{Request: r, Location: "/anotherpath", Code: safehttp.StatusFound})
+			},
+			wantHeaders: map[string][]string{"Location": {"/anotherpath"}},
+			wantBody:    "<a href=\"/anotherpath\">Found</a>.\n\n",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -129,6 +141,13 @@ func TestDefaultDispatcherValidResponse(t *testing.T) {
 			if gotBody := rw.Body.String(); tt.wantBody != gotBody {
 				t.Errorf("response body: got %q, want %q", gotBody, tt.wantBody)
 			}
+
+			for k, want := range tt.wantHeaders {
+				got := rw.Header().Values(k)
+				if diff := cmp.Diff(want, got); diff != "" {
+					t.Errorf("response header %q: -want +got %s", k, diff)
+				}
+			}
 		})
 	}
 }
diff --git a/safehttp/flight_test.go b/safehttp/flight_test.go
index 4a85b0e..1a3b467 100644
--- a/safehttp/flight_test.go
+++ b/safehttp/flight_test.go
@@ -128,9 +128,6 @@ func TestFlightDoubleWritePanics(t *testing.T) {
 		"WriteError": func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
 			return w.WriteError(safehttp.StatusPreconditionFailed)
 		},
-		"Redirect": func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
-			return w.Redirect(r, "google.com", safehttp.StatusPermanentRedirect)
-		},
 	}
 
 	for firstWriteName, firstWrite := range writeFuncs {
diff --git a/safehttp/plugins/fetchmetadata/fetchmetadata_test.go b/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
index 2c91f0b..793ce5c 100644
--- a/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
+++ b/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
@@ -459,8 +459,13 @@ func TestCORSAfterRedirect(t *testing.T) {
 			if want, got := safehttp.StatusMovedPermanently, safehttp.StatusCode(rr.Code); got != want {
 				t.Errorf("rr.Code got: %v want: %v", got, want)
 			}
-			if got, want := fakeRW.RedirectURL, "https://spaghetti.com/carbonara"; got != want {
-				t.Errorf("RedirectURL got %q, want %q", got, want)
+			resp := fakeRW.Dispatcher.Written
+			redir, ok := resp.(safehttp.RedirectResponse)
+			if !ok {
+				t.Fatalf("got %T, wanted a RedirectResponse", resp)
+			}
+			if got, want := redir.Location, "https://spaghetti.com/carbonara"; got != want {
+				t.Errorf("RedirectResponse.Location got %q, want %q", got, want)
 			}
 		})
 	}
diff --git a/safehttp/plugins/hsts/hsts_test.go b/safehttp/plugins/hsts/hsts_test.go
index f30570e..3f6f902 100644
--- a/safehttp/plugins/hsts/hsts_test.go
+++ b/safehttp/plugins/hsts/hsts_test.go
@@ -58,8 +58,15 @@ func TestHSTSReject(t *testing.T) {
 				t.Errorf("rr.Code got: %v want: %v", gotStatus, tt.wantStatus)
 			}
 
-			if got, want := fakeRW.RedirectURL, tt.wantRedirect; got != want {
-				t.Errorf("RedirectURL got %q, want %q", got, want)
+			if tt.wantRedirect != "" {
+				resp := fakeRW.Dispatcher.Written
+				redir, ok := resp.(safehttp.RedirectResponse)
+				if !ok {
+					t.Fatalf("got %T, wanted a RedirectResponse", resp)
+				}
+				if got, want := redir.Location, tt.wantRedirect; got != want {
+					t.Errorf("RedirectResponse.Location got %q, want %q", got, want)
+				}
 			}
 		})
 	}
diff --git a/safehttp/safehttptest/recorder.go b/safehttp/safehttptest/recorder.go
index 03db8ad..e6cb278 100644
--- a/safehttp/safehttptest/recorder.go
+++ b/safehttp/safehttptest/recorder.go
@@ -23,10 +23,10 @@ import (
 
 // FakeResponseWriter creates a fake safehttp.ResponseWriter implementation.
 //
-// It does no error checking nor runs interceptors.
+// It performs no error checking nor runs interceptors.
 type FakeResponseWriter struct {
 	// The Dispatcher implementation
-	Dispatcher safehttp.Dispatcher
+	Dispatcher *FakeDispatcher
 
 	// ResponseWriter is only used for calls to Dispatcher. Calls to AddCookie()
 	// do not affect it.
@@ -38,21 +38,26 @@ type FakeResponseWriter struct {
 
 	// Response headers.
 	Headers safehttp.Header
-
-	// The redirect URL.
-	RedirectURL string
 }
 
 // FakeDispatcher provides a minimal implementation of the Dispatcher to be used for testing Interceptors.
-type FakeDispatcher struct{}
+type FakeDispatcher struct {
+	Written    safehttp.Response
+	Dispatcher safehttp.Dispatcher
+}
 
-// Write is not implemented.
-func (d FakeDispatcher) Write(rw http.ResponseWriter, resp safehttp.Response) error {
-	panic("not implemented") // TODO: Implement
+// Write records the written responses and calls Dispatcher.Write.
+// If Dispatcher is nil, the default one is used.
+func (d *FakeDispatcher) Write(rw http.ResponseWriter, resp safehttp.Response) error {
+	d.Written = resp
+	if d.Dispatcher != nil {
+		return d.Dispatcher.Write(rw, resp)
+	}
+	return safehttp.DefaultDispatcher{}.Write(rw, resp)
 }
 
 // Error writes just the status code.
-func (d FakeDispatcher) Error(rw http.ResponseWriter, resp safehttp.ErrorResponse) error {
+func (d *FakeDispatcher) Error(rw http.ResponseWriter, resp safehttp.ErrorResponse) error {
 	rw.WriteHeader(int(resp.Code()))
 	return nil
 }
@@ -62,7 +67,7 @@ func (d FakeDispatcher) Error(rw http.ResponseWriter, resp safehttp.ErrorRespons
 func NewFakeResponseWriter() (*FakeResponseWriter, *httptest.ResponseRecorder) {
 	recorder := httptest.NewRecorder()
 	return &FakeResponseWriter{
-		Dispatcher:     FakeDispatcher{},
+		Dispatcher:     &FakeDispatcher{},
 		ResponseWriter: recorder,
 		Headers:        safehttp.NewHeader(recorder.HeaderMap),
 	}, recorder
@@ -99,18 +104,10 @@ func (frw *FakeResponseWriter) NoContent() safehttp.Result {
 	return safehttp.Result{}
 }
 
-// WrriteError forwards the error response to Dispatcher.WriteError.
+// WriteError forwards the error response to Dispatcher.WriteError.
 func (frw *FakeResponseWriter) WriteError(resp safehttp.ErrorResponse) safehttp.Result {
 	if err := frw.Dispatcher.Error(frw.ResponseWriter, resp); err != nil {
 		panic(err)
 	}
 	return safehttp.Result{}
 }
-
-// Redirect saves the URL in RedirectURL and writes the MovedPermentantly status
-// code.
-func (frw *FakeResponseWriter) Redirect(r *safehttp.IncomingRequest, url string, code safehttp.StatusCode) safehttp.Result {
-	frw.RedirectURL = url
-	frw.ResponseWriter.WriteHeader(int(safehttp.StatusMovedPermanently))
-	return safehttp.Result{}
-}