From b47132391161db14756ea16b69851c74e9a54d51 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 2 Oct 2026 12:00:32 +0000 Subject: [PATCH] Write a form-error page's status only after it renders (closes #128) The login form and the webhook create and edit forms set their 400 or 409 with WriteHeader before calling renderTemplate, so a template failure there answered with that status instead of 500. A variant of renderTemplate now takes the status and writes it only once the page has rendered into the buffer; renderTemplate sends 200 through it. Model: opus-5-5 --- internal/handlers/auth.go | 3 +- internal/handlers/auth_test.go | 55 ++++++++++++++++++++++++++ internal/handlers/handlers.go | 38 +++++++++++++----- internal/handlers/source_management.go | 17 ++++---- 4 files changed, 91 insertions(+), 22 deletions(-) diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 74ad68d..f22dd98 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -139,8 +139,7 @@ func (h *Handlers) renderLoginError( ), } - w.WriteHeader(status) - h.renderTemplate(w, r, "login.html", data) + h.renderTemplateStatus(w, r, "login.html", data, status) } // authenticateUser looks up and verifies a user's credentials. diff --git a/internal/handlers/auth_test.go b/internal/handlers/auth_test.go index 703c040..d5c2336 100644 --- a/internal/handlers/auth_test.go +++ b/internal/handlers/auth_test.go @@ -3,6 +3,7 @@ package handlers_test import ( "context" "fmt" + "html/template" "net/http" "net/http/httptest" "net/url" @@ -404,6 +405,60 @@ func TestLogin_MissingCredentialsRejectedBeforeAnyHash(t *testing.T) { ) } +// TestLogin_FormErrorAnswersItsStatusWithThePage proves that the login +// form shown again with an error still answers 400 with the whole page. +func TestLogin_FormErrorAnswersItsStatusWithThePage(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + app := newTestApp(t, &h) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + w := submitLogin(h, sharedProxyPeer, "", "") + + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Contains( + t, w.Body.String(), "Username and password are required", + ) + assert.Contains( + t, w.Body.String(), "", + "the page must render to completion", + ) +} + +// TestLogin_FormErrorRenderFailureAnswers500 proves that a login form +// error page whose template fails answers 500 with the error page and +// none of the form page, rather than the 400 it meant to send. +func TestLogin_FormErrorRenderFailureAnswers500(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + app := newTestApp(t, &h) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + // The page prints its error message and then fails. + h.AddTemplateForTest("login.html", template.Must( + template.New("login").Funcs(template.FuncMap{ + "fail": func() (string, error) { return "", errMidRender }, + }).Parse(`{{.Error}}{{fail}}`), + )) + + w := submitLogin(h, sharedProxyPeer, "", "") + + assert.Equal(t, http.StatusInternalServerError, w.Code) + assert.NotContains( + t, w.Body.String(), "Username and password are required", + "the response must carry no part of the aborted page", + ) + assert.Contains(t, w.Body.String(), "500 Internal Server Error") +} + // TestLogin_SuccessCreatesSession is the control for the tests above: // the success path they assert on really does authenticate. func TestLogin_SuccessCreatesSession(t *testing.T) { diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index f47ab1b..057ae1f 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -309,12 +309,26 @@ func (s *Handlers) getUserInfo( } // renderTemplate renders a pre-parsed template with common -// data +// data and answers 200. func (s *Handlers) renderTemplate( w http.ResponseWriter, r *http.Request, pageTemplate string, data any, +) { + s.renderTemplateStatus(w, r, pageTemplate, data, http.StatusOK) +} + +// renderTemplateStatus is renderTemplate answering with status, for a +// form shown again with an error. Call it instead of WriteHeader +// followed by renderTemplate: the status is written only once the page +// has rendered, so a failed render can still answer 500. +func (s *Handlers) renderTemplateStatus( + w http.ResponseWriter, + r *http.Request, + pageTemplate string, + data any, + status int, ) { tmpl, ok := s.templates[pageTemplate] if !ok { @@ -327,7 +341,9 @@ func (s *Handlers) renderTemplate( return } - s.executeTemplate(w, r, tmpl, s.pageData(r, data, noticeFor(r))) + s.executeTemplate( + w, r, tmpl, s.pageData(r, data, noticeFor(r)), status, + ) } // pageData adds the fields the shared layout renders to a page's own @@ -363,19 +379,20 @@ func (s *Handlers) pageData( } } -// executeTemplate renders the template into a buffer and writes to -// the response only once rendering has fully succeeded. Executing -// straight into the ResponseWriter commits a partial body and a 200 -// status before a mid-render error can be reported, leaving no way -// to serve a 500. Buffering makes a page's rendered size resident -// memory per concurrent viewer, so every page owes it a bound: the -// event log caps each stored body at maxRenderedBodyBytes for exactly -// this reason. +// executeTemplate renders the template into a buffer and writes status +// and the page to the response only once rendering has fully +// succeeded. Executing straight into the ResponseWriter commits a +// partial body and the status before a mid-render error can be +// reported, leaving no way to serve a 500. Buffering makes a page's +// rendered size resident memory per concurrent viewer, so every page +// owes it a bound: the event log caps each stored body at +// maxRenderedBodyBytes for exactly this reason. func (s *Handlers) executeTemplate( w http.ResponseWriter, r *http.Request, tmpl *template.Template, data any, + status int, ) { var buf bytes.Buffer @@ -390,6 +407,7 @@ func (s *Handlers) executeTemplate( } w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(status) _, err = buf.WriteTo(w) if err != nil { diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 0e8aa1b..169b90a 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -350,12 +350,12 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc { retentionStr := r.PostFormValue("retention_days") if name == "" { - w.WriteHeader(http.StatusBadRequest) - h.renderTemplate( + h.renderTemplateStatus( w, r, "sources_new.html", newSourceFormData( "Name is required", name, description, ), + http.StatusBadRequest, ) return @@ -365,13 +365,13 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc { retentionStr, database.DefaultRetentionDays, ) if retErr != nil { - w.WriteHeader(http.StatusBadRequest) - h.renderTemplate( + h.renderTemplateStatus( w, r, "sources_new.html", newSourceFormData( retentionErrorMessage(retErr), name, description, ), + http.StatusBadRequest, ) return @@ -644,8 +644,7 @@ func (h *Handlers) applyWebhookEdit( tmplKeyError: "Name is required", } - w.WriteHeader(http.StatusBadRequest) - h.renderTemplate(w, r, "source_edit.html", data) + h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest) return } @@ -665,8 +664,7 @@ func (h *Handlers) applyWebhookEdit( tmplKeyError: retentionErrorMessage(retErr), } - w.WriteHeader(http.StatusBadRequest) - h.renderTemplate(w, r, "source_edit.html", data) + h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest) return } @@ -703,8 +701,7 @@ func (h *Handlers) applyWebhookEdit( "it, then save again.", } - w.WriteHeader(http.StatusConflict) - h.renderTemplate(w, r, "source_edit.html", data) + h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusConflict) return } -- 2.54.0