Serve an event's full stored body over HTTP (closes #157)

Capping the event log page at 8 KB of body per event left no
in-app way to see a larger one: storage keeps it, but no route
served it, so a payload over the cap was reachable only by an
operator with filesystem access. GitHub pull_request and
multi-commit push payloads, expanded Stripe events and Shopify
orders all routinely clear 8 KB, which is exactly when the tool
is supposed to be useful.

GET /source/{sourceID}/logs/{eventID}/body now serves one whole
body, and the truncation marker links to it when — and only
when — a body was actually cut.

The response is deliberately inert. Its bytes are chosen by
whoever can reach the public receiver and it hands them back
inside the operator's own authenticated origin, so it goes out
as application/octet-stream with Content-Disposition: attachment
and nosniff, and the filename is built from a parsed uuid rather
than from anything in the request. The application CSP is no
help on this path: script-src allows 'unsafe-inline' from
'self', so a document served from this origin could run its own
script.

The body is read in one query and held whole while it is
written. There is no cheaper bound to take. database/sql
exposes no incremental handle on a SQLite blob, and reading
byte ranges with substr does not avoid the cost either: SQLite
materialises the entire column value to evaluate each substr
call, so range reads pay for the whole body once per range
rather than once per download. Measured over a 1 MiB body,
64 KiB ranges cost 11-15x a single read to move the same bytes.
The bound is therefore the one the issue asks for: the route is
owner-authenticated and ingest is capped at 1 MB, so peak is
one body per concurrent download. Nothing goes through
renderTemplate, which buffers a whole response before writing
it.

Reading the body before the first header is written also means
an event reaped mid-request cannot produce a torn response: it
is either served whole or 404s cleanly, and both are tested.

The ownership check the log page applies is extracted as
ownedWebhook and shared with the download, so the two cannot
drift apart. A webhook owned by someone else and one that does
not exist are the same 404.

The route registration and the link the template emits are
covered end to end through the production router, so a typo in
either fails the suite rather than leaving the feature dead
behind green handler tests.

internal/handlers/event_body.go

@@ -1,6 +1,7 @@
 package handlers
 
 import (
+	"database/sql"
 	"errors"
 	"net/http"
 	"strconv"
@@ -11,34 +12,16 @@ import (
 	"sneak.berlin/go/webhooker/internal/database"
 )
 
-// bodyChunkBytes is how much of a stored body is resident at
-// once while it is being written to the client. The event log
-// page caps what it renders at maxRenderedBodyBytes, so this
-// route is the only way to reach a whole body and by
-// construction serves the largest ones in the system. Reading
-// it in fixed chunks keeps the peak a property of this constant
-// rather than of the payload.
-const bodyChunkBytes = 64 * 1024
-
-// eventBodySizeColumn measures a stored body the same way
-// eventLogColumns cuts one: the cast to blob makes length count
-// bytes rather than characters, so Content-Length matches what
-// substr will actually hand back.
-const eventBodySizeColumn = "length(cast(body as blob))"
-
-// eventBodyChunkQuery reads one byte range of a stored body.
-// substr over a blob is 1-indexed over bytes. The soft-delete
+// eventBodyQuery reads one event's stored body as bytes. The cast
+// to blob is what makes the driver hand back the stored bytes
+// rather than a string conversion, so Content-Length taken from
+// the result matches what goes on the wire. The soft-delete
 // predicate is spelled out because Raw bypasses GORM's default
 // scope, and it is what stops a reaped event still being
 // downloadable.
-const eventBodyChunkQuery = "SELECT substr(cast(body as blob), ?, ?) " +
+const eventBodyQuery = "SELECT cast(body as blob) " +
 	"FROM events WHERE id = ? AND webhook_id = ? AND deleted_at IS NULL"
 
-// errShortBodyRead reports that a chunk query returned nothing
-// while bytes were still owed, which means the row went away
-// mid-download.
-var errShortBodyRead = errors.New("stored body ended early")
-
 // HandleEventBodyDownload serves one event's stored body in
 // full, which the event log page cannot: it caps each rendered
 // body at maxRenderedBodyBytes.
@@ -85,11 +68,28 @@ func (h *Handlers) HandleEventBodyDownload() http.HandlerFunc {
 // things enforce that and they are not equally strong. The
 // operative one is that events live in a per-webhook SQLite
 // file, so a sibling webhook's event is not in the database
-// being queried at all. The webhook_id predicate on every query
+// being queried at all. The webhook_id predicate on the query
 // below is the second guard, and it is currently redundant
 // against that isolation; it is there so the scoping survives
 // any future change that puts more than one webhook's events in
 // one file.
+//
+// The body is read in one query and held whole in memory while
+// it is written. That is the bound: one body per concurrent
+// download, and a body is capped at 1 MB when it is ingested,
+// so a download cannot cost more than that. There is no
+// cheaper bound available — database/sql exposes no incremental
+// handle on a SQLite BLOB, and reading byte ranges with substr
+// does not avoid the cost either, because SQLite materialises
+// the whole column value to evaluate each substr call. Range
+// reads only pay for that materialisation once per range.
+//
+// One consequence is worth keeping in view: the read finishes
+// before the client is written to, so no read lock is held for
+// the length of a slow download. These per-webhook databases
+// run in SQLite's default journal mode rather than WAL, so a
+// lock held that long would block the receiver from recording
+// new events.
 func (h *Handlers) serveEventBody(
 	w http.ResponseWriter,
 	r *http.Request,
@@ -109,25 +109,29 @@ func (h *Handlers) serveEventBody(
 		return
 	}
 
-	size, found, err := eventBodySize(webhookDB, webhook.ID, eventID)
+	body, found, err := eventBody(webhookDB, webhook.ID, eventID)
 	if err != nil {
-		h.serverError(w, "failed to size event body", err)
+		h.serverError(w, "failed to read event body", err)
 
 		return
 	}
 
 	// A miss is a 404 whether the event belongs to another
 	// webhook or does not exist at all, so the response does
-	// not report which.
+	// not report which. Reading the body before any header is
+	// written is also what keeps an event reaped mid-request
+	// from producing a torn response: either the read finds the
+	// row and the whole body is served, or it does not and the
+	// response is a clean 404.
 	if !found {
 		http.NotFound(w, r)
 
 		return
 	}
 
-	setEventBodyHeaders(w, eventID, size)
+	setEventBodyHeaders(w, eventID, int64(len(body)))
 
-	err = writeEventBody(w, webhookDB, webhook.ID, eventID, size)
+	_, err = w.Write(body)
 	if err != nil {
 		// The status and Content-Length are already committed,
 		// so the client sees a short download. There is no way
@@ -141,32 +145,27 @@ func (h *Handlers) serveEventBody(
 	}
 }
 
-// eventBodySize returns the stored size in bytes of an event's
-// body and whether the event exists within the webhook. The
-// size is read separately from the body so Content-Length can
-// be set before any bytes are written.
-func eventBodySize(
+// eventBody returns an event's stored body and whether the event
+// exists within the webhook.
+func eventBody(
 	webhookDB *gorm.DB,
 	webhookID, eventID string,
-) (int64, bool, error) {
-	var size int64
+) ([]byte, bool, error) {
+	var body []byte
 
-	result := webhookDB.Model(&database.Event{}).
-		Select(eventBodySizeColumn).
-		Where(
-			"id = ? AND webhook_id = ?", eventID, webhookID,
-		).
-		Limit(1).
-		Scan(&size)
-	if result.Error != nil {
-		return 0, false, result.Error
+	err := webhookDB.Raw(
+		eventBodyQuery, eventID, webhookID,
+	).Row().Scan(&body)
+
+	if errors.Is(err, sql.ErrNoRows) {
+		return nil, false, nil
 	}
 
-	if result.RowsAffected == 0 {
-		return 0, false, nil
+	if err != nil {
+		return nil, false, err
 	}
 
-	return size, true, nil
+	return body, true, nil
 }
 
 // setEventBodyHeaders applies the response headers that make
@@ -191,61 +190,3 @@ func setEventBodyHeaders(
 	)
 	w.Header().Set("Content-Length", strconv.FormatInt(size, 10))
 }
-
-// writeEventBody copies size bytes of the event's stored body to
-// w in bodyChunkBytes-sized reads.
-//
-// This is where the route earns its memory bound. database/sql
-// exposes no incremental handle on a SQLite BLOB, so scanning
-// the column would materialise the whole body regardless of the
-// wrapper around it; reading byte ranges instead keeps the
-// resident cost at one chunk. Nothing goes through
-// renderTemplate, which buffers a whole response before writing
-// it.
-//
-// There is deliberately no wrapping read transaction. These
-// per-webhook databases run in SQLite's default journal mode,
-// not WAL, so a read lock held for the length of a slow client's
-// download would block the receiver from recording new events.
-// The cost of that choice is that a body deleted mid-download
-// ends the response short, which is reported as an error rather
-// than passed off as a complete file.
-func writeEventBody(
-	w http.ResponseWriter,
-	webhookDB *gorm.DB,
-	webhookID, eventID string,
-	size int64,
-) error {
-	// Flushing each chunk keeps the claim above true at the
-	// socket as well as in this loop. A ResponseWriter that
-	// cannot flush is not an error: net/http's own output
-	// buffer is a fixed size either way.
-	flusher := http.NewResponseController(w)
-
-	for written := int64(0); written < size; {
-		var chunk []byte
-
-		err := webhookDB.Raw(
-			eventBodyChunkQuery,
-			written+1, bodyChunkBytes, eventID, webhookID,
-		).Row().Scan(&chunk)
-		if err != nil {
-			return err
-		}
-
-		if len(chunk) == 0 {
-			return errShortBodyRead
-		}
-
-		n, err := w.Write(chunk)
-		written += int64(n)
-
-		if err != nil {
-			return err
-		}
-
-		_ = flusher.Flush()
-	}
-
-	return nil
-}

internal/handlers/event_body_test.go

@@ -112,9 +112,9 @@ func TestHandleEventBodyDownload_ServesOversizeBodyInFull(
 
 	t.Cleanup(app.RequireStop)
 
-	// Larger than one read chunk as well as the render cap, so
-	// the chunked read has to reassemble the body in order and
-	// the tail past the last whole chunk is exercised.
+	// Far above the render cap, with multibyte runes and a
+	// distinctive tail, so a body that the log page can only
+	// show a slice of comes back whole and in order.
 	const sentinel = "TAIL-SENTINEL-1f4a9c"
 
 	stored := strings.Repeat("A", 200*1024) +
@@ -134,6 +134,65 @@ func TestHandleEventBodyDownload_ServesOversizeBodyInFull(
 	)
 }
 
+// TestHandleEventBodyDownload_BodiesRoundTripByteIdentical
+// covers the sizes and byte values a stored body can actually
+// take: empty, one byte, either side of the render cap, and
+// bytes that are not text at all. Content-Length has to equal
+// the bytes written in every case, since it is derived from the
+// same read that produces them.
+func TestHandleEventBodyDownload_BodiesRoundTripByteIdentical(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	// A NUL, invalid UTF-8 and a multibyte rune, so nothing on
+	// the path can be treating the body as text.
+	binary := "\x00\x01\xff\xfe" + snowman + "\x00tail"
+
+	cases := map[string]string{
+		"empty":         "",
+		"single byte":   "x",
+		"one below cap": strings.Repeat("b", bodyCap-1),
+		"exactly cap":   strings.Repeat("c", bodyCap),
+		"one above cap": strings.Repeat("d", bodyCap+1),
+		"binary":        binary,
+	}
+
+	for name, stored := range cases {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			var (
+				h     *handlers.Handlers
+				sess  *session.Session
+				db    *database.Database
+				dbMgr *database.WebhookDBManager
+			)
+
+			app := newTestApp(t, &h, &sess, &db, &dbMgr)
+			app.RequireStart()
+
+			t.Cleanup(app.RequireStop)
+
+			wh := seedWebhook(t, db)
+			evt := seedEventWithBody(t, dbMgr, wh.ID, stored)
+
+			w := fetchEventBody(t, h, sess, wh.ID, evt.ID)
+
+			require.Equal(t, http.StatusOK, w.Code)
+			assert.Equal(t, stored, w.Body.String())
+			assert.Equal(
+				t, strconv.Itoa(len(stored)),
+				w.Header().Get("Content-Length"),
+			)
+			assert.Equal(
+				t, len(stored), w.Body.Len(),
+				"Content-Length must equal bytes written",
+			)
+		})
+	}
+}
+
 // TestHandleEventBodyDownload_HeadersAreNotRenderable pins the
 // response headers that stop attacker-supplied bytes executing
 // in the operator's own origin. They are a security control, not
@@ -341,6 +400,68 @@ func TestHandleEventBodyDownload_UnknownEvent404s(t *testing.T) {
 	}
 }
 
+// TestHandleEventBodyDownload_ReapedEvent404s pins what happens
+// when the retention reaper takes an event out from under this
+// route. The body is read in one query before any header is
+// written, so a reaped event cannot produce a partial download:
+// it is a clean 404 with no Content-Length and no
+// Content-Disposition. Both removals the codebase performs are
+// covered — the reaper hard-deletes, and a soft-deleted row is
+// excluded by the query's own deleted_at predicate rather than
+// by GORM's default scope, which Raw bypasses.
+func TestHandleEventBodyDownload_ReapedEvent404s(t *testing.T) {
+	t.Parallel()
+
+	for name, hard := range map[string]bool{
+		"soft deleted": false,
+		"hard deleted": true,
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			var (
+				h     *handlers.Handlers
+				sess  *session.Session
+				db    *database.Database
+				dbMgr *database.WebhookDBManager
+			)
+
+			app := newTestApp(t, &h, &sess, &db, &dbMgr)
+			app.RequireStart()
+
+			t.Cleanup(app.RequireStop)
+
+			const payload = "REAPED-PAYLOAD-4d2a"
+
+			wh := seedWebhook(t, db)
+			evt := seedEventWithBody(t, dbMgr, wh.ID, payload)
+
+			webhookDB, err := dbMgr.GetDB(wh.ID)
+			require.NoError(t, err)
+
+			del := webhookDB
+			if hard {
+				del = del.Unscoped()
+			}
+
+			require.NoError(
+				t,
+				del.Delete(&database.Event{}, "id = ?", evt.ID).
+					Error,
+			)
+
+			w := fetchEventBody(t, h, sess, wh.ID, evt.ID)
+
+			assert.Equal(t, http.StatusNotFound, w.Code)
+			assert.NotContains(t, w.Body.String(), payload)
+			assert.Empty(t, w.Header().Get("Content-Length"))
+			assert.Empty(
+				t, w.Header().Get("Content-Disposition"),
+			)
+		})
+	}
+}
+
 // TestHandleSourceLogs_TruncationMarkerLinksToDownload proves
 // the page tells the reader where the rest of the body is, and
 // only when there is a rest to fetch.

internal/server/routes_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"regexp"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -14,6 +15,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"go.uber.org/fx"
 	"go.uber.org/fx/fxtest"
+	"gorm.io/gorm/clause"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
@@ -48,6 +50,7 @@ type testEnv struct {
 	router http.Handler
 	sess   *session.Session
 	db     *database.Database
+	dbMgr  *database.WebhookDBManager
 }
 
 // newTestEnv wires the dependency graph with fx and builds the
@@ -63,6 +66,7 @@ func newTestEnv(t *testing.T) *testEnv {
 		hnd   *handlers.Handlers
 		sess  *session.Session
 		db    *database.Database
+		dbMgr *database.WebhookDBManager
 	)
 
 	app := fxtest.New(
@@ -85,7 +89,7 @@ func newTestEnv(t *testing.T) *testEnv {
 			middleware.New,
 			handlers.New,
 		),
-		fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db),
+		fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db, &dbMgr),
 	)
 	app.RequireStart()
 	t.Cleanup(app.RequireStop)
@@ -94,6 +98,7 @@ func newTestEnv(t *testing.T) *testEnv {
 		router: server.NewRouterForTest(log.Get(), cfg, mw, hnd),
 		sess:   sess,
 		db:     db,
+		dbMgr:  dbMgr,
 	}
 }
 
@@ -232,6 +237,49 @@ func (e *testEnv) seedUser(
 	return user.ID, hash
 }
 
+// seedWebhook creates a webhook owned by the given user.
+func (e *testEnv) seedWebhook(
+	t *testing.T,
+	userID string,
+) *database.Webhook {
+	t.Helper()
+
+	wh := &database.Webhook{UserID: userID, Name: "routed"}
+
+	require.NoError(
+		t,
+		e.db.DB().Omit(clause.Associations).Create(wh).Error,
+	)
+
+	return wh
+}
+
+// seedEvent records one event with the given body in a webhook's
+// own database.
+func (e *testEnv) seedEvent(
+	t *testing.T,
+	webhookID, body string,
+) *database.Event {
+	t.Helper()
+
+	webhookDB, err := e.dbMgr.GetDB(webhookID)
+	require.NoError(t, err)
+
+	event := &database.Event{
+		WebhookID:   webhookID,
+		Method:      http.MethodPost,
+		Body:        body,
+		ContentType: "application/octet-stream",
+	}
+
+	require.NoError(
+		t,
+		webhookDB.Omit(clause.Associations).Create(event).Error,
+	)
+
+	return event
+}
+
 // storedHash reads the current password hash for a username.
 func (e *testEnv) storedHash(t *testing.T, username string) string {
 	t.Helper()
@@ -381,3 +429,95 @@ func TestPasswordChange_UnderLimit_Succeeds(t *testing.T) {
 		"an under-limit password change should still apply",
 	)
 }
+
+// --- /source/{sourceID} group ---
+
+// TestSourceLogs_TruncationLinkDownloadsTheBody walks the whole
+// feature the way a user does: render the event log page through
+// the production router, take the download URL out of the markup
+// the template emitted, and fetch that URL through the router
+// again. Nothing here is hand-written, so a typo in either the
+// route pattern or the template href fails this test — the
+// handler-level tests cannot catch that, because they forge
+// their own route context and assert a URL string they wrote
+// themselves.
+func TestSourceLogs_TruncationLinkDownloadsTheBody(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	userID, _ := env.seedUser(t, "loguser", "somepassword")
+	cookies := env.authCookies(t, userID, "loguser")
+
+	// Comfortably over the event log page's render cap, so the
+	// page truncates the body and renders the download link at
+	// all. The exact cap is the handlers package's business and
+	// is pinned by its own tests; this only needs to exceed it.
+	stored := strings.Repeat("Z", 64*1024)
+
+	wh := env.seedWebhook(t, userID)
+	env.seedEvent(t, wh.ID, stored)
+
+	page := env.get("/source/"+wh.ID+"/logs", cookies)
+	require.Equal(t, http.StatusOK, page.Code)
+
+	link := regexp.MustCompile(
+		`href="(/source/[^"]+/body)"`,
+	).FindStringSubmatch(page.Body.String())
+	require.Len(
+		t, link, 2,
+		"truncated body should render a download link",
+	)
+
+	w := env.get(html.UnescapeString(link[1]), cookies)
+
+	require.Equal(
+		t, http.StatusOK, w.Code,
+		"the link the page emits must be a live route",
+	)
+	assert.Equal(t, stored, w.Body.String())
+	assert.Equal(
+		t, strconv.Itoa(len(stored)),
+		w.Header().Get("Content-Length"),
+	)
+	assert.Equal(
+		t, "application/octet-stream",
+		w.Header().Get("Content-Type"),
+	)
+	assert.Contains(
+		t, w.Header().Get("Content-Disposition"), "attachment",
+	)
+	assert.Equal(
+		t, "nosniff", w.Header().Get("X-Content-Type-Options"),
+	)
+}
+
+// TestSourceLogsBody_OtherUser404s pins that the download route
+// as registered is behind the auth the group provides and the
+// ownership check the handler applies: another logged-in user
+// asking the real router for the same URL gets a 404, and an
+// unauthenticated request never reaches the handler at all.
+func TestSourceLogsBody_OtherUser404s(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	ownerID, _ := env.seedUser(t, "owner", "somepassword")
+	wh := env.seedWebhook(t, ownerID)
+
+	const payload = "OWNERS-PAYLOAD-77c1"
+
+	evt := env.seedEvent(t, wh.ID, payload)
+	path := "/source/" + wh.ID + "/logs/" + evt.ID + "/body"
+
+	intruderID, _ := env.seedUser(t, "intruder", "somepassword")
+	intruder := env.authCookies(t, intruderID, "intruder")
+
+	w := env.get(path, intruder)
+	assert.Equal(t, http.StatusNotFound, w.Code)
+	assert.NotContains(t, w.Body.String(), payload)
+
+	anon := env.get(path, nil)
+	assert.Equal(t, http.StatusSeeOther, anon.Code)
+	assert.Equal(t, "/pages/login", anon.Header().Get("Location"))
+}