From 718865e075e258cffa5016515e2cadf3b2c348f8 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 1 Oct 2026 20:43:01 +0000 Subject: [PATCH] Render admin page errors in the normal layout (closes #382) Every 400, 403, 404 and 500 on an admin page now answers with an error page in the normal layout: one fixed line for the status and a link back to the webhook list, or to sign-in when nobody is signed in. The router's handler for unknown paths and the CSRF middleware's refusal use the same page. Status codes are unchanged. The receiver, the healthcheck and /metrics keep their plain answers. If the error page itself cannot render, the answer is the same status in plain text. Model: opus-5-5 --- internal/handlers/auth.go | 28 +---- internal/handlers/delivery_replay.go | 16 ++- internal/handlers/error_page_test.go | 53 +++++++++ internal/handlers/event_body.go | 10 +- internal/handlers/event_resubmit.go | 16 +-- internal/handlers/export_test.go | 14 ++- internal/handlers/handlers.go | 103 +++++++++++++--- internal/handlers/handlers_test.go | 8 +- internal/handlers/profile.go | 44 +++---- internal/handlers/profile_test.go | 6 +- internal/handlers/source_management.go | 96 ++++++--------- internal/handlers/target_edit.go | 8 +- internal/handlers/webhook.go | 19 ++- internal/middleware/csrf.go | 8 +- internal/middleware/csrf_test.go | 24 ++-- internal/middleware/logbound_test.go | 4 +- internal/server/error_page_test.go | 155 +++++++++++++++++++++++++ internal/server/routes.go | 16 ++- templates/error.html | 15 +++ templates/navbar.html | 6 + 20 files changed, 467 insertions(+), 182 deletions(-) create mode 100644 internal/handlers/error_page_test.go create mode 100644 internal/server/error_page_test.go create mode 100644 templates/error.html diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 39fa5dc..d4b93d7 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -36,7 +36,7 @@ func (h *Handlers) HandleLoginSubmit() http.HandlerFunc { err := r.ParseForm() if err != nil { h.log.Error("failed to parse form", "error", err) - http.Error(w, "Bad request", http.StatusBadRequest) + h.renderError(w, r, http.StatusBadRequest) return } @@ -165,11 +165,7 @@ func (h *Handlers) authenticateUser( valid, err := database.VerifyPassword(password, user.Password) if err != nil { - h.log.Error("failed to verify password", "error", err) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, r, "failed to verify password", err) return user, err } @@ -241,24 +237,14 @@ func (h *Handlers) createAuthenticatedSession( ) error { oldSess, err := h.session.Get(r) if err != nil { - h.log.Error("failed to get session", "error", err) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, r, "failed to get session", err) return err } sess, err := h.session.Regenerate(r, w, oldSess) if err != nil { - h.log.Error( - "failed to regenerate session", "error", err, - ) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, r, "failed to regenerate session", err) return err } @@ -267,11 +253,7 @@ func (h *Handlers) createAuthenticatedSession( err = h.session.Save(r, w, sess) if err != nil { - h.log.Error("failed to save session", "error", err) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, r, "failed to save session", err) return err } diff --git a/internal/handlers/delivery_replay.go b/internal/handlers/delivery_replay.go index ee9425b..d7a6f4f 100644 --- a/internal/handlers/delivery_replay.go +++ b/internal/handlers/delivery_replay.go @@ -105,9 +105,7 @@ func (h *Handlers) HandleDeliveryReplay() http.HandlerFunc { // middleware, which runs before CSRF parses the form. err := r.ParseForm() if err != nil { - http.Error( - w, "Bad request", http.StatusBadRequest, - ) + h.renderError(w, r, http.StatusBadRequest) return } @@ -124,14 +122,14 @@ func (h *Handlers) replayDelivery( webhook database.Webhook, ) { if !h.dbMgr.DBExists(webhook.ID) { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } webhookDB, err := h.dbMgr.GetDB(webhook.ID) if err != nil { - h.serverError(w, "failed to get webhook database", err) + h.serverError(w, r, "failed to get webhook database", err) return } @@ -173,7 +171,7 @@ func (h *Handlers) loadReplaySource( &original, "id = ?", chi.URLParam(r, "deliveryID"), ).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return nil, false } @@ -195,7 +193,7 @@ func (h *Handlers) queueReplay( ) if err != nil { h.serverError( - w, "failed to count in-flight deliveries", err, + w, r, "failed to count in-flight deliveries", err, ) return @@ -212,7 +210,7 @@ func (h *Handlers) queueReplay( err = webhookDB. First(&event, "id = ?", original.EventID).Error if err != nil { - h.serverError(w, "failed to load event for replay", err) + h.serverError(w, r, "failed to load event for replay", err) return } @@ -222,7 +220,7 @@ func (h *Handlers) queueReplay( ) if err != nil { h.serverError( - w, "failed to create replay delivery", err, + w, r, "failed to create replay delivery", err, ) return diff --git a/internal/handlers/error_page_test.go b/internal/handlers/error_page_test.go new file mode 100644 index 0000000..1b87fe9 --- /dev/null +++ b/internal/handlers/error_page_test.go @@ -0,0 +1,53 @@ +package handlers_test + +import ( + "context" + "html/template" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "sneak.berlin/go/webhooker/internal/handlers" +) + +// TestErrorPage_RenderFailureKeepsStatus proves that an error page +// which cannot render answers with the status it was reporting, as +// plain text, and is not attempted again: a page whose own render +// fails reaches the error page, and the error page failing as well +// ends there with the 500. +func TestErrorPage_RenderFailureKeepsStatus(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + app := newTestApp(t, &h) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + // .Status is an int, so asking it for a field fails the render. + failing := `{{.Status.Missing}}` + h.AddTemplateForTest("error.html", template.Must( + template.New("error").Parse(failing), + )) + h.AddTemplateForTest("failing.html", template.Must( + template.New("failing").Parse(`{{.Data.Missing}}`), + )) + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil, + ) + + w := httptest.NewRecorder() + h.HandleErrorPage(http.StatusNotFound).ServeHTTP(w, req) + + assert.Equal(t, http.StatusNotFound, w.Code) + assert.Equal(t, "Not Found\n", w.Body.String()) + + w = httptest.NewRecorder() + h.RenderTemplateForTest(w, req, "failing.html", 0) + + assert.Equal(t, http.StatusInternalServerError, w.Code) + assert.Equal(t, "Internal Server Error\n", w.Body.String()) +} diff --git a/internal/handlers/event_body.go b/internal/handlers/event_body.go index d46b9a5..cf2452f 100644 --- a/internal/handlers/event_body.go +++ b/internal/handlers/event_body.go @@ -52,7 +52,7 @@ func (h *Handlers) HandleEventBodyDownload() http.HandlerFunc { // steered by a client. eventID, err := uuid.Parse(chi.URLParam(r, "eventID")) if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -103,21 +103,21 @@ func (h *Handlers) serveEventBody( eventID string, ) { if !h.dbMgr.DBExists(webhook.ID) { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } webhookDB, err := h.dbMgr.GetDB(webhook.ID) if err != nil { - h.serverError(w, "failed to get webhook database", err) + h.serverError(w, r, "failed to get webhook database", err) return } body, found, err := eventBody(webhookDB, webhook.ID, eventID) if err != nil { - h.serverError(w, "failed to read event body", err) + h.serverError(w, r, "failed to read event body", err) return } @@ -130,7 +130,7 @@ func (h *Handlers) serveEventBody( // row and the whole body is served, or it does not and the // response is a clean 404. if !found { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } diff --git a/internal/handlers/event_resubmit.go b/internal/handlers/event_resubmit.go index 21e54b8..46df054 100644 --- a/internal/handlers/event_resubmit.go +++ b/internal/handlers/event_resubmit.go @@ -99,7 +99,7 @@ func (h *Handlers) HandleEventResubmit() http.HandlerFunc { // middleware, which runs before CSRF parses the form. err := r.ParseForm() if err != nil { - http.Error(w, "Bad request", http.StatusBadRequest) + h.renderError(w, r, http.StatusBadRequest) return } @@ -120,20 +120,20 @@ func (h *Handlers) resubmitEvent( // alphabet rather than from the request. eventID, err := uuid.Parse(chi.URLParam(r, "eventID")) if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } if !h.dbMgr.DBExists(webhook.ID) { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } webhookDB, err := h.dbMgr.GetDB(webhook.ID) if err != nil { - h.serverError(w, "failed to get webhook database", err) + h.serverError(w, r, "failed to get webhook database", err) return } @@ -147,7 +147,7 @@ func (h *Handlers) resubmitEvent( webhookDB, webhook.ID, eventID.String(), ) if err != nil { - h.serverError(w, "failed to load event to resubmit", err) + h.serverError(w, r, "failed to load event to resubmit", err) return } @@ -155,7 +155,7 @@ func (h *Handlers) resubmitEvent( // A miss is a 404 whether the event was reaped, belongs to // another webhook, or never existed. if !found { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -207,7 +207,7 @@ func (h *Handlers) queueResubmit( // inactive one is skipped rather than refused. targets, err := h.loadActiveTargets(webhook.ID) if err != nil { - h.serverError(w, "failed to query targets", err) + h.serverError(w, r, "failed to query targets", err) return } @@ -225,7 +225,7 @@ func (h *Handlers) queueResubmit( targets, ) if err != nil { - h.serverError(w, "failed to store resubmitted event", err) + h.serverError(w, r, "failed to store resubmitted event", err) return } diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go index 4612e94..dfd9018 100644 --- a/internal/handlers/export_test.go +++ b/internal/handlers/export_test.go @@ -1,9 +1,11 @@ package handlers import ( + "context" "html/template" "log/slog" "net/http" + "net/http/httptest" "sneak.berlin/go/webhooker/internal/database" ) @@ -63,12 +65,20 @@ func (s *Handlers) LoadEventLogViewsForTest( page int, ) []EventLogView { views, _, _ := s.loadEventsWithDeliveries( - w, webhook, nil, page, + w, newRequestForTest(), webhook, nil, page, ) return views } +// newRequestForTest is the request the helpers here pass on for +// callers that have none: it is used only to render the error page. +func newRequestForTest() *http.Request { + return httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil, + ) +} + // AddTemplateForTest registers a template under a page name so that // the handlers_test package can drive the render path with a // template of its own. @@ -122,5 +132,5 @@ func (s *Handlers) BuildDatabaseTargetConfigForTest( w http.ResponseWriter, expiry string, ) (string, error) { - return s.buildDatabaseTargetConfig(w, expiry) + return s.buildDatabaseTargetConfig(w, newRequestForTest(), expiry) } diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 349771e..2b0523f 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -135,6 +135,7 @@ func New( "source_edit.html": parsePageTemplate("source_edit.html"), "source_logs.html": parsePageTemplate("source_logs.html"), "target_edit.html": parsePageTemplate("target_edit.html"), + "error.html": parsePageTemplate("error.html"), } lc.Append(fx.Hook{ @@ -146,6 +147,15 @@ func New( return s, nil } +// HandleErrorPage returns a handler that answers every request with +// the error page for status. The router uses it for unknown paths and +// the CSRF middleware for a refused form. +func (s *Handlers) HandleErrorPage(status int) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + s.renderError(w, r, status) + } +} + func (s *Handlers) respondJSON( w http.ResponseWriter, _ *http.Request, @@ -163,15 +173,72 @@ func (s *Handlers) respondJSON( } } -// serverError logs an error and sends a 500 response. +// serverError logs an error and answers with the 500 error page. func (s *Handlers) serverError( - w http.ResponseWriter, msg string, err error, + w http.ResponseWriter, r *http.Request, msg string, err error, ) { s.log.Error(msg, "error", err) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + s.renderError(w, r, http.StatusInternalServerError) +} + +// renderError answers with status and the error page: the normal +// layout, one fixed line explaining the status, and a link back to the +// webhook list, or to sign-in when nobody is signed in. +// +// It renders the page itself rather than through renderTemplate, +// whose own failure comes here. If the error page cannot render +// either, the answer is the same status in plain text: never a second +// attempt, and never a different status. +func (s *Handlers) renderError( + w http.ResponseWriter, + r *http.Request, + status int, +) { + data := s.pageData(r, map[string]any{ + "Status": status, + "StatusText": http.StatusText(status), + "Message": errorPageText(status), + }) + + var buf bytes.Buffer + + err := s.templates["error.html"].Execute(&buf, data) + if err != nil { + s.log.Error("failed to render error page", "error", err) + http.Error(w, http.StatusText(status), status) + + return + } + + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(status) + + _, err = buf.WriteTo(w) + if err != nil { + s.log.Error("failed to write error page", "error", err) + } +} + +// errorPageText is the line the error page shows for status. It is +// fixed per status, so the page tells the reader no more than the +// plain-text answers it replaced did. +func errorPageText(status int) string { + switch status { + case http.StatusBadRequest: + return "The request could not be read." + case http.StatusForbidden: + return "The request was refused. If it came from a form " + + "left open for a long time, reload the page and try " + + "again." + case http.StatusNotFound: + return "There is nothing here. It may have been deleted, " + + "or the address may be wrong." + case http.StatusServiceUnavailable: + return "The server is busy. Please try again in a moment." + default: // http.StatusInternalServerError + return "Something went wrong on the server. Please try " + + "again." + } } // UserInfo represents user information for templates @@ -224,14 +291,17 @@ func (s *Handlers) renderTemplate( "template not found", "template", pageTemplate, ) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + s.renderError(w, r, http.StatusInternalServerError) return } + s.executeTemplate(w, r, tmpl, s.pageData(r, data)) +} + +// pageData adds the fields the shared layout renders to a page's own +// data. +func (s *Handlers) pageData(r *http.Request, data any) any { userInfo := s.getUserInfo(r) csrfToken := middleware.CSRFToken(r) @@ -245,19 +315,16 @@ func (s *Handlers) renderTemplate( m["User"] = userInfo m["CSRFToken"] = csrfToken m["Version"] = version - s.executeTemplate(w, tmpl, m) - return + return m } - wrapper := templateDataWrapper{ + return templateDataWrapper{ User: userInfo, CSRFToken: csrfToken, Version: version, Data: data, } - - s.executeTemplate(w, tmpl, wrapper) } // executeTemplate renders the template into a buffer and writes to @@ -270,6 +337,7 @@ func (s *Handlers) renderTemplate( // this reason. func (s *Handlers) executeTemplate( w http.ResponseWriter, + r *http.Request, tmpl *template.Template, data any, ) { @@ -280,10 +348,7 @@ func (s *Handlers) executeTemplate( s.log.Error( "failed to execute template", "error", err, ) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + s.renderError(w, r, http.StatusInternalServerError) return } diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 3e9874a..a18c914 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -307,10 +307,14 @@ func TestRenderTemplateMidRenderErrorSendsNoPartialBody(t *testing.T) { t, http.StatusInternalServerError, w.Code, "a failed render must report a 500", ) - assert.Equal( - t, "Internal server error\n", w.Body.String(), + assert.NotContains( + t, w.Body.String(), partialPageMarker, "the response must carry no part of the aborted page", ) + assert.Contains( + t, w.Body.String(), "500 Internal Server Error", + "a failed render must answer with the error page", + ) } func TestBuildDatabaseTargetConfig_Valid(t *testing.T) { diff --git a/internal/handlers/profile.go b/internal/handlers/profile.go index 9d929b4..71fc163 100644 --- a/internal/handlers/profile.go +++ b/internal/handlers/profile.go @@ -1,7 +1,6 @@ package handlers import ( - "context" "net/http" "github.com/go-chi/chi" @@ -37,14 +36,14 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc { err := r.ParseForm() if err != nil { h.log.Error("failed to parse form", "error", err) - http.Error(w, "Bad request", http.StatusBadRequest) + h.renderError(w, r, http.StatusBadRequest) return } successMessage, errorMessage, handled := h.applyPasswordChange( - r.Context(), w, + r, sessionUsername, // PostFormValue, not FormValue: the credential must // come from the body, never from the query string. @@ -66,12 +65,12 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc { // applyPasswordChange verifies the current password and, on success, // persists a fresh hash for the user, reusing the same helpers that // bootstrap the admin user. It returns the success and error messages -// to display on the profile page. On an internal failure it writes a -// 500 response itself and returns handled=false, signalling the caller +// to display on the profile page. On an internal failure it writes the +// error page itself and returns handled=false, signalling the caller // to stop without re-rendering the page. func (h *Handlers) applyPasswordChange( - ctx context.Context, w http.ResponseWriter, + r *http.Request, username, currentPassword, newPassword, confirmPassword string, ) (string, string, bool) { // This endpoint verifies one password and hashes another, at @@ -79,15 +78,10 @@ func (h *Handlers) applyPasswordChange( // endpoint uses. The bound is per hash, not per endpoint: leaving // this path outside it would leave a hole in it. The slot is held // across both hashes. - release, ok := h.mw.BeginPasswordVerification(ctx) + release, ok := h.mw.BeginPasswordVerification(r.Context()) if !ok { h.log.Warn("password verification capacity exhausted") - http.Error( - w, - "The server is busy verifying credentials. "+ - "Please try again.", - http.StatusServiceUnavailable, - ) + h.renderError(w, r, http.StatusServiceUnavailable) return "", "", false } @@ -103,7 +97,7 @@ func (h *Handlers) applyPasswordChange( ).First(&user).Error if err != nil { h.serverError( - w, "failed to load user for password change", err, + w, r, "failed to load user for password change", err, ) return "", "", false @@ -113,7 +107,7 @@ func (h *Handlers) applyPasswordChange( currentPassword, user.Password, ) if err != nil { - h.serverError(w, "failed to verify password", err) + h.serverError(w, r, "failed to verify password", err) return "", "", false } @@ -132,7 +126,7 @@ func (h *Handlers) applyPasswordChange( hashedPassword, err := database.HashPassword(newPassword) if err != nil { - h.serverError(w, "failed to hash new password", err) + h.serverError(w, r, "failed to hash new password", err) return "", "", false } @@ -141,7 +135,7 @@ func (h *Handlers) applyPasswordChange( "password", hashedPassword, ).Error if err != nil { - h.serverError(w, "failed to update password", err) + h.serverError(w, r, "failed to update password", err) return "", "", false } @@ -162,7 +156,7 @@ func (h *Handlers) profileOwnerOrDeny( ) (string, string, bool) { requestedUsername := chi.URLParam(r, "username") if requestedUsername == "" { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return "", "", false } @@ -172,7 +166,7 @@ func (h *Handlers) profileOwnerOrDeny( // unexpected retrieval error. sess, err := h.session.Get(r) if err != nil { - h.serverError(w, "failed to get session", err) + h.serverError(w, r, "failed to get session", err) return "", "", false } @@ -180,10 +174,7 @@ func (h *Handlers) profileOwnerOrDeny( sessionUsername, ok := h.session.GetUsername(sess) if !ok { h.log.Error("authenticated session missing username") - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.renderError(w, r, http.StatusInternalServerError) return "", "", false } @@ -191,17 +182,14 @@ func (h *Handlers) profileOwnerOrDeny( sessionUserID, ok := h.session.GetUserID(sess) if !ok { h.log.Error("authenticated session missing user ID") - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.renderError(w, r, http.StatusInternalServerError) return "", "", false } // Only allow users to act on their own profile. if requestedUsername != sessionUsername { - http.Error(w, "Forbidden", http.StatusForbidden) + h.renderError(w, r, http.StatusForbidden) return "", "", false } diff --git a/internal/handlers/profile_test.go b/internal/handlers/profile_test.go index 18692c3..21c73a8 100644 --- a/internal/handlers/profile_test.go +++ b/internal/handlers/profile_test.go @@ -126,7 +126,9 @@ func TestUserRoute_Unauthenticated_RedirectedByMiddleware(t *testing.T) { var sess *session.Session - app := newTestApp(t, &log, &cfg, &sess) + var h *handlers.Handlers + + app := newTestApp(t, &log, &cfg, &sess, &h) app.RequireStart() t.Cleanup(app.RequireStop) @@ -137,7 +139,7 @@ func TestUserRoute_Unauthenticated_RedirectedByMiddleware(t *testing.T) { router := chi.NewRouter() router.Route("/user/{username}", func(r chi.Router) { - r.Use(mw.CSRF()) + r.Use(mw.CSRF(h.HandleErrorPage(http.StatusForbidden))) r.Use(mw.RequireAuth()) r.Get("/", func(w http.ResponseWriter, _ *http.Request) { handlerReached = true diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 4abb7fc..8b9e615 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -149,13 +149,7 @@ func (h *Handlers) HandleSourceList() http.HandlerFunc { "user_id = ?", userID, ).Order("created_at DESC").Find(&webhooks).Error if err != nil { - h.log.Error( - "failed to list webhooks", "error", err, - ) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, r, "failed to list webhooks", err) return } @@ -249,9 +243,7 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc { // middleware, which runs before CSRF parses the form. err := r.ParseForm() if err != nil { - http.Error( - w, "Bad request", http.StatusBadRequest, - ) + h.renderError(w, r, http.StatusBadRequest) return } @@ -311,7 +303,7 @@ func (h *Handlers) createWebhookWithEntrypoint( err := h.commitWebhook(webhook) if err != nil { - h.serverError(w, "failed to create webhook", err) + h.serverError(w, r, "failed to create webhook", err) return } @@ -388,7 +380,7 @@ func (h *Handlers) HandleSourceDetail() http.HandlerFunc { "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -475,7 +467,7 @@ func (h *Handlers) HandleSourceEdit() http.HandlerFunc { "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -510,7 +502,7 @@ func (h *Handlers) HandleSourceEditSubmit() http.HandlerFunc { "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -519,9 +511,7 @@ func (h *Handlers) HandleSourceEditSubmit() http.HandlerFunc { // middleware, which runs before CSRF parses the form. err = r.ParseForm() if err != nil { - http.Error( - w, "Bad request", http.StatusBadRequest, - ) + h.renderError(w, r, http.StatusBadRequest) return } @@ -575,7 +565,7 @@ func (h *Handlers) applyWebhookEdit( err := h.db.DB().Save(webhook).Error if err != nil { - h.serverError(w, "failed to update webhook", err) + h.serverError(w, r, "failed to update webhook", err) return } @@ -605,7 +595,7 @@ func (h *Handlers) HandleSourceDelete() http.HandlerFunc { "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -632,7 +622,7 @@ func (h *Handlers) deleteWebhookResources( // be removed by hand; deleted history cannot be recovered. err := h.commitWebhookDeletion(&webhook) if err != nil { - h.serverError(w, "failed to delete webhook", err) + h.serverError(w, r, "failed to delete webhook", err) return } @@ -658,7 +648,7 @@ func (h *Handlers) deleteWebhookResources( // redirecting as though everything succeeded: the file // needs removing by hand, and the logged error names it. h.serverError( - w, "failed to delete webhook event database", err, + w, r, "failed to delete webhook event database", err, ) return @@ -802,7 +792,7 @@ func (h *Handlers) ownedWebhook( "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return database.Webhook{}, false } @@ -824,7 +814,7 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc { // Without the map every delivery renders through a // zero redactor, so failing the page is the only // safe answer. - h.serverError(w, "failed to load targets", err) + h.serverError(w, r, "failed to load targets", err) return } @@ -832,7 +822,7 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc { page := h.parsePage(r) evts, total, ok := h.loadEventsWithDeliveries( - w, webhook, targets, page, + w, r, webhook, targets, page, ) if !ok { return @@ -942,6 +932,7 @@ func (h *Handlers) parsePage(r *http.Request) int { // caller must then render nothing further. func (h *Handlers) loadEventsWithDeliveries( w http.ResponseWriter, + r *http.Request, webhook database.Webhook, targetMap map[string]eventLogTarget, page int, @@ -955,7 +946,7 @@ func (h *Handlers) loadEventsWithDeliveries( webhookDB, err := h.dbMgr.GetDB(webhook.ID) if err != nil { h.serverError( - w, "failed to get webhook database", err, + w, r, "failed to get webhook database", err, ) return nil, 0, false @@ -992,7 +983,7 @@ func (h *Handlers) loadEventsWithDeliveries( ) if err != nil { h.serverError( - w, "failed to load delivery attempts", err, + w, r, "failed to load delivery attempts", err, ) return nil, 0, false @@ -1001,7 +992,7 @@ func (h *Handlers) loadEventsWithDeliveries( resubmits, err := resubmitCounts(webhookDB, eventIDs) if err != nil { h.serverError( - w, "failed to count event resubmissions", err, + w, r, "failed to count event resubmissions", err, ) return nil, 0, false @@ -1224,7 +1215,7 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc { "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -1233,9 +1224,7 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc { // middleware, which runs before CSRF parses the form. err = r.ParseForm() if err != nil { - http.Error( - w, "Bad request", http.StatusBadRequest, - ) + h.renderError(w, r, http.StatusBadRequest) return } @@ -1251,7 +1240,7 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc { err = h.db.DB().Create(entrypoint).Error if err != nil { - h.serverError(w, "failed to create entrypoint", err) + h.serverError(w, r, "failed to create entrypoint", err) return } @@ -1282,7 +1271,7 @@ func (h *Handlers) HandleTargetCreate() http.HandlerFunc { "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -1291,9 +1280,7 @@ func (h *Handlers) HandleTargetCreate() http.HandlerFunc { // middleware, which runs before CSRF parses the form. err = r.ParseForm() if err != nil { - http.Error( - w, "Bad request", http.StatusBadRequest, - ) + h.renderError(w, r, http.StatusBadRequest) return } @@ -1364,7 +1351,7 @@ func (h *Handlers) processTargetCreate( err = h.db.DB().Create(target).Error if err != nil { - h.serverError(w, "failed to create target", err) + h.serverError(w, r, "failed to create target", err) return } @@ -1458,7 +1445,7 @@ func (h *Handlers) buildTargetConfig( case database.TargetTypeSlack: return h.buildSlackTargetConfig(w, r, in.URL) case database.TargetTypeDatabase: - return h.buildDatabaseTargetConfig(w, in.Expiry) + return h.buildDatabaseTargetConfig(w, r, in.Expiry) case database.TargetTypeLog: return "", nil default: @@ -1508,7 +1495,7 @@ func (h *Handlers) buildHTTPTargetConfig( return "", err } - return marshalTargetConfig(w, delivery.HTTPTargetConfig{ + return h.marshalTargetConfig(w, r, delivery.HTTPTargetConfig{ URL: in.URL, Headers: headers, Timeout: timeout, @@ -1530,7 +1517,7 @@ func (h *Handlers) buildSlackTargetConfig( return "", err } - return marshalTargetConfig(w, delivery.SlackTargetConfig{ + return h.marshalTargetConfig(w, r, delivery.SlackTargetConfig{ WebhookURL: targetURL, }) } @@ -1584,16 +1571,14 @@ func (h *Handlers) validateTargetURL( // marshalTargetConfig serialises a target configuration for storage, // writing a 500 itself if it cannot. -func marshalTargetConfig( +func (h *Handlers) marshalTargetConfig( w http.ResponseWriter, + r *http.Request, cfg any, ) (string, error) { configBytes, err := json.Marshal(cfg) if err != nil { - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, r, "failed to encode target config", err) return "", err } @@ -1609,6 +1594,7 @@ func marshalTargetConfig( // expiry yields an empty config (the keep-forever default). func (h *Handlers) buildDatabaseTargetConfig( w http.ResponseWriter, + r *http.Request, expiry string, ) (string, error) { expiry = strings.TrimSpace(expiry) @@ -1627,8 +1613,8 @@ func (h *Handlers) buildDatabaseTargetConfig( return "", err } - return marshalTargetConfig( - w, map[string]any{"expiry": expiry}, + return h.marshalTargetConfig( + w, r, map[string]any{"expiry": expiry}, ) } @@ -1682,7 +1668,7 @@ func (h *Handlers) deleteChildResource( "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } @@ -1692,11 +1678,7 @@ func (h *Handlers) deleteChildResource( childID, webhook.ID, ).Delete(model) if result.Error != nil { - h.log.Error(errMsg, "error", result.Error) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, r, errMsg, result.Error) return } @@ -1786,18 +1768,14 @@ func (h *Handlers) toggleChildResource( "id = ? AND user_id = ?", sourceID, userID, ).First(&webhook).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return } err = toggleFn(webhook.ID, childID) if err != nil { - h.log.Error(errMsg, "error", err) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, r, errMsg, err) return } diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go index 8ff03b5..31e0eaa 100644 --- a/internal/handlers/target_edit.go +++ b/internal/handlers/target_edit.go @@ -88,9 +88,7 @@ func (h *Handlers) HandleTargetEditSubmit() http.HandlerFunc { // middleware, which runs before CSRF parses the form. err := r.ParseForm() if err != nil { - http.Error( - w, "Bad request", http.StatusBadRequest, - ) + h.renderError(w, r, http.StatusBadRequest) return } @@ -157,7 +155,7 @@ func (h *Handlers) applyTargetEdit( err = h.db.DB().Save(target).Error if err != nil { - h.serverError(w, "failed to update target", err) + h.serverError(w, r, "failed to update target", err) return } @@ -220,7 +218,7 @@ func (h *Handlers) ownedTarget( chi.URLParam(r, "targetID"), webhook.ID, ).First(&target).Error if err != nil { - http.NotFound(w, r) + h.renderError(w, r, http.StatusNotFound) return database.Webhook{}, nil, false } diff --git a/internal/handlers/webhook.go b/internal/handlers/webhook.go index 6a890f4..c071a2e 100644 --- a/internal/handlers/webhook.go +++ b/internal/handlers/webhook.go @@ -88,14 +88,14 @@ func (h *Handlers) processWebhookRequest( headersJSON, err := json.Marshal(r.Header) if err != nil { - h.serverError(w, "failed to serialize headers", err) + h.receiverError(w, "failed to serialize headers", err) return } targets, err := h.loadActiveTargets(entrypoint.WebhookID) if err != nil { - h.serverError(w, "failed to query targets", err) + h.receiverError(w, "failed to query targets", err) return } @@ -196,7 +196,7 @@ func (h *Handlers) createAndDeliverEvent( targets, ) if err != nil { - h.serverError(w, "failed to store webhook event", err) + h.receiverError(w, "failed to store webhook event", err) return } @@ -204,6 +204,19 @@ func (h *Handlers) createAndDeliverEvent( h.finishWebhookResponse(w, event, entrypoint, tasks) } +// receiverError logs an error and answers the sender with a plain-text +// 500. The receiver's answers are for programs, so it never sends the +// error page the web UI uses. +func (h *Handlers) receiverError( + w http.ResponseWriter, msg string, err error, +) { + h.log.Error(msg, "error", err) + http.Error( + w, "Internal server error", + http.StatusInternalServerError, + ) +} + // eventSource carries the fields a new event is built from. The // receiver fills it from the live request; the resubmit handler fills // it from a stored event. Both then go through createAndFanOut, so an diff --git a/internal/middleware/csrf.go b/internal/middleware/csrf.go index 03a27a4..9e74138 100644 --- a/internal/middleware/csrf.go +++ b/internal/middleware/csrf.go @@ -19,7 +19,7 @@ func CSRFToken(r *http.Request) string { // key to sign a CSRF cookie and validates a masked token submitted via // the "csrf_token" form field (or the "X-CSRF-Token" header) on // POST/PUT/PATCH/DELETE requests. Requests with an invalid or missing -// token receive a 403 Forbidden response. +// token are logged and answered by forbidden, which must write the 403. // // The middleware detects the client-facing transport protocol // per-request via reqtls.IsTLS, the single TLS predicate the session @@ -36,7 +36,9 @@ func CSRFToken(r *http.Request) string { // Two gorilla/csrf instances are maintained — one with Secure cookies // (for TLS) and one without (for plaintext HTTP) — because the // csrf.Secure option is set at creation time, not per-request. -func (m *Middleware) CSRF() func(http.Handler) http.Handler { +func (m *Middleware) CSRF( + forbidden http.Handler, +) func(http.Handler) http.Handler { csrfErrorHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // CSRF is registered ahead of RequireAuth on every route // group that uses it, so this WARN is reachable by an @@ -57,7 +59,7 @@ func (m *Middleware) CSRF() func(http.Handler) http.Handler { "remote_addr", r.RemoteAddr, "reason", csrf.FailureReason(r), ) - http.Error(w, "Forbidden - invalid CSRF token", http.StatusForbidden) + forbidden.ServeHTTP(w, r) }) key := m.session.GetKey() diff --git a/internal/middleware/csrf_test.go b/internal/middleware/csrf_test.go index f1660ed..6cf12f3 100644 --- a/internal/middleware/csrf_test.go +++ b/internal/middleware/csrf_test.go @@ -18,6 +18,12 @@ import ( // csrfCookieName is the gorilla/csrf cookie name. const csrfCookieName = "_gorilla_csrf" +// forbidden stands in for the error page the server hands CSRF to +// answer a refused request with. +func forbidden(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) +} + // csrfGetToken performs a GET request through the CSRF middleware // and returns the token and cookies. func csrfGetToken( @@ -98,7 +104,7 @@ func TestCSRF_GETSetsToken(t *testing.T) { var gotToken string - handler := m.CSRF()(http.HandlerFunc( + handler := m.CSRF(http.HandlerFunc(forbidden))(http.HandlerFunc( func(_ http.ResponseWriter, r *http.Request) { gotToken = middleware.CSRFToken(r) }, @@ -120,7 +126,7 @@ func TestCSRF_POSTWithValidToken(t *testing.T) { t.Parallel() m, _ := testMiddleware(t, config.EnvironmentDev) - csrfMW := m.CSRF() + csrfMW := m.CSRF(http.HandlerFunc(forbidden)) getReq := httptest.NewRequestWithContext( context.Background(), @@ -152,7 +158,7 @@ func csrfPOSTWithoutTokenTest( t.Helper() m, _ := testMiddleware(t, env) - csrfMW := m.CSRF() + csrfMW := m.CSRF(http.HandlerFunc(forbidden)) // GET to establish the CSRF cookie getHandler := csrfMW(http.HandlerFunc( @@ -209,7 +215,7 @@ func TestCSRF_POSTWithInvalidToken(t *testing.T) { t.Parallel() m, _ := testMiddleware(t, config.EnvironmentDev) - csrfMW := m.CSRF() + csrfMW := m.CSRF(http.HandlerFunc(forbidden)) // GET to establish the CSRF cookie getHandler := csrfMW(http.HandlerFunc( @@ -265,7 +271,7 @@ func TestCSRF_GETDoesNotValidate(t *testing.T) { var called bool - handler := m.CSRF()(http.HandlerFunc( + handler := m.CSRF(http.HandlerFunc(forbidden))(http.HandlerFunc( func(_ http.ResponseWriter, _ *http.Request) { called = true }, @@ -328,7 +334,7 @@ func csrfTookStrictPath( t.Helper() m, _ := testMiddleware(t, env) - csrfMW := m.CSRF() + csrfMW := m.CSRF(http.HandlerFunc(forbidden)) newReq := func(method string) *http.Request { r := httptest.NewRequestWithContext( @@ -477,7 +483,7 @@ func TestCSRF_ProdMode_PlaintextHTTP_POSTWithValidToken( t.Parallel() m, _ := testMiddleware(t, config.EnvironmentProd) - csrfMW := m.CSRF() + csrfMW := m.CSRF(http.HandlerFunc(forbidden)) getReq := httptest.NewRequestWithContext( context.Background(), @@ -517,7 +523,7 @@ func TestCSRF_ProdMode_BehindProxy_POSTWithValidToken( t.Parallel() m, _ := testMiddleware(t, config.EnvironmentProd) - csrfMW := m.CSRF() + csrfMW := m.CSRF(http.HandlerFunc(forbidden)) getReq := httptest.NewRequestWithContext( context.Background(), @@ -562,7 +568,7 @@ func TestCSRF_ProdMode_DirectTLS_POSTWithValidToken( t.Parallel() m, _ := testMiddleware(t, config.EnvironmentProd) - csrfMW := m.CSRF() + csrfMW := m.CSRF(http.HandlerFunc(forbidden)) getReq := httptest.NewRequestWithContext( context.Background(), diff --git a/internal/middleware/logbound_test.go b/internal/middleware/logbound_test.go index 649370c..2c850d9 100644 --- a/internal/middleware/logbound_test.go +++ b/internal/middleware/logbound_test.go @@ -260,7 +260,9 @@ func logSites() map[string]logSite { ) http.Handler { t.Helper() - return m.CSRF()(unreachable(t)) + return m.CSRF(http.HandlerFunc(forbidden))( + unreachable(t), + ) }, send: postNoToken, wantStatus: http.StatusForbidden, diff --git a/internal/server/error_page_test.go b/internal/server/error_page_test.go new file mode 100644 index 0000000..40284c3 --- /dev/null +++ b/internal/server/error_page_test.go @@ -0,0 +1,155 @@ +package server_test + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/config" +) + +// The link back the error page offers: to the webhook list for a +// signed-in user, to sign-in for anyone else. +const ( + backToWebhooks = `` + + `Back to webhooks` + backToSignIn = `` + + `Sign in` +) + +// assertErrorPage checks that w is the error page for status, in the +// normal layout, offering link. +func assertErrorPage( + t *testing.T, + w *httptest.ResponseRecorder, + status int, + link string, +) { + t.Helper() + + body := w.Body.String() + + assert.Equal(t, status, w.Code) + assert.Equal( + t, "text/html; charset=utf-8", w.Header().Get("Content-Type"), + ) + assert.Contains(t, body, `