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.
2026-08-20 06:15:23 +00:00
9 changed files with 477 additions and 28 deletions
--- a/README.md
+++ b/README.md
@@ -1049,6 +1049,41 @@ 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. Configured `headers` are dropped as soon as a
+hop leaves the origin the target names — a different host, a different
+port, or a step down from `https` to `http` — because a configured
+header is routinely a credential (`X-Api-Key`, `PRIVATE-TOKEN`) 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`. Redirects within the target's own origin keep every
+configured header, so a destination that redirects its own paths is
+unaffected. 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.
--- a/internal/delivery/client_ssrf_test.go
+++ b/internal/delivery/client_ssrf_test.go
@@ -91,9 +91,10 @@ func TestClientForConfig_TimeoutKeepsSSRFGuard(t *testing.T) {
 	}
 }

-// TestClientForConfig_NoTimeoutUnchanged asserts that with
-// no per-target timeout the shared SSRF-safe client is
-// returned unchanged.
+// TestClientForConfig_NoTimeoutUnchanged asserts that a
+// config overriding neither the timeout nor the headers gets
+// the shared SSRF-safe client unchanged: with no configured
+// headers there is nothing for a redirect policy to strip.
 func TestClientForConfig_NoTimeoutUnchanged(t *testing.T) {
 	t.Parallel()

@@ -110,3 +111,37 @@ func TestClientForConfig_NoTimeoutUnchanged(t *testing.T) {
 			"must be returned unchanged",
 	)
 }
+
+// TestClientForConfig_HeadersKeepSSRFGuard asserts that the
+// redirect policy a target's configured headers install 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 TestClientForConfig_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.ExportClientForConfig(cfg)
+
+	require.NotNil(t, client.CheckRedirect,
+		"configured headers 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",
+	)
+}
--- a/internal/delivery/export_test.go
+++ b/internal/delivery/export_test.go
@@ -5,6 +5,7 @@ import (
 	"log/slog"
 	"net"
 	"net/http"
+	"net/url"
 	"time"

 	"go.uber.org/fx"
@@ -167,6 +168,11 @@ func (e *Engine) ExportClientForConfig(
 	return e.httpTarget.clientForConfig(cfg)
 }

+// ExportSameDeliveryOrigin exposes sameDeliveryOrigin.
+func ExportSameDeliveryOrigin(origin, dest *url.URL) bool {
+	return sameDeliveryOrigin(origin, dest)
+}
+
 // ExportClient returns the http target's shared HTTP client.
 func (e *Engine) ExportClient() *http.Client {
 	return e.httpTarget.client
--- a/internal/delivery/redirect.go
+++ b/internal/delivery/redirect.go
@@ -0,0 +1,97 @@
+package delivery
+
+import (
+	"errors"
+	"fmt"
+	"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")
+
+// configuredHeaderRedirectPolicy returns a CheckRedirect that
+// drops a target's configured headers once a redirect leaves the
+// origin the operator configured.
+//
+// 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 — so an open redirect at an otherwise trusted
+// destination would hand that credential 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.
+//
+// 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 configuredHeaderRedirectPolicy(
+	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(
+				"%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 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
+// 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.
+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 host + ":" + port
+}
--- a/internal/delivery/redirect_test.go
+++ b/internal/delivery/redirect_test.go
@@ -0,0 +1,233 @@
+package delivery_test
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"sync"
+	"testing"
+
+	"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 header these tests configure stands in for the credential
+// headers net/http forwards across a host change: it withholds
+// Authorization and Cookie, and nothing else.
+const (
+	probeHeaderName  = "X-Api-Key"
+	probeHeaderValue = "QQNEVERONTHEWIREQQ"
+)
+
+// redirectProbe records what the last hop of a redirect chain
+// actually received.
+type redirectProbe struct {
+	mu   sync.Mutex
+	seen string
+	hits int
+}
+
+func (p *redirectProbe) serve(
+	w http.ResponseWriter, r *http.Request,
+) {
+	p.mu.Lock()
+	p.seen = r.Header.Get(probeHeaderName)
+	p.hits++
+	p.mu.Unlock()
+
+	w.WriteHeader(http.StatusOK)
+}
+
+func (p *redirectProbe) result() (string, int) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+
+	return p.seen, p.hits
+}
+
+// deliverWithConfiguredHeader runs one real delivery of a new task
+// through the engine to targetURL, with probeHeaderName set on the
+// target, and returns the delivery status the engine recorded.
+func deliverWithConfiguredHeader(
+	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()
+
+	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. The chain is
+// still followed, so the delivery is recorded from the final hop.
+func TestDelivery_CrossOriginRedirectDropsConfiguredHeader(
+	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 := deliverWithConfiguredHeader(t, origin.URL)
+
+	seen, hits := probe.result()
+
+	assert.Equal(t, 1, hits,
+		"the redirect must still be followed",
+	)
+	assert.Empty(t, seen,
+		"a configured credential 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.
+func TestDelivery_SameOriginRedirectKeepsConfiguredHeader(
+	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 := deliverWithConfiguredHeader(t, srv.URL+"/hook")
+
+	seen, hits := probe.result()
+
+	assert.Equal(t, 1, hits)
+	assert.Equal(t, probeHeaderValue, seen,
+		"a redirect within the configured origin must keep "+
+			"the configured 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},
+	}
+
+	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,
+				),
+			)
+		})
+	}
+}
--- a/internal/delivery/target_headers.go
+++ b/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)
--- a/internal/delivery/target_headers_test.go
+++ b/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
--- a/internal/delivery/target_http.go
+++ b/internal/delivery/target_http.go
@@ -432,23 +432,40 @@ func (t *httpTarget) doHTTPRequest(
 	return resp.StatusCode, string(body), dur, nil
 }

+// clientForConfig returns the client for one target's requests.
+// A config that overrides neither the timeout nor the headers gets
+// the shared client: with no configured headers there is nothing
+// for the redirect policy to strip, and net/http's default policy
+// already withholds Authorization and Cookie across hosts.
 func (t *httpTarget) clientForConfig(
 	cfg *HTTPTargetConfig,
 ) *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(cfg.Headers) == 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(cfg.Headers) > 0 {
+		client.CheckRedirect = configuredHeaderRedirectPolicy(
+			cfg.Headers,
+		)
+	}
+
+	return client
 }

 func parseHTTPConfig(
--- a/templates/target_edit.html
+++ b/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">