Repository: google/go-safeweb (go)

Issue:
Proposal: merge `ClientError` and `ServerError`
I think w.Error(Status, optionalResponse) could be enough, the response writer has enough information to decide what to do when given a status that is >400. 

Pull request:
Replaced ClientError and ServerError with WriteError in ResponseWriter
Fixes #127 

Patch:
diff --git a/safehttp/mux.go b/safehttp/mux.go
index b55f746..95e0ffd 100644
--- a/safehttp/mux.go
+++ b/safehttp/mux.go
@@ -206,7 +206,7 @@ func (h handlerWithInterceptors) ServeHTTP(w http.ResponseWriter, r *http.Reques
 	// respond with a 500 Internal Server Error.
 	defer func() {
 		if r := recover(); r != nil {
-			rw.ServerError(StatusInternalServerError)
+			rw.WriteError(StatusInternalServerError)
 		}
 	}()
 
diff --git a/safehttp/plugins/collector/collector.go b/safehttp/plugins/collector/collector.go
index 563e1a2..697f668 100644
--- a/safehttp/plugins/collector/collector.go
+++ b/safehttp/plugins/collector/collector.go
@@ -87,12 +87,12 @@ type CSPReport struct {
 func Handler(handler func(Report), cspHandler func(CSPReport)) safehttp.Handler {
 	return safehttp.HandlerFunc(func(w *safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
 		if r.Method() != safehttp.MethodPost {
-			return w.ClientError(safehttp.StatusMethodNotAllowed)
+			return w.WriteError(safehttp.StatusMethodNotAllowed)
 		}
 
 		b, err := ioutil.ReadAll(r.Body())
 		if err != nil {
-			return w.ClientError(safehttp.StatusBadRequest)
+			return w.WriteError(safehttp.StatusBadRequest)
 		}
 
 		ct := r.Header.Get("Content-Type")
@@ -102,7 +102,7 @@ func Handler(handler func(Report), cspHandler func(CSPReport)) safehttp.Handler
 			return handleReport(handler, w, b)
 		}
 
-		return w.ClientError(safehttp.StatusUnsupportedMediaType)
+		return w.WriteError(safehttp.StatusUnsupportedMediaType)
 	})
 }
 
@@ -142,12 +142,12 @@ func handleDeprecatedCSPReports(h func(CSPReport), w *safehttp.ResponseWriter, b
 		ColumnNumber      uint            `json:"column-number"`
 	}{}
 	if err := json.Unmarshal(b, &r); err != nil {
-		return w.ClientError(safehttp.StatusBadRequest)
+		return w.WriteError(safehttp.StatusBadRequest)
 	}
 
 	if len(r.CSPReport) != 0 {
 		if err := json.Unmarshal(r.CSPReport, &r); err != nil {
-			return w.ClientError(safehttp.StatusBadRequest)
+			return w.WriteError(safehttp.StatusBadRequest)
 		}
 	}
 
@@ -192,7 +192,7 @@ func handleReport(h func(Report), w *safehttp.ResponseWriter, b []byte) safehttp
 		Body      json.RawMessage `json:"body"`
 	}
 	if err := json.Unmarshal(b, &rList); err != nil {
-		return w.ClientError(safehttp.StatusBadRequest)
+		return w.WriteError(safehttp.StatusBadRequest)
 	}
 
 	badReport := false
@@ -224,7 +224,7 @@ func handleReport(h func(Report), w *safehttp.ResponseWriter, b []byte) safehttp
 	}
 
 	if badReport {
-		return w.ClientError(safehttp.StatusBadRequest)
+		return w.WriteError(safehttp.StatusBadRequest)
 	}
 
 	return w.NoContent()
diff --git a/safehttp/plugins/fetch_metadata/fetch_metadata.go b/safehttp/plugins/fetch_metadata/fetch_metadata.go
index 3179c07..7a6b551 100644
--- a/safehttp/plugins/fetch_metadata/fetch_metadata.go
+++ b/safehttp/plugins/fetch_metadata/fetch_metadata.go
@@ -158,7 +158,7 @@ func (p *Plugin) Before(w *safehttp.ResponseWriter, r *safehttp.IncomingRequest)
 		if p.reportOnly {
 			return safehttp.Result{}
 		}
-		return w.ClientError(safehttp.StatusForbidden)
+		return w.WriteError(safehttp.StatusForbidden)
 	}
 
 	return safehttp.Result{}
diff --git a/safehttp/plugins/hsts/hsts.go b/safehttp/plugins/hsts/hsts.go
index 8d9fcc0..aabe90d 100644
--- a/safehttp/plugins/hsts/hsts.go
+++ b/safehttp/plugins/hsts/hsts.go
@@ -67,13 +67,13 @@ func Default() Interceptor {
 // response.
 func (it Interceptor) Before(w *safehttp.ResponseWriter, r *safehttp.IncomingRequest, cfgs interface{}) safehttp.Result {
 	if it.MaxAge < 0 {
-		return w.ServerError(safehttp.StatusInternalServerError)
+		return w.WriteError(safehttp.StatusInternalServerError)
 	}
 
 	if !it.BehindProxy && r.TLS == nil {
 		u, err := url.Parse(r.URL.String())
 		if err != nil {
-			return w.ServerError(safehttp.StatusInternalServerError)
+			return w.WriteError(safehttp.StatusInternalServerError)
 		}
 		u.Scheme = "https"
 		return w.Redirect(r, u.String(), safehttp.StatusMovedPermanently)
diff --git a/safehttp/plugins/xsrf/xsrf.go b/safehttp/plugins/xsrf/xsrf.go
index a9f1587..d7d9966 100644
--- a/safehttp/plugins/xsrf/xsrf.go
+++ b/safehttp/plugins/xsrf/xsrf.go
@@ -17,6 +17,7 @@ package xsrf
 import (
 	"context"
 	"errors"
+
 	"github.com/google/go-safeweb/safehttp"
 	"golang.org/x/net/xsrftoken"
 )
@@ -76,7 +77,7 @@ func Token(r *safehttp.IncomingRequest) (string, error) {
 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)
+		return w.WriteError(safehttp.StatusUnauthorized)
 	}
 
 	actionID := r.Method() + " " + r.URL.Path()
@@ -89,18 +90,18 @@ func (i *Interceptor) Before(w *safehttp.ResponseWriter, r *safehttp.IncomingReq
 			// present.
 			mf, err := r.MultipartForm(32 << 20)
 			if err != nil {
-				return w.ClientError(safehttp.StatusBadRequest)
+				return w.WriteError(safehttp.StatusBadRequest)
 			}
 			f = &mf.Form
 		}
 
 		tok := f.String(TokenKey, "")
 		if f.Err() != nil || tok == "" {
-			return w.ClientError(safehttp.StatusUnauthorized)
+			return w.WriteError(safehttp.StatusUnauthorized)
 		}
 
 		if ok := xsrftoken.Valid(tok, i.SecretAppKey, userID, actionID); !ok {
-			return w.ClientError(safehttp.StatusForbidden)
+			return w.WriteError(safehttp.StatusForbidden)
 		}
 	}
 
diff --git a/safehttp/response_writer.go b/safehttp/response_writer.go
index d917eed..21fc577 100644
--- a/safehttp/response_writer.go
+++ b/safehttp/response_writer.go
@@ -89,26 +89,10 @@ func (w *ResponseWriter) NoContent() Result {
 	return Result{}
 }
 
-// ClientError TODO
-func (w *ResponseWriter) ClientError(code StatusCode) Result {
+// WriteError writes an error response (400-599) according to the provided status
+// code.
+func (w *ResponseWriter) WriteError(code StatusCode) Result {
 	w.markWritten()
-	if code < 400 || code >= 500 {
-		// TODO(@mihalimara22): Replace this when we decide how to handle this case
-		panic("wrong method called")
-	}
-	http.Error(w.rw, http.StatusText(int(code)), int(code))
-	return Result{}
-}
-
-// ServerError TODO
-func (w *ResponseWriter) ServerError(code StatusCode) Result {
-	w.markWritten()
-	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.
-		panic("wrong method called")
-	}
 	http.Error(w.rw, http.StatusText(int(code)), int(code))
 	return Result{}
 }


Test patch:
diff --git a/safehttp/mux_test.go b/safehttp/mux_test.go
index 171b9c3..baa82f9 100644
--- a/safehttp/mux_test.go
+++ b/safehttp/mux_test.go
@@ -176,7 +176,7 @@ func (p setHeaderInterceptor) Before(w *safehttp.ResponseWriter, _ *safehttp.Inc
 type internalErrorInterceptor struct{}
 
 func (internalErrorInterceptor) Before(w *safehttp.ResponseWriter, _ *safehttp.IncomingRequest, cfg interface{}) safehttp.Result {
-	return w.ServerError(safehttp.StatusInternalServerError)
+	return w.WriteError(safehttp.StatusInternalServerError)
 }
 
 type claimHeaderInterceptor struct {
@@ -408,7 +408,7 @@ type interceptorTwo struct {
 
 func (interceptorTwo) Before(w *safehttp.ResponseWriter, r *safehttp.IncomingRequest, cfg interface{}) safehttp.Result {
 	if w.Header().Get("pizza") != "diavola" {
-		w.ServerError(safehttp.StatusInternalServerError)
+		w.WriteError(safehttp.StatusInternalServerError)
 	}
 	w.Header().Set("spaghetti", "bolognese")
 	return safehttp.Result{}
@@ -419,7 +419,7 @@ type interceptorThree struct {
 
 func (interceptorThree) Before(w *safehttp.ResponseWriter, r *safehttp.IncomingRequest, cfg interface{}) safehttp.Result {
 	if w.Header().Get("spaghetti") != "bolognese" {
-		w.ServerError(safehttp.StatusInternalServerError)
+		w.WriteError(safehttp.StatusInternalServerError)
 	}
 	w.Header().Set("dessert", "tiramisu")
 	return safehttp.Result{}
diff --git a/safehttp/response_writer_test.go b/safehttp/response_writer_test.go
index 3de1b3c..d8ee1cc 100644
--- a/safehttp/response_writer_test.go
+++ b/safehttp/response_writer_test.go
@@ -81,17 +81,10 @@ func TestResponseWriterWriteTwicePanic(t *testing.T) {
 			},
 		},
 		{
-			name: "Call ClientError twice",
+			name: "Call WriteError twice",
 			write: func(w *safehttp.ResponseWriter) {
-				w.ClientError(safehttp.StatusBadRequest)
-				w.ClientError(safehttp.StatusBadRequest)
-			},
-		},
-		{
-			name: "Call ServerError twice",
-			write: func(w *safehttp.ResponseWriter) {
-				w.ServerError(safehttp.StatusInternalServerError)
-				w.ServerError(safehttp.StatusInternalServerError)
+				w.WriteError(safehttp.StatusInternalServerError)
+				w.WriteError(safehttp.StatusInternalServerError)
 			},
 		},
 		{