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.

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,
+				),
+			)
+		})
+	}
+}