All checks were successful
check / check (push) Successful in 3m5s
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 and always travel; a 307 preserves the body across hosts and it has to stay typed. 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. The edit form's hint gains Trailer and the redirect note. Closes #243
293 lines
7.7 KiB
Go
293 lines
7.7 KiB
Go
package delivery_test
|
|
|
|
import (
|
|
"encoding/json"
|
|
"strconv"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
"sneak.berlin/go/webhooker/internal/database"
|
|
"sneak.berlin/go/webhooker/internal/delivery"
|
|
)
|
|
|
|
// Literals these tests repeat, named so that the header name and the
|
|
// keep-forever archive config each have one definition.
|
|
const (
|
|
headerAuthorization = "Authorization"
|
|
bearerValue = "Bearer abc"
|
|
archiveConfigNever = "{\"expiry\":\"never\"}"
|
|
)
|
|
|
|
func TestParseTargetHeaders_AcceptsPairs(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
got, err := delivery.ParseTargetHeaders(
|
|
" Authorization: Bearer abc \n\n" +
|
|
"x-tenant:acme\r\n" +
|
|
"X-Empty:\n",
|
|
)
|
|
require.NoError(t, err)
|
|
|
|
assert.Equal(
|
|
t,
|
|
map[string]string{
|
|
headerAuthorization: bearerValue,
|
|
"X-Tenant": "acme",
|
|
"X-Empty": "",
|
|
},
|
|
got,
|
|
)
|
|
}
|
|
|
|
// A configuration with no headers must stay indistinguishable from
|
|
// one written before the field existed, so omitempty drops the key.
|
|
func TestParseTargetHeaders_EmptyInputYieldsNoHeaders(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
got, err := delivery.ParseTargetHeaders("\n \n")
|
|
require.NoError(t, err)
|
|
assert.Empty(t, got)
|
|
|
|
encoded, err := json.Marshal(delivery.HTTPTargetConfig{
|
|
URL: "https://example.com/h",
|
|
Headers: got,
|
|
})
|
|
require.NoError(t, err)
|
|
assert.JSONEq(
|
|
t, `{"url":"https://example.com/h"}`, string(encoded),
|
|
)
|
|
}
|
|
|
|
func TestParseTargetHeaders_Rejects(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
cases := map[string]string{
|
|
"no colon": "Authorization Bearer abc",
|
|
"empty name": ": value",
|
|
"space in name": "X Bad: value",
|
|
"reserved host": "Host: evil.example",
|
|
"reserved ua": "User-Agent: curl/8",
|
|
"reserved length": "Content-Length: 0",
|
|
"duplicate any case": "X-A: 1\nx-a: 2",
|
|
}
|
|
|
|
for name, input := range cases {
|
|
t.Run(name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
_, err := delivery.ParseTargetHeaders(input)
|
|
require.Error(t, err)
|
|
})
|
|
}
|
|
}
|
|
|
|
// 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) {
|
|
t.Parallel()
|
|
|
|
const secret = "QQNEVERINAMESSAGEQQ"
|
|
|
|
inputs := []string{
|
|
// The value, after the colon, in a duplicate name.
|
|
"X-A: " + secret + "\nx-a: " + secret,
|
|
// The value after the colon of an unusable name.
|
|
"X Bad Name: " + 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
|
|
// the textarea, which Go's map iteration order would otherwise do.
|
|
func TestFormatTargetHeaders_IsSorted(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
got := delivery.FormatTargetHeaders(map[string]string{
|
|
"X-Zed": "z",
|
|
headerAuthorization: bearerValue,
|
|
"X-Alpha": "a",
|
|
})
|
|
|
|
assert.Equal(
|
|
t,
|
|
"Authorization: Bearer abc\nX-Alpha: a\nX-Zed: z\n",
|
|
got,
|
|
)
|
|
assert.Empty(t, delivery.FormatTargetHeaders(nil))
|
|
}
|
|
|
|
func TestFormatTargetHeaders_RoundTripsThroughParse(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
want := map[string]string{
|
|
headerAuthorization: bearerValue,
|
|
"X-Tenant": "acme",
|
|
}
|
|
|
|
got, err := delivery.ParseTargetHeaders(
|
|
delivery.FormatTargetHeaders(want),
|
|
)
|
|
require.NoError(t, err)
|
|
assert.Equal(t, want, got)
|
|
}
|
|
|
|
func TestParseTargetTimeout(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
got, err := delivery.ParseTargetTimeout(" 30 ")
|
|
require.NoError(t, err)
|
|
assert.Equal(t, 30, got)
|
|
|
|
got, err = delivery.ParseTargetTimeout("")
|
|
require.NoError(t, err)
|
|
assert.Zero(t, got)
|
|
|
|
for _, bad := range []string{"soon", "-1", "1e3", "100000"} {
|
|
_, err = delivery.ParseTargetTimeout(bad)
|
|
require.Error(t, err, bad)
|
|
}
|
|
}
|
|
|
|
func TestFormatTargetTimeout(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
assert.Equal(t, "30", delivery.FormatTargetTimeout(30))
|
|
assert.Empty(t, delivery.FormatTargetTimeout(0))
|
|
assert.Empty(t, delivery.FormatTargetTimeout(-1))
|
|
}
|
|
|
|
func TestNewTargetConfigForm(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
form, err := delivery.NewTargetConfigForm(&database.Target{
|
|
Type: database.TargetTypeHTTP,
|
|
Config: `{"url":"https://example.com/h",` +
|
|
`"headers":{"Authorization":"Bearer abc"},` +
|
|
`"timeout":9}`,
|
|
})
|
|
require.NoError(t, err)
|
|
assert.Equal(t, "https://example.com/h", form.URL)
|
|
assert.Equal(t, "Authorization: Bearer abc\n", form.Headers)
|
|
assert.Equal(t, "9", form.Timeout)
|
|
|
|
form, err = delivery.NewTargetConfigForm(&database.Target{
|
|
Type: database.TargetTypeSlack,
|
|
Config: `{"webhookUrl":"https://hooks.example/s"}`,
|
|
})
|
|
require.NoError(t, err)
|
|
assert.Equal(t, "https://hooks.example/s", form.URL)
|
|
|
|
form, err = delivery.NewTargetConfigForm(&database.Target{
|
|
Type: database.TargetTypeDatabase,
|
|
Config: `{"expiry":"720h"}`,
|
|
})
|
|
require.NoError(t, err)
|
|
assert.Equal(t, "720h", form.Expiry)
|
|
|
|
form, err = delivery.NewTargetConfigForm(&database.Target{
|
|
Type: database.TargetTypeLog,
|
|
})
|
|
require.NoError(t, err)
|
|
assert.Empty(t, form.URL)
|
|
}
|
|
|
|
// A keep-forever archive target must pre-fill as an empty field, so
|
|
// saving the form back unchanged stores the same empty config.
|
|
func TestNewTargetConfigForm_DatabaseNeverIsBlank(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
for _, cfg := range []string{"", `{}`, archiveConfigNever} {
|
|
form, err := delivery.NewTargetConfigForm(
|
|
&database.Target{
|
|
Type: database.TargetTypeDatabase,
|
|
Config: cfg,
|
|
},
|
|
)
|
|
require.NoError(t, err, cfg)
|
|
assert.Empty(t, form.Expiry, cfg)
|
|
}
|
|
}
|
|
|
|
// An unreadable stored config is an error rather than a blank form
|
|
// that looks like a target with no settings, so the caller can tell
|
|
// the operator that saving replaces the stored value.
|
|
func TestNewTargetConfigForm_UnreadableConfigErrors(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
cases := []*database.Target{
|
|
{Type: database.TargetTypeHTTP, Config: "not json"},
|
|
{Type: database.TargetTypeHTTP, Config: `{}`},
|
|
{Type: database.TargetTypeSlack, Config: ""},
|
|
{
|
|
Type: database.TargetTypeDatabase,
|
|
Config: `{"expiry":"soon"}`,
|
|
},
|
|
{Type: database.TargetType("nope")},
|
|
}
|
|
|
|
for _, target := range cases {
|
|
_, err := delivery.NewTargetConfigForm(target)
|
|
require.Error(t, err, target.Type)
|
|
}
|
|
}
|
|
|
|
// The ceiling exists so one misconfigured target cannot hold a
|
|
// delivery worker indefinitely, and it is inclusive.
|
|
func TestParseTargetTimeout_CeilingIsInclusive(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
assert.Positive(t, delivery.MaxTargetTimeoutSeconds)
|
|
|
|
got, err := delivery.ParseTargetTimeout(
|
|
strconv.Itoa(delivery.MaxTargetTimeoutSeconds),
|
|
)
|
|
require.NoError(t, err)
|
|
assert.Equal(t, delivery.MaxTargetTimeoutSeconds, got)
|
|
|
|
_, err = delivery.ParseTargetTimeout(
|
|
strconv.Itoa(delivery.MaxTargetTimeoutSeconds + 1),
|
|
)
|
|
require.Error(t, err)
|
|
}
|
|
|
|
// Control characters in a value are how a second header would be
|
|
// smuggled into the outbound request.
|
|
func TestParseTargetHeaders_RejectsControlCharactersInValues(
|
|
t *testing.T,
|
|
) {
|
|
t.Parallel()
|
|
|
|
for _, bad := range []string{
|
|
"X-A: one\x01two",
|
|
"X-A: one\ttwo",
|
|
"X-A: one\x7ftwo",
|
|
} {
|
|
_, err := delivery.ParseTargetHeaders(bad)
|
|
require.Error(t, err, bad)
|
|
}
|
|
}
|