diff --git a/internal/delivery/engine_test.go b/internal/delivery/engine_test.go index 213b13a..13d1625 100644 --- a/internal/delivery/engine_test.go +++ b/internal/delivery/engine_test.go @@ -1166,6 +1166,10 @@ func TestIsForwardableHeader(t *testing.T) { assert.False(t, delivery.ExportIsForwardableHeader("Content-Length"), ) + + assert.False(t, + delivery.ExportIsForwardableHeader("Content-Type"), + ) } func TestTruncate(t *testing.T) { @@ -1247,6 +1251,81 @@ func TestDoHTTPRequest_ForwardsHeaders(t *testing.T) { ) } +// The event's stored inbound headers carry the same Content-Type the +// receiver saved as the event's ContentType, so a delivery could send +// it twice. It must go out exactly once, with a Content-Type configured +// on the target winning, then the event's ContentType. +func TestApplyRequestHeaders_SendsOneContentType(t *testing.T) { + t.Parallel() + + cases := map[string]struct { + inbound string + event string + configured string + want []string + }{ + "inbound and event agree": { + inbound: testContentType, + event: testContentType, + want: []string{testContentType}, + }, + "inbound and event disagree": { + inbound: "text/plain", + event: testContentType, + want: []string{testContentType}, + }, + "event has none": { + inbound: testContentType, + want: nil, + }, + "target configures its own": { + inbound: testContentType, + event: testContentType, + configured: "application/xml", + want: []string{"application/xml"}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + inbound, err := json.Marshal(map[string][]string{ + headerContentType: {tc.inbound}, + }) + require.NoError(t, err) + + cfg := &delivery.HTTPTargetConfig{} + if tc.configured != "" { + cfg.Headers = map[string]string{ + headerContentType: tc.configured, + } + } + + req, err := http.NewRequestWithContext( + context.Background(), + http.MethodPost, + "https://target.example.com/hook", + http.NoBody, + ) + require.NoError(t, err) + + delivery.ExportApplyRequestHeaders( + req, + &database.Event{ + Headers: string(inbound), + ContentType: tc.event, + }, + cfg, + ) + + assert.Equal(t, + tc.want, req.Header.Values(headerContentType), + ) + }) + } +} + func TestProcessDelivery_RoutesToCorrectHandler( t *testing.T, ) { diff --git a/internal/delivery/redirect_test.go b/internal/delivery/redirect_test.go index 1534697..e5d5347 100644 --- a/internal/delivery/redirect_test.go +++ b/internal/delivery/redirect_test.go @@ -339,10 +339,11 @@ func TestRedirectPolicy_StopsAtHopCap(t *testing.T) { // 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. +// is not in the set, and neither is the inbound Content-Type, because +// it is not forwarded. Two more are deliberately excluded: a +// Content-Type configured on the target 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() @@ -371,6 +372,7 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) { &delivery.HTTPTargetConfig{ Headers: map[string]string{ probeHeaderName: probeHeaderValue, + "Content-Type": testContentType, }, }, ) @@ -378,7 +380,11 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) { 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", + "Host and the inbound Content-Type are never "+ + "forwarded, User-Agent is the delivery path's own", + ) + assert.NotContains(t, names, "Content-Type", + "a Content-Type configured on the target must survive "+ + "a cross-origin 307/308 with the body it describes", ) } diff --git a/internal/delivery/target_headers_test.go b/internal/delivery/target_headers_test.go index 1b1c7ae..3237132 100644 --- a/internal/delivery/target_headers_test.go +++ b/internal/delivery/target_headers_test.go @@ -11,10 +11,11 @@ import ( "sneak.berlin/go/webhooker/internal/delivery" ) -// Literals these tests repeat, named so that the header name and the +// Literals these tests repeat, named so that the header names and the // keep-forever archive config each have one definition. const ( headerAuthorization = "Authorization" + headerContentType = "Content-Type" bearerValue = "Bearer abc" archiveConfigNever = "{\"expiry\":\"never\"}" ) diff --git a/internal/delivery/target_http.go b/internal/delivery/target_http.go index fc264f9..9c7b539 100644 --- a/internal/delivery/target_http.go +++ b/internal/delivery/target_http.go @@ -541,6 +541,11 @@ func isForwardableHeader(name string) bool { "Upgrade", "Proxy-Authorization", "Proxy-Connection", "Content-Length": return false + case "Content-Type": + // applyRequestHeaders sets Content-Type itself. The receiver + // already stored this inbound value as the event's + // ContentType, so forwarding it too would send it twice. + return false default: return true } @@ -553,6 +558,10 @@ func isForwardableHeader(name string) bool { // 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. +// +// Content-Type goes out once: a Content-Type configured on the target +// wins, otherwise the event's ContentType, otherwise none. The inbound +// Content-Type in the event's headers is never forwarded. func applyRequestHeaders( req *http.Request, event *database.Event, @@ -573,10 +582,10 @@ func applyRequestHeaders( 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. + // A Content-Type configured on the target describes the body + // being sent rather than the sender. 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