Repository: google/go-safeweb
Pull request #225 (2020-11-30 14:47:00)

All panics are now propagated to `net/http`.
Previously we were making attempts to recover from some of the panics
and possibly run `OnError` interceptors. This complicates the codebase,
pushes it further from `net/http` behavior and still isn't bulletproof
(what if `OnError` panics?).

In order to prevent from information disclosure, when a panic occurs we
clear all the HTTP response headers. Note that this is only possible if
nothing has been written to the underlying `net/http.ResponseWriter`
yet.

#164, #79

Fixes: #157.


Patch:
diff --git a/safehttp/flight.go b/safehttp/flight.go
index 99090d8..5baefc1 100644
--- a/safehttp/flight.go
+++ b/safehttp/flight.go
@@ -29,10 +29,7 @@ type flight struct {
 	code   StatusCode
 	header Header
 
-	// TODO(kele): we need to distinguish between calling Write and actually
-	// writing to the net/http.ResponseWriter.
-	written      bool
-	writtenError bool
+	written bool
 }
 
 // DeprecatedNewResponseWriter creates a ResponseWriter implementation that has
@@ -70,13 +67,18 @@ func processRequest(cfg HandlerConfig, rw http.ResponseWriter, req *http.Request
 		req:    NewIncomingRequest(req),
 	}
 
-	// The net/http package recovers handler panics, but we cannot rely on that
-	// behavior here. The reason is, we might need to run some interceptor
-	// stages interceptors before we respond with a 500 Internal Server Error.
-	// Therefore we're calling WriteError.
+	// The net/http package handles all panics. In the early days of the
+	// framework we were handling them ourselves and running interceptors after
+	// a panic happened, but this adds lots of complexity to the codebase and
+	// still isn't perfect (e.g. what if OnError panics?). Instead, we just make
+	// sure to clear all the headers and cookies.
 	defer func() {
 		if r := recover(); r != nil {
-			f.WriteError(StatusInternalServerError)
+			// Clear all headers.
+			for h := range f.rw.Header() {
+				delete(f.rw.Header(), h)
+			}
+			panic(r)
 		}
 	}()
 
@@ -86,7 +88,6 @@ func processRequest(cfg HandlerConfig, rw http.ResponseWriter, req *http.Request
 			return
 		}
 	}
-
 	f.cfg.Handler.ServeHTTP(f, f.req)
 	if !f.written {
 		f.NoContent()
@@ -95,8 +96,6 @@ func processRequest(cfg HandlerConfig, rw http.ResponseWriter, req *http.Request
 
 // Write dispatches the response to the Dispatcher. This will be written to the
 // underlying http.ResponseWriter if the Dispatcher decides it's safe to do so.
-//
-// TODO: replace panics with proper error handling when writing the response fails.
 func (f *flight) Write(resp Response) Result {
 	if f.written {
 		panic("ResponseWriter was already written to")
@@ -142,13 +141,10 @@ func (f *flight) NoContent() Result {
 // If the ResponseWriter has already been written to, then this method will panic.
 func (f *flight) WriteError(code StatusCode) Result {
 	// TODO: accept custom error responses that need to go through the dispatcher.
-	if f.writtenError {
-		panic("ResponseWriter.WriteError called twice")
+	if f.written {
+		panic("ResponseWriter was already written to")
 	}
 	f.written = true
-	f.writtenError = true
-	// TODO: we cannot really write if the Dispatcher already started writing
-	// but panicked, resulting in a WriteError call.
 	resp := &ErrorResponse{Code: code}
 	f.errorPhase(resp)
 	http.Error(f.rw, http.StatusText(int(resp.Code)), int(resp.Code))


Test patch:
diff --git a/safehttp/flight_test.go b/safehttp/flight_test.go
new file mode 100644
index 0000000..ddf33c5
--- /dev/null
+++ b/safehttp/flight_test.go
@@ -0,0 +1,179 @@
+// 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 (
+	"fmt"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/google/go-safeweb/safehttp"
+	"github.com/google/go-safeweb/safehttp/safehttptest"
+	"github.com/google/safehtml"
+)
+
+type panickingInterceptor struct {
+	before, commit, onError bool
+}
+
+func (p panickingInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
+	if p.before {
+		panic("before")
+	}
+	return safehttp.NotWritten()
+}
+
+func (p panickingInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, cfg safehttp.InterceptorConfig) {
+	if p.commit {
+		panic("commit")
+	}
+}
+
+func (p panickingInterceptor) OnError(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, cfg safehttp.InterceptorConfig) {
+	if p.onError {
+		panic("onError")
+	}
+}
+
+func TestFlightInterceptorPanic(t *testing.T) {
+	tests := []struct {
+		desc        string
+		interceptor panickingInterceptor
+		wantPanic   bool
+	}{
+		{
+			desc:        "panic in Before",
+			interceptor: panickingInterceptor{before: true},
+			wantPanic:   true,
+		},
+		{
+			desc:        "panic in Commit",
+			interceptor: panickingInterceptor{commit: true},
+			wantPanic:   true,
+		},
+		{
+			desc:        "panic in OnError, but handler finishes successfully, so it doesn't happen",
+			interceptor: panickingInterceptor{onError: true},
+			wantPanic:   false,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.desc, func(t *testing.T) {
+			mb := &safehttp.ServeMuxConfig{}
+			mb.Intercept(tc.interceptor)
+			mb.Handle("/search", safehttp.MethodGet, safehttp.HandlerFunc(func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
+				// IMPORTANT: We are setting the header here and expecting to be
+				// cleared if a panic occurs.
+				w.Header().Set("foo", "bar")
+				return w.Write(safehtml.HTMLEscaped("<h1>Hello World!</h1>"))
+			}))
+			mux := mb.Mux()
+
+			req := httptest.NewRequest(safehttp.MethodGet, "http://foo.com/search", nil)
+			b := &strings.Builder{}
+			rw := safehttptest.NewTestResponseWriter(b)
+
+			defer func() {
+				r := recover()
+				if !tc.wantPanic {
+					if r != nil {
+						t.Fatalf("unexpected panic %v", r)
+					}
+					return
+				}
+				if r == nil {
+					t.Fatal("expected panic")
+				}
+				// Good, the panic got propagated.
+				if len(rw.Header()) > 0 {
+					t.Errorf("ResponseWriter.Header() got %v, want empty", rw.Header())
+				}
+			}()
+			mux.ServeHTTP(rw, req)
+		})
+	}
+}
+
+func TestFlightHandlerPanic(t *testing.T) {
+	mb := &safehttp.ServeMuxConfig{}
+	mb.Handle("/search", safehttp.MethodGet, safehttp.HandlerFunc(func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
+		// IMPORTANT: We are setting the header here and expecting to be
+		// cleared if a panic occurs.
+		w.Header().Set("foo", "bar")
+		panic("handler")
+	}))
+	mux := mb.Mux()
+
+	req := httptest.NewRequest(safehttp.MethodGet, "http://foo.com/search", nil)
+	b := &strings.Builder{}
+	rw := safehttptest.NewTestResponseWriter(b)
+
+	defer func() {
+		r := recover()
+		if r == nil {
+			t.Fatalf("expected panic")
+		}
+		// Good, the panic got propagated.
+		if len(rw.Header()) > 0 {
+			t.Errorf("ResponseWriter.Header() got %v, want empty", rw.Header())
+		}
+	}()
+	mux.ServeHTTP(rw, req)
+}
+
+func TestFlightDoubleWritePanics(t *testing.T) {
+	writeFuncs := map[string]func(safehttp.ResponseWriter, *safehttp.IncomingRequest) safehttp.Result{
+		"Write": func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
+			return w.Write(safehtml.HTMLEscaped("Hello"))
+		},
+		"NoContent": func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result { return w.NoContent() },
+		"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 {
+		for secondWriteName, secondWrite := range writeFuncs {
+			t.Run(fmt.Sprintf("%s->%s", firstWriteName, secondWriteName), func(t *testing.T) {
+				mb := &safehttp.ServeMuxConfig{}
+				mb.Handle("/search", safehttp.MethodGet, safehttp.HandlerFunc(func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
+					firstWrite(w, r)
+					secondWrite(w, r) // this should panic
+					t.Fatal("should never reach this point")
+					return safehttp.Result{}
+				}))
+				mux := mb.Mux()
+
+				req := httptest.NewRequest(safehttp.MethodGet, "http://foo.com/search", nil)
+				b := &strings.Builder{}
+				rw := safehttptest.NewTestResponseWriter(b)
+				defer func() {
+					if r := recover(); r == nil {
+						t.Fatalf("expected panic")
+					}
+					// Good, the panic got propagated.
+					// Note: we are not testing the response headers here, as the first write might have already succeeded.
+				}()
+				mux.ServeHTTP(rw, req)
+			})
+
+		}
+	}
+
+}
diff --git a/safehttp/mux_test.go b/safehttp/mux_test.go
index 47fb638..4abe298 100644
--- a/safehttp/mux_test.go
+++ b/safehttp/mux_test.go
@@ -216,18 +216,6 @@ func claimInterceptorSetHeader(w safehttp.ResponseWriter, r *safehttp.IncomingRe
 	f([]string{value})
 }
 
-type panickingInterceptor struct{}
-
-func (panickingInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
-	panic("bad")
-}
-
-func (panickingInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, cfg safehttp.InterceptorConfig) {
-}
-
-func (panickingInterceptor) OnError(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, cfg safehttp.InterceptorConfig) {
-}
-
 type committerInterceptor struct{}
 
 func (committerInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
@@ -326,26 +314,6 @@ func TestMuxInterceptors(t *testing.T) {
 			},
 			wantBody: "&lt;h1&gt;Hello World!&lt;/h1&gt;",
 		},
-		{
-			name: "Panicking interceptor recovered by ServeMux",
-			mux: func() *safehttp.ServeMux {
-				mb := &safehttp.ServeMuxConfig{}
-				mb.Intercept(panickingInterceptor{})
-
-				registeredHandler := safehttp.HandlerFunc(func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
-					return w.Write(safehtml.HTMLEscaped("<h1>Hello World!</h1>"))
-				})
-				mb.Handle("/bar", safehttp.MethodGet, registeredHandler)
-
-				return mb.Mux()
-			}(),
-			wantStatus: safehttp.StatusInternalServerError,
-			wantHeaders: map[string][]string{
-				"Content-Type":           {"text/plain; charset=utf-8"},
-				"X-Content-Type-Options": {"nosniff"},
-			},
-			wantBody: "Internal Server Error\n",
-		},
 		{
 			name: "Commit phase sets header",
 			mux: func() *safehttp.ServeMux {
@@ -640,39 +608,3 @@ func TestMuxHandlerReturnsNotWritten(t *testing.T) {
 		t.Errorf(`response body got: %q want: ""`, got)
 	}
 }
-
-type onErrorWriteInterceptor struct{}
-
-func (onErrorWriteInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.IncomingRequest, _ safehttp.InterceptorConfig) safehttp.Result {
-	return w.WriteError(safehttp.StatusBadRequest)
-}
-
-func (onErrorWriteInterceptor) Commit(_ safehttp.ResponseHeadersWriter, _ *safehttp.IncomingRequest, _ safehttp.Response, _ safehttp.InterceptorConfig) {
-}
-
-func (onErrorWriteInterceptor) OnError(w safehttp.ResponseHeadersWriter, _ *safehttp.IncomingRequest, _ safehttp.Response, _ safehttp.InterceptorConfig) {
-	panic("panic during onError")
-}
-
-func TestMuxInterceptorWriteInOnError(t *testing.T) {
-	b := &strings.Builder{}
-	rw := safehttptest.NewTestResponseWriter(b)
-
-	req := httptest.NewRequest(safehttp.MethodGet, "http://foo.com/bar", nil)
-
-	mb := &safehttp.ServeMuxConfig{}
-	mb.Intercept(onErrorWriteInterceptor{})
-
-	registeredHandler := safehttp.HandlerFunc(func(w safehttp.ResponseWriter, r *safehttp.IncomingRequest) safehttp.Result {
-		return w.Write(safehtml.HTMLEscaped("<h1>Hello World!</h1>"))
-	})
-	mb.Handle("/bar", safehttp.MethodGet, registeredHandler)
-
-	defer func() {
-		if r := recover(); r == nil {
-			t.Errorf(`mb.Mux().ServeHTTP(rw, req): expected panic`)
-		}
-	}()
-
-	mb.Mux().ServeHTTP(rw, req)
-}