diff --git a/README.md b/README.md
index 844e410..c627ebd 100644
--- a/README.md
+++ b/README.md
@@ -1049,6 +1049,48 @@ 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. 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.
diff --git a/internal/delivery/client_ssrf_test.go b/internal/delivery/client_ssrf_test.go
index d31164a..df1faa7 100644
--- a/internal/delivery/client_ssrf_test.go
+++ b/internal/delivery/client_ssrf_test.go
@@ -25,12 +25,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()
@@ -50,7 +50,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 "+
@@ -91,10 +91,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()
@@ -103,10 +104,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",
+ )
+}
diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go
index 890c3ce..07bd170 100644
--- 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"
@@ -49,13 +50,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.
@@ -165,12 +167,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.
diff --git a/internal/delivery/redirect.go b/internal/delivery/redirect.go
new file mode 100644
index 0000000..af78042
--- /dev/null
+++ b/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)
+}
diff --git a/internal/delivery/redirect_test.go b/internal/delivery/redirect_test.go
new file mode 100644
index 0000000..231073a
--- /dev/null
+++ b/internal/delivery/redirect_test.go
@@ -0,0 +1,380 @@
+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 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",
+ )
+}
diff --git a/internal/delivery/target_headers.go b/internal/delivery/target_headers.go
index aff657e..20e3c9b 100644
--- 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)
diff --git a/internal/delivery/target_headers_test.go b/internal/delivery/target_headers_test.go
index c7db5f8..1b1c7ae 100644
--- 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
diff --git a/internal/delivery/target_http.go b/internal/delivery/target_http.go
index c8d025a..5e9d1b4 100644
--- a/internal/delivery/target_http.go
+++ b/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,82 @@ 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")
+
+ 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
diff --git a/templates/target_edit.html b/templates/target_edit.html
index e01db89..9194721 100644
--- a/templates/target_edit.html
+++ b/templates/target_edit.html
@@ -39,7 +39,7 @@