Close archive writers when the delivery engine stops (closes #280)

The engine cached archive writers and never closed them at shutdown,
so after a clean stop an archive's rows could sit in its -wal while
the .db held no table. The engine's stop hook now evicts every cached
writer once its workers have returned, the same way deleting a webhook
does, so a clean stop leaves each archive as one file and a late write
is refused. If the workers do not return within the stop budget, the
writers are left open as a kill would leave them.

The README no longer says archives keep their sidecars across a clean
stop.

Model: opus-5-5

README.md

@@ -157,11 +157,6 @@ private and reserved ranges — RFC 1918, loopback, CGNAT, link-local and
 the rest — are refused, which stops a target from being used to make
 webhooker probe the network it sits in.
 
-Besides the private and reserved ranges, the default blocklist refuses
-public cloud metadata addresses: currently only `168.63.129.16`, Azure's
-WireServer, which serves an Azure VM its credentials. Because it is a
-public address, listing it in `ALLOWED_EGRESS_CIDRS` reopens it.
-
 That default is also inconvenient for the thing webhooker is mostly
 for: taking a public webhook and forwarding it to something on your own
 network. A container on the same Docker network, a box on `10.x`, a
@@ -200,16 +195,15 @@ Two things this setting cannot do:
   the list is always an allowlist; an empty list (the default) means
   every private and reserved range stays refused. Note that
   `0.0.0.0/0` gets you most of the way there anyway, per above.
-- **It cannot open link-local, or a cloud metadata endpoint at a
-  non-public address that discloses credentials or user data.** An
-  address is on the list below when it is not a public address and both
-  of these hold: the provider fixes it, so it cannot collide with
-  anything you run; and reaching it hands out credentials, user data or
-  bootstrap material. Those stay blocked no matter what you list,
-  including when you list them outright or list a supernet such as
-  `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
-  effort rather than a guarantee — it is a hand-maintained list and the
-  caveat below the table applies:
+- **It cannot open link-local, or a cloud metadata endpoint that
+  discloses credentials or user data.** An address is on the list below
+  when both of these hold: the provider fixes it, so it cannot collide
+  with anything you run; and reaching it hands out credentials, user
+  data or bootstrap material. Those stay blocked no matter what you
+  list, including when you list them outright or list a supernet such
+  as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as
+  best effort rather than a guarantee — it is a hand-maintained list
+  and the caveat below the table applies:
 
   | Blocked unconditionally | What it is |
   | ----------------------- | ---------- |
@@ -248,8 +242,7 @@ Two things this setting cannot do:
   encodings, which the default blocklist does not match. A publicly
   routable metadata address is not listed here, because nothing on this
   list can be reopened and blocking one that way would leave you no
-  escape hatch at all; Azure's `168.63.129.16` is refused by the default
-  blocklist instead, as described above.
+  escape hatch at all.
 
   This list is not exhaustive of every cloud's metadata address — if
   yours is not here, do not allowlist the block that contains it.
@@ -2721,7 +2714,7 @@ abuse limit later; they are tracked as future work.
 | ------ | --------------------------- | ----------- |
 | `GET`  | `/`                         | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
 | `GET`  | `/.well-known/healthcheck`  | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
-| `GET`, `HEAD` | `/s/*`              | Static file serving (embedded CSS, JS). `GET` and `HEAD` only — `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` and `CONNECT` are answered `405 Method Not Allowed` with `Allow: GET, HEAD`. Any other method (such as `PROPFIND`) is refused by chi before it reaches this route, and gets `405` without an `Allow` header. Pinned by `TestStaticServesOnlyGetAndHead` |
+| any    | `/s/*`                      | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` |
 | `POST` | `/webhook/{uuid}`           | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) |
 
 #### Authentication Endpoints
@@ -3280,5 +3273,3 @@ MIT
 ## Author
 
 [@sneak](https://sneak.berlin)
-
-

go.mod

@@ -4,7 +4,6 @@ go 1.26.1
 
 require (
 	github.com/99designs/basicauth-go v0.0.0-20230316000542-bf6f9cbbf0f8
-	github.com/dustin/go-humanize v1.0.1
 	github.com/getsentry/sentry-go v0.25.0
 	github.com/go-chi/chi v1.5.5
 	github.com/go-chi/cors v1.2.1
@@ -30,6 +29,7 @@ require (
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/cespare/xxhash/v2 v2.2.0 // indirect
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
+	github.com/dustin/go-humanize v1.0.1 // indirect
 	github.com/gorilla/securecookie v1.1.2 // indirect
 	github.com/jinzhu/inflection v1.0.0 // indirect
 	github.com/jinzhu/now v1.1.5 // indirect

internal/config/config.go

@@ -192,10 +192,9 @@ type Config struct {
 	// alwaysBlockedNetworks stays blocked no matter what is listed
 	// here. That set is link-local plus the cloud metadata
 	// endpoints outside it that disclose credentials or user data
-	// at a provider-fixed, non-public address; it is not
-	// exhaustive of every cloud's metadata address. See
-	// alwaysBlockedNetworks for the authoritative list and the
-	// criterion it is built from.
+	// at a provider-fixed address; it is not exhaustive of every
+	// cloud's metadata address. See alwaysBlockedNetworks for the
+	// authoritative list and the criterion it is built from.
 	AllowedEgressCIDRs []netip.Prefix
 
 	params *ConfigParams
@@ -747,14 +746,12 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
 
 	log.Warn(
 		"ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+
-			"otherwise-blocked networks. Anyone who can create a "+
-			"delivery target can now make this process issue "+
-			"requests into them, and read back the response. Only "+
-			"the addresses the README lists as blocked "+
-			"unconditionally stay blocked regardless of what is "+
-			"listed here; a public cloud metadata address such as "+
-			"168.63.129.16 is reachable once it, or a block "+
-			"covering it, is listed.",
+			"otherwise-blocked private/reserved networks. Anyone "+
+			"who can create a delivery target can now make this "+
+			"process issue requests into them, and read back the "+
+			"response. Link-local and the known cloud instance "+
+			"metadata endpoints outside it stay blocked "+
+			"regardless of what is listed here.",
 		"allowedEgressCIDRs",
 		strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","),
 	)

internal/config/config_test.go

@@ -834,13 +834,12 @@ func TestEgressAllowlistWarning(t *testing.T) {
 			// to be able to read back which networks are open.
 			assert.Contains(t, logged, "10.0.0.0/8")
 			assert.Contains(t, logged, "127.0.0.0/8")
-			// What stays shut is the whole unconditional set, not
-			// link-local alone; a public metadata address is not in
-			// it, so a listed block covering it opens it.
-			assert.Contains(t, logged, "blocked unconditionally")
-			assert.Contains(t, logged, "168.63.129.16 is reachable")
-			// The listed blocks need not be private or reserved.
-			assert.NotContains(t, logged, "private/reserved")
+			// What stays shut. Asserted on the clause naming the
+			// wider set rather than on "Link-local" alone, so the
+			// string cannot narrow back to link-local only while
+			// the always-blocked set covers ULA, CGNAT and two
+			// public metadata addresses as well.
+			assert.Contains(t, logged, "metadata endpoints outside it")
 		})
 	}
 }

internal/delivery/engine.go

@@ -368,9 +368,9 @@ func (e *Engine) start() {
 // writer for long by then: the archive sweeper stops before the
 // engine, and deleting a webhook only closes one. If the pool did
 // not drain in time, the writers are left open, as a kill would
-// leave them. Closing them would wait for any write in progress,
-// and a worker still running would then open new writers that
-// nothing closes, so it gains nothing over a kill.
+// leave them: a worker still running may be mid-write, and
+// closing its writer would wait on that write and then fail the
+// next delivery the worker archives.
 func (e *Engine) stop(ctx context.Context) error {
 	e.log.Info("delivery engine stopping")
 

internal/delivery/engine_lifecycle_test.go

@@ -327,10 +327,10 @@ func TestEngine_StopHookClosesArchives(t *testing.T) {
 }
 
 // TestEngine_StopHookTimeoutLeavesArchivesOpen covers a stop whose
-// budget runs out while a worker is still running. The archive
-// writers are left open, as a kill would leave them: closing them
-// would wait for any write in progress, and that worker would then
-// open new writers that nothing closes.
+// budget runs out while a worker is still running. That worker may
+// be in the middle of an archive write, so the archive writers are
+// left open, as a kill would leave them, rather than closed
+// underneath it.
 func TestEngine_StopHookTimeoutLeavesArchivesOpen(t *testing.T) {
 	t.Parallel()
 

internal/delivery/engine_test.go

@@ -1166,10 +1166,6 @@ func TestIsForwardableHeader(t *testing.T) {
 	assert.False(t,
 		delivery.ExportIsForwardableHeader("Content-Length"),
 	)
-
-	assert.False(t,
-		delivery.ExportIsForwardableHeader("Content-Type"),
-	)
 }
 
 func TestTruncate(t *testing.T) {
@@ -1251,81 +1247,6 @@ func TestDoHTTPRequest_ForwardsHeaders(t *testing.T) {
 	)
 }
 
-// The event's stored inbound headers carry the same Content-Type the
-// receiver saved as the event's ContentType, so a delivery could send
-// it twice. It must go out exactly once, with a Content-Type configured
-// on the target winning, then the event's ContentType.
-func TestApplyRequestHeaders_SendsOneContentType(t *testing.T) {
-	t.Parallel()
-
-	cases := map[string]struct {
-		inbound    string
-		event      string
-		configured string
-		want       []string
-	}{
-		"inbound and event agree": {
-			inbound: testContentType,
-			event:   testContentType,
-			want:    []string{testContentType},
-		},
-		"inbound and event disagree": {
-			inbound: "text/plain",
-			event:   testContentType,
-			want:    []string{testContentType},
-		},
-		"event has none": {
-			inbound: testContentType,
-			want:    nil,
-		},
-		"target configures its own": {
-			inbound:    testContentType,
-			event:      testContentType,
-			configured: "application/xml",
-			want:       []string{"application/xml"},
-		},
-	}
-
-	for name, tc := range cases {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			inbound, err := json.Marshal(map[string][]string{
-				headerContentType: {tc.inbound},
-			})
-			require.NoError(t, err)
-
-			cfg := &delivery.HTTPTargetConfig{}
-			if tc.configured != "" {
-				cfg.Headers = map[string]string{
-					headerContentType: tc.configured,
-				}
-			}
-
-			req, err := http.NewRequestWithContext(
-				context.Background(),
-				http.MethodPost,
-				"https://target.example.com/hook",
-				http.NoBody,
-			)
-			require.NoError(t, err)
-
-			delivery.ExportApplyRequestHeaders(
-				req,
-				&database.Event{
-					Headers:     string(inbound),
-					ContentType: tc.event,
-				},
-				cfg,
-			)
-
-			assert.Equal(t,
-				tc.want, req.Header.Values(headerContentType),
-			)
-		})
-	}
-}
-
 func TestProcessDelivery_RoutesToCorrectHandler(
 	t *testing.T,
 ) {

internal/delivery/redirect_test.go

@@ -339,11 +339,10 @@ func TestRedirectPolicy_StopsAtHopCap(t *testing.T) {
 // The set the redirect policy strips is whatever the delivery path
 // actually put on the wire, so a header added to the forward set is
 // covered without a second edit. A header the event never carried
-// is not in the set, and neither is the inbound Content-Type, because
-// it is not forwarded. Two more are deliberately excluded: a
-// Content-Type configured on the target describes the body, which a
-// 307 carries across hosts, and the inbound User-Agent every real
-// sender supplies is overwritten before the request goes out.
+// is not in the set, and the delivery path's own two are deliberately
+// excluded: Content-Type describes the body, which a 307 carries
+// across hosts, and the inbound User-Agent every real sender supplies
+// is overwritten before the request goes out.
 func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	t.Parallel()
 
@@ -372,7 +371,6 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 		&delivery.HTTPTargetConfig{
 			Headers: map[string]string{
 				probeHeaderName: probeHeaderValue,
-				"Content-Type":  testContentType,
 			},
 		},
 	)
@@ -380,11 +378,7 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	assert.Equal(t,
 		[]string{probeHeaderName, inboundHeaderName}, names,
 		"both header classes are reported, and only those: "+
-			"Host and the inbound Content-Type are never "+
-			"forwarded, User-Agent is the delivery path's own",
-	)
-	assert.NotContains(t, names, "Content-Type",
-		"a Content-Type configured on the target must survive "+
-			"a cross-origin 307/308 with the body it describes",
+			"Host is never forwarded, Content-Type and "+
+			"User-Agent are the delivery path's own",
 	)
 }

internal/delivery/ssrf.go

@@ -26,7 +26,7 @@ var (
 		"hostname resolved to no IP addresses",
 	)
 	errBlockedIP = errors.New(
-		"blocked private, reserved or cloud metadata address",
+		"blocked private/reserved IP range",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -37,10 +37,9 @@ var (
 	)
 )
 
-// blockedNetworks is the default blocklist: the private and
-// reserved IP ranges, plus the public cloud metadata addresses,
-// that are blocked to prevent SSRF attacks. An operator can
-// permit specific blocks out of this set with
+// blockedNetworks contains all private/reserved IP ranges
+// that should be blocked to prevent SSRF attacks. An operator
+// can permit specific blocks out of this set with
 // ALLOWED_EGRESS_CIDRS; see Guard.
 //
 //nolint:gochecknoglobals // package-level network list is appropriate here
@@ -123,8 +122,6 @@ func init() {
 		"::1/128",
 		"fc00::/7",
 		"fe80::/10",
-		// Azure WireServer, a public address that serves VM credentials.
-		"168.63.129.16/32",
 	})
 
 	// Every entry is named. The set must not grow or shrink
@@ -219,8 +216,8 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
 }
 
 // isBlockedIP checks whether an IP address falls within
-// the default blocklist, before any operator allowlist is
-// considered.
+// any blocked private/reserved network range, before any
+// operator allowlist is considered.
 func isBlockedIP(ip net.IP) bool {
 	return matchesAny(blockedNetworks, ip)
 }
@@ -323,7 +320,7 @@ func (g *Guard) allows(ip net.IP) bool {
 //
 //  1. alwaysBlockedNetworks is refused before the allowlist is
 //     consulted, so no configured CIDR reaches link-local or a
-//     cloud metadata endpoint at a non-public address.
+//     cloud instance metadata endpoint.
 //  2. The allowlist is consulted next, so a listed private
 //     network becomes reachable.
 //  3. Everything else keeps the default blocklist's answer.

internal/delivery/ssrf_allowlist_test.go

@@ -390,41 +390,6 @@ func TestGuardAllowlist_PublicUnaffected(t *testing.T) {
 	}
 }
 
-// TestGuardAllowlist_AzureWireServerReopenable covers Azure's
-// WireServer, a public address that serves VM credentials. The
-// default guard refuses it, but because it is public it sits in
-// the default blocklist rather than the unconditional set, so an
-// operator who lists it can reach it.
-func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) {
-	t.Parallel()
-
-	const wireServerIP = "168.63.129.16"
-
-	target := "http://" + wireServerIP + "/?comp=versions"
-
-	defaultGuard := delivery.NewTestGuard()
-
-	err := defaultGuard.ValidateTargetURL(context.Background(), target)
-	require.Error(t, err,
-		"WireServer must be refused with no allowlist set",
-	)
-	assert.NotContains(t, err.Error(), metadataRefusalClause,
-		"WireServer must be refused by the default blocklist, "+
-			"which an allowlist can override",
-	)
-
-	assertDialRefused(t, defaultGuard, target)
-
-	listed := delivery.NewTestGuard(
-		netip.MustParsePrefix(wireServerIP + "/32"),
-	)
-
-	assert.NoError(t,
-		listed.ValidateTargetURL(context.Background(), target),
-		"an operator who lists WireServer must be able to reach it",
-	)
-}
-
 // TestGuardCheckIP_BothPathsShareOneDecision asserts that the
 // validator and the dialer are not two policies that happen to
 // agree: both are defined in terms of checkIP, so the exported

internal/delivery/target_headers_test.go

@@ -11,11 +11,10 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-// Literals these tests repeat, named so that the header names and the
+// Literals these tests repeat, named so that the header name and the
 // keep-forever archive config each have one definition.
 const (
 	headerAuthorization = "Authorization"
-	headerContentType   = "Content-Type"
 	bearerValue         = "Bearer abc"
 	archiveConfigNever  = "{\"expiry\":\"never\"}"
 )

internal/delivery/target_http.go

@@ -541,11 +541,6 @@ func isForwardableHeader(name string) bool {
 		"Upgrade", "Proxy-Authorization",
 		"Proxy-Connection", "Content-Length":
 		return false
-	case "Content-Type":
-		// applyRequestHeaders sets Content-Type itself. The receiver
-		// already stored this inbound value as the event's
-		// ContentType, so forwarding it too would send it twice.
-		return false
 	default:
 		return true
 	}
@@ -558,10 +553,6 @@ func isForwardableHeader(name string) bool {
 // policy strips exactly that set on a hop that leaves the origin,
 // so the forward set is decided here and only here — a header added
 // to it is covered off-origin without a second edit elsewhere.
-//
-// Content-Type goes out once: a Content-Type configured on the target
-// wins, otherwise the event's ContentType, otherwise none. The inbound
-// Content-Type in the event's headers is never forwarded.
 func applyRequestHeaders(
 	req *http.Request,
 	event *database.Event,
@@ -582,10 +573,10 @@ func applyRequestHeaders(
 
 	req.Header.Set("User-Agent", "webhooker/1.0")
 
-	// A Content-Type configured on the target describes the body
-	// being sent rather than the sender. A 307/308 preserves the
-	// body across hosts, so stripping it would send that body
-	// untyped.
+	// Content-Type describes the body being sent rather than the
+	// sender, and the delivery path sets it from the event itself.
+	// A 307/308 preserves the body across hosts, so stripping it
+	// would send that body untyped.
 	delete(originScoped, "Content-Type")
 
 	// User-Agent is overwritten just above, so an inbound one never

internal/handlers/delivery_result_view.go

@@ -1,8 +1,6 @@
 package handlers
 
 import (
-	"time"
-
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
@@ -27,7 +25,7 @@ const maxRenderedResponseBytes = 4096
 // cut, so an oversized stored response never becomes a Go
 // string at all.
 const deliveryResultColumns = "delivery_id, attempt_num, success, " +
-	"status_code, error, duration, created_at, " +
+	"status_code, error, duration, " +
 	"substr(cast(response_body as blob), 1, ?) AS response_body, " +
 	"length(cast(response_body as blob)) AS response_bytes"
 
@@ -108,10 +106,6 @@ type deliveryResultRow struct {
 	Duration      int64
 	ResponseBody  []byte
 	ResponseBytes int64
-
-	// CreatedAt is when the attempt's result was recorded, which
-	// is when the attempt finished.
-	CreatedAt time.Time
 }
 
 // view projects a loaded row for rendering, stripping the

internal/handlers/handlers.go

@@ -28,7 +28,7 @@ const (
 	// maxBodyShift is the bit shift for 1 MB body limit.
 	maxBodyShift = 20
 	// recentEventLimit is the number of recent events to show.
-	recentEventLimit = 50
+	recentEventLimit = 20
 	// paginationPerPage is the number of items per page.
 	paginationPerPage = 25
 

internal/handlers/recent_events.go

@@ -1,248 +0,0 @@
-package handlers
-
-import (
-	"net/http"
-	"strconv"
-	"time"
-
-	"github.com/dustin/go-humanize"
-	"gorm.io/gorm"
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// recentEventColumns is the recent events list's projection. It
-// reads the body's size and never the body itself, for the reason
-// maxRenderedBodyBytes gives; the cast to blob makes length count
-// bytes rather than characters.
-const recentEventColumns = "id, created_at, method, content_type, " +
-	"resubmitted_from_id, length(cast(body as blob)) AS body_bytes"
-
-// RecentEventView is one row of the recent events list on a
-// webhook's page.
-type RecentEventView struct {
-	Method      string
-	ContentType string
-
-	// ResubmittedFromID names the event this one was copied from,
-	// empty for an event that arrived on the receiver.
-	ResubmittedFromID string
-
-	// Received is how long ago the event arrived, and ReceivedUTC
-	// the full timestamp the page shows on hover.
-	Received    string
-	ReceivedUTC string
-
-	// Size is the size of the stored body.
-	Size string
-
-	// ProcessingTime is how long the event's slowest delivery
-	// took; see processingTime.
-	ProcessingTime string
-
-	// Status is what the webhook's HTTP target answered, and
-	// StatusClass its colour; see targetStatus. Both are empty
-	// unless the webhook has exactly one HTTP target.
-	Status      string
-	StatusClass string
-}
-
-// recentEventRow is one row of recentEventColumns.
-type recentEventRow struct {
-	ID                string
-	CreatedAt         time.Time
-	Method            string
-	ContentType       string
-	ResubmittedFromID *string
-	BodyBytes         uint64
-}
-
-// singleHTTPTargetID returns the ID of the webhook's HTTP target
-// when it has exactly one, and "" when it has none or several.
-func singleHTTPTargetID(targets []database.Target) string {
-	id := ""
-	count := 0
-
-	for i := range targets {
-		if targets[i].Type == database.TargetTypeHTTP {
-			id = targets[i].ID
-			count++
-		}
-	}
-
-	if count != 1 {
-		return ""
-	}
-
-	return id
-}
-
-// loadRecentEvents loads the webhook's recentEventLimit newest
-// events for its page, newest first. statusTargetID is the
-// webhook's only HTTP target, or "" when the list shows no status.
-func (h *Handlers) loadRecentEvents(
-	webhookDB *gorm.DB, webhookID, statusTargetID string,
-) ([]RecentEventView, error) {
-	var rows []recentEventRow
-
-	err := webhookDB.Model(&database.Event{}).
-		Select(recentEventColumns).
-		Where("webhook_id = ?", webhookID).
-		Order("created_at DESC").
-		Limit(recentEventLimit).
-		Find(&rows).Error
-	if err != nil {
-		return nil, err
-	}
-
-	eventIDs := make([]string, len(rows))
-	for i := range rows {
-		eventIDs[i] = rows[i].ID
-	}
-
-	// Oldest first, so an event's last delivery to a target is its
-	// newest: a replay adds a delivery rather than changing the
-	// earlier one.
-	var deliveries []database.Delivery
-
-	err = webhookDB.
-		Select("id, event_id, target_id, status, created_at").
-		Where("event_id IN ?", eventIDs).
-		Order("created_at ASC").
-		Find(&deliveries).Error
-	if err != nil {
-		return nil, err
-	}
-
-	byEvent := make(map[string][]database.Delivery, len(rows))
-	deliveryIDs := make([]string, len(deliveries))
-
-	for i := range deliveries {
-		eventID := deliveries[i].EventID
-		byEvent[eventID] = append(byEvent[eventID], deliveries[i])
-		deliveryIDs[i] = deliveries[i].ID
-	}
-
-	attempts, err := h.loadDeliveryResults(webhookDB, deliveryIDs)
-	if err != nil {
-		return nil, err
-	}
-
-	views := make([]RecentEventView, len(rows))
-	for i := range rows {
-		views[i] = rows[i].view(
-			byEvent[rows[i].ID], attempts, statusTargetID,
-		)
-	}
-
-	return views, nil
-}
-
-// view projects a loaded row for rendering. deliveries is the
-// event's deliveries, oldest first, and attempts their recorded
-// attempts keyed by delivery ID.
-func (r *recentEventRow) view(
-	deliveries []database.Delivery,
-	attempts map[string][]deliveryResultRow,
-	statusTargetID string,
-) RecentEventView {
-	v := RecentEventView{
-		Method:         r.Method,
-		ContentType:    r.ContentType,
-		Received:       humanize.Time(r.CreatedAt),
-		ReceivedUTC:    r.CreatedAt.UTC().Format(time.DateTime) + " UTC",
-		Size:           humanize.Bytes(r.BodyBytes),
-		ProcessingTime: processingTime(deliveries, attempts),
-	}
-
-	if r.ResubmittedFromID != nil {
-		v.ResubmittedFromID = *r.ResubmittedFromID
-	}
-
-	if statusTargetID != "" {
-		v.Status, v.StatusClass = targetStatus(
-			deliveries, attempts, statusTargetID,
-		)
-	}
-
-	return v
-}
-
-// processingTime is how long the event's slowest delivery took,
-// from being queued to its last recorded attempt, time spent
-// waiting between retries included. A delivery is queued when its
-// event is received, or when an operator replays it, so a replay
-// is timed from the replay rather than from the event's arrival.
-// It is "in progress" while any delivery is pending or retrying,
-// and empty for an event with no deliveries.
-func processingTime(
-	deliveries []database.Delivery,
-	attempts map[string][]deliveryResultRow,
-) string {
-	if len(deliveries) == 0 {
-		return ""
-	}
-
-	var slowest time.Duration
-
-	for i := range deliveries {
-		if !deliveries[i].Status.Terminal() {
-			return "in progress"
-		}
-
-		tries := attempts[deliveries[i].ID]
-		if len(tries) == 0 {
-			continue
-		}
-
-		last := tries[len(tries)-1].CreatedAt
-		slowest = max(slowest, last.Sub(deliveries[i].CreatedAt))
-	}
-
-	return slowest.Round(time.Millisecond).String()
-}
-
-// targetStatus is what the target answered for the event, and the
-// colour to show it in: the HTTP status code of the last attempt of
-// the event's newest delivery to the target. Without a code it is
-// "no response" when that attempt failed before a response
-// arrived, the delivery's status ("pending") before any attempt,
-// and "not sent" when the event has no delivery to the target.
-func targetStatus(
-	deliveries []database.Delivery,
-	attempts map[string][]deliveryResultRow,
-	targetID string,
-) (string, string) {
-	newest := -1
-
-	for i := range deliveries {
-		if deliveries[i].TargetID == targetID {
-			newest = i
-		}
-	}
-
-	if newest < 0 {
-		return "not sent", "text-gray-400"
-	}
-
-	tries := attempts[deliveries[newest].ID]
-	if len(tries) == 0 {
-		return string(deliveries[newest].Status), "text-gray-400"
-	}
-
-	code := tries[len(tries)-1].StatusCode
-
-	switch {
-	case code == 0:
-		return "no response", "text-red-600"
-	case code >= http.StatusInternalServerError:
-		return strconv.Itoa(code), "text-red-600"
-	case code >= http.StatusBadRequest:
-		return strconv.Itoa(code), "text-yellow-600"
-	case code >= http.StatusMultipleChoices:
-		return strconv.Itoa(code), "text-gray-500"
-	case code >= http.StatusOK:
-		return strconv.Itoa(code), "text-green-600"
-	default:
-		return strconv.Itoa(code), "text-gray-500"
-	}
-}

internal/handlers/recent_events_test.go

@@ -1,295 +0,0 @@
-package handlers_test
-
-import (
-	"fmt"
-	"net/http"
-	"strings"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"gorm.io/gorm"
-	"gorm.io/gorm/clause"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/handlers"
-	"sneak.berlin/go/webhooker/internal/session"
-)
-
-// statusTitle marks the status column's cell in a recent events
-// row; it is absent from the page when the column is not shown.
-const statusTitle = `title="HTTP status from the HTTP target"`
-
-// recentEventsFixture is one started app and a webhook whose
-// recent events list a test fills.
-type recentEventsFixture struct {
-	h         *handlers.Handlers
-	sess      *session.Session
-	db        *database.Database
-	webhook   *database.Webhook
-	webhookDB *gorm.DB
-}
-
-func newRecentEventsFixture(t *testing.T) *recentEventsFixture {
-	t.Helper()
-
-	f := &recentEventsFixture{}
-
-	var dbMgr *database.WebhookDBManager
-
-	app := newTestApp(t, &f.h, &f.sess, &f.db, &dbMgr)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	f.webhook = seedWebhook(t, f.db)
-
-	webhookDB, err := dbMgr.GetDB(f.webhook.ID)
-	require.NoError(t, err)
-
-	f.webhookDB = webhookDB
-
-	return f
-}
-
-func (f *recentEventsFixture) render(t *testing.T) string {
-	t.Helper()
-
-	return renderSourceDetailPage(t, f.h, f.sess, f.webhook.ID)
-}
-
-// event records an event received at receivedAt.
-func (f *recentEventsFixture) event(
-	t *testing.T, contentType, body string, receivedAt time.Time,
-) *database.Event {
-	t.Helper()
-
-	event := &database.Event{
-		WebhookID:   f.webhook.ID,
-		Method:      http.MethodPost,
-		Body:        body,
-		ContentType: contentType,
-	}
-	event.CreatedAt = receivedAt
-
-	require.NoError(t, f.webhookDB.Omit(
-		clause.Associations,
-	).Create(event).Error)
-
-	return event
-}
-
-// delivery records a delivery of the event to the target, queued
-// when the event was received.
-func (f *recentEventsFixture) delivery(
-	t *testing.T,
-	event *database.Event,
-	targetID string,
-	status database.DeliveryStatus,
-) *database.Delivery {
-	t.Helper()
-
-	return f.deliveryQueuedAt(
-		t, event, targetID, status, event.CreatedAt,
-	)
-}
-
-// deliveryQueuedAt records a delivery of the event to the target,
-// queued at queuedAt, as a replay is.
-func (f *recentEventsFixture) deliveryQueuedAt(
-	t *testing.T,
-	event *database.Event,
-	targetID string,
-	status database.DeliveryStatus,
-	queuedAt time.Time,
-) *database.Delivery {
-	t.Helper()
-
-	dlv := &database.Delivery{
-		EventID:  event.ID,
-		TargetID: targetID,
-		Status:   status,
-	}
-	dlv.CreatedAt = queuedAt
-
-	require.NoError(t, f.webhookDB.Omit(
-		clause.Associations,
-	).Create(dlv).Error)
-
-	return dlv
-}
-
-// attempt records one attempt of the delivery that finished took
-// after the delivery was queued, with HTTP status code (0 for no
-// response).
-func (f *recentEventsFixture) attempt(
-	t *testing.T, dlv *database.Delivery, code int, took time.Duration,
-) {
-	t.Helper()
-
-	result := &database.DeliveryResult{
-		DeliveryID: dlv.ID,
-		AttemptNum: 1,
-		StatusCode: code,
-	}
-	result.CreatedAt = dlv.CreatedAt.Add(took)
-
-	require.NoError(t, f.webhookDB.Omit(
-		clause.Associations,
-	).Create(result).Error)
-}
-
-// statusCell is the status column's cell as the page renders it.
-func statusCell(class, text string) string {
-	return `<span class="font-medium ` + class + `" ` + statusTitle +
-		`>` + text + `</span>`
-}
-
-// TestHandleSourceDetail_ShowsFiftyNewestEvents proves the list
-// holds the 50 newest events, newest first, and not one more.
-func TestHandleSourceDetail_ShowsFiftyNewestEvents(t *testing.T) {
-	t.Parallel()
-
-	f := newRecentEventsFixture(t)
-	base := time.Now().Add(-time.Hour)
-
-	for i := range 51 {
-		f.event(
-			t, fmt.Sprintf("application/x-recent-%02d", i), "{}",
-			base.Add(time.Duration(i)*time.Second),
-		)
-	}
-
-	body := f.render(t)
-
-	assert.Equal(t, 50, strings.Count(body, `title="Body size"`))
-	assert.NotContains(t, body, "application/x-recent-00")
-	assert.Contains(t, body, "application/x-recent-01")
-	assert.Less(
-		t,
-		strings.Index(body, "application/x-recent-50"),
-		strings.Index(body, "application/x-recent-49"),
-	)
-}
-
-// TestHandleSourceDetail_RecentEventColumns proves a row shows its
-// time relative with the UTC timestamp on hover, its body size,
-// and its processing time once every delivery has finished.
-func TestHandleSourceDetail_RecentEventColumns(t *testing.T) {
-	t.Parallel()
-
-	f := newRecentEventsFixture(t)
-	logTarget := seedTarget(t, f.db, f.webhook.ID, database.TargetTypeLog)
-
-	receivedAt := time.Now().Add(-210 * time.Second).
-		UTC().Truncate(time.Second)
-
-	done := f.event(
-		t, contentTypeJSON, strings.Repeat("x", 2048), receivedAt,
-	)
-	f.attempt(
-		t,
-		f.delivery(t, done, logTarget.ID, database.DeliveryStatusDelivered),
-		0, 1500*time.Millisecond,
-	)
-
-	waiting := f.event(t, "text/plain", "{}", receivedAt)
-	f.delivery(t, waiting, logTarget.ID, database.DeliveryStatusPending)
-
-	body := f.render(t)
-
-	assert.Contains(
-		t, body,
-		`<span title="`+receivedAt.Format(time.DateTime)+
-			` UTC">3 minutes ago</span>`,
-	)
-	assert.Contains(t, body, `<span title="Body size">2.0 kB</span>`)
-	assert.Contains(t, body, ">1.5s</span>")
-	assert.Contains(t, body, ">in progress</span>")
-}
-
-// TestHandleSourceDetail_StatusWithSingleHTTPTarget proves that a
-// webhook with exactly one HTTP target shows, colour-coded, what
-// that target answered for each event. The log target beside it
-// does not count against "exactly one".
-func TestHandleSourceDetail_StatusWithSingleHTTPTarget(t *testing.T) {
-	t.Parallel()
-
-	f := newRecentEventsFixture(t)
-	target := seedTarget(t, f.db, f.webhook.ID, database.TargetTypeHTTP)
-	seedTarget(t, f.db, f.webhook.ID, database.TargetTypeLog)
-
-	now := time.Now()
-
-	for _, code := range []int{204, 302, 404, 503, 0} {
-		dlv := f.delivery(
-			t, f.event(t, contentTypeJSON, "{}", now), target.ID,
-			database.DeliveryStatusDelivered,
-		)
-		f.attempt(t, dlv, code, time.Second)
-	}
-
-	f.delivery(
-		t, f.event(t, contentTypeJSON, "{}", now), target.ID,
-		database.DeliveryStatusPending,
-	)
-	f.event(t, contentTypeJSON, "{}", now)
-
-	// A replay is a newer delivery, and its answer is the one shown.
-	replayed := f.event(t, contentTypeJSON, "{}", now)
-	f.attempt(t, f.delivery(
-		t, replayed, target.ID, database.DeliveryStatusFailed,
-	), 502, time.Second)
-	f.attempt(t, f.deliveryQueuedAt(
-		t, replayed, target.ID, database.DeliveryStatusDelivered,
-		now.Add(time.Minute),
-	), 200, time.Second)
-
-	body := f.render(t)
-
-	assert.Contains(t, body, statusCell("text-green-600", "204"))
-	assert.Contains(t, body, statusCell("text-gray-500", "302"))
-	assert.Contains(t, body, statusCell("text-yellow-600", "404"))
-	assert.Contains(t, body, statusCell("text-red-600", "503"))
-	assert.Contains(t, body, statusCell("text-red-600", "no response"))
-	assert.Contains(t, body, statusCell("text-gray-400", "pending"))
-	assert.Contains(t, body, statusCell("text-gray-400", "not sent"))
-	assert.Contains(t, body, statusCell("text-green-600", "200"))
-	assert.NotContains(t, body, ">502<")
-}
-
-// TestHandleSourceDetail_NoStatusWithoutSingleHTTPTarget proves the
-// status column is absent when the webhook has no HTTP target or
-// more than one.
-func TestHandleSourceDetail_NoStatusWithoutSingleHTTPTarget(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	cases := map[string][]database.TargetType{
-		"none":    {database.TargetTypeLog},
-		"several": {database.TargetTypeHTTP, database.TargetTypeHTTP},
-	}
-
-	for name, types := range cases {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			f := newRecentEventsFixture(t)
-			event := f.event(t, contentTypeJSON, "{}", time.Now())
-
-			for _, tt := range types {
-				target := seedTarget(t, f.db, f.webhook.ID, tt)
-				f.attempt(t, f.delivery(
-					t, event, target.ID,
-					database.DeliveryStatusDelivered,
-				), 200, time.Second)
-			}
-
-			body := f.render(t)
-
-			assert.Contains(t, body, `title="Body size"`)
-			assert.NotContains(t, body, statusTitle)
-		})
-	}
-}

internal/handlers/source_management.go

@@ -415,21 +415,16 @@ func (h *Handlers) renderSourceDetail(
 		"webhook_id = ?", webhook.ID,
 	).Find(&targets)
 
-	var events []RecentEventView
+	var events []database.Event
 
 	if h.dbMgr.DBExists(webhook.ID) {
 		webhookDB, dbErr := h.dbMgr.GetDB(webhook.ID)
 		if dbErr == nil {
-			events, dbErr = h.loadRecentEvents(
-				webhookDB, webhook.ID, singleHTTPTargetID(targets),
-			)
-		}
-
-		if dbErr != nil {
-			h.log.Error(
-				"failed to load recent events",
-				"webhook_id", webhook.ID, "error", dbErr,
-			)
+			webhookDB.Where(
+				"webhook_id = ?", webhook.ID,
+			).Order("created_at DESC").Limit(
+				recentEventLimit,
+			).Find(&events)
 		}
 	}
 

internal/middleware/recoverer.go

@@ -133,11 +133,6 @@ 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(
@@ -169,8 +164,6 @@ func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
 					return
 				}
 
-				rw.Header().Del("Set-Cookie")
-
 				http.Error(
 					rw,
 					http.StatusText(

internal/middleware/recoverer_test.go

@@ -304,44 +304,16 @@ 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,
-// cookie included, so a second WriteHeader would change nothing the
-// client sees and would draw net/http's "superfluous
-// response.WriteHeader" report.
+// 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"))
 
@@ -359,7 +331,6 @@ 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"])

internal/server/routes.go

@@ -92,25 +92,11 @@ func (s *Server) setupGlobalMiddleware() {
 func (s *Server) setupRoutes() {
 	s.router.Get("/", s.h.HandleIndex())
 
-	// Static assets answer GET and HEAD only. chi's default 405
-	// carries no Allow header, so this group supplies its own.
-	staticFiles := http.StripPrefix(
-		"/s", http.FileServer(http.FS(static.Static)),
+	s.router.Mount(
+		"/s",
+		http.StripPrefix("/s", http.FileServer(http.FS(static.Static))),
 	)
 
-	s.router.Route("/s", func(r chi.Router) {
-		r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Allow", "GET, HEAD")
-			http.Error(
-				w,
-				"Method Not Allowed",
-				http.StatusMethodNotAllowed,
-			)
-		})
-		r.Method(http.MethodGet, "/*", staticFiles)
-		r.Method(http.MethodHead, "/*", staticFiles)
-	})
-
 	s.router.Route("/api/v1", func(_ chi.Router) {
 		// API routes will be added here.
 	})

internal/server/routes_test.go

@@ -396,15 +396,13 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
 
 // --- /s static group ---
 
-// TestStaticServesOnlyGetAndHead pins the methods the static group
-// answers: GET and HEAD are served the asset, and the other methods
-// chi routes (POST, PUT, DELETE and the rest) are refused with 405
-// and an Allow header naming those two. A method chi does not route,
-// such as PROPFIND, is refused with 405 by the top-level router
-// before it reaches the static group, so it gets no Allow header.
-// The README documents this; the test is what keeps the two from
-// drifting.
-func TestStaticServesOnlyGetAndHead(t *testing.T) {
+// TestStaticServesEveryMethod pins what the static mount actually
+// answers. chi's Mount registers the handler for all methods and
+// http.FileServer only special-cases HEAD (by suppressing the body),
+// so a POST or a DELETE to an asset is served the file rather than
+// refused. The README documents this; the test is what keeps the two
+// from drifting.
+func TestStaticServesEveryMethod(t *testing.T) {
 	t.Parallel()
 
 	env := newTestEnv(t)
@@ -419,7 +417,6 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 		http.MethodPost,
 		http.MethodPut,
 		http.MethodDelete,
-		"PROPFIND",
 	} {
 		t.Run(method, func(t *testing.T) {
 			t.Parallel()
@@ -431,38 +428,18 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 			w := httptest.NewRecorder()
 			env.router.ServeHTTP(w, req)
 
-			switch method {
-			case http.MethodGet:
-				assert.Equal(t, http.StatusOK, w.Code)
-				assert.Equal(t, body, w.Body.Bytes(),
-					"the asset itself is returned")
-			case http.MethodHead:
-				assert.Equal(t, http.StatusOK, w.Code)
+			assert.Equal(t, http.StatusOK, w.Code,
+				"static mount answers every method")
+
+			if method == http.MethodHead {
 				assert.Empty(t, w.Body.Bytes(),
 					"HEAD must not carry a body")
-			case "PROPFIND":
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Empty(t, w.Header().Get("Allow"),
-					"chi refuses a method it does not route "+
-						"before the static group runs")
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
-			default:
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Equal(
-					t, "GET, HEAD", w.Header().Get("Allow"),
-				)
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
+
+				return
 			}
+
+			assert.Equal(t, body, w.Body.Bytes(),
+				"the asset itself is returned")
 		})
 	}
 }

templates/source_detail.html

@@ -187,24 +187,12 @@
         <div class="divide-y divide-gray-100">
             {{range .Events}}
             <div class="p-4">
-                <div class="flex flex-wrap items-center justify-between gap-3">
-                    <div class="flex flex-wrap items-center gap-3">
+                <div class="flex items-center justify-between">
+                    <div class="flex items-center gap-3">
                         <span class="badge-info">{{.Method}}</span>
-                        <span class="text-sm text-gray-500 break-all">{{.ContentType}}</span>
-                        {{if .ResubmittedFromID}}
-                        <span class="text-xs text-gray-500" title="This event is a copy of {{.ResubmittedFromID}}">resubmitted copy</span>
-                        {{end}}
-                    </div>
-                    <div class="flex flex-wrap items-center gap-3 text-xs text-gray-400">
-                        <span title="Body size">{{.Size}}</span>
-                        {{if .ProcessingTime}}
-                        <span title="Processing time: how long the slowest delivery took, from being queued to its last attempt">{{.ProcessingTime}}</span>
-                        {{end}}
-                        {{if .Status}}
-                        <span class="font-medium {{.StatusClass}}" title="HTTP status from the HTTP target">{{.Status}}</span>
-                        {{end}}
-                        <span title="{{.ReceivedUTC}}">{{.Received}}</span>
+                        <span class="text-sm text-gray-500">{{.ContentType}}</span>
                     </div>
+                    <span class="text-xs text-gray-400">{{.CreatedAt.Format "2006-01-02 15:04:05 UTC"}}</span>
                 </div>
             </div>
             {{else}}