diff --git a/internal/middleware/recoverer.go b/internal/middleware/recoverer.go index 1b47d6c..8dcb76e 100644 --- a/internal/middleware/recoverer.go +++ b/internal/middleware/recoverer.go @@ -133,6 +133,11 @@ func (w *recoverResponseWriter) Unwrap() http.ResponseWriter { // what the access log records and the metrics count, and outside the // sentryhttp handler, whose Repanic option depends on something // further out recovering what it re-raises. +// +// Unlike http.Error on its own, it deletes any Set-Cookie the handler +// set before panicking, because a request that failed must not hand +// the client a credential; every other header is left to http.Error. +// See https://git.eeqj.de/sneak/webhooker/issues/193. func (s *Middleware) Recoverer() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func( @@ -164,6 +169,8 @@ func (s *Middleware) Recoverer() func(http.Handler) http.Handler { return } + rw.Header().Del("Set-Cookie") + http.Error( rw, http.StatusText( diff --git a/internal/middleware/recoverer_test.go b/internal/middleware/recoverer_test.go index bbfd309..fac5cc4 100644 --- a/internal/middleware/recoverer_test.go +++ b/internal/middleware/recoverer_test.go @@ -304,16 +304,44 @@ func TestRecovererRepanicsErrAbortHandler(t *testing.T) { ) } +// TestRecovererDropsSetCookieFromTheRecovered500 covers a handler that +// sets a cookie and a redirect target and then panics before sending +// anything. A request that failed must not hand the client a +// credential, so the 500 carries no cookie; Location is left alone. +func TestRecovererDropsSetCookieFromTheRecovered500(t *testing.T) { + t.Parallel() + + probe := newRecovererProbe( + t, false, + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Set-Cookie", "session=x") + w.Header().Set("Location", "/after") + + panic(panicMarker) + }, + ) + + resp, err := probe.get(t) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + assert.Empty(t, resp.Cookies()) + assert.Equal(t, "/after", resp.Header.Get("Location")) +} + // TestRecovererKeepsAnAlreadyCommittedResponse covers a handler that // panics after sending its status. The bytes are already on the wire, -// so a second WriteHeader would change nothing the client sees and -// would draw net/http's "superfluous response.WriteHeader" report. +// cookie included, so a second WriteHeader would change nothing the +// client sees and would draw net/http's "superfluous +// response.WriteHeader" report. func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) { t.Parallel() probe := newRecovererProbe( t, false, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Set-Cookie", "session=x") w.WriteHeader(committedStatus) _, _ = w.Write([]byte("partial")) @@ -331,6 +359,7 @@ func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) { assert.Equal(t, committedStatus, resp.StatusCode) assert.Equal(t, "partial", string(body)) + assert.Len(t, resp.Cookies(), 1) record := probe.panicRecord(t) assert.Equal(t, panicMarker, record["panic"])