Harden operator-set target headers (closes #233)

Three findings from the review of the per-target request headers
feature.

Configured headers no longer follow a redirect off the origin the
target names. net/http withholds only Authorization and Cookie
across a host change, so an operator's X-Api-Key or PRIVATE-TOKEN
would follow a 302 to a host they never configured. Redirects are
still followed — refusing them would break every destination that
legitimately redirects and would record the 3xx as the delivery's
result — but a hop to another host, another port, or down from
https to http drops every header the target configured. The shared
SSRF-safe transport is kept on that client, so each hop is still
dialled through the private-IP guard.

Trailer joins the reserved names. net/http strips it from the
request it writes, so a configured one was accepted, stored, and
provably never sent.

The invalid-header-name error no longer quotes the text before the
first colon. That text is only a name if it parses as one; when it
does not, a pasted value whose own colon split the line put half a
token into a 400 body. TestParseTargetHeaders_ErrorsNeverQuoteAValue
asserted this invariant while only exercising the after-the-colon
case, and now covers the before-the-colon one.

README documents the http target's config keys, the 300-second
timeout ceiling, the reserved-header list and the redirect
behaviour; the edit form's hint gains Trailer and the redirect note.

README.md

@@ -1072,24 +1072,17 @@ reserved: a configured one deliberately overrides the event's.
 
 **Redirects.** A redirect from an `http` target's destination is
 followed, up to ten hops, and the delivery's recorded status and body
-come from the final hop. One rule governs every header the delivery
+come from the final hop. Configured `headers` are dropped as soon as a
-carries for someone else — the configured `headers` and the inbound
+hop leaves the origin the target names — a different host, a different
-event headers forwarded from the sender alike: **a hop that leaves the
+port, or a step down from `https` to `http` — because a configured
-origin the target names carries none of them.** Leaving the origin
+header is routinely a credential (`X-Api-Key`, `PRIVATE-TOKEN`) and an
-means a different host, a different port, or a step down from `https`
-to `http`. Both classes routinely carry a secret — a configured
-`X-Api-Key` or `PRIVATE-TOKEN`, an inbound `X-Hub-Signature` — and an
 open redirect at the destination would otherwise hand it to a host the
 operator never chose. `net/http` already does this for `Authorization`
-and `Cookie`. The delivery path's own headers (`Content-Type`,
+and `Cookie`. Redirects within the target's own origin keep every
-`User-Agent`) are not origin-scoped and always travel, so a body
+configured header, so a destination that redirects its own paths is
-preserved across a `307` is still typed. Redirects within the target's
+unaffected. Each hop is dialled through the same SSRF guard as the
-own origin keep everything, so a destination that redirects its own
+first, so a redirect aimed at a private or reserved address is refused
-paths is unaffected; the drop is per hop rather than permanent, so a
+at connect time.
-chain that returns to the configured origin carries the headers again,
-exactly as `net/http` treats `Authorization`. Each hop is dialled
-through the same SSRF guard as the first, so a redirect aimed at a
-private or reserved address is refused at connect time.
 
 #### APIKey
 

internal/delivery/client_ssrf_test.go

@@ -25,12 +25,12 @@ func newSSRFTestEngine() *delivery.Engine {
 	return delivery.NewTestEngine(log, client, 1)
 }
 
-// TestClientForRequest_TimeoutKeepsSSRFGuard asserts that a
+// TestClientForConfig_TimeoutKeepsSSRFGuard asserts that a
-// client returned by clientForRequest for a config with a
+// client returned by clientForConfig for a config with a
 // per-target timeout still refuses connections to
 // private/reserved addresses (the timeout must not drop the
 // SSRF-safe transport).
-func TestClientForRequest_TimeoutKeepsSSRFGuard(t *testing.T) {
+func TestClientForConfig_TimeoutKeepsSSRFGuard(t *testing.T) {
 	t.Parallel()
 
 	engine := newSSRFTestEngine()
@@ -50,7 +50,7 @@ func TestClientForRequest_TimeoutKeepsSSRFGuard(t *testing.T) {
 				Timeout: 5,
 			}
 
-			client := engine.ExportClientForRequest(cfg, nil)
+			client := engine.ExportClientForConfig(cfg)
 
 			require.NotSame(t, engine.ExportClient(), client,
 				"a per-target timeout must yield a "+
@@ -91,11 +91,11 @@ func TestClientForRequest_TimeoutKeepsSSRFGuard(t *testing.T) {
 	}
 }
 
-// TestClientForRequest_NoTimeoutUnchanged asserts that a
+// TestClientForConfig_NoTimeoutUnchanged asserts that a
-// request with neither a per-target timeout nor an origin-scoped
+// config overriding neither the timeout nor the headers gets
-// header gets the shared SSRF-safe client unchanged: there is then
+// the shared SSRF-safe client unchanged: with no configured
-// nothing for a redirect policy to strip.
+// headers there is nothing for a redirect policy to strip.
-func TestClientForRequest_NoTimeoutUnchanged(t *testing.T) {
+func TestClientForConfig_NoTimeoutUnchanged(t *testing.T) {
 	t.Parallel()
 
 	engine := newSSRFTestEngine()
@@ -104,7 +104,7 @@ func TestClientForRequest_NoTimeoutUnchanged(t *testing.T) {
 		URL: "https://example.com/hook",
 	}
 
-	client := engine.ExportClientForRequest(cfg, nil)
+	client := engine.ExportClientForConfig(cfg)
 
 	assert.Same(t, engine.ExportClient(), client,
 		"without a per-target timeout the shared client "+
@@ -112,12 +112,12 @@ func TestClientForRequest_NoTimeoutUnchanged(t *testing.T) {
 	)
 }
 
-// TestClientForRequest_HeadersKeepSSRFGuard asserts that the
+// TestClientForConfig_HeadersKeepSSRFGuard asserts that the
-// redirect policy an origin-scoped header installs is added to a
+// redirect policy a target's configured headers install is
-// client that still carries the SSRF-safe transport. The guard is
+// added to a client that still carries the SSRF-safe
-// a dial hook, so keeping it is what makes each redirect hop pass
+// transport. The guard is a dial hook, so keeping it is what
-// the private-IP check too.
+// makes each redirect hop pass the private-IP check too.
-func TestClientForRequest_HeadersKeepSSRFGuard(t *testing.T) {
+func TestClientForConfig_HeadersKeepSSRFGuard(t *testing.T) {
 	t.Parallel()
 
 	engine := newSSRFTestEngine()
@@ -129,12 +129,10 @@ func TestClientForRequest_HeadersKeepSSRFGuard(t *testing.T) {
 		},
 	}
 
-	client := engine.ExportClientForRequest(
+	client := engine.ExportClientForConfig(cfg)
-		cfg, []string{"X-Api-Key"},
-	)
 
 	require.NotNil(t, client.CheckRedirect,
-		"an origin-scoped header must install a redirect policy",
+		"configured headers must install a redirect policy",
 	)
 
 	assert.Same(t,

internal/delivery/engine_integration_test.go

@@ -1201,83 +1201,6 @@ func TestDeliverHTTP_TargetTimeout(t *testing.T) {
 	iAssertResultFailed(t, db, del.ID)
 }
 
-// TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog pins the size
-// this engine stores for an oversized response, because the
-// event log's redaction is written against it: the row holds
-// exactly maxBodyLog bytes and records nothing about how much
-// more the remote sent, so a credential echoed across that
-// boundary reaches the database already severed and no reader
-// of the row can tell the cut happened.
-func TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	// Padded so the cut falls five bytes before the end of the
-	// echoed webhook URL.
-	const (
-		severedTail = 5
-		overshoot   = 100000
-	)
-
-	sent := strings.Repeat(
-		"A",
-		delivery.ExportMaxBodyLog-len(slackWebhookURL)+
-			severedTail,
-	) + slackWebhookURL + strings.Repeat("Z", overshoot)
-
-	s := newISetup(t)
-
-	ts := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusBadGateway)
-			_, _ = io.WriteString(w, sent)
-		},
-	))
-	defer ts.Close()
-
-	cfgJSON := iHTTPConfig(ts.URL)
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"cut":"test"}`,
-	)
-	targetID := uuid.New().String()
-
-	del := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	bodyStr := event.Body
-	task := iTask(
-		del, event, s.WebhookID, targetID,
-		"cut-target", cfgJSON, 0, 1, &bodyStr,
-	)
-
-	s.Engine.ExportProcessNewTask(context.TODO(), &task)
-
-	results := iResults(t, s.WebhookDB, del.ID)
-	require.Len(t, results, 1)
-
-	stored := results[0].ResponseBody
-
-	assert.Len(
-		t, stored, delivery.ExportMaxBodyLog,
-		"an oversized response is stored at exactly the cap",
-	)
-	assert.Equal(
-		t, sent[:delivery.ExportMaxBodyLog], stored,
-	)
-	assert.NotContains(
-		t, stored, slackWebhookURL,
-		"the echoed URL is severed by the cut",
-	)
-	assert.Contains(
-		t, stored, "T00000000",
-		"the severed prefix still carries the credential",
-	)
-}
-
 // iSeedEventAndDelivery creates event + delivery
 // for standalone tests.
 func iSeedEventAndDelivery(

internal/delivery/export_test.go

@@ -26,12 +26,6 @@ const (
 	ExportRetryChannelSize        = retryChannelSize
 	ExportDefaultFailureThreshold = defaultFailureThreshold
 	ExportDefaultCooldown         = defaultCooldown
-
-	// ExportMaxBodyLog is the cap the engine applies to a
-	// recorded response body. The event log's handling of a cut
-	// response is written against this number, so a test has to
-	// be able to name it.
-	ExportMaxBodyLog = maxBodyLog
 )
 
 // ExportIsBlockedIP exposes isBlockedIP for testing.
@@ -50,14 +44,13 @@ func ExportIsForwardableHeader(name string) bool {
 }
 
 // ExportApplyRequestHeaders exposes applyRequestHeaders, so a test
-// can inspect the header set an outbound delivery actually carries
+// can inspect the header set an outbound delivery actually carries.
-// and the origin-scoped names it reports for the redirect policy.
 func ExportApplyRequestHeaders(
 	req *http.Request,
 	event *database.Event,
 	cfg *HTTPTargetConfig,
-) []string {
+) {
-	return applyRequestHeaders(req, event, cfg)
+	applyRequestHeaders(req, event, cfg)
 }
 
 // ExportTruncate exposes truncate for testing.
@@ -167,24 +160,14 @@ func (e *Engine) ExportDoHTTPRequest(
 	return e.httpTarget.doHTTPRequest(ctx, cfg, event)
 }
 
-// ExportClientForRequest exposes the http target's
+// ExportClientForConfig exposes the http target's
-// clientForRequest.
+// clientForConfig.
-func (e *Engine) ExportClientForRequest(
+func (e *Engine) ExportClientForConfig(
 	cfg *HTTPTargetConfig,
-	originScoped []string,
 ) *http.Client {
-	return e.httpTarget.clientForRequest(cfg, originScoped)
+	return e.httpTarget.clientForConfig(cfg)
 }
 
-// ErrExportTooManyRedirects exposes the sentinel the redirect
-// policy returns once a chain exceeds the hop cap. It carries the
-// Err prefix rather than this file's usual Export one because it
-// is a sentinel error.
-var ErrExportTooManyRedirects = errTooManyRedirects
-
-// ExportMaxDeliveryRedirects exposes the redirect hop cap.
-const ExportMaxDeliveryRedirects = maxDeliveryRedirects
-
 // ExportSameDeliveryOrigin exposes sameDeliveryOrigin.
 func ExportSameDeliveryOrigin(origin, dest *url.URL) bool {
 	return sameDeliveryOrigin(origin, dest)

internal/delivery/redirect.go

@@ -3,7 +3,6 @@ package delivery
 import (
 	"errors"
 	"fmt"
-	"net"
 	"net/http"
 	"net/url"
 	"strings"
@@ -20,33 +19,30 @@ const schemeHTTPS = "https"
 
 var errTooManyRedirects = errors.New("too many redirects")
 
-// offOriginHeaderPolicy returns a CheckRedirect that drops every
+// configuredHeaderRedirectPolicy returns a CheckRedirect that
-// origin-scoped header once a redirect leaves the origin the
+// drops a target's configured headers once a redirect leaves the
-// operator configured. names is the set applyRequestHeaders
+// origin the operator configured.
-// reports: the operator's configured headers and the inbound event
-// headers this delivery forwarded, under one rule rather than two.
 //
 // net/http withholds Authorization and Cookie across a host change
 // and forwards everything else. A target header is routinely a
 // credential under another name — X-Api-Key, PRIVATE-TOKEN,
-// X-Auth-Token — and a forwarded inbound header is routinely a
+// X-Auth-Token — so an open redirect at an otherwise trusted
-// sender's signature — X-Hub-Signature — so an open redirect at an
+// destination would hand that credential to a host the operator
-// otherwise trusted destination would hand either to a host the
+// never named. Redirects are still followed: refusing them would
-// operator never named. Redirects are still followed: refusing them
+// break every destination that legitimately redirects and would
-// would break every destination that legitimately redirects and
+// record the 3xx as the delivery's result.
-// would record the 3xx as the delivery's result.
-//
-// The strip is per hop, not permanent: net/http re-copies the
-// initial request's headers for every hop, so a chain that returns
-// to the configured origin carries them again, exactly as net/http
-// treats Authorization.
 //
 // Each hop is dialled through the same SSRF-safe transport, whose
 // guard runs per connection, so a redirect aimed at a private or
 // reserved address is still refused at connect time.
-func offOriginHeaderPolicy(
+func configuredHeaderRedirectPolicy(
-	names []string,
+	headers map[string]string,
 ) func(*http.Request, []*http.Request) error {
+	names := make([]string, 0, len(headers))
+	for name := range headers {
+		names = append(names, http.CanonicalHeaderKey(name))
+	}
+
 	return func(req *http.Request, via []*http.Request) error {
 		if len(via) >= maxDeliveryRedirects {
 			return fmt.Errorf(
@@ -68,7 +64,7 @@ func offOriginHeaderPolicy(
 }
 
 // sameDeliveryOrigin reports whether dest is close enough to the
-// configured target URL to keep carrying its origin-scoped headers.
+// configured target URL to keep carrying its configured headers.
 //
 // This is stricter than the rule net/http applies to Authorization:
 // the port is part of the comparison (a different port is a
@@ -87,12 +83,6 @@ func sameDeliveryOrigin(origin, dest *url.URL) bool {
 // originHostPort renders a URL's host for comparison, lowercased
 // and with the scheme's default port normalised away so that
 // "https://h" and "https://h:443" are one origin.
-//
-// The port is joined with net.JoinHostPort rather than a bare
-// colon: Hostname() unwraps an IPv6 literal's brackets, so
-// "[2001:db8::1]:8080" and "[2001:db8::1:8080]" — a different
-// address on a different port — would otherwise render the same
-// string and pass as one origin.
 func originHostPort(u *url.URL) string {
 	host := strings.ToLower(u.Hostname())
 
@@ -103,5 +93,5 @@ func originHostPort(u *url.URL) string {
 		return host
 	}
 
-	return net.JoinHostPort(host, port)
+	return host + ":" + port
 }

internal/delivery/redirect_test.go

@@ -3,14 +3,11 @@ package delivery_test
 import (
 	"context"
 	"encoding/json"
-	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"sync"
-	"sync/atomic"
 	"testing"
-	"time"
 
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
@@ -19,22 +16,19 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-// The headers these tests drive stand in for the two classes the
+// The header these tests configure stands in for the credential
-// off-origin rule covers: an operator-configured credential and an
+// headers net/http forwards across a host change: it withholds
-// inbound header the delivery path forwards. net/http withholds
+// Authorization and Cookie, and nothing else.
-// Authorization and Cookie across a host change, and nothing else.
 const (
-	probeHeaderName    = "X-Api-Key"
+	probeHeaderName  = "X-Api-Key"
-	probeHeaderValue   = "QQNEVERONTHEWIREQQ"
+	probeHeaderValue = "QQNEVERONTHEWIREQQ"
-	inboundHeaderName  = "X-Hub-Signature"
-	inboundHeaderValue = "sha1=QQINBOUNDQQ"
 )
 
 // redirectProbe records what the last hop of a redirect chain
 // actually received.
 type redirectProbe struct {
 	mu   sync.Mutex
-	seen http.Header
+	seen string
 	hits int
 }
 
@@ -42,26 +36,24 @@ func (p *redirectProbe) serve(
 	w http.ResponseWriter, r *http.Request,
 ) {
 	p.mu.Lock()
-	p.seen = r.Header.Clone()
+	p.seen = r.Header.Get(probeHeaderName)
 	p.hits++
 	p.mu.Unlock()
 
 	w.WriteHeader(http.StatusOK)
 }
 
-func (p *redirectProbe) result() (http.Header, int) {
+func (p *redirectProbe) result() (string, int) {
 	p.mu.Lock()
 	defer p.mu.Unlock()
 
 	return p.seen, p.hits
 }
 
-// deliverWithProbeHeaders runs one real delivery of a new task
+// deliverWithConfiguredHeader runs one real delivery of a new task
-// through the engine to targetURL, carrying both probe headers —
+// through the engine to targetURL, with probeHeaderName set on the
-// probeHeaderName configured on the target, inboundHeaderName
+// target, and returns the delivery status the engine recorded.
-// forwarded from the event — and returns the delivery status the
+func deliverWithConfiguredHeader(
-// engine recorded.
-func deliverWithProbeHeaders(
 	t *testing.T, targetURL string,
 ) database.DeliveryStatus {
 	t.Helper()
@@ -73,13 +65,6 @@ func deliverWithProbeHeaders(
 	)
 	targetID := uuid.New().String()
 
-	inbound, err := json.Marshal(map[string][]string{
-		inboundHeaderName: {inboundHeaderValue},
-	})
-	require.NoError(t, err)
-
-	event.Headers = string(inbound)
-
 	d := iSeedDelivery(
 		t, s.WebhookDB, event.ID, targetID,
 		database.DeliveryStatusPending,
@@ -111,11 +96,9 @@ func deliverWithProbeHeaders(
 }
 
 // A 302 to an origin the operator never configured must not carry
-// the credential they configured for the one they did, nor the
+// the credential they configured for the one they did. The chain is
-// inbound header this delivery forwarded — one rule for both
+// still followed, so the delivery is recorded from the final hop.
-// classes. The chain is still followed, so the delivery is recorded
+func TestDelivery_CrossOriginRedirectDropsConfiguredHeader(
-// from the final hop.
-func TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders(
 	t *testing.T,
 ) {
 	t.Parallel()
@@ -146,21 +129,17 @@ func TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders(
 	))
 	defer origin.Close()
 
-	status := deliverWithProbeHeaders(t, origin.URL)
+	status := deliverWithConfiguredHeader(t, origin.URL)
 
 	seen, hits := probe.result()
 
 	assert.Equal(t, 1, hits,
 		"the redirect must still be followed",
 	)
-	assert.Empty(t, seen.Get(probeHeaderName),
+	assert.Empty(t, seen,
 		"a configured credential header must not reach an "+
 			"origin the operator did not configure",
 	)
-	assert.Empty(t, seen.Get(inboundHeaderName),
-		"a forwarded inbound header must not reach an origin "+
-			"the operator did not configure",
-	)
 	assert.Equal(t,
 		database.DeliveryStatusDelivered, status,
 		"the final hop's 200 is the delivery's result",
@@ -169,9 +148,8 @@ func TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders(
 
 // Stripping must not fire within the configured origin, or every
 // destination that redirects its own path would lose its
-// credential and start answering 401 — and would lose the inbound
+// credential and start answering 401.
-// signature the receiver verifies.
+func TestDelivery_SameOriginRedirectKeepsConfiguredHeader(
-func TestDelivery_SameOriginRedirectKeepsOriginScopedHeaders(
 	t *testing.T,
 ) {
 	t.Parallel()
@@ -193,20 +171,15 @@ func TestDelivery_SameOriginRedirectKeepsOriginScopedHeaders(
 	))
 	defer srv.Close()
 
-	status := deliverWithProbeHeaders(t, srv.URL+"/hook")
+	status := deliverWithConfiguredHeader(t, srv.URL+"/hook")
 
 	seen, hits := probe.result()
 
 	assert.Equal(t, 1, hits)
-	assert.Equal(t, probeHeaderValue, seen.Get(probeHeaderName),
+	assert.Equal(t, probeHeaderValue, seen,
 		"a redirect within the configured origin must keep "+
 			"the configured header",
 	)
-	assert.Equal(t,
-		inboundHeaderValue, seen.Get(inboundHeaderName),
-		"a redirect within the configured origin must keep "+
-			"the forwarded inbound header",
-	)
 	assert.Equal(t,
 		database.DeliveryStatusDelivered, status,
 	)
@@ -238,27 +211,6 @@ func TestSameDeliveryOrigin(t *testing.T) {
 		"a subdomain":          {configured, "https://x.h/i", false},
 		"the parent domain":    {"https://x.h/a", "https://h/j", false},
 		"another port":         {configured, "https://h:8443/k", false},
-
-		// Hostname() unwraps an IPv6 literal's brackets, so a
-		// bracketed host whose last group is the origin's port
-		// renders identically to the origin unless the port is
-		// re-joined with brackets. Each dest below differs from
-		// its origin in address AND in port.
-		"ipv6 port as final group": {
-			"https://[2001:db8::1]:8080/a",
-			"https://[2001:db8::1:8080]/l",
-			false,
-		},
-		"ipv6 loopback port as final group": {
-			"https://[::1]:8080/a",
-			"https://[::1:8080]/m",
-			false,
-		},
-		"ipv6 same origin": {
-			"https://[2001:db8::1]:8080/a",
-			"https://[2001:DB8::1]:8080/n",
-			true,
-		},
 	}
 
 	for name, tc := range cases {
@@ -279,102 +231,3 @@ func TestSameDeliveryOrigin(t *testing.T) {
 		})
 	}
 }
-
-// Installing a CheckRedirect discards net/http's own redirect
-// limit, so the cap this policy restates is the only thing between
-// a self-redirecting destination and an unbounded chain. A
-// destination that always redirects must be cut off after exactly
-// maxDeliveryRedirects requests, with the sentinel surfacing to the
-// caller rather than a generic net/http error.
-func TestRedirectPolicy_StopsAtHopCap(t *testing.T) {
-	t.Parallel()
-
-	var hits atomic.Int64
-
-	srv := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, r *http.Request) {
-			hits.Add(1)
-			http.Redirect(
-				w, r, "/loop", http.StatusFound,
-			)
-		},
-	))
-	defer srv.Close()
-
-	engine := delivery.NewTestEngine(
-		slog.New(slog.DiscardHandler),
-		&http.Client{Timeout: 10 * time.Second},
-		1,
-	)
-
-	client := engine.ExportClientForRequest(
-		&delivery.HTTPTargetConfig{URL: srv.URL},
-		[]string{probeHeaderName},
-	)
-	require.NotNil(t, client.CheckRedirect)
-
-	req, err := http.NewRequestWithContext(
-		context.Background(), http.MethodGet, srv.URL, http.NoBody,
-	)
-	require.NoError(t, err)
-
-	resp, doErr := client.Do(req)
-	if resp != nil {
-		_ = resp.Body.Close()
-	}
-
-	require.Error(t, doErr,
-		"an endless redirect chain must not be followed forever",
-	)
-	require.ErrorIs(t, doErr, delivery.ErrExportTooManyRedirects)
-
-	assert.Equal(t,
-		int64(delivery.ExportMaxDeliveryRedirects), hits.Load(),
-		"the chain must stop after exactly %d hops",
-		delivery.ExportMaxDeliveryRedirects,
-	)
-}
-
-// 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 Content-Type is deliberately excluded: it
-// describes the body, which a 307 carries across hosts.
-func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
-	t.Parallel()
-
-	inbound, err := json.Marshal(map[string][]string{
-		inboundHeaderName: {inboundHeaderValue},
-		"Content-Type":    {testContentType},
-		"Host":            {"inbound.example.com"},
-	})
-	require.NoError(t, err)
-
-	req, err := http.NewRequestWithContext(
-		context.Background(),
-		http.MethodPost,
-		"https://target.example.com/hook",
-		http.NoBody,
-	)
-	require.NoError(t, err)
-
-	names := delivery.ExportApplyRequestHeaders(
-		req,
-		&database.Event{
-			Headers:     string(inbound),
-			ContentType: testContentType,
-		},
-		&delivery.HTTPTargetConfig{
-			Headers: map[string]string{
-				probeHeaderName: probeHeaderValue,
-			},
-		},
-	)
-
-	assert.Equal(t,
-		[]string{probeHeaderName, inboundHeaderName}, names,
-		"both header classes are reported, and only those: "+
-			"Host is never forwarded, Content-Type and "+
-			"User-Agent are the delivery path's own",
-	)
-}

internal/delivery/target_http.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"sort"
 	"sync"
 	"time"
 
@@ -405,9 +404,9 @@ func (t *httpTarget) doHTTPRequest(
 		)
 	}
 
-	originScoped := applyRequestHeaders(req, event, cfg)
+	applyRequestHeaders(req, event, cfg)
 
-	client := t.clientForRequest(cfg, originScoped)
+	client := t.clientForConfig(cfg)
 
 	resp, doErr := executeHTTPRequest(client, req)
 
@@ -433,18 +432,15 @@ func (t *httpTarget) doHTTPRequest(
 	return resp.StatusCode, string(body), dur, nil
 }
 
-// clientForRequest returns the client for one delivery attempt.
+// clientForConfig returns the client for one target's requests.
-// originScoped is the header set applyRequestHeaders built for that
+// A config that overrides neither the timeout nor the headers gets
-// attempt; a request with neither a per-target timeout nor an
+// the shared client: with no configured headers there is nothing
-// origin-scoped header gets the shared client, because there is
+// for the redirect policy to strip, and net/http's default policy
-// then nothing for the redirect policy to strip and net/http's
+// already withholds Authorization and Cookie across hosts.
-// default policy already withholds Authorization and Cookie across
+func (t *httpTarget) clientForConfig(
-// hosts.
-func (t *httpTarget) clientForRequest(
 	cfg *HTTPTargetConfig,
-	originScoped []string,
 ) *http.Client {
-	if cfg.Timeout <= 0 && len(originScoped) == 0 {
+	if cfg.Timeout <= 0 && len(cfg.Headers) == 0 {
 		return t.client
 	}
 
@@ -463,8 +459,10 @@ func (t *httpTarget) clientForRequest(
 		) * time.Second
 	}
 
-	if len(originScoped) > 0 {
+	if len(cfg.Headers) > 0 {
-		client.CheckRedirect = offOriginHeaderPolicy(originScoped)
+		client.CheckRedirect = configuredHeaderRedirectPolicy(
+			cfg.Headers,
+		)
 	}
 
 	return client
@@ -509,82 +507,40 @@ func isForwardableHeader(name string) bool {
 	}
 }
 
-// applyRequestHeaders builds one outbound delivery's header set and
-// returns the canonical names of every header in it that is scoped
-// to the configured origin: the inbound event headers this delivery
-// forwarded, plus the operator's configured headers. The redirect
-// 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.
 func applyRequestHeaders(
 	req *http.Request,
 	event *database.Event,
 	cfg *HTTPTargetConfig,
-) []string {
+) {
 	if event.ContentType != "" {
 		req.Header.Set(
 			"Content-Type", event.ContentType,
 		)
 	}
 
-	originScoped := forwardEventHeaders(req, event)
+	var originalHeaders map[string][]string
+
+	if event.Headers != "" {
+		jsonErr := json.Unmarshal(
+			[]byte(event.Headers),
+			&originalHeaders,
+		)
+		if jsonErr == nil {
+			for k, vals := range originalHeaders {
+				if isForwardableHeader(k) {
+					for _, v := range vals {
+						req.Header.Add(k, v)
+					}
+				}
+			}
+		}
+	}
 
 	for k, v := range cfg.Headers {
 		req.Header.Set(k, v)
-		originScoped[http.CanonicalHeaderKey(k)] = struct{}{}
 	}
 
 	req.Header.Set("User-Agent", "webhooker/1.0")
-
-	// 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")
-
-	names := make([]string, 0, len(originScoped))
-	for name := range originScoped {
-		names = append(names, name)
-	}
-
-	sort.Strings(names)
-
-	return names
-}
-
-// forwardEventHeaders copies the inbound event's forwardable
-// headers onto the outbound request and returns the canonical names
-// it forwarded. Headers the event never carried are absent from the
-// result, so the redirect policy strips what was actually sent.
-func forwardEventHeaders(
-	req *http.Request,
-	event *database.Event,
-) map[string]struct{} {
-	forwarded := make(map[string]struct{})
-
-	if event.Headers == "" {
-		return forwarded
-	}
-
-	var inbound map[string][]string
-
-	if json.Unmarshal([]byte(event.Headers), &inbound) != nil {
-		return forwarded
-	}
-
-	for k, vals := range inbound {
-		if !isForwardableHeader(k) || len(vals) == 0 {
-			continue
-		}
-
-		for _, v := range vals {
-			req.Header.Add(k, v)
-		}
-
-		forwarded[http.CanonicalHeaderKey(k)] = struct{}{}
-	}
-
-	return forwarded
 }
 
 // executeHTTPRequest sends an HTTP request using the provided

internal/delivery/target_redact.go

@@ -1,272 +0,0 @@
-package delivery
-
-import (
-	"net/url"
-	"slices"
-	"strings"
-
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// RedactionMarker stands in for a target credential found in
-// text the target's remote peer chose.
-const RedactionMarker = "(redacted)"
-
-// Redactor removes one target's own credential material from
-// text that target's remote peer chose: a delivery response
-// body, or a delivery error stored before the delivery path
-// learned to mask the URLs it embeds.
-//
-// It removes byte-identical echoes of strings taken from the
-// target's stored configuration, and nothing else. Anything
-// the remote re-encodes survives: JSON "\/" escaping (what
-// PHP's json_encode emits by default), percent-encoding, HTML
-// entities, and an echo of only part of a path. It cannot
-// remove a secret the remote invented.
-//
-// The zero Redactor removes nothing, which is what a caller
-// holding no target for a delivery gets.
-type Redactor struct {
-	secrets []string
-}
-
-// NewRedactor builds the redactor for one target.
-func NewRedactor(t *database.Target) Redactor {
-	// Drop empty strings here rather than at the site that
-	// produced one. strings.ReplaceAll with an empty old string
-	// inserts the marker at every byte boundary, so a single
-	// empty secret destroys every body and error the target
-	// renders; filtering at the collection point means no field
-	// added to targetSecrets later can reintroduce that.
-	// url.Parse("https://@example.com/in") is the known
-	// producer: a non-nil User whose String is "".
-	secrets := slices.DeleteFunc(
-		targetSecrets(t),
-		func(s string) bool { return s == "" },
-	)
-
-	// Longest first, so replacing a secret that is contained
-	// in a longer one cannot leave a fragment of the longer
-	// one behind. Configured headers arrive in map order, so
-	// the sort is also what makes the result deterministic.
-	slices.SortFunc(secrets, func(a, b string) int {
-		if d := len(b) - len(a); d != 0 {
-			return d
-		}
-
-		return strings.Compare(a, b)
-	})
-
-	return Redactor{secrets: secrets}
-}
-
-// Redact replaces every occurrence of the target's credential
-// material in s.
-func (r Redactor) Redact(s string) string {
-	if s == "" {
-		return s
-	}
-
-	for _, secret := range r.secrets {
-		s = strings.ReplaceAll(s, secret, RedactionMarker)
-	}
-
-	return s
-}
-
-// RedactCut redacts s, which its caller has already cut to a
-// byte budget, and additionally drops any tail of s that is a
-// proper prefix of a secret.
-//
-// The cut lands wherever the remote's padding puts it, so the
-// remote chooses where inside the credential it falls. The
-// severed prefix left behind equals no secret, so plain
-// Redact would render it verbatim.
-func (r Redactor) RedactCut(s string) string {
-	s = r.Redact(s)
-
-	if n := r.secretPrefixSuffix(s); n > 0 {
-		return s[:len(s)-n] + RedactionMarker
-	}
-
-	return s
-}
-
-// secretPrefixSuffix returns the length of the longest suffix
-// of s that is a proper prefix of one of the secrets, or 0
-// when there is none.
-func (r Redactor) secretPrefixSuffix(s string) int {
-	longest := 0
-
-	for _, secret := range r.secrets {
-		// Proper prefixes only: a whole secret at the tail was
-		// already replaced by Redact.
-		n := min(len(secret)-1, len(s))
-		for ; n > longest; n-- {
-			if strings.HasSuffix(s, secret[:n]) {
-				longest = n
-
-				break
-			}
-		}
-	}
-
-	return longest
-}
-
-// targetSecrets returns the credential-bearing strings a
-// target's configuration carries.
-//
-// The destination URL contributes. Its path, query and
-// userinfo are the credential for both target types that have
-// one — an incoming-webhook URL is a bearer token, which is
-// why MaskURL elides exactly those parts — and they are the
-// material this service actually sends, so a remote that
-// echoes the request back echoes them.
-//
-// Configured request headers contribute their values, but
-// only for the credential-shaped names isCredentialHeaderName
-// picks out. That is the same class-based rule applied to
-// URLs: an echoed Accept or User-Agent still renders, an
-// echoed Authorization does not.
-func targetSecrets(t *database.Target) []string {
-	if t == nil {
-		return nil
-	}
-
-	switch t.Type {
-	case database.TargetTypeSlack:
-		cfg, err := parseSlackConfig(t.Config)
-		if err != nil {
-			return nil
-		}
-
-		return urlSecrets(cfg.WebhookURL)
-	case database.TargetTypeHTTP:
-		cfg, err := parseHTTPConfig(t.Config)
-		if err != nil {
-			return nil
-		}
-
-		return append(
-			urlSecrets(cfg.URL),
-			headerSecrets(cfg.Headers)...,
-		)
-	case database.TargetTypeDatabase, database.TargetTypeLog:
-		// Neither has a destination URL, so neither has
-		// anything to redact.
-		return nil
-	default:
-		return nil
-	}
-}
-
-// urlSecrets returns the substrings of a destination URL that
-// must not survive into a rendered page: the whole URL, the
-// parts of it MaskURL elides, and any userinfo.
-//
-// No length floor is applied to the path, and none to the
-// userinfo. A short path or a four-byte username is treated as
-// a credential exactly like a long one, because the field takes
-// an arbitrary URL and no part of it can be assumed non-secret —
-// the same rule MaskURL applies. headerSecrets does carry a
-// floor, and the difference is deliberate: a header is picked
-// out by a name-shaped guess and its value may be ordinary
-// text, whereas a URL's path and userinfo are credential
-// material by position.
-func urlSecrets(raw string) []string {
-	raw = strings.TrimSpace(raw)
-	if raw == "" {
-		return nil
-	}
-
-	secrets := []string{raw}
-
-	parsed, err := url.Parse(raw)
-	if err != nil {
-		return secrets
-	}
-
-	if parsed.Path != "" && parsed.Path != "/" {
-		requestURI := parsed.RequestURI()
-		secrets = append(secrets, requestURI)
-
-		if escaped := parsed.EscapedPath(); escaped != requestURI {
-			secrets = append(secrets, escaped)
-		}
-	}
-
-	if parsed.User != nil {
-		secrets = append(secrets, parsed.User.String())
-
-		if pw, ok := parsed.User.Password(); ok && pw != "" {
-			secrets = append(secrets, pw)
-		}
-	}
-
-	return secrets
-}
-
-// minHeaderSecretBytes is the shortest header value treated as
-// a credential. Unlike a URL path, a header value can be a
-// couple of bytes long, and redacting those would scatter the
-// marker through ordinary response text for no gain.
-const minHeaderSecretBytes = 4
-
-// headerSecrets returns the values of the configured headers
-// whose names are credential-shaped.
-func headerSecrets(headers map[string]string) []string {
-	var secrets []string
-
-	for name, value := range headers {
-		value = strings.TrimSpace(value)
-		if len(value) < minHeaderSecretBytes {
-			continue
-		}
-
-		if isCredentialHeaderName(name) {
-			secrets = append(secrets, value)
-		}
-	}
-
-	return secrets
-}
-
-// isCredentialHeaderName classifies a header by its name. The
-// value is never inspected, so the rule is the same
-// class-based one MaskURL applies to a destination URL.
-//
-// The fragments are short on purpose, and match anywhere in
-// the name, so abbreviations an operator might use are covered
-// too: X-Sig, X-Pass, X-HMAC. That over-matches — a header
-// named X-Design contains "sig" — and over-matching is the
-// safe direction here: the cost is a marker where an echoed
-// header value would have rendered.
-func isCredentialHeaderName(name string) bool {
-	name = strings.ToLower(strings.TrimSpace(name))
-
-	// Names that carry a credential by definition.
-	switch name {
-	case "authorization", "proxy-authorization", "cookie":
-		return true
-	}
-
-	// What operators call their own credential headers:
-	// X-Api-Key, X-Hub-Signature, X-Auth-Token.
-	for _, fragment := range []string{
-		"auth",
-		"credential",
-		"hmac",
-		"key",
-		"pass",
-		"secret",
-		"sig",
-		"token",
-	} {
-		if strings.Contains(name, fragment) {
-			return true
-		}
-	}
-
-	return false
-}

internal/delivery/target_redact_test.go

@@ -1,284 +0,0 @@
-package delivery_test
-
-import (
-	"net/url"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// The secret path segments of a Slack incoming webhook URL.
-const (
-	redactSecretPath = "/services/T11111111/B11111111/" +
-		"YYYYYYYYYYYYYYYYYYYYYYYY"
-	redactWebhookURL = "https://hooks.slack.com" +
-		redactSecretPath
-)
-
-func TestRedactor_RemovesSlackWebhookURL(t *testing.T) {
-	t.Parallel()
-
-	r := delivery.NewRedactor(&database.Target{
-		Type:   database.TargetTypeSlack,
-		Config: `{"webhookUrl":"` + redactWebhookURL + `"}`,
-	})
-
-	got := r.Redact("no_service for " + redactWebhookURL)
-
-	assert.NotContains(t, got, redactSecretPath)
-	assert.NotContains(t, got, "T11111111")
-
-	// One marker, not a marker with the host left in front of
-	// it: the whole URL is replaced before the path it
-	// contains, which is what sorting the secrets longest
-	// first buys.
-	assert.Equal(
-		t,
-		"no_service for "+delivery.RedactionMarker,
-		got,
-	)
-}
-
-// TestRedactor_RemovesSecretSeveredByACut covers the input the
-// redactor exists for: text cut to a byte budget with the
-// credential straddling the cut. The remote chooses the
-// padding, so it chooses where the cut lands inside the
-// credential, and the severed prefix that remains equals no
-// secret.
-func TestRedactor_RemovesSecretSeveredByACut(t *testing.T) {
-	t.Parallel()
-
-	r := delivery.NewRedactor(&database.Target{
-		Type:   database.TargetTypeSlack,
-		Config: `{"webhookUrl":"` + redactWebhookURL + `"}`,
-	})
-
-	// Every cut position inside the credential, not just a
-	// convenient one.
-	for n := 1; n < len(redactWebhookURL); n++ {
-		severed := redactWebhookURL[:n]
-		cut := "padding " + severed
-
-		got := r.RedactCut(cut)
-
-		assert.Equal(
-			t,
-			"padding "+delivery.RedactionMarker,
-			got,
-			"cut after %d bytes of the credential", n,
-		)
-	}
-}
-
-// TestRedactor_RedactsCredentialShapedHeaderValues pins the
-// class-based header rule: a header whose name says credential
-// has its value redacted, and a routine header does not, so
-// ordinary response content survives.
-func TestRedactor_RedactsCredentialShapedHeaderValues(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	r := delivery.NewRedactor(&database.Target{
-		Type: database.TargetTypeHTTP,
-		Config: `{"url":"https://example.com/in",` +
-			`"headers":{` +
-			`"Authorization":"Bearer AAAAAAAAAAAA",` +
-			`"Cookie":"session=BBBBBBBBBBBB",` +
-			`"X-Api-Key":"CCCCCCCCCCCC",` +
-			`"X-Hub-Signature":"sha256=DDDDDDDDDDDD",` +
-			`"X-Sig":"EEEEEEEEEEEE",` +
-			`"X-Pass":"FFFFFFFFFFFF",` +
-			`"X-HMAC":"GGGGGGGGGGGG",` +
-			`"X-Credential":"HHHHHHHHHHHH",` +
-			`"Accept":"application/json",` +
-			`"User-Agent":"webhooker/1.0"}}`,
-	})
-
-	for _, secret := range []string{
-		"Bearer AAAAAAAAAAAA",
-		"session=BBBBBBBBBBBB",
-		"CCCCCCCCCCCC",
-		"sha256=DDDDDDDDDDDD",
-		// Abbreviated names an operator might use.
-		"EEEEEEEEEEEE",
-		"FFFFFFFFFFFF",
-		"GGGGGGGGGGGG",
-		"HHHHHHHHHHHH",
-	} {
-		got := r.Redact("echo: " + secret)
-
-		assert.Equal(
-			t,
-			"echo: "+delivery.RedactionMarker,
-			got,
-			secret,
-		)
-	}
-
-	const routine = "Accept: application/json, " +
-		"User-Agent: webhooker/1.0"
-
-	assert.Equal(t, routine, r.Redact(routine))
-}
-
-// TestRedactor_IgnoresVeryShortHeaderValues pins the floor
-// under a header value. Redacting a two-byte value would put
-// the marker through every response that happens to contain
-// those bytes.
-func TestRedactor_IgnoresVeryShortHeaderValues(t *testing.T) {
-	t.Parallel()
-
-	r := delivery.NewRedactor(&database.Target{
-		Type: database.TargetTypeHTTP,
-		Config: `{"url":"https://example.com/in",` +
-			`"headers":{"X-Api-Key":"ab"}}`,
-	})
-
-	const response = "rabbit"
-
-	assert.Equal(t, response, r.Redact(response))
-}
-
-// TestRedactor_RemovesBarePath covers a remote that echoes
-// only the request path rather than the whole URL. The path
-// segments are the credential on their own.
-func TestRedactor_RemovesBarePath(t *testing.T) {
-	t.Parallel()
-
-	r := delivery.NewRedactor(&database.Target{
-		Type:   database.TargetTypeSlack,
-		Config: `{"webhookUrl":"` + redactWebhookURL + `"}`,
-	})
-
-	got := r.Redact("POST " + redactSecretPath + " 404")
-
-	assert.NotContains(t, got, redactSecretPath)
-	assert.Equal(
-		t,
-		"POST "+delivery.RedactionMarker+" 404",
-		got,
-	)
-}
-
-// TestRedactor_RemovesHTTPURLQueryAndUserinfo covers the HTTP
-// target, whose destination is an arbitrary URL: the query
-// string and the userinfo carry credentials as readily as the
-// path does.
-func TestRedactor_RemovesHTTPURLQueryAndUserinfo(t *testing.T) {
-	t.Parallel()
-
-	// Assembled rather than written out, so the literal is
-	// not itself a credential-shaped string.
-	dest := url.URL{
-		Scheme:   "https",
-		User:     url.UserPassword("user", "hunter2"),
-		Host:     "example.com",
-		Path:     "/in",
-		RawQuery: "token=s3cr3t",
-	}
-	raw := dest.String()
-
-	r := delivery.NewRedactor(&database.Target{
-		Type:   database.TargetTypeHTTP,
-		Config: `{"url":"` + raw + `"}`,
-	})
-
-	for _, echoed := range []string{
-		raw,
-		"/in?token=s3cr3t",
-		"hunter2",
-	} {
-		got := r.Redact("rejected: " + echoed)
-
-		assert.NotContains(t, got, "s3cr3t", echoed)
-		assert.NotContains(t, got, "hunter2", echoed)
-		assert.Contains(
-			t, got, delivery.RedactionMarker, echoed,
-		)
-	}
-}
-
-// TestRedactor_LeavesUnrelatedTextAlone pins that the
-// redactor matches literally: it does not guess at what a
-// secret looks like, so ordinary response content survives.
-func TestRedactor_LeavesUnrelatedTextAlone(t *testing.T) {
-	t.Parallel()
-
-	const response = "ok=false error=channel_not_found"
-
-	r := delivery.NewRedactor(&database.Target{
-		Type:   database.TargetTypeSlack,
-		Config: `{"webhookUrl":"` + redactWebhookURL + `"}`,
-	})
-
-	assert.Equal(t, response, r.Redact(response))
-}
-
-// TestRedactor_EmptyUserinfoDoesNotShredTheBody covers a
-// destination URL written with a bare "@" and no userinfo:
-// url.Parse returns a non-nil User whose String is empty. An
-// empty secret in the list would make strings.ReplaceAll
-// insert the marker at every byte boundary, destroying every
-// body and error string the target renders.
-func TestRedactor_EmptyUserinfoDoesNotShredTheBody(t *testing.T) {
-	t.Parallel()
-
-	const dest = "https://@example.com/in"
-
-	// The premise: this URL really does parse to a non-nil
-	// User contributing an empty string.
-	parsed, err := url.Parse(dest)
-	require.NoError(t, err)
-	require.NotNil(t, parsed.User)
-	require.Empty(t, parsed.User.String())
-
-	r := delivery.NewRedactor(&database.Target{
-		Type:   database.TargetTypeHTTP,
-		Config: `{"url":"` + dest + `"}`,
-	})
-
-	const body = "ok=false error=channel_not_found"
-
-	assert.Equal(t, body, r.Redact(body))
-	assert.Equal(t, body, r.RedactCut(body))
-
-	// The real credential material still goes, so filtering the
-	// empty string out did not disarm the redactor.
-	assert.Equal(
-		t,
-		"POST "+delivery.RedactionMarker,
-		r.Redact("POST "+dest),
-	)
-}
-
-// TestRedactor_ZeroValueAndConfiglessTargets pins that a
-// caller with no target, an unparseable config, or a target
-// type with no destination URL gets a redactor that changes
-// nothing rather than one that panics.
-func TestRedactor_ZeroValueAndConfiglessTargets(t *testing.T) {
-	t.Parallel()
-
-	const text = "some response body"
-
-	var zero delivery.Redactor
-
-	assert.Equal(t, text, zero.Redact(text))
-	assert.Equal(t, text, delivery.NewRedactor(nil).Redact(text))
-
-	for _, tgt := range []database.Target{
-		{Type: database.TargetTypeLog},
-		{Type: database.TargetTypeDatabase},
-		{Type: database.TargetTypeSlack, Config: "not json"},
-		{Type: database.TargetTypeHTTP, Config: ""},
-	} {
-		assert.Equal(
-			t, text,
-			delivery.NewRedactor(&tgt).Redact(text),
-			tgt.Type,
-		)
-	}
-}

internal/handlers/delivery_replay_test.go

@@ -42,7 +42,7 @@ func seedFailedDelivery(
 		Method:       http.MethodPost,
 		Headers:      `{"X-Test":["yes"]}`,
 		Body:         `{"replay":"me"}`,
-		ContentType:  contentTypeJSON,
+		ContentType:  "application/json",
 	}
 
 	require.NoError(t, webhookDB.Omit(

internal/handlers/delivery_result_view.go

@@ -1,169 +0,0 @@
-package handlers
-
-import (
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// maxRenderedResponseBytes caps how many bytes of one stored
-// delivery response body reach the event log page.
-//
-// The bound is enforced here and in SQL, because this page's
-// memory profile must not depend on a constant in another
-// package staying where it is, and because rows predating the
-// delivery engine's own cap or restored from an archive are
-// not covered by it at all.
-//
-// It happens to equal that engine cap, so a row written by the
-// current engine reaches this one exactly and is never cut
-// twice. Nothing here may assume the two differ: see view.
-const maxRenderedResponseBytes = 4096
-
-// deliveryResultColumns is the delivery attempt projection.
-// The casts to blob are load-bearing for the same reason they
-// are in eventLogColumns: they make substr and length count
-// bytes rather than characters, and they make SQLite do the
-// cut, so an oversized stored response never becomes a Go
-// string at all.
-const deliveryResultColumns = "delivery_id, attempt_num, success, " +
-	"status_code, error, duration, " +
-	"substr(cast(response_body as blob), 1, ?) AS response_body, " +
-	"length(cast(response_body as blob)) AS response_bytes"
-
-// DeliveryResultView is the display-safe projection of one
-// delivery attempt for the event log page. It carries a
-// capped response body plus the true stored size, so the page
-// can mark a response as truncated without holding the whole
-// thing.
-//
-// Both Error and ResponseBody have been through the target's
-// Redactor. The engine already masks the URL out of the
-// errors it stores, so for errors this is a second line
-// covering rows written before it did; for response bodies it
-// is the only line, and its reach is what
-// delivery.Redactor documents.
-type DeliveryResultView struct {
-	AttemptNum int
-	Success    bool
-
-	// StatusCode is 0 when the attempt never got a response,
-	// which is why the page asks HasStatusCode rather than
-	// printing the number.
-	StatusCode int
-
-	// Error is the stored failure message, redacted.
-	Error string
-
-	// DurationMS is how long the attempt took.
-	DurationMS int64
-
-	// ResponseBody holds at most maxRenderedResponseBytes
-	// bytes of the stored response, redacted. It is remote
-	// content and must only ever be rendered escaped.
-	ResponseBody string
-
-	// ResponseBytes is the size of the stored response body,
-	// before the cut and before redaction. It is what the
-	// remote sent only when ResponseSizeKnown is set.
-	ResponseBytes int64
-
-	// ResponseShownBytes is how much of that the page is
-	// showing. It is the size of the cut, taken before
-	// redaction, so the truncation marker reports what SQLite
-	// returned rather than how much the marker substitution
-	// then changed the length.
-	ResponseShownBytes int
-
-	// ResponseTruncated reports that the body shown may be
-	// incomplete, so the page owes the reader a marker. Every
-	// body that reaches the cap counts, because one the
-	// delivery engine cut at its own equal cap is
-	// indistinguishable from a complete one.
-	ResponseTruncated bool
-
-	// ResponseSizeKnown reports that ResponseBytes is the whole
-	// response the remote sent, so the marker may quote it. It
-	// is false for a body that only reaches the cap, where how
-	// much came after it was never recorded.
-	ResponseSizeKnown bool
-}
-
-// HasStatusCode reports whether the attempt got as far as an
-// HTTP response. A transport failure stores no status code,
-// and rendering that as "0" would read as a real status.
-func (v DeliveryResultView) HasStatusCode() bool {
-	return v.StatusCode != 0
-}
-
-// deliveryResultRow is one row of the delivery attempt
-// projection. Its response body arrives already cut to the
-// cap by SQLite, with the true size beside it.
-type deliveryResultRow struct {
-	DeliveryID    string
-	AttemptNum    int
-	Success       bool
-	StatusCode    int
-	Error         string
-	Duration      int64
-	ResponseBody  []byte
-	ResponseBytes int64
-}
-
-// view projects a loaded row for rendering, stripping the
-// target's own credential out of the two fields a remote peer
-// gets to influence.
-func (r *deliveryResultRow) view(
-	redactor delivery.Redactor,
-) DeliveryResultView {
-	body := r.ResponseBody
-
-	// Two different cuts can have shortened this body, and the
-	// row records only one of them. SQLite cuts here, whenever
-	// the stored value is larger than the cap. The delivery
-	// engine cut earlier, whenever the remote sent more than
-	// its own maxBodyLog — which is this same number, so such a
-	// row stores the cut length as its whole length and nothing
-	// in it separates a response that ended at the cap from one
-	// severed there.
-	//
-	// So a body that reaches the cap is treated as cut either
-	// way. Gating on ResponseBytes alone would assume the two
-	// caps differ, and they do not: under the current engine
-	// that gate never opens.
-	cut := r.ResponseBytes > int64(len(body)) ||
-		len(body) >= maxRenderedResponseBytes
-
-	// The row holds more than the page shows only in the first
-	// of those cases. In the second the stored row is all there
-	// is, and its size is a floor rather than the true one.
-	sizeKnown := r.ResponseBytes > int64(len(body))
-
-	// Only a cut response can have been left mid-sequence,
-	// exactly as with an event body.
-	if cut {
-		body = trimPartialRune(body)
-	}
-
-	// A cut body goes through RedactCut: the remote controls
-	// the padding ahead of a credential it echoes, so it
-	// controls where the cut falls inside that credential, and
-	// the severed prefix left behind matches no secret whole.
-	rendered := string(body)
-	if cut {
-		rendered = redactor.RedactCut(rendered)
-	} else {
-		rendered = redactor.Redact(rendered)
-	}
-
-	return DeliveryResultView{
-		AttemptNum:         r.AttemptNum,
-		Success:            r.Success,
-		StatusCode:         r.StatusCode,
-		Error:              redactor.Redact(r.Error),
-		DurationMS:         r.Duration,
-		ResponseBody:       rendered,
-		ResponseBytes:      r.ResponseBytes,
-		ResponseShownBytes: len(body),
-		ResponseTruncated:  cut,
-		ResponseSizeKnown:  sizeKnown,
-	}
-}

internal/handlers/delivery_result_view_test.go

@@ -1,515 +0,0 @@
-package handlers_test
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"strconv"
-	"strings"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"gorm.io/gorm/clause"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-	"sneak.berlin/go/webhooker/internal/handlers"
-	"sneak.berlin/go/webhooker/internal/session"
-)
-
-// responseCap is the number of response bytes the event log
-// page is allowed to render for one delivery attempt.
-const responseCap = handlers.MaxRenderedResponseBytesForTest
-
-// failedAttempt describes the failed delivery every test in
-// this file seeds. The values are distinctive so that finding
-// them in the rendered page cannot be a coincidence.
-const (
-	attemptStatusCode = 502
-	attemptDurationMS = 1234
-	attemptNumber     = 3
-	attemptError      = "upstream returned 502 Bad Gateway"
-)
-
-// seedFailedDeliveryWithResponse records an event, a failed
-// delivery against targetID, and one delivery result carrying
-// the given response body. It returns the delivery.
-//
-// Distinct from seedFailedDelivery in delivery_replay_test.go,
-// which seeds an attempt with no response body and returns the
-// event as well; these tests need the recorded response.
-func seedFailedDeliveryWithResponse(
-	t *testing.T,
-	dbMgr *database.WebhookDBManager,
-	webhookID, targetID, responseBody string,
-) *database.Delivery {
-	t.Helper()
-
-	webhookDB, err := dbMgr.GetDB(webhookID)
-	require.NoError(t, err)
-
-	event := &database.Event{
-		WebhookID:   webhookID,
-		Method:      http.MethodPost,
-		Body:        `{"test":true}`,
-		ContentType: contentTypeJSON,
-	}
-
-	require.NoError(t, webhookDB.Omit(
-		clause.Associations,
-	).Create(event).Error)
-
-	dlv := &database.Delivery{
-		EventID:  event.ID,
-		TargetID: targetID,
-		Status:   database.DeliveryStatusFailed,
-	}
-
-	require.NoError(t, webhookDB.Omit(
-		clause.Associations,
-	).Create(dlv).Error)
-
-	result := &database.DeliveryResult{
-		DeliveryID:   dlv.ID,
-		AttemptNum:   attemptNumber,
-		Success:      false,
-		StatusCode:   attemptStatusCode,
-		ResponseBody: responseBody,
-		Error:        attemptError,
-		Duration:     attemptDurationMS,
-	}
-
-	require.NoError(t, webhookDB.Omit(
-		clause.Associations,
-	).Create(result).Error)
-
-	return dlv
-}
-
-// seedFailureAndRender seeds a failed delivery against a
-// target of the given type and config, and returns the
-// rendered event log page.
-func seedFailureAndRender(
-	t *testing.T,
-	targetType database.TargetType,
-	config, responseBody string,
-) string {
-	t.Helper()
-
-	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)
-	tgt := seedConfiguredTarget(
-		t, db, wh.ID, targetType, config,
-	)
-
-	seedFailedDeliveryWithResponse(
-		t, dbMgr, wh.ID, tgt.ID, responseBody,
-	)
-
-	return renderSourceLogsPage(t, h, sess, wh.ID)
-}
-
-// TestHandleSourceLogs_RendersFailedAttempt is the regression
-// test for the reported gap: a failed delivery used to render
-// as the status word alone, so diagnosing it meant opening the
-// per-webhook SQLite file by hand.
-func TestHandleSourceLogs_RendersFailedAttempt(t *testing.T) {
-	t.Parallel()
-
-	body := seedFailureAndRender(
-		t,
-		database.TargetTypeHTTP,
-		`{"url":"https://example.com/hook/abc"}`,
-		"upstream exploded",
-	)
-
-	assert.Contains(
-		t, body, strconv.Itoa(attemptStatusCode),
-		"the attempt's status code must reach the page",
-	)
-	assert.Contains(
-		t, body, attemptError,
-		"the attempt's error must reach the page",
-	)
-	assert.Contains(
-		t, body, strconv.Itoa(attemptDurationMS),
-		"the attempt's duration must reach the page",
-	)
-	assert.Contains(
-		t, body, "Attempt "+strconv.Itoa(attemptNumber),
-		"the attempt number must reach the page",
-	)
-	assert.Contains(
-		t, body, "upstream exploded",
-		"the attempt's response body must reach the page",
-	)
-}
-
-// TestHandleSourceLogs_EscapesResponseBody proves the
-// response body is treated as the untrusted remote content it
-// is. The remote chooses these bytes and the page is rendered
-// inside the operator's authenticated origin, where the
-// application's own CSP allows inline script from 'self'.
-func TestHandleSourceLogs_EscapesResponseBody(t *testing.T) {
-	t.Parallel()
-
-	const payload = `<script>alert("xss")</script>`
-
-	body := seedFailureAndRender(
-		t,
-		database.TargetTypeHTTP,
-		`{"url":"https://example.com/hook/abc"}`,
-		payload,
-	)
-
-	assert.NotContains(t, body, payload)
-	assert.NotContains(t, body, "<script>alert")
-	assert.Contains(t, body, "alert")
-}
-
-// TestHandleSourceLogs_RedactsCredentialEchoedInResponse
-// covers the case that makes rendering a response body a
-// disclosure question at all: the remote echoes back the
-// credential the request carried, and the page would then put
-// it on the operator's screen.
-func TestHandleSourceLogs_RedactsCredentialEchoedInResponse(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	body := seedFailureAndRender(
-		t,
-		database.TargetTypeSlack,
-		`{"webhookUrl":"`+slackWebhookURL+`"}`,
-		"no_service: "+slackWebhookURL,
-	)
-
-	assert.NotContains(t, body, slackSecretPath)
-	assert.NotContains(t, body, "T00000000")
-	assert.NotContains(t, body, "B00000000")
-	assert.Contains(t, body, delivery.RedactionMarker)
-
-	// The rest of the response is still shown, or the
-	// redaction would have cost the operator the diagnosis.
-	assert.Contains(t, body, "no_service")
-}
-
-// TestHandleSourceLogs_RedactsCredentialEchoedInError covers
-// the same disclosure through the error field. The delivery
-// engine masks the URL out of the errors it stores, so this
-// holds the read path to the rows written before it did.
-func TestHandleSourceLogs_RedactsCredentialEchoedInError(
-	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)
-	tgt := seedConfiguredTarget(
-		t, db, wh.ID,
-		database.TargetTypeSlack,
-		`{"webhookUrl":"`+slackWebhookURL+`"}`,
-	)
-
-	dlv := seedFailedDeliveryWithResponse(t, dbMgr, wh.ID, tgt.ID, "")
-
-	webhookDB, err := dbMgr.GetDB(wh.ID)
-	require.NoError(t, err)
-
-	// An unmasked transport error, exactly as Go's HTTP
-	// client renders one.
-	require.NoError(t, webhookDB.Model(
-		&database.DeliveryResult{},
-	).Where(
-		"delivery_id = ?", dlv.ID,
-	).Update(
-		"error",
-		`Post "`+slackWebhookURL+`": dial tcp: i/o timeout`,
-	).Error)
-
-	body := renderSourceLogsPage(t, h, sess, wh.ID)
-
-	assert.NotContains(t, body, slackSecretPath)
-	assert.Contains(t, body, delivery.RedactionMarker)
-	assert.Contains(t, body, "i/o timeout")
-}
-
-// severedPadding is the filler that puts the end of an echoed
-// webhook URL five bytes past a cut at the response cap, so
-// the cut leaves the workspace ID, the bot ID and all but the
-// last few token characters behind.
-func severedPadding() string {
-	const severedTail = 5
-
-	return strings.Repeat(
-		"A", responseCap-len(slackWebhookURL)+severedTail,
-	)
-}
-
-// TestHandleSourceLogs_RedactsCredentialSeveredByTheEngineCut
-// is the regression test for a redactor gated on the SQL cut
-// alone. The delivery engine stops reading a response at its
-// own cap, which is the same number of bytes this page
-// renders, so a row the engine cut is byte-for-byte
-// indistinguishable from a complete response and that gate
-// never opened on anything the engine writes.
-//
-// The seeded body is what the engine stores for any remote
-// that sends at least that much: exactly responseCap bytes,
-// ending in a severed webhook URL.
-// TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog in
-// internal/delivery pins that this is the size it produces.
-func TestHandleSourceLogs_RedactsCredentialSeveredByTheEngineCut(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	sent := severedPadding() + slackWebhookURL +
-		strings.Repeat("Z", 128)
-	stored := sent[:responseCap]
-
-	require.Len(
-		t, stored, responseCap,
-		"the engine stores exactly the cap, never more",
-	)
-	require.Contains(
-		t, stored, "T00000000",
-		"the severed credential must be in what is seeded",
-	)
-
-	body := seedFailureAndRender(
-		t,
-		database.TargetTypeSlack,
-		`{"webhookUrl":"`+slackWebhookURL+`"}`,
-		stored,
-	)
-
-	assert.NotContains(t, body, "T00000000")
-	assert.NotContains(t, body, "B00000000")
-	assert.Contains(t, body, delivery.RedactionMarker)
-	assert.Contains(
-		t, body, "reached the recording limit",
-		"a body the engine cut must not be shown as complete",
-	)
-}
-
-// TestHandleSourceLogs_RedactsCredentialSeveredBySQLCut covers
-// the same severing for a row larger than the cap, which is
-// SQLite's cut rather than the engine's. The current engine
-// writes no such row; rows predating its cap or restored from
-// an archive are not bounded by it, which is why the page cuts
-// again in SQL and has to redact that cut too.
-func TestHandleSourceLogs_RedactsCredentialSeveredBySQLCut(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	stored := severedPadding() + slackWebhookURL +
-		strings.Repeat("Z", 128)
-
-	require.Greater(
-		t, len(stored), responseCap,
-		"the stored body must exceed the cap or nothing is cut",
-	)
-
-	body := seedFailureAndRender(
-		t,
-		database.TargetTypeSlack,
-		`{"webhookUrl":"`+slackWebhookURL+`"}`,
-		stored,
-	)
-
-	assert.NotContains(t, body, "T00000000")
-	assert.NotContains(t, body, "B00000000")
-	assert.NotContains(
-		t, body, slackWebhookURL[:len(slackWebhookURL)-10],
-	)
-	assert.Contains(t, body, delivery.RedactionMarker)
-}
-
-// TestHandleSourceLogs_RedactsForSoftDeletedTarget covers a
-// target an operator has deleted. The row is only soft deleted
-// and its deliveries survive in the per-webhook database, so
-// its redactor has to survive with it or every response body
-// it ever recorded renders unredacted.
-func TestHandleSourceLogs_RedactsForSoftDeletedTarget(
-	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)
-	tgt := seedConfiguredTarget(
-		t, db, wh.ID,
-		database.TargetTypeSlack,
-		`{"webhookUrl":"`+slackWebhookURL+`"}`,
-	)
-
-	seedFailedDeliveryWithResponse(
-		t, dbMgr, wh.ID, tgt.ID,
-		"no_service: "+slackWebhookURL,
-	)
-
-	require.NoError(t, db.DB().Delete(tgt).Error)
-
-	body := renderSourceLogsPage(t, h, sess, wh.ID)
-
-	assert.NotContains(t, body, slackSecretPath)
-	assert.NotContains(t, body, "T00000000")
-	assert.Contains(t, body, delivery.RedactionMarker)
-	assert.Contains(t, body, "no_service")
-}
-
-// TestHandleSourceLogs_BoundsRenderedAttempts pins the ceiling
-// on how many of one delivery's attempts reach the page, and
-// that what it drops is counted rather than hidden.
-func TestHandleSourceLogs_BoundsRenderedAttempts(t *testing.T) {
-	t.Parallel()
-
-	const extraAttempts = 7
-
-	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)
-	tgt := seedConfiguredTarget(
-		t, db, wh.ID, database.TargetTypeLog, "",
-	)
-
-	dlv := seedFailedDeliveryWithResponse(t, dbMgr, wh.ID, tgt.ID, "")
-
-	webhookDB, err := dbMgr.GetDB(wh.ID)
-	require.NoError(t, err)
-
-	total := handlers.MaxRenderedAttemptsForTest + extraAttempts
-
-	// seedFailedDeliveryWithResponse already recorded one attempt.
-	for i := range total - 1 {
-		require.NoError(t, webhookDB.Omit(
-			clause.Associations,
-		).Create(&database.DeliveryResult{
-			DeliveryID: dlv.ID,
-			AttemptNum: attemptNumber + 1 + i,
-			Error:      attemptError,
-		}).Error)
-	}
-
-	views := h.LoadEventLogViewsForTest(
-		httptest.NewRecorder(), *wh, 1,
-	)
-	require.Len(t, views, 1)
-	require.Len(t, views[0].Deliveries, 1)
-
-	dv := views[0].Deliveries[0]
-
-	assert.Equal(t, total, dv.AttemptCount)
-	assert.Len(
-		t, dv.Results, handlers.MaxRenderedAttemptsForTest,
-	)
-	assert.Equal(t, extraAttempts, dv.AttemptsOmitted)
-
-	page := renderSourceLogsPage(t, h, sess, wh.ID)
-
-	assert.Contains(t, page, "attempts omitted")
-	assert.Contains(
-		t, page, strconv.Itoa(total)+" attempts",
-		"the header must count every recorded attempt",
-	)
-}
-
-// TestHandleSourceLogs_BoundsOversizeResponse proves the
-// rendered page is bounded by the response cap rather than by
-// the stored response size. The cut happens in SQLite, so the
-// oversized value never becomes a Go string; this asserts the
-// observable consequence, that neither the page nor the
-// projection carries the tail.
-func TestHandleSourceLogs_BoundsOversizeResponse(t *testing.T) {
-	t.Parallel()
-
-	const tail = "QQRESPONSETAILQQ"
-
-	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)
-	tgt := seedConfiguredTarget(
-		t, db, wh.ID, database.TargetTypeLog, "",
-	)
-
-	stored := strings.Repeat("A", responseCap*4) + tail
-	seedFailedDeliveryWithResponse(t, dbMgr, wh.ID, tgt.ID, stored)
-
-	views := h.LoadEventLogViewsForTest(
-		httptest.NewRecorder(), *wh, 1,
-	)
-	require.Len(t, views, 1)
-	require.Len(t, views[0].Deliveries, 1)
-	require.Len(t, views[0].Deliveries[0].Results, 1)
-
-	attempt := views[0].Deliveries[0].Results[0]
-
-	assert.LessOrEqual(
-		t, len(attempt.ResponseBody), responseCap,
-	)
-	assert.Equal(
-		t, int64(len(stored)), attempt.ResponseBytes,
-	)
-	assert.True(t, attempt.ResponseTruncated)
-
-	page := renderSourceLogsPage(t, h, sess, wh.ID)
-
-	assert.NotContains(t, page, tail)
-	assert.Contains(
-		t, page, "Response truncated for display",
-	)
-}

internal/handlers/export_test.go

@@ -19,14 +19,6 @@ func (s *Handlers) SetLogForTest(log *slog.Logger) {
 // to the handlers_test package.
 const MaxRenderedBodyBytesForTest = maxRenderedBodyBytes
 
-// MaxRenderedResponseBytesForTest exposes the event log's
-// delivery response cap to the handlers_test package.
-const MaxRenderedResponseBytesForTest = maxRenderedResponseBytes
-
-// MaxRenderedAttemptsForTest exposes the event log's
-// per-delivery attempt ceiling to the handlers_test package.
-const MaxRenderedAttemptsForTest = maxRenderedAttempts
-
 // DummyVerificationsForTest reports how many equivalent-cost
 // verifications were charged for usernames that do not exist. It
 // lets a test prove the anti-enumeration path ran without timing
@@ -51,9 +43,7 @@ func (s *Handlers) LoadEventLogViewsForTest(
 	webhook database.Webhook,
 	page int,
 ) []EventLogView {
-	views, _, _ := s.loadEventsWithDeliveries(
+	views, _ := s.loadEventsWithDeliveries(w, webhook, nil, page)
-		w, webhook, nil, page,
-	)
 
 	return views
 }

internal/handlers/source_logs_test.go

@@ -15,11 +15,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/session"
 )
 
-// contentTypeJSON is the content type the seeded events in this
-// package carry. Shared across the seed helpers so the literal
-// appears once.
-const contentTypeJSON = "application/json"
-
 // seedDeliveredEvent records an event and a delivery for it in
 // the webhook's own database, so the log page has a delivery
 // to render against the target.
@@ -37,7 +32,7 @@ func seedDeliveredEvent(
 		WebhookID:   webhookID,
 		Method:      http.MethodPost,
 		Body:        `{"test":true}`,
-		ContentType: contentTypeJSON,
+		ContentType: "application/json",
 	}
 
 	require.NoError(t, webhookDB.Omit(

internal/handlers/source_management.go

@@ -4,13 +4,11 @@ import (
 	"encoding/json"
 	"errors"
 	"net/http"
-	"slices"
 	"strconv"
 	"strings"
 
 	"github.com/go-chi/chi"
 	"github.com/google/uuid"
-	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
 	"sneak.berlin/go/webhooker/internal/signature"
@@ -103,32 +101,6 @@ type DeliveryView struct {
 	ID     string
 	Status database.DeliveryStatus
 	Target delivery.TargetView
-
-	// Results is this delivery's attempts in attempt order,
-	// bounded by maxRenderedAttempts. Without them a failure
-	// renders as the status word alone and says nothing about
-	// why.
-	Results []DeliveryResultView
-
-	// AttemptCount is how many attempts were recorded, which
-	// is more than len(Results) once the middle was dropped.
-	AttemptCount int
-
-	// AttemptsOmitted is how many attempts were dropped from
-	// the middle of Results. The page must show it, or the
-	// bound would hide history rather than fold it.
-	AttemptsOmitted int
-}
-
-// eventLogTarget is what the event log needs to know about
-// one target: the display-safe view its template renders, and
-// the redactor that keeps that target's own credential out of
-// the text its remote peer chose. The two are kept together
-// so a caller cannot pick up one without the other, and apart
-// from TargetView so the secrets never reach a template.
-type eventLogTarget struct {
-	View     delivery.TargetView
-	Redactor delivery.Redactor
 }
 
 // HandleSourceList shows a list of user's webhooks.
@@ -797,24 +769,12 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc {
 			return
 		}
 
-		targets, err := h.loadTargetMap(webhook.ID)
+		targets := h.loadTargetMap(webhook.ID)
-		if err != nil {
-			// 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)
-
-			return
-		}
-
 		page := h.parsePage(r)
 
-		evts, total, ok := h.loadEventsWithDeliveries(
+		evts, total := h.loadEventsWithDeliveries(
 			w, webhook, targets, page,
 		)
-		if !ok {
-			return
-		}
 
 		totalPages := int(total) / paginationPerPage
 		if int(total)%paginationPerPage != 0 {
@@ -847,54 +807,29 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc {
 }
 
 // loadTargetMap loads targets into a map of display-safe
-// views keyed by target ID, each paired with its redactor.
+// views keyed by target ID. The projection happens here so
-// The projection happens here so that no caller can hand a
+// that no caller can hand a raw target, configuration blob
-// raw target, configuration blob and all, to a template: the
+// and all, to a template.
-// raw rows do not leave this function.
-//
-// The load is Unscoped because deleting a target only soft
-// deletes the row while its deliveries survive in the
-// per-webhook database: a scoped load leaves those deliveries
-// with a zero redactor, which renders their response bodies
-// unredacted. Only the redactor half of the map is built from
-// deleted rows. The view half, which is what the page lists,
-// stays scoped.
 func (h *Handlers) loadTargetMap(
 	webhookID string,
-) (map[string]eventLogTarget, error) {
+) map[string]delivery.TargetView {
 	var targets []database.Target
 
-	err := h.db.DB().Unscoped().Where(
+	h.db.DB().Where(
 		"webhook_id = ?", webhookID,
-	).Find(&targets).Error
+	).Find(&targets)
-	if err != nil {
+
-		return nil, err
+	views := delivery.NewTargetViews(targets)
-	}
 
 	targetMap := make(
-		map[string]eventLogTarget, len(targets),
+		map[string]delivery.TargetView, len(views),
 	)
-	live := make([]database.Target, 0, len(targets))
 
-	for i := range targets {
+	for _, v := range views {
-		targetMap[targets[i].ID] = eventLogTarget{
+		targetMap[v.ID] = v
-			Redactor: delivery.NewRedactor(&targets[i]),
-		}
-
-		if !targets[i].DeletedAt.Valid {
-			live = append(live, targets[i])
-		}
 	}
 
-	// The views come from NewTargetViews rather than being
+	return targetMap
-	// rebuilt here, so the masking rules stay in one place.
-	for _, v := range delivery.NewTargetViews(live) {
-		entry := targetMap[v.ID]
-		entry.View = v
-		targetMap[v.ID] = entry
-	}
-
-	return targetMap, nil
 }
 
 // parsePage extracts a page number from the query string.
@@ -915,22 +850,18 @@ func (h *Handlers) parsePage(r *http.Request) int {
 // deliveries from the per-webhook database. Events come back
 // as capped projections rather than database.Event rows: see
 // eventLogColumns for why the cut happens in SQL.
-//
-// The bool reports whether the load succeeded. It is false
-// once this has answered the request with an error, and the
-// caller must then render nothing further.
 func (h *Handlers) loadEventsWithDeliveries(
 	w http.ResponseWriter,
 	webhook database.Webhook,
-	targetMap map[string]eventLogTarget,
+	targetMap map[string]delivery.TargetView,
 	page int,
-) ([]EventLogView, int64, bool) {
+) ([]EventLogView, int64) {
 	var totalEvents int64
 
 	var result []EventLogView
 
 	if !h.dbMgr.DBExists(webhook.ID) {
-		return result, totalEvents, true
+		return result, totalEvents
 	}
 
 	webhookDB, err := h.dbMgr.GetDB(webhook.ID)
@@ -939,7 +870,7 @@ func (h *Handlers) loadEventsWithDeliveries(
 			w, "failed to get webhook database", err,
 		)
 
-		return nil, 0, false
+		return nil, 0
 	}
 
 	webhookDB.Model(&database.Event{}).Where(
@@ -959,170 +890,43 @@ func (h *Handlers) loadEventsWithDeliveries(
 	).Find(&rows)
 
 	result = make([]EventLogView, len(rows))
-	eventDeliveries := make([][]database.Delivery, len(rows))
-
-	var deliveryIDs []string
 
 	for i := range rows {
 		result[i] = rows[i].view()
 
+		var deliveries []database.Delivery
+
 		webhookDB.Where(
 			"event_id = ?", rows[i].ID,
-		).Find(&eventDeliveries[i])
+		).Find(&deliveries)
 
-		for j := range eventDeliveries[i] {
-			deliveryIDs = append(
-				deliveryIDs, eventDeliveries[i][j].ID,
-			)
-		}
-	}
-
-	attempts, err := h.loadDeliveryResults(
-		webhookDB, deliveryIDs,
-	)
-	if err != nil {
-		h.serverError(
-			w, "failed to load delivery attempts", err,
-		)
-
-		return nil, 0, false
-	}
-
-	for i := range rows {
 		result[i].Deliveries = newDeliveryViews(
-			eventDeliveries[i], targetMap, attempts,
+			deliveries, targetMap,
 		)
 	}
 
-	return result, totalEvents, true
+	return result, totalEvents
-}
-
-// deliveryIDChunkSize bounds how many delivery IDs go into one
-// IN clause. SQLite refuses a statement carrying more than
-// SQLITE_MAX_VARIABLE_NUMBER (32766) bound parameters, and a
-// page holds one delivery per target per event, so a webhook
-// with enough targets would turn the whole query into an error
-// and the page into zero attempts.
-const deliveryIDChunkSize = 500
-
-// loadDeliveryResults loads the recorded attempts for the
-// page's deliveries, keyed by delivery ID.
-//
-// Each response body is cut by SQLite rather than in Go, for
-// the reason deliveryResultColumns gives. How many attempts a
-// delivery has is the target's MaxRetries, which the
-// authenticated operator sets; how many of them reach the page
-// is bounded again by maxRenderedAttempts.
-func (h *Handlers) loadDeliveryResults(
-	webhookDB *gorm.DB,
-	deliveryIDs []string,
-) (map[string][]deliveryResultRow, error) {
-	byDelivery := make(map[string][]deliveryResultRow)
-
-	for chunk := range slices.Chunk(
-		deliveryIDs, deliveryIDChunkSize,
-	) {
-		var rows []deliveryResultRow
-
-		err := webhookDB.Model(
-			&database.DeliveryResult{},
-		).Select(
-			deliveryResultColumns, maxRenderedResponseBytes,
-		).Where(
-			"delivery_id IN ?", chunk,
-		).Order("attempt_num ASC").Find(&rows).Error
-		if err != nil {
-			// Returning what was loaded so far renders the
-			// deliveries in the failed chunk as never having run,
-			// which is indistinguishable from ones that really
-			// never ran. The page fails instead.
-			return nil, err
-		}
-
-		for i := range rows {
-			byDelivery[rows[i].DeliveryID] = append(
-				byDelivery[rows[i].DeliveryID], rows[i],
-			)
-		}
-	}
-
-	return byDelivery, nil
 }
 
 // newDeliveryViews projects deliveries for rendering,
-// resolving each one's target to its display-safe view and
+// resolving each one's target to its display-safe view.
-// each one's attempts through that target's redactor.
 func newDeliveryViews(
 	deliveries []database.Delivery,
-	targetMap map[string]eventLogTarget,
+	targetMap map[string]delivery.TargetView,
-	attempts map[string][]deliveryResultRow,
 ) []DeliveryView {
 	views := make([]DeliveryView, len(deliveries))
 
 	for i := range deliveries {
-		target := targetMap[deliveries[i].TargetID]
-		rows := attempts[deliveries[i].ID]
-
-		results, omitted := renderedAttempts(
-			rows, target.Redactor,
-		)
-
 		views[i] = DeliveryView{
-			ID:              deliveries[i].ID,
+			ID:     deliveries[i].ID,
-			Status:          deliveries[i].Status,
+			Status: deliveries[i].Status,
-			Target:          target.View,
+			Target: targetMap[deliveries[i].TargetID],
-			Results:         results,
-			AttemptCount:    len(rows),
-			AttemptsOmitted: omitted,
 		}
 	}
 
 	return views
 }
 
-// maxRenderedAttempts bounds how many of one delivery's
-// attempts the page renders. Past it the middle is dropped and
-// counted, keeping the first attempts and the last ones: how
-// the delivery started failing and how it ended are what a
-// reader needs, and the count says plainly that the rest was
-// dropped rather than never recorded.
-const (
-	renderedAttemptsHead = 10
-	renderedAttemptsTail = 10
-	maxRenderedAttempts  = renderedAttemptsHead +
-		renderedAttemptsTail
-)
-
-// renderedAttempts projects a delivery's attempts through the
-// target's redactor, at most maxRenderedAttempts of them, and
-// reports how many it dropped.
-func renderedAttempts(
-	rows []deliveryResultRow,
-	redactor delivery.Redactor,
-) ([]DeliveryResultView, int) {
-	omitted := 0
-
-	if len(rows) > maxRenderedAttempts {
-		omitted = len(rows) - maxRenderedAttempts
-
-		kept := make(
-			[]deliveryResultRow, 0, maxRenderedAttempts,
-		)
-		kept = append(kept, rows[:renderedAttemptsHead]...)
-		kept = append(
-			kept, rows[len(rows)-renderedAttemptsTail:]...,
-		)
-		rows = kept
-	}
-
-	views := make([]DeliveryResultView, len(rows))
-	for i := range rows {
-		views[i] = rows[i].view(redactor)
-	}
-
-	return views, omitted
-}
-
 // HandleEntrypointCreate handles adding a new entrypoint.
 func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {

templates/source_logs.html

@@ -46,64 +46,19 @@
                     {{end}}
 
                     {{if .Deliveries}}
-                    <div class="mt-4 border-t border-gray-200 pt-3">
+                    <div class="mt-3 pt-4 border-t border-gray-200">
-                        <h3 class="text-xs font-medium uppercase tracking-wide text-gray-500">Deliveries</h3>
+                        {{range .Deliveries}}
-                        <div class="mt-2 divide-y divide-gray-200">
+                        <div class="flex items-center justify-between py-2 text-xs text-gray-700">
-                            {{range .Deliveries}}
+                            <span><span class="font-medium">{{.Target.Name}}</span>: {{.Status}}</span>
-                            <div class="py-2" x-data="{ attempts: false }">
+                            {{if .Status.Terminal}}
-                                <div class="flex items-center justify-between cursor-pointer" @click="attempts = !attempts">
+                            <form method="POST" action="/source/{{$.Webhook.ID}}/deliveries/{{.ID}}/replay" class="inline">
-                                    <div class="flex items-center gap-3">
+                                <input type="hidden" name="csrf_token" value="{{$.CSRFToken}}">
-                                        <span class="text-sm text-gray-700">{{.Target.Name}}</span>
+                                <input type="hidden" name="page" value="{{$.Page}}">
-                                        <span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">{{.Status}}</span>
+                                <button type="submit" class="text-xs text-primary-600 hover:text-primary-700" title="Send this event to the target again">Replay</button>
-                                    </div>
+                            </form>
-                                    <div class="flex items-center gap-3">
-                                        {{if .Status.Terminal}}
-                                        <form method="POST" action="/source/{{$.Webhook.ID}}/deliveries/{{.ID}}/replay" class="inline" @click.stop>
-                                            <input type="hidden" name="csrf_token" value="{{$.CSRFToken}}">
-                                            <input type="hidden" name="page" value="{{$.Page}}">
-                                            <button type="submit" class="text-xs text-primary-600 hover:text-primary-700" title="Send this event to the target again">Replay</button>
-                                        </form>
-                                        {{end}}
-                                        <span class="text-xs text-gray-400">{{.AttemptCount}} attempt{{if ne .AttemptCount 1}}s{{end}}</span>
-                                        <svg class="w-3 h-3 text-gray-400 transition-transform" :class="{ 'rotate-180': attempts }" fill="none" stroke="currentColor" viewBox="0 0 24 24">
-                                            <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M19 9l-7 7-7-7"/>
-                                        </svg>
-                                    </div>
-                                </div>
-
-                                <div x-show="attempts" x-cloak class="mt-2 space-y-2">
-                                    {{if .AttemptsOmitted}}
-                                    <p class="text-xs text-gray-500">{{.AttemptsOmitted}} attempt{{if ne .AttemptsOmitted 1}}s{{end}} omitted between the first and last shown.</p>
-                                    {{end}}
-                                    {{range .Results}}
-                                    <div class="rounded-md bg-white border border-gray-200 p-2">
-                                        <div class="flex flex-wrap items-center gap-3 text-xs">
-                                            <span class="text-gray-500">Attempt {{.AttemptNum}}</span>
-                                            <span class="{{if .Success}}text-green-600{{else}}text-red-600{{end}}">{{if .Success}}success{{else}}failure{{end}}</span>
-                                            <span class="text-gray-500">Status: {{if .HasStatusCode}}{{.StatusCode}}{{else}}&mdash; (no response){{end}}</span>
-                                            <span class="text-gray-500">Duration: {{.DurationMS}} ms</span>
-                                        </div>
-                                        {{if .Error}}
-                                        <p class="mt-2 text-xs text-red-700 break-all">Error: {{.Error}}</p>
-                                        {{end}}
-                                        {{if .ResponseBody}}
-                                        <pre class="mt-2 text-xs text-gray-700 overflow-x-auto whitespace-pre-wrap break-all">{{.ResponseBody}}</pre>
-                                        {{end}}
-                                        {{if .ResponseTruncated}}
-                                        {{if .ResponseSizeKnown}}
-                                        <p class="mt-1 text-xs text-gray-500">Response truncated for display: showing {{.ResponseShownBytes}} of {{.ResponseBytes}} bytes.</p>
-                                        {{else}}
-                                        <p class="mt-1 text-xs text-gray-500">Showing {{.ResponseShownBytes}} of the {{.ResponseBytes}} recorded bytes. The response reached the recording limit, so the remote may have sent more that was never stored.</p>
-                                        {{end}}
-                                        {{end}}
-                                    </div>
-                                    {{else}}
-                                    <p class="text-xs text-gray-500">No attempts recorded yet.</p>
-                                    {{end}}
-                                </div>
-                            </div>
                             {{end}}
                         </div>
+                        {{end}}
                     </div>
                     {{end}}
                 </div>