Repository: google/go-safeweb (go)

Issue:
Remove the Response argument from ResponseWriter.ServerError until we come up with how to support default error responses
`safehttp.ResponseWriter.ServerError` currently has the following signature:
```go
func (w *ResponseWriter) ServerError(code StatusCode, resp Response) Result
```
It takes a `safehttp.Response` as an argument, but there might be places where we want to respond with a server error without having the ability to create safe responses. For example if a plugin needs to interrupt the handling of an incoming request. We therefore need to reconsider the signature of `ServerError` so that it doesn't take a `Response` or so that it is possible to optionally call without a `Response`.

Pull request:
Removed Response from ServerError
Fixes #40 

`safehttp.ResponseWriter.ServerError` currently has the following signature:
```go
func (w *ResponseWriter) ServerError(code StatusCode, resp Response) Result
```
It takes a `safehttp.Response` as an argument, but there might be places where we want to respond with a server error without having the ability to create safe responses. For example if a plugin needs to interrupt the handling of an incoming request. We have therefore decided that **for now** `Response` should be removed from the `ServerError` signature.

Patch:
diff --git a/safehttp/plugins/hsts/hsts.go b/safehttp/plugins/hsts/hsts.go
index b1cdd8e..dd2ceac 100644
--- a/safehttp/plugins/hsts/hsts.go
+++ b/safehttp/plugins/hsts/hsts.go
@@ -67,13 +67,13 @@ func NewPlugin() Plugin {
 // response.
 func (p *Plugin) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
 	if p.MaxAge < 0 {
-		return w.ServerError(safehttp.StatusInternalServerError, "Internal Server Error")
+		return w.ServerError(safehttp.StatusInternalServerError)
 	}
 
 	if !p.BehindProxy && r.TLS == nil {
 		u, err := url.Parse(r.URL.String())
 		if err != nil {
-			return w.ServerError(safehttp.StatusInternalServerError, "Internal Server Error")
+			return w.ServerError(safehttp.StatusInternalServerError)
 		}
 		u.Scheme = "https"
 		return w.Redirect(r, u.String(), safehttp.StatusMovedPermanently)
@@ -90,11 +90,10 @@ func (p *Plugin) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequest)
 	}
 	h := w.Header()
 	if err := h.Set("Strict-Transport-Security", value.String()); err != nil {
-		// TODO(@mattiasgrenfeldt): Replace the response with an actual saferesponse somehow.
-		return w.ServerError(safehttp.StatusInternalServerError, "Internal Server Error")
+		return w.ServerError(safehttp.StatusInternalServerError)
 	}
 	if _, err := h.Claim("Strict-Transport-Security"); err != nil {
-		return w.ServerError(safehttp.StatusInternalServerError, "Internal Server Error")
+		return w.ServerError(safehttp.StatusInternalServerError)
 	}
 	return safehttp.Result{}
 }
diff --git a/safehttp/response_writer.go b/safehttp/response_writer.go
index 57857f8..3cda51a 100644
--- a/safehttp/response_writer.go
+++ b/safehttp/response_writer.go
@@ -54,7 +54,14 @@ func (w *ResponseWriter) WriteTemplate(t Template, data interface{}) Result {
 }
 
 // ServerError TODO
-func (w *ResponseWriter) ServerError(code StatusCode, resp Response) Result {
+func (w *ResponseWriter) ServerError(code StatusCode) Result {
+	if code < 500 || code >= 600 {
+		// TODO(@mattiasgrenfeldt, @mihalimara22, @kele, @empijei): Decide how it should
+		// be communicated to the user of the framework that they've called the wrong
+		// method.
+		return Result{}
+	}
+	http.Error(w.rw, http.StatusText(int(code)), int(code))
 	return Result{}
 }
 


Test patch:
diff --git a/safehttp/plugins/hsts/hsts_test.go b/safehttp/plugins/hsts/hsts_test.go
index 122fdf1..eb1e846 100644
--- a/safehttp/plugins/hsts/hsts_test.go
+++ b/safehttp/plugins/hsts/hsts_test.go
@@ -156,6 +156,17 @@ func TestHSTS(t *testing.T) {
 			},
 			wantBody: "",
 		},
+		{
+			name:       "Negative maxage",
+			plugin:     hsts.Plugin{MaxAge: -1 * time.Second},
+			req:        httptest.NewRequest("GET", "https://localhost/", nil),
+			wantStatus: 500,
+			wantHeaders: map[string][]string{
+				"Content-Type":           {"text/plain; charset=utf-8"},
+				"X-Content-Type-Options": {"nosniff"},
+			},
+			wantBody: "Internal Server Error\n",
+		},
 	}
 
 	for _, tt := range test {
@@ -191,98 +202,24 @@ func TestStrictTransportSecurityAlreadyImmutable(t *testing.T) {
 
 	req := httptest.NewRequest("GET", "https://localhost/", nil)
 
-	// TODO(@mattiasgrenfeldt): Change from current to desired behavior once
-	// safehttp.ResponseWriter.ServerError has changed signature and been
-	// implemented.
-	t.Run("Current Behavior", func(t *testing.T) {
-		b := &strings.Builder{}
-		rr := newResponseRecorder(b)
-
-		m.HandleRequest(rr, req)
-
-		if want := 200; rr.status != want {
-			t.Errorf("status code got: %v want: %v", rr.status, want)
-		}
-
-		wantHeaders := map[string][]string{}
-		if diff := cmp.Diff(wantHeaders, map[string][]string(rr.Header())); diff != "" {
-			t.Errorf("rr.Header() mismatch (-want +got):\n%s", diff)
-		}
-
-		if got, want := b.String(), ""; got != want {
-			t.Errorf("response body got: %q want: %q", got, want)
-		}
-	})
-
-	t.Run("Desired Behavior", func(t *testing.T) {
-		t.Skip()
-		b := &strings.Builder{}
-		rr := newResponseRecorder(b)
-
-		m.HandleRequest(rr, req)
-
-		if want := 500; rr.status != want {
-			t.Errorf("status code got: %v want: %v", rr.status, want)
-		}
-
-		wantHeaders := map[string][]string{}
-		if diff := cmp.Diff(wantHeaders, map[string][]string(rr.Header())); diff != "" {
-			t.Errorf("rr.Header() mismatch (-want +got):\n%s", diff)
-		}
-
-		if got, want := b.String(), "Internal Server Error"; got != want {
-			t.Errorf("response body got: %q want: %q", got, want)
-		}
-	})
-}
-
-func TestNegativeMaxAge(t *testing.T) {
-	p := hsts.Plugin{MaxAge: -1 * time.Second}
-	m := safehttp.NewMachinery(p.Before, &testDispatcher{})
-
-	req := httptest.NewRequest("GET", "https://localhost/", nil)
-
-	// TODO(@mattiasgrenfeldt): Change from current to desired behavior once
-	// safehttp.ResponseWriter.ServerError has changed signature and been
-	// implemented.
-	t.Run("Current Behavior", func(t *testing.T) {
-		b := &strings.Builder{}
-		rr := newResponseRecorder(b)
-
-		m.HandleRequest(rr, req)
-
-		if want := 200; rr.status != want {
-			t.Errorf("status code got: %v want: %v", rr.status, want)
-		}
+	b := &strings.Builder{}
+	rr := newResponseRecorder(b)
 
-		wantHeaders := map[string][]string{}
-		if diff := cmp.Diff(wantHeaders, map[string][]string(rr.Header())); diff != "" {
-			t.Errorf("rr.Header() mismatch (-want +got):\n%s", diff)
-		}
+	m.HandleRequest(rr, req)
 
-		if got, want := b.String(), ""; got != want {
-			t.Errorf("response body got: %q want: %q", got, want)
-		}
-	})
-
-	t.Run("Desired Behavior", func(t *testing.T) {
-		t.Skip()
-		b := &strings.Builder{}
-		rr := newResponseRecorder(b)
-
-		m.HandleRequest(rr, req)
-
-		if want := 500; rr.status != want {
-			t.Errorf("status code got: %v want: %v", rr.status, want)
-		}
+	if want := 500; rr.status != want {
+		t.Errorf("status code got: %v want: %v", rr.status, want)
+	}
 
-		wantHeaders := map[string][]string{}
-		if diff := cmp.Diff(wantHeaders, map[string][]string(rr.Header())); diff != "" {
-			t.Errorf("rr.Header() mismatch (-want +got):\n%s", diff)
-		}
+	wantHeaders := map[string][]string{
+		"Content-Type":           {"text/plain; charset=utf-8"},
+		"X-Content-Type-Options": {"nosniff"},
+	}
+	if diff := cmp.Diff(wantHeaders, map[string][]string(rr.Header())); diff != "" {
+		t.Errorf("rr.Header() mismatch (-want +got):\n%s", diff)
+	}
 
-		if got, want := b.String(), "Internal Server Error"; got != want {
-			t.Errorf("response body got: %q want: %q", got, want)
-		}
-	})
+	if got, want := b.String(), "Internal Server Error\n"; got != want {
+		t.Errorf("response body got: %q want: %q", got, want)
+	}
 }