All checks were successful
check / check (push) Successful in 2m56s
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
384 lines
10 KiB
Go
384 lines
10 KiB
Go
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",
|
|
)
|
|
}
|