Harden operator-set target headers (closes #233)

Three findings from the review of the per-target request headers
feature, plus the follow-up they raised about the inbound headers
the same delivery path forwards.

One rule now governs every header a delivery carries on someone
else's behalf: a redirect hop that leaves the origin the target
names carries none of them. That covers the operator's configured
headers and the inbound event headers forwarded from the sender
alike. net/http withholds only Authorization and Cookie across a
host change, so an operator's X-Api-Key or a sender's
X-Hub-Signature would otherwise follow a 302 to a host nobody
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 the lot. The
shared SSRF-safe transport is kept on that client, so each hop is
still dialled through the private-IP guard.

The set to strip is not a name list. applyRequestHeaders now
returns the canonical names of everything it applied on the
sender's or operator's behalf, and the redirect policy strips
exactly that, so a header added to the forward set is covered
without a second edit. Content-Type and User-Agent are the
delivery path's own rather than anyone else's, and both are
excluded from that set so they always travel: Content-Type is set
from the event and a 307 preserves the body across hosts, so it
has to stay typed, and User-Agent is overwritten with this
delivery path's own after the forwarded headers are applied, so
the sender's never reaches the wire and stripping it off-origin
would only substitute net/http's default.

The origin comparison no longer collapses two IPv6 origins into
one. Hostname() unwraps a literal's brackets, so re-appending the
port with a bare colon rendered https://[2001:db8::1]:8080 and
https://[2001:db8::1:8080] identically — a different address on a
different port passing as the same origin. The port is joined with
net.JoinHostPort, and both spellings are in TestSameDeliveryOrigin.

The ten-hop cap gains a regression test. Installing a CheckRedirect
is precisely what discards net/http's own limit, so a
self-redirecting destination is driven through the policy and
asserted to stop after exactly ten requests with the sentinel
surfacing to the caller.

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 as one rule over both header classes, including that the
drop is per hop rather than permanent: net/http re-copies the
initial request's headers each hop, so a chain returning to the
configured origin carries them again, exactly as it treats
Authorization. It also records what following a 301, 302 or 303
costs, since that is net/http's own behaviour and the decision to
follow redirects is what buys it: the POST becomes a GET and the
event body and its Content-Type are dropped, so the destination
the chain ends at receives no event while the delivery is still
recorded Delivered. The edit form's hint gains Trailer and the
redirect note.

Closes #243

README.md

@@ -1160,6 +1160,53 @@ events should be forwarded.
 The `config` field stores type-specific configuration as JSON (e.g.,
 destination URL, custom headers, timeout settings).
 
+**`http` target configuration:**
+
+| Key       | Type          | Description |
+| --------- | ------------- | ----------- |
+| `url`     | string        | Destination the event is POSTed to |
+| `headers` | object        | Extra request headers, applied last so they win over the event's own forwarded headers |
+| `timeout` | integer (sec) | Per-target request timeout; unset (or 0) uses the shared 30-second client timeout |
+
+`timeout` is capped at **300 seconds**, and the form rejects anything
+above it rather than substituting the cap. A delivery attempt holds one
+of the bounded pool's workers for its whole duration, so an unbounded
+timeout would let a single unresponsive destination stall the queue.
+
+`headers` rejects the names the delivery path or `net/http` writes
+regardless of what is configured: `Host`, `Content-Length`,
+`Transfer-Encoding`, `Connection`, `Trailer` and `User-Agent`. These are
+refused at the form rather than accepted and ignored, because a stored
+header that provably never reaches the wire tells the operator their
+configuration took effect when it did not. `Content-Type` is _not_
+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
+carries for someone else — the configured `headers` and the inbound
+event headers forwarded from the sender alike: **a hop that leaves the
+origin the target names carries none of them.** Leaving the origin
+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`,
+`User-Agent`) are not origin-scoped and always travel, so a body
+preserved across a `307` is still typed. A `301`, `302` or `303` is a
+different matter, and this is `net/http`'s behaviour rather than
+webhooker's: the POST becomes a GET and the event body and its
+`Content-Type` are dropped, so the destination the chain ends at
+receives no event at all — and the delivery is still recorded
+`Delivered` on that hop's `2xx`. Redirects within the target's own
+origin keep everything, so a destination that redirects its own paths
+is unaffected; the drop is per hop rather than permanent, so a 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
 
 A programmatic access credential for API authentication.

internal/delivery/client_ssrf_test.go

@@ -26,12 +26,12 @@ func newSSRFTestEngine() *delivery.Engine {
 	return delivery.NewTestEngine(log, client, 1)
 }
 
-// TestClientForConfig_TimeoutKeepsSSRFGuard asserts that a
-// client returned by clientForConfig for a config with a
+// TestClientForRequest_TimeoutKeepsSSRFGuard asserts that a
+// client returned by clientForRequest 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 TestClientForConfig_TimeoutKeepsSSRFGuard(t *testing.T) {
+func TestClientForRequest_TimeoutKeepsSSRFGuard(t *testing.T) {
 	t.Parallel()
 
 	engine := newSSRFTestEngine()
@@ -51,7 +51,7 @@ func TestClientForConfig_TimeoutKeepsSSRFGuard(t *testing.T) {
 				Timeout: 5,
 			}
 
-			client := engine.ExportClientForConfig(cfg)
+			client := engine.ExportClientForRequest(cfg, nil)
 
 			require.NotSame(t, engine.ExportClient(), client,
 				"a per-target timeout must yield a "+
@@ -92,10 +92,11 @@ func TestClientForConfig_TimeoutKeepsSSRFGuard(t *testing.T) {
 	}
 }
 
-// TestClientForConfig_NoTimeoutUnchanged asserts that with
-// no per-target timeout the shared SSRF-safe client is
-// returned unchanged.
-func TestClientForConfig_NoTimeoutUnchanged(t *testing.T) {
+// TestClientForRequest_NoTimeoutUnchanged asserts that a
+// request with neither a per-target timeout nor an origin-scoped
+// header gets the shared SSRF-safe client unchanged: there is then
+// nothing for a redirect policy to strip.
+func TestClientForRequest_NoTimeoutUnchanged(t *testing.T) {
 	t.Parallel()
 
 	engine := newSSRFTestEngine()
@@ -104,10 +105,46 @@ func TestClientForConfig_NoTimeoutUnchanged(t *testing.T) {
 		URL: "https://example.com/hook",
 	}
 
-	client := engine.ExportClientForConfig(cfg)
+	client := engine.ExportClientForRequest(cfg, nil)
 
 	assert.Same(t, engine.ExportClient(), client,
 		"without a per-target timeout the shared client "+
 			"must be returned unchanged",
 	)
 }
+
+// TestClientForRequest_HeadersKeepSSRFGuard asserts that the
+// redirect policy an origin-scoped header installs is added to a
+// client that still carries the SSRF-safe transport. The guard is
+// a dial hook, so keeping it is what makes each redirect hop pass
+// the private-IP check too.
+func TestClientForRequest_HeadersKeepSSRFGuard(t *testing.T) {
+	t.Parallel()
+
+	engine := newSSRFTestEngine()
+
+	cfg := &delivery.HTTPTargetConfig{
+		URL: "https://example.com/with-headers",
+		Headers: map[string]string{
+			"X-Api-Key": "configured",
+		},
+	}
+
+	client := engine.ExportClientForRequest(
+		cfg, []string{"X-Api-Key"},
+	)
+
+	require.NotNil(t, client.CheckRedirect,
+		"an origin-scoped header must install a redirect policy",
+	)
+
+	assert.Same(t,
+		engine.ExportClient().Transport, client.Transport,
+		"the SSRF-safe transport must be reused, not dropped",
+	)
+
+	assert.Equal(t,
+		engine.ExportClient().Timeout, client.Timeout,
+		"the shared client's timeout must be inherited",
+	)
+}

internal/delivery/export_test.go

@@ -6,6 +6,7 @@ import (
 	"net"
 	"net/http"
 	"net/netip"
+	"net/url"
 	"time"
 
 	"go.uber.org/fx"
@@ -70,13 +71,14 @@ 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,
-) {
-	applyRequestHeaders(req, event, cfg)
+) []string {
+	return applyRequestHeaders(req, event, cfg)
 }
 
 // ExportTruncate exposes truncate for testing.
@@ -186,12 +188,27 @@ func (e *Engine) ExportDoHTTPRequest(
 	return e.httpTarget.doHTTPRequest(ctx, cfg, event)
 }
 
-// ExportClientForConfig exposes the http target's
-// clientForConfig.
-func (e *Engine) ExportClientForConfig(
+// ExportClientForRequest exposes the http target's
+// clientForRequest.
+func (e *Engine) ExportClientForRequest(
 	cfg *HTTPTargetConfig,
+	originScoped []string,
 ) *http.Client {
-	return e.httpTarget.clientForConfig(cfg)
+	return e.httpTarget.clientForRequest(cfg, originScoped)
+}
+
+// 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)
 }
 
 // ExportClient returns the http target's shared HTTP client.

internal/delivery/redirect.go

@@ -0,0 +1,107 @@
+package delivery
+
+import (
+	"errors"
+	"fmt"
+	"net"
+	"net/http"
+	"net/url"
+	"strings"
+)
+
+// maxDeliveryRedirects caps a redirect chain. Installing a
+// CheckRedirect replaces net/http's default policy including its
+// own limit, so the limit is restated rather than dropped.
+const maxDeliveryRedirects = 10
+
+// schemeHTTPS names the scheme the origin comparison treats
+// specially: a step down from it is never the same origin.
+const schemeHTTPS = "https"
+
+var errTooManyRedirects = errors.New("too many redirects")
+
+// offOriginHeaderPolicy returns a CheckRedirect that drops every
+// origin-scoped header once a redirect leaves the origin the
+// operator configured. names is the set applyRequestHeaders
+// 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
+// sender's signature — X-Hub-Signature — so an open redirect at an
+// otherwise trusted destination would hand either to a host the
+// operator never named. Redirects are still followed: refusing them
+// would break every destination that legitimately redirects and
+// 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(
+	names []string,
+) func(*http.Request, []*http.Request) error {
+	return func(req *http.Request, via []*http.Request) error {
+		if len(via) >= maxDeliveryRedirects {
+			return fmt.Errorf(
+				"%w: stopped after %d",
+				errTooManyRedirects, maxDeliveryRedirects,
+			)
+		}
+
+		if sameDeliveryOrigin(via[0].URL, req.URL) {
+			return nil
+		}
+
+		for _, name := range names {
+			req.Header.Del(name)
+		}
+
+		return nil
+	}
+}
+
+// sameDeliveryOrigin reports whether dest is close enough to the
+// configured target URL to keep carrying its origin-scoped headers.
+//
+// This is stricter than the rule net/http applies to Authorization:
+// the port is part of the comparison (a different port is a
+// different service), and a subdomain of the configured host is not
+// the same origin. An https origin stepping down to http is never
+// the same origin whatever the hosts are, because that puts the
+// header on the wire in clear.
+func sameDeliveryOrigin(origin, dest *url.URL) bool {
+	if origin.Scheme == schemeHTTPS && dest.Scheme != schemeHTTPS {
+		return false
+	}
+
+	return originHostPort(origin) == originHostPort(dest)
+}
+
+// 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())
+
+	port := u.Port()
+	if port == "" ||
+		(u.Scheme == "http" && port == "80") ||
+		(u.Scheme == schemeHTTPS && port == "443") {
+		return host
+	}
+
+	return net.JoinHostPort(host, port)
+}

internal/delivery/redirect_test.go

@@ -0,0 +1,383 @@
+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"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// The headers these tests drive stand in for the two classes the
+// off-origin rule covers: an operator-configured credential and an
+// inbound header the delivery path forwards. net/http withholds
+// Authorization and Cookie across a host change, and nothing else.
+const (
+	probeHeaderName    = "X-Api-Key"
+	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
+	hits int
+}
+
+func (p *redirectProbe) serve(
+	w http.ResponseWriter, r *http.Request,
+) {
+	p.mu.Lock()
+	p.seen = r.Header.Clone()
+	p.hits++
+	p.mu.Unlock()
+
+	w.WriteHeader(http.StatusOK)
+}
+
+func (p *redirectProbe) result() (http.Header, int) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+
+	return p.seen, p.hits
+}
+
+// deliverWithProbeHeaders runs one real delivery of a new task
+// through the engine to targetURL, carrying both probe headers —
+// probeHeaderName configured on the target, inboundHeaderName
+// forwarded from the event — and returns the delivery status the
+// engine recorded.
+func deliverWithProbeHeaders(
+	t *testing.T, targetURL string,
+) database.DeliveryStatus {
+	t.Helper()
+
+	s := newISetup(t)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"hello":"world"}`,
+	)
+	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,
+	)
+
+	cfg, err := json.Marshal(delivery.HTTPTargetConfig{
+		URL: targetURL,
+		Headers: map[string]string{
+			probeHeaderName: probeHeaderValue,
+		},
+	})
+	require.NoError(t, err)
+
+	body := event.Body
+	task := iTask(
+		d, event, s.WebhookID, targetID,
+		"redirect-target", string(cfg), 0, 1, &body,
+	)
+
+	s.Engine.ExportProcessNewTask(context.TODO(), &task)
+
+	var updated database.Delivery
+
+	require.NoError(t, s.WebhookDB.First(
+		&updated, "id = ?", d.ID,
+	).Error)
+
+	return updated.Status
+}
+
+// A 302 to an origin the operator never configured must not carry
+// the credential they configured for the one they did, nor the
+// inbound header this delivery forwarded — one rule for both
+// classes. The chain is still followed, so the delivery is recorded
+// from the final hop.
+func TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	var probe redirectProbe
+
+	final := httptest.NewServer(
+		http.HandlerFunc(probe.serve),
+	)
+	defer final.Close()
+
+	// httptest listens on loopback, so reach the second server
+	// under loopback's other name: the hop then differs in
+	// hostname as well as port and is cross-host by any reading.
+	finalURL, err := url.Parse(final.URL)
+	require.NoError(t, err)
+
+	finalURL.Host = "localhost:" + finalURL.Port()
+	finalURL.Path = "/moved"
+
+	origin := httptest.NewServer(http.HandlerFunc(
+		func(w http.ResponseWriter, r *http.Request) {
+			http.Redirect(
+				w, r, finalURL.String(),
+				http.StatusFound,
+			)
+		},
+	))
+	defer origin.Close()
+
+	status := deliverWithProbeHeaders(t, origin.URL)
+
+	seen, hits := probe.result()
+
+	assert.Equal(t, 1, hits,
+		"the redirect must still be followed",
+	)
+	assert.Empty(t, seen.Get(probeHeaderName),
+		"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",
+	)
+}
+
+// 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
+// signature the receiver verifies.
+func TestDelivery_SameOriginRedirectKeepsOriginScopedHeaders(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	var probe redirectProbe
+
+	srv := httptest.NewServer(http.HandlerFunc(
+		func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Path == "/moved" {
+				probe.serve(w, r)
+
+				return
+			}
+
+			http.Redirect(
+				w, r, "/moved", http.StatusFound,
+			)
+		},
+	))
+	defer srv.Close()
+
+	status := deliverWithProbeHeaders(t, srv.URL+"/hook")
+
+	seen, hits := probe.result()
+
+	assert.Equal(t, 1, hits)
+	assert.Equal(t, probeHeaderValue, seen.Get(probeHeaderName),
+		"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,
+	)
+}
+
+// The origin comparison is deliberately stricter than the one
+// net/http applies to Authorization: the port counts and a
+// subdomain does not inherit. Only the default-port spellings of
+// one origin are the same origin.
+func TestSameDeliveryOrigin(t *testing.T) {
+	t.Parallel()
+
+	// The configured target URL every case redirects away from.
+	// Destination paths differ only so that no literal repeats.
+	const configured = "https://h/a"
+
+	cases := map[string]struct {
+		origin string
+		dest   string
+		want   bool
+	}{
+		"other path":           {configured, "https://h/b", true},
+		"default port spelled": {configured, "https://h:443/c", true},
+		"host in another case": {configured, "https://H/d", true},
+		"http default port":    {"http://h:80/a", "http://h/e", true},
+		"upgrade to https":     {"http://h/a", "https://h/f", true},
+		"downgrade to http":    {configured, "http://h/g", false},
+		"another host":         {configured, "https://i/h", false},
+		"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 {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			origin, err := url.Parse(tc.origin)
+			require.NoError(t, err)
+
+			dest, err := url.Parse(tc.dest)
+			require.NoError(t, err)
+
+			assert.Equal(t, tc.want,
+				delivery.ExportSameDeliveryOrigin(
+					origin, dest,
+				),
+			)
+		})
+	}
+}
+
+// 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 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()
+
+	inbound, err := json.Marshal(map[string][]string{
+		inboundHeaderName: {inboundHeaderValue},
+		"Content-Type":    {testContentType},
+		"User-Agent":      {"curl/8.7.1"},
+		"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_headers.go

@@ -65,6 +65,11 @@ func isReservedTargetHeader(name string) bool {
 		// the configured headers, so a configured one would always
 		// be overwritten.
 		return true
+	case "Trailer":
+		// net/http strips Trailer from the request it writes
+		// (reqWriteExcludeHeader), so a configured one is accepted
+		// and stored and then provably never reaches the wire.
+		return true
 	default:
 		return false
 	}
@@ -118,9 +123,11 @@ func parseHeaderLine(line string) (string, string, error) {
 
 	rawName = strings.TrimSpace(rawName)
 	if !validHeaderName(rawName) {
-		return "", "", fmt.Errorf(
-			"%w: %q", errHeaderNameInvalid, rawName,
-		)
+		// Quotes nothing. The text before the first colon is only
+		// a name if it parses as one; when it does not, it is as
+		// likely to be a pasted value whose own colon split the
+		// line, and half of a token would be echoed into the 400.
+		return "", "", errHeaderNameInvalid
 	}
 
 	name := http.CanonicalHeaderKey(rawName)

internal/delivery/target_headers_test.go

@@ -82,6 +82,16 @@ func TestParseTargetHeaders_Rejects(t *testing.T) {
 	}
 }
 
+// net/http strips Trailer from the request it writes, so accepting
+// one would store a header that never reaches the target.
+func TestParseTargetHeaders_RejectsTrailer(t *testing.T) {
+	t.Parallel()
+
+	_, err := delivery.ParseTargetHeaders("Trailer: X-Checksum")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "Trailer")
+}
+
 // A header value is routinely a bearer token and these errors are
 // rendered into a 400 body, so no message may quote one.
 func TestParseTargetHeaders_ErrorsNeverQuoteAValue(t *testing.T) {
@@ -89,17 +99,26 @@ func TestParseTargetHeaders_ErrorsNeverQuoteAValue(t *testing.T) {
 
 	const secret = "QQNEVERINAMESSAGEQQ"
 
-	_, err := delivery.ParseTargetHeaders(
+	inputs := []string{
+		// The value, after the colon, in a duplicate name.
 		"X-A: " + secret + "\nx-a: " + secret,
-	)
-	require.Error(t, err)
-	assert.NotContains(t, err.Error(), secret)
-
-	_, err = delivery.ParseTargetHeaders(
+		// The value after the colon of an unusable name.
 		"X Bad Name: " + secret,
-	)
-	require.Error(t, err)
-	assert.NotContains(t, err.Error(), secret)
+		// The line splits on the value's own colon, so the
+		// secret lands in the text an unusable-name error is
+		// tempted to quote as the name.
+		"X-Api-Key " + secret + ":x",
+		// The same, with nothing before the secret at all.
+		secret + " and more:x",
+		// A control character in the value.
+		"X-A: " + secret + "\x01",
+	}
+
+	for _, input := range inputs {
+		_, err := delivery.ParseTargetHeaders(input)
+		require.Error(t, err, input)
+		assert.NotContains(t, err.Error(), secret, input)
+	}
 }
 
 // Loading the edit form twice without saving must not reshuffle

internal/delivery/target_http.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"sort"
 	"sync"
 	"time"
 
@@ -404,9 +405,9 @@ func (t *httpTarget) doHTTPRequest(
 		)
 	}
 
-	applyRequestHeaders(req, event, cfg)
+	originScoped := applyRequestHeaders(req, event, cfg)
 
-	client := t.clientForConfig(cfg)
+	client := t.clientForRequest(cfg, originScoped)
 
 	resp, doErr := executeHTTPRequest(client, req)
 
@@ -432,23 +433,41 @@ func (t *httpTarget) doHTTPRequest(
 	return resp.StatusCode, string(body), dur, nil
 }
 
-func (t *httpTarget) clientForConfig(
+// clientForRequest returns the client for one delivery attempt.
+// originScoped is the header set applyRequestHeaders built for that
+// attempt; a request with neither a per-target timeout nor an
+// origin-scoped header gets the shared client, because there is
+// then nothing for the redirect policy to strip and net/http's
+// default policy already withholds Authorization and Cookie across
+// hosts.
+func (t *httpTarget) clientForRequest(
 	cfg *HTTPTargetConfig,
+	originScoped []string,
 ) *http.Client {
-	if cfg.Timeout > 0 {
-		// Reuse the shared client's SSRF-safe transport so
-		// a per-target timeout does not drop the
-		// request-time private-IP guard. Only the timeout
-		// is overridden.
-		return &http.Client{
-			Timeout: time.Duration(
-				cfg.Timeout,
-			) * time.Second,
-			Transport: t.client.Transport,
-		}
+	if cfg.Timeout <= 0 && len(originScoped) == 0 {
+		return t.client
 	}
 
-	return t.client
+	// Reuse the shared client's SSRF-safe transport so neither a
+	// per-target timeout nor the redirect policy drops the
+	// request-time private-IP guard — which, being a dial hook,
+	// also covers every redirect hop.
+	client := &http.Client{
+		Timeout:   t.client.Timeout,
+		Transport: t.client.Transport,
+	}
+
+	if cfg.Timeout > 0 {
+		client.Timeout = time.Duration(
+			cfg.Timeout,
+		) * time.Second
+	}
+
+	if len(originScoped) > 0 {
+		client.CheckRedirect = offOriginHeaderPolicy(originScoped)
+	}
+
+	return client
 }
 
 func parseHTTPConfig(
@@ -490,40 +509,88 @@ 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,
 		)
 	}
 
-	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)
-					}
-				}
-			}
-		}
-	}
+	originScoped := forwardEventHeaders(req, event)
 
 	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")
+
+	// User-Agent is overwritten just above, so an inbound one never
+	// reaches the wire and the value that does identifies this
+	// delivery path rather than the sender. Reporting it would strip
+	// it off-origin and leave net/http's own default in its place.
+	delete(originScoped, "User-Agent")
+
+	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

templates/target_edit.html

@@ -39,7 +39,7 @@
             <div class="form-group">
                 <label for="headers" class="label">Headers</label>
                 <textarea id="headers" name="headers" rows="4" class="input" placeholder="Authorization: Bearer ...">{{.Target.Config.Headers}}</textarea>
-                <p class="text-xs text-gray-500 mt-1">One <code>Name: value</code> per line, sent with every delivery. Leave blank for none. <code>Host</code>, <code>Content-Length</code>, <code>Transfer-Encoding</code>, <code>Connection</code> and <code>User-Agent</code> are set by the delivery engine and are rejected here rather than silently ignored.</p>
+                <p class="text-xs text-gray-500 mt-1">One <code>Name: value</code> per line, sent with every delivery. Leave blank for none. <code>Host</code>, <code>Content-Length</code>, <code>Transfer-Encoding</code>, <code>Connection</code>, <code>Trailer</code> and <code>User-Agent</code> are set by the delivery engine and are rejected here rather than silently ignored. Headers set here are dropped if a redirect leaves the destination's own origin, so a credential cannot follow one to another host.</p>
             </div>
 
             <div class="form-group">