From 4a91635b2af68d021f335b7f99f6432ea7af2b65 Mon Sep 17 00:00:00 2001
From: sneak <sneak@sneak.berlin>
Date: Wed, 12 Aug 2026 09:40:50 +0000
Subject: [PATCH] Render templates via a buffer, not the ResponseWriter (closes
 #123)

executeTemplate ran the template straight into the ResponseWriter, so a
mid-render failure left the already-emitted prefix written and the
response committed: the handler could no longer set a 500 and the
client got a truncated page, typically with a 200. It also let handler
tests pass against the flushed prefix of a page that aborted below the
assertions.

Execute into a bytes.Buffer instead, and set the content type and copy
the buffer out only once rendering has fully succeeded. On failure
nothing has been written, so the 500 still reaches the client.

Add a test that renders a template failing partway through and asserts
both the 500 and that the body carries no part of the aborted page.
Against the previous streaming renderer it fails on both counts (200,
body "PARTIAL PAGE CONTENTInternal server error").
---
 internal/handlers/export_test.go   | 15 ++++++-
 internal/handlers/handlers.go      | 23 ++++++++++-
 internal/handlers/handlers_test.go | 64 ++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 3 deletions(-)

diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go
index 8d65e54..ac9fee3 100644
--- a/internal/handlers/export_test.go
+++ b/internal/handlers/export_test.go
@@ -1,6 +1,19 @@
 package handlers
 
-import "net/http"
+import (
+	"html/template"
+	"net/http"
+)
+
+// 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.
+func (s *Handlers) AddTemplateForTest(
+	pageTemplate string,
+	tmpl *template.Template,
+) {
+	s.templates[pageTemplate] = tmpl
+}
 
 // RenderTemplateForTest exposes renderTemplate for use in the
 // handlers_test package.
diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go
index ce99774..e26d788 100644
--- a/internal/handlers/handlers.go
+++ b/internal/handlers/handlers.go
@@ -3,6 +3,7 @@
 package handlers
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -224,13 +225,20 @@ func (s *Handlers) renderTemplate(
 	s.executeTemplate(w, tmpl, wrapper)
 }
 
-// executeTemplate runs the template and handles errors.
+// 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. These pages are small, so holding one in memory is
+// the right trade.
 func (s *Handlers) executeTemplate(
 	w http.ResponseWriter,
 	tmpl *template.Template,
 	data any,
 ) {
-	err := tmpl.Execute(w, data)
+	var buf bytes.Buffer
+
+	err := tmpl.Execute(&buf, data)
 	if err != nil {
 		s.log.Error(
 			"failed to execute template", "error", err,
@@ -239,5 +247,16 @@ func (s *Handlers) executeTemplate(
 			w, "Internal server error",
 			http.StatusInternalServerError,
 		)
+
+		return
+	}
+
+	w.Header().Set("Content-Type", "text/html; charset=utf-8")
+
+	_, err = buf.WriteTo(w)
+	if err != nil {
+		s.log.Error(
+			"failed to write rendered page", "error", err,
+		)
 	}
 }
diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go
index ad927c5..3390fe0 100644
--- a/internal/handlers/handlers_test.go
+++ b/internal/handlers/handlers_test.go
@@ -2,6 +2,8 @@ package handlers_test
 
 import (
 	"context"
+	"errors"
+	"html/template"
 	"net/http"
 	"net/http/httptest"
 	"sync"
@@ -220,6 +222,68 @@ func TestRenderTemplate(t *testing.T) {
 	)
 }
 
+// errMidRender is the failure a test template raises partway through
+// rendering.
+var errMidRender = errors.New("deliberate mid-render failure")
+
+// midRenderFailure is template data whose first method renders and
+// whose second fails, so the template aborts after output has
+// already been produced.
+type midRenderFailure struct{}
+
+// Prefix is the output a streaming renderer would flush before the
+// failure below aborts the template.
+func (midRenderFailure) Prefix() string { return partialPageMarker }
+
+// Boom aborts template execution.
+func (midRenderFailure) Boom() (string, error) {
+	return "", errMidRender
+}
+
+// partialPageMarker is content the failing template emits before it
+// aborts.
+const partialPageMarker = "PARTIAL PAGE CONTENT"
+
+// TestRenderTemplateMidRenderErrorSendsNoPartialBody proves the
+// renderer does not commit output it cannot finish: a template that
+// fails partway through must yield a 500 and a body carrying none of
+// the content emitted before the failure. Against a renderer that
+// executes straight into the ResponseWriter this fails on both
+// counts, returning 200 with the prefix already flushed.
+func TestRenderTemplateMidRenderErrorSendsNoPartialBody(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	app := newTestApp(t, &h)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	h.AddTemplateForTest("failing.html", template.Must(
+		template.New("failing").Parse(
+			`{{.Data.Prefix}}{{.Data.Boom}}TAIL`,
+		),
+	))
+
+	req := httptest.NewRequestWithContext(
+		context.Background(), http.MethodGet, "/", nil)
+	w := httptest.NewRecorder()
+
+	h.RenderTemplateForTest(
+		w, req, "failing.html", midRenderFailure{},
+	)
+
+	assert.Equal(
+		t, http.StatusInternalServerError, w.Code,
+		"a failed render must report a 500",
+	)
+	assert.Equal(
+		t, "Internal server error\n", w.Body.String(),
+		"the response must carry no part of the aborted page",
+	)
+}
+
 func TestBuildDatabaseTargetConfig_Valid(t *testing.T) {
 	t.Parallel()