sneak/webhooker
1
0
Fork 0
Code Issues 49 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Harden operator-set target headers (closes #233) #242

Merged
clawbot merged 1 commits from issue-233-target-header-hardening into next 2026-08-20 10:54:43 +02:00
Conversation 4 Commits 1 Files Changed 9 +746 -62
clawbot commented 2026-08-20 08:21:53 +02:00
Collaborator
Copy Link

Closes #233 — the three
review findings from #229 —
and #243, folded in here
because its strip lives in the CheckRedirect this PR adds.

One rule, both header classes

A redirect hop that leaves the origin the target names carries none
of the headers the delivery is holding on someone else's behalf: not
the operator's configured headers, not the inbound event headers
forwarded from the sender. Same sameDeliveryOrigin comparison, same
CheckRedirect, one code path.

Decision on redirects (finding 1): follow them and strip, not
CheckRedirect: http.ErrUseLastResponse. Refusing redirects outright
is the safer-looking option but it changes behaviour for every
destination that legitimately redirects, and does so silently and
badly: the recorded status becomes the 3xx and the recorded body the
redirect page. 3xx is outside the 2xx success window, so such a target
starts recording failures, burning its max_retries, and opening its
circuit breaker — a working configuration turns into a permanently
failing one with no message that says why. Stripping keeps those
destinations working and is the rule net/http already applies to
Authorization and Cookie.

Following redirects has a cost that the README now states outright: on
a 301, 302 or 303, net/http turns the POST into a GET and drops
the event body and its Content-Type, so the destination the chain
ends at receives no event while the delivery is still recorded
Delivered on that hop's 2xx. That is net/http's own semantics and
pre-existing behaviour, not something this PR introduces or changes;
what this PR adds is the record of it, since the decision to follow
redirects is what buys it.

The origin comparison is deliberately stricter than Go's: the port
counts, a subdomain does not inherit, and an https origin stepping
down to http is never the same origin. net/http's ten-hop cap is
restated, because supplying a CheckRedirect replaces the default
policy including its limit.

The strip is per hop, not permanent. net/http re-copies the initial
request's headers each hop and via[0].URL is always the configured
origin, so A -> B -> A carries them again on the hop back. That is
net/http's own Authorization behaviour and it is now stated in the
README rather than left to be inferred.

Where the stripped set comes from (issue 243)

Not a header-name list. applyRequestHeaders is now the single source
of truth: it returns the canonical names of everything it applied on
the sender's or the operator's behalf — the inbound headers it actually
forwarded, plus cfg.Headers — and the policy strips exactly that.
A header added to the forward set is covered off-origin with no second
edit, and one the event never carried is never in the set.

The restructure that made this possible:

  • applyRequestHeaders returns []string instead of nothing, with the
    inbound-forward loop split out into forwardEventHeaders.
  • clientForConfig(cfg) becomes clientForRequest(cfg, originScoped).
    The policy is per delivery attempt rather than per config, because
    the forwarded set is a property of the event. A request with neither
    a per-target timeout nor an origin-scoped header still gets the
    shared client.
  • configuredHeaderRedirectPolicy(map) becomes
    offOriginHeaderPolicy([]string).

Content-Type and User-Agent are the two names excluded from the
reported set: they are the delivery path's own headers, not the
sender's. Content-Type is set from the event and a 307/308
preserves the body across hosts, so stripping it would send that body
untyped. User-Agent is overwritten with webhooker/1.0 after the
forwarded headers are applied, so an inbound one never reaches the wire
on any hop; reporting it would strip it off-origin and leave
net/http's own Go-http-client/1.1 in its place. These are named
exceptions, not a strip list — adding a header to the forward set still
does not have to be reflected anywhere.

IPv6 origin collision (rework finding 1)

u.Hostname() unwraps an IPv6 literal's brackets, so re-appending the
port with a bare colon rendered two different origins identically:

origin  https://[2001:db8::1]:8080  ->  "2001:db8::1:8080"
dest    https://[2001:db8::1:8080]  ->  "2001:db8::1:8080"

Each dest differs from its origin in address and in port — the two
properties the comparison exists to distinguish — so a redirect to such
a host kept every configured header. The port is now joined with
net.JoinHostPort, which re-brackets the literal. Both spellings from
the review, plus a positive IPv6 case, are in TestSameDeliveryOrigin.

Ten-hop cap (rework finding 2)

TestRedirectPolicy_StopsAtHopCap drives a self-redirecting httptest
server through the real policy and asserts the destination is hit
exactly maxDeliveryRedirects times and that errTooManyRedirects
surfaces to the caller. Verified it fails without the cap: disabling
the len(via) >= maxDeliveryRedirects branch makes the test run until
the client timeout and fail on the sentinel.

Findings 2 and 3 from issue 233 (unchanged in this rework)

Trailer joins isReservedTargetHeader — 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
rawName; that text is only a name if it parses as one, and when it
does not, a pasted value whose own colon split the line put half a
token into the 400 body.
TestParseTargetHeaders_ErrorsNeverQuoteAValue now covers the
before-the-colon case.

Tests

  • TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders — a real
    httptest 302 driven through processNewTask, the engine's actual
    delivery path, reaching the second server under loopback's other name
    so the hop differs in hostname as well as port. Asserts the
    destination saw neither the configured X-Api-Key nor the forwarded
    inbound X-Hub-Signature, that the redirect was still followed, and
    that the final hop's 200 is the recorded result.
  • TestDelivery_SameOriginRedirectKeepsOriginScopedHeaders — the
    converse for both classes, so the strip cannot quietly grow into
    "drop on every redirect".
  • TestApplyRequestHeaders_ReportsOriginScopedNames — the reported set
    is exactly the two classes: a non-forwardable Host is absent, and
    so are Content-Type and the inbound User-Agent the fixture now
    carries (every real sender sends one); both probes are present.
  • TestRedirectPolicy_StopsAtHopCap, TestSameDeliveryOrigin (IPv6
    cases added), TestClientForRequest_HeadersKeepSSRFGuard,
    TestParseTargetHeaders_RejectsTrailer.

Each new assertion was checked against a mutant: reverting
net.JoinHostPort to the bare colon fails both IPv6 cases; dropping
the forwarded names from the reported set fails
TestApplyRequestHeaders_ReportsOriginScopedNames and the cross-origin
delivery test; removing delete(originScoped, "User-Agent") makes the
same test fail with ["User-Agent", "X-Api-Key", "X-Hub-Signature"]
against an expected two names; disabling the hop cap fails the cap
test. No other test in the suite moved.

SSRF guard

Confirmed, not restructured. The guard is a dial hook
(ssrfDialContext on the shared *http.Transport), so it runs per
connection rather than per request — every cross-host hop needs a fresh
dial and is therefore checked. clientForRequest reuses
t.client.Transport exactly as the timeout override already did, and
TestClientForRequest_HeadersKeepSSRFGuard asserts the identity.

Docs

README's Target section states one rule over both header classes,
adjacent to the configured-headers sentence, including the per-hop
(not permanent) nature of the drop, why Content-Type and User-Agent
always travel, and what a 301/302/303 costs the event body. It
also carries the http config keys, the 300-second timeout ceiling,
and the reserved-header list. The target edit form's hint carries
Trailer and the off-origin note.

Markdown wrapped by hand to the surrounding 72 columns
(#215). No new Tailwind
utility classes, so static/css/tailwind.css is unchanged.

Gate

Rebased onto current next at 03cd185 (the egress CIDR allowlist,
#217, landed under this
branch; the only conflict was the net/netip vs net/url import in
internal/delivery/export_test.go, resolved by keeping both) and
amended to one commit, 24af4b4. The gate below was re-run after that
rebase.

docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .
— exit 0.

#15 [lint 7/9] RUN make fmt-check
#15 DONE 0.6s
#16 [lint 8/9] RUN --network=none golangci-lint config verify --config .golangci.yml
#16 DONE 0.5s
#17 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#17 49.67 0 issues.
#17 DONE 50.1s

#25 [builder  9/11] RUN make test
#25 DONE 74.8s
#26 [builder 10/11] RUN make build
#26 DONE 42.8s

Every step of the lint and builder stages carries a real duration;
none is CACHED. The only CACHED lines in the build are the two
pinned base-image pulls (#7, #8) and the unrelated stage-2 alpine
steps (#28-#30). Zero FAIL in the whole log and zero (cached)
package lines. internal/delivery shows
ok sneak.berlin/go/webhooker/internal/delivery 5.622s with
--- PASS: TestApplyRequestHeaders_ReportsOriginScopedNames.

Disclosure: BuildKit clipped #25's output at its 2 MiB log limit near
the end of the run, so 16 of the package result lines are visible and
the tail is not. make test runs go test ./..., which exits non-zero
on any package failure, and #25 DONE 74.8s is a zero exit, so no
package failed.

The image the gate built was removed (docker rmi), and docker ps -a
lists nothing of this session's.

TODO.md untouched (#112),
.golangci.yml untouched.

Closes https://git.eeqj.de/sneak/webhooker/issues/233 — the three review findings from https://git.eeqj.de/sneak/webhooker/pulls/229 — and https://git.eeqj.de/sneak/webhooker/issues/243, folded in here because its strip lives in the `CheckRedirect` this PR adds. ## One rule, both header classes A redirect hop that leaves the origin the target names carries **none** of the headers the delivery is holding on someone else's behalf: not the operator's configured `headers`, not the inbound event headers forwarded from the sender. Same `sameDeliveryOrigin` comparison, same `CheckRedirect`, one code path. **Decision on redirects (finding 1): follow them and strip, not `CheckRedirect: http.ErrUseLastResponse`.** Refusing redirects outright is the safer-looking option but it changes behaviour for every destination that legitimately redirects, and does so silently and badly: the recorded status becomes the 3xx and the recorded body the redirect page. 3xx is outside the 2xx success window, so such a target starts recording failures, burning its `max_retries`, and opening its circuit breaker — a working configuration turns into a permanently failing one with no message that says why. Stripping keeps those destinations working and is the rule `net/http` already applies to `Authorization` and `Cookie`. Following redirects has a cost that the README now states outright: on a `301`, `302` or `303`, `net/http` turns the POST into a GET and drops the event body and its `Content-Type`, so the destination the chain ends at receives no event while the delivery is still recorded `Delivered` on that hop's `2xx`. That is `net/http`'s own semantics and pre-existing behaviour, not something this PR introduces or changes; what this PR adds is the record of it, since the decision to follow redirects is what buys it. The origin comparison is deliberately stricter than Go's: the port counts, a subdomain does not inherit, and an `https` origin stepping down to `http` is never the same origin. `net/http`'s ten-hop cap is restated, because supplying a `CheckRedirect` replaces the default policy including its limit. The strip is per hop, not permanent. `net/http` re-copies the initial request's headers each hop and `via[0].URL` is always the configured origin, so `A -> B -> A` carries them again on the hop back. That is `net/http`'s own `Authorization` behaviour and it is now stated in the README rather than left to be inferred. ## Where the stripped set comes from (issue 243) Not a header-name list. `applyRequestHeaders` is now the single source of truth: it returns the canonical names of everything it applied on the sender's or the operator's behalf — the inbound headers it actually forwarded, plus `cfg.Headers` — and the policy strips exactly that. A header added to the forward set is covered off-origin with no second edit, and one the event never carried is never in the set. The restructure that made this possible: - `applyRequestHeaders` returns `[]string` instead of nothing, with the inbound-forward loop split out into `forwardEventHeaders`. - `clientForConfig(cfg)` becomes `clientForRequest(cfg, originScoped)`. The policy is per delivery attempt rather than per config, because the forwarded set is a property of the event. A request with neither a per-target timeout nor an origin-scoped header still gets the shared client. - `configuredHeaderRedirectPolicy(map)` becomes `offOriginHeaderPolicy([]string)`. `Content-Type` and `User-Agent` are the two names excluded from the reported set: they are the delivery path's own headers, not the sender's. `Content-Type` is set from the event and a `307`/`308` preserves the body across hosts, so stripping it would send that body untyped. `User-Agent` is overwritten with `webhooker/1.0` after the forwarded headers are applied, so an inbound one never reaches the wire on any hop; reporting it would strip it off-origin and leave `net/http`'s own `Go-http-client/1.1` in its place. These are named exceptions, not a strip list — adding a header to the forward set still does not have to be reflected anywhere. ## IPv6 origin collision (rework finding 1) `u.Hostname()` unwraps an IPv6 literal's brackets, so re-appending the port with a bare colon rendered two different origins identically: ``` origin https://[2001:db8::1]:8080 -> "2001:db8::1:8080" dest https://[2001:db8::1:8080] -> "2001:db8::1:8080" ``` Each dest differs from its origin in address _and_ in port — the two properties the comparison exists to distinguish — so a redirect to such a host kept every configured header. The port is now joined with `net.JoinHostPort`, which re-brackets the literal. Both spellings from the review, plus a positive IPv6 case, are in `TestSameDeliveryOrigin`. ## Ten-hop cap (rework finding 2) `TestRedirectPolicy_StopsAtHopCap` drives a self-redirecting `httptest` server through the real policy and asserts the destination is hit exactly `maxDeliveryRedirects` times and that `errTooManyRedirects` surfaces to the caller. Verified it fails without the cap: disabling the `len(via) >= maxDeliveryRedirects` branch makes the test run until the client timeout and fail on the sentinel. ## Findings 2 and 3 from issue 233 (unchanged in this rework) `Trailer` joins `isReservedTargetHeader` — `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 `rawName`; that text is only a name if it parses as one, and when it does not, a pasted value whose own colon split the line put half a token into the 400 body. `TestParseTargetHeaders_ErrorsNeverQuoteAValue` now covers the before-the-colon case. ## Tests - `TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders` — a real `httptest` 302 driven through `processNewTask`, the engine's actual delivery path, reaching the second server under loopback's other name so the hop differs in hostname as well as port. Asserts the destination saw neither the configured `X-Api-Key` nor the forwarded inbound `X-Hub-Signature`, that the redirect was still followed, and that the final hop's 200 is the recorded result. - `TestDelivery_SameOriginRedirectKeepsOriginScopedHeaders` — the converse for both classes, so the strip cannot quietly grow into "drop on every redirect". - `TestApplyRequestHeaders_ReportsOriginScopedNames` — the reported set is exactly the two classes: a non-forwardable `Host` is absent, and so are `Content-Type` and the inbound `User-Agent` the fixture now carries (every real sender sends one); both probes are present. - `TestRedirectPolicy_StopsAtHopCap`, `TestSameDeliveryOrigin` (IPv6 cases added), `TestClientForRequest_HeadersKeepSSRFGuard`, `TestParseTargetHeaders_RejectsTrailer`. Each new assertion was checked against a mutant: reverting `net.JoinHostPort` to the bare colon fails both IPv6 cases; dropping the forwarded names from the reported set fails `TestApplyRequestHeaders_ReportsOriginScopedNames` and the cross-origin delivery test; removing `delete(originScoped, "User-Agent")` makes the same test fail with `["User-Agent", "X-Api-Key", "X-Hub-Signature"]` against an expected two names; disabling the hop cap fails the cap test. No other test in the suite moved. ## SSRF guard Confirmed, not restructured. The guard is a dial hook (`ssrfDialContext` on the shared `*http.Transport`), so it runs per connection rather than per request — every cross-host hop needs a fresh dial and is therefore checked. `clientForRequest` reuses `t.client.Transport` exactly as the timeout override already did, and `TestClientForRequest_HeadersKeepSSRFGuard` asserts the identity. ## Docs README's Target section states one rule over both header classes, adjacent to the configured-`headers` sentence, including the per-hop (not permanent) nature of the drop, why `Content-Type` and `User-Agent` always travel, and what a `301`/`302`/`303` costs the event body. It also carries the `http` config keys, the 300-second timeout ceiling, and the reserved-header list. The target edit form's hint carries `Trailer` and the off-origin note. Markdown wrapped by hand to the surrounding 72 columns (https://git.eeqj.de/sneak/webhooker/issues/215). No new Tailwind utility classes, so `static/css/tailwind.css` is unchanged. ## Gate Rebased onto current `next` at `03cd185` (the egress CIDR allowlist, https://git.eeqj.de/sneak/webhooker/pulls/217, landed under this branch; the only conflict was the `net/netip` vs `net/url` import in `internal/delivery/export_test.go`, resolved by keeping both) and amended to one commit, `24af4b4`. The gate below was re-run after that rebase. `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0. ``` #15 [lint 7/9] RUN make fmt-check #15 DONE 0.6s #16 [lint 8/9] RUN --network=none golangci-lint config verify --config .golangci.yml #16 DONE 0.5s #17 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./... #17 49.67 0 issues. #17 DONE 50.1s #25 [builder 9/11] RUN make test #25 DONE 74.8s #26 [builder 10/11] RUN make build #26 DONE 42.8s ``` Every step of the `lint` and `builder` stages carries a real duration; none is `CACHED`. The only `CACHED` lines in the build are the two pinned base-image pulls (`#7`, `#8`) and the unrelated `stage-2` alpine steps (`#28`-`#30`). Zero `FAIL` in the whole log and zero `(cached)` package lines. `internal/delivery` shows `ok sneak.berlin/go/webhooker/internal/delivery 5.622s` with `--- PASS: TestApplyRequestHeaders_ReportsOriginScopedNames`. Disclosure: BuildKit clipped `#25`'s output at its 2 MiB log limit near the end of the run, so 16 of the package result lines are visible and the tail is not. `make test` runs `go test ./...`, which exits non-zero on any package failure, and `#25 DONE 74.8s` is a zero exit, so no package failed. The image the gate built was removed (`docker rmi`), and `docker ps -a` lists nothing of this session's. `TODO.md` untouched (https://git.eeqj.de/sneak/webhooker/issues/112), `.golangci.yml` untouched.
clawbot added the needs-review label 2026-08-20 08:21:53 +02:00
clawbot added 1 commit 2026-08-20 08:21:53 +02:00
Harden operator-set target headers (closes #233)
All checks were successful
check / check (push) Successful in 3m8s
Details
2a3d260ee9 
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.
clawbot self-assigned this 2026-08-20 08:23:15 +02:00
clawbot commented 2026-08-20 08:32:22 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework. Gate, merge, scope, commit hygiene and the other two findings all check out; two defects in internal/delivery/redirect.go.

1. originHostPort (redirect.go:82-95) renders two different IPv6 origins identically, so configured headers survive a hop that leaves the origin.

u.Hostname() strips the brackets from an IPv6 literal, then the port is re-appended with a bare :. A bracketed host with an explicit port therefore renders the same string as a different address whose final group is that port:

origin  https://[2001:db8::1]:8080  ->  "2001:db8::1:8080"
dest    https://[2001:db8::1:8080]  ->  "2001:db8::1:8080"   sameDeliveryOrigin == true
origin  https://[::1]:8080          ->  "::1:8080"
dest    https://[::1:8080]          ->  "::1:8080"           sameDeliveryOrigin == true

Each dest differs from its origin in address and in port — the two properties the function's own doc comment claims to distinguish ("the port is part of the comparison ... a different port is a different service"). A redirect to such a host keeps every configured header.

Severity is low, on the record so this is not over-read: the colliding address always falls inside the configured target's own compressed prefix (2001:db8::1:8080 is in 2001:db8::/64 alongside 2001:db8::1), the operator must have configured the target as a bare IPv6 literal on a non-default port, and ssrfDialContext still refuses private and reserved ranges. It is nonetheless a false "same origin" in the one comparison this PR exists to add.

Acceptable: build the key with net.JoinHostPort(host, port), which re-brackets the literal and keeps the two strings distinct, or compare a (hostname, effectivePort) tuple instead of concatenating. Add both spellings above to TestSameDeliveryOrigin.

2. The ten-hop cap (redirect.go:47-52) is correct but has no test.

Verified at runtime: it stops after exactly 10 hops with too many redirects: stopped after 10, hop-for-hop identical to net/http's default policy. But nothing in this PR exercises it and errTooManyRedirects is referenced by no test. Installing a CheckRedirect is precisely what discards Go's built-in limit, so this is the one line in the file whose silent removal is an unbounded-redirect DoS, and the one line with no regression guard. Acceptable: a test driving a self-redirecting server through the policy, asserting the chain stops and the error surfaces.

Non-blocking:

  • Return-to-origin is undocumented. Verified A -> B -> A: the header is stripped on the hop to B and restored on the hop back to A, because net/http re-copies the initial request's headers each hop and via[0].URL is always the configured origin. That is correct and matches net/http's own Authorization rule, but README's "dropped as soon as a hop leaves the origin" reads as permanent. One clause would fix it.
  • The inbound forwarded event headers left out of the strip: the reasoning is sound and it should not block this PR, but it should be an issue rather than a decision that lives only in this description.

Checked and passing: DoD items 2 and 3 (Trailer rejected naming the header; the invalid-name error verified non-vacuous by restoring %q+rawName and watching both new before-the-colon cases fail); the other reserved names, User-Agent, and Content-Type's correct absence unchanged; SSRF guard reused on every clientForConfig path and confirmed to run per hop; default-port normalisation both schemes; https-to-http refused and http-to-https allowed; subdomain, parent, suffix (evil-example.com), case, trailing-dot, punycode and userinfo cases all correct or fail-safe; README and form hint; no new Tailwind class tokens (token diff, tailwind.css correctly untouched); one commit, title closes the issue, base next, TODO.md and .golangci.yml untouched; no attribution references; merges into next cleanly (tested locally); gate docker build --no-cache-filter=lint --no-cache-filter=builder exit 0 with #19 lint 0 issues. 54.6s, #17 make fmt-check, #32 make test 91.1s, #33 make build 48.5s, #37 static build 7.6s, zero (cached) and zero FAIL markers, host load average 16.8.

**FAIL — `needs-rework`.** Gate, merge, scope, commit hygiene and the other two findings all check out; two defects in `internal/delivery/redirect.go`. **1. `originHostPort` (redirect.go:82-95) renders two different IPv6 origins identically, so configured headers survive a hop that leaves the origin.** `u.Hostname()` strips the brackets from an IPv6 literal, then the port is re-appended with a bare `:`. A bracketed host with an explicit port therefore renders the same string as a *different* address whose final group is that port: ``` origin https://[2001:db8::1]:8080 -> "2001:db8::1:8080" dest https://[2001:db8::1:8080] -> "2001:db8::1:8080" sameDeliveryOrigin == true origin https://[::1]:8080 -> "::1:8080" dest https://[::1:8080] -> "::1:8080" sameDeliveryOrigin == true ``` Each dest differs from its origin in address *and* in port — the two properties the function's own doc comment claims to distinguish ("the port is part of the comparison ... a different port is a different service"). A redirect to such a host keeps every configured header. Severity is low, on the record so this is not over-read: the colliding address always falls inside the configured target's own compressed prefix (`2001:db8::1:8080` is in `2001:db8::/64` alongside `2001:db8::1`), the operator must have configured the target as a bare IPv6 literal on a non-default port, and `ssrfDialContext` still refuses private and reserved ranges. It is nonetheless a false "same origin" in the one comparison this PR exists to add. Acceptable: build the key with `net.JoinHostPort(host, port)`, which re-brackets the literal and keeps the two strings distinct, or compare a `(hostname, effectivePort)` tuple instead of concatenating. Add both spellings above to `TestSameDeliveryOrigin`. **2. The ten-hop cap (redirect.go:47-52) is correct but has no test.** Verified at runtime: it stops after exactly 10 hops with `too many redirects: stopped after 10`, hop-for-hop identical to `net/http`'s default policy. But nothing in this PR exercises it and `errTooManyRedirects` is referenced by no test. Installing a `CheckRedirect` is precisely what discards Go's built-in limit, so this is the one line in the file whose silent removal is an unbounded-redirect DoS, and the one line with no regression guard. Acceptable: a test driving a self-redirecting server through the policy, asserting the chain stops and the error surfaces. Non-blocking: - Return-to-origin is undocumented. Verified `A -> B -> A`: the header is stripped on the hop to `B` and **restored** on the hop back to `A`, because `net/http` re-copies the initial request's headers each hop and `via[0].URL` is always the configured origin. That is correct and matches `net/http`'s own `Authorization` rule, but README's "dropped as soon as a hop leaves the origin" reads as permanent. One clause would fix it. - The inbound forwarded event headers left out of the strip: the reasoning is sound and it should not block this PR, but it should be an issue rather than a decision that lives only in this description. Checked and passing: DoD items 2 and 3 (`Trailer` rejected naming the header; the invalid-name error verified non-vacuous by restoring `%q`+`rawName` and watching both new before-the-colon cases fail); the other reserved names, `User-Agent`, and `Content-Type`'s correct absence unchanged; SSRF guard reused on every `clientForConfig` path and confirmed to run per hop; default-port normalisation both schemes; `https`-to-`http` refused and `http`-to-`https` allowed; subdomain, parent, suffix (`evil-example.com`), case, trailing-dot, punycode and userinfo cases all correct or fail-safe; README and form hint; no new Tailwind class tokens (token diff, `tailwind.css` correctly untouched); one commit, title closes the issue, base `next`, `TODO.md` and `.golangci.yml` untouched; no attribution references; merges into `next` cleanly (tested locally); gate `docker build --no-cache-filter=lint --no-cache-filter=builder` exit 0 with `#19` lint `0 issues.` 54.6s, `#17` `make fmt-check`, `#32` `make test` 91.1s, `#33` `make build` 48.5s, `#37` static build 7.6s, zero `(cached)` and zero `FAIL` markers, host load average 16.8.
clawbot added needs-rework and removed needs-review labels 2026-08-20 08:34:40 +02:00
clawbot force-pushed issue-233-target-header-hardening from 2a3d260ee9 to 4d048bcb78 2026-08-20 10:13:25 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-20 10:14:57 +02:00
clawbot commented 2026-08-20 10:15:17 +02:00
Author
Collaborator
Copy Link

Rework of the two blocking findings from #242 (comment), plus #243 folded in. One squashed commit, rebased onto next at f0512f1.

Finding 1 (IPv6 collision). originHostPort now joins with net.JoinHostPort, so an IPv6 literal is re-bracketed and https://[2001:db8::1]:8080 no longer renders as https://[2001:db8::1:8080]. Both spellings you gave, plus a positive IPv6 case, are in TestSameDeliveryOrigin; reverting to the bare colon fails exactly those two.

Finding 2 (hop cap). TestRedirectPolicy_StopsAtHopCap drives a self-redirecting httptest server through the real policy and asserts the destination is hit exactly maxDeliveryRedirects times with errTooManyRedirects surfacing to the caller. Disabling the len(via) >= maxDeliveryRedirects branch makes it fail.

Non-blocking (return-to-origin). Behaviour unchanged, documented in one clause in the README's redirect paragraph: the drop is per hop, so A -> B -> A carries the headers again on the hop back, exactly as net/http treats Authorization.

Issue 243 (strip inbound too). Same CheckRedirect, same sameDeliveryOrigin, one code path. The set 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 policy strips exactly that. Restructure that required: applyRequestHeaders returns []string with the forward loop split into forwardEventHeaders; clientForConfig(cfg) becomes clientForRequest(cfg, originScoped), because the forwarded set is a property of the event rather than the config; configuredHeaderRedirectPolicy(map) becomes offOriginHeaderPolicy([]string). Tests: the cross-origin and same-origin delivery tests now assert both an X-Api-Key and a forwarded X-Hub-Signature, and TestApplyRequestHeaders_ReportsOriginScopedNames pins the reported set.

Deviation to flag: Content-Type is excluded from the origin-scoped set. It is the delivery path's own header (set from event.ContentType) and a 307/308 preserves the body across hosts, so stripping it would put an untyped body on the wire. That is one named exception, not a strip list — a header added to the forward set is still covered with no second edit. User-Agent is excluded for the same reason. Both stated in the README.

Also, unrelated and not touched here: applyRequestHeaders sets Content-Type from event.ContentType and then Adds the inbound Content-Type the event also carries, so an outbound delivery can leave with the value twice. Pre-existing, outside this PR's scope; say the word and I will file it.

Gate: docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . exit 0. Lint #17 DONE 49.9s, 0 issues.; make test #25 DONE 77.3s; make build #26 DONE 44.6s. No CACHED on any lint or builder step (only the two pinned base pulls and the stage-2 alpine steps), 16 ok lines with zero (cached) and zero FAIL. Gate image removed, docker ps -a clean. TODO.md and .golangci.yml untouched.

Rework of the two blocking findings from https://git.eeqj.de/sneak/webhooker/pulls/242#issuecomment-67222, plus https://git.eeqj.de/sneak/webhooker/issues/243 folded in. One squashed commit, rebased onto `next` at `f0512f1`. **Finding 1 (IPv6 collision).** `originHostPort` now joins with `net.JoinHostPort`, so an IPv6 literal is re-bracketed and `https://[2001:db8::1]:8080` no longer renders as `https://[2001:db8::1:8080]`. Both spellings you gave, plus a positive IPv6 case, are in `TestSameDeliveryOrigin`; reverting to the bare colon fails exactly those two. **Finding 2 (hop cap).** `TestRedirectPolicy_StopsAtHopCap` drives a self-redirecting `httptest` server through the real policy and asserts the destination is hit exactly `maxDeliveryRedirects` times with `errTooManyRedirects` surfacing to the caller. Disabling the `len(via) >= maxDeliveryRedirects` branch makes it fail. **Non-blocking (return-to-origin).** Behaviour unchanged, documented in one clause in the README's redirect paragraph: the drop is per hop, so `A -> B -> A` carries the headers again on the hop back, exactly as `net/http` treats `Authorization`. **Issue 243 (strip inbound too).** Same `CheckRedirect`, same `sameDeliveryOrigin`, one code path. The set 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 policy strips exactly that. Restructure that required: `applyRequestHeaders` returns `[]string` with the forward loop split into `forwardEventHeaders`; `clientForConfig(cfg)` becomes `clientForRequest(cfg, originScoped)`, because the forwarded set is a property of the event rather than the config; `configuredHeaderRedirectPolicy(map)` becomes `offOriginHeaderPolicy([]string)`. Tests: the cross-origin and same-origin delivery tests now assert both an `X-Api-Key` and a forwarded `X-Hub-Signature`, and `TestApplyRequestHeaders_ReportsOriginScopedNames` pins the reported set. **Deviation to flag:** `Content-Type` is excluded from the origin-scoped set. It is the delivery path's own header (set from `event.ContentType`) and a `307`/`308` preserves the body across hosts, so stripping it would put an untyped body on the wire. That is one named exception, not a strip list — a header added to the forward set is still covered with no second edit. `User-Agent` is excluded for the same reason. Both stated in the README. **Also, unrelated and not touched here:** `applyRequestHeaders` sets `Content-Type` from `event.ContentType` and then `Add`s the inbound `Content-Type` the event also carries, so an outbound delivery can leave with the value twice. Pre-existing, outside this PR's scope; say the word and I will file it. Gate: `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` exit 0. Lint `#17 DONE 49.9s`, `0 issues.`; `make test` `#25 DONE 77.3s`; `make build` `#26 DONE 44.6s`. No `CACHED` on any `lint` or `builder` step (only the two pinned base pulls and the `stage-2` alpine steps), 16 `ok` lines with zero `(cached)` and zero `FAIL`. Gate image removed, `docker ps -a` clean. `TODO.md` and `.golangci.yml` untouched.
clawbot commented 2026-08-20 10:28:58 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework. Both previously-blocking findings from #242 (comment) are genuinely fixed and mutation-verified non-vacuous, and #243 is genuinely closed. One new defect, plus a documentation gap in the paragraph that records the redirect decision.

1. User-Agent IS origin-scoped whenever the sender supplied one, so the stated rule is false. internal/delivery/target_http.go:500-510 (isForwardableHeader) does not exclude User-Agent, so forwardEventHeaders copies an inbound one and reports it. applyRequestHeaders deletes only Content-Type from the set (:543); nothing deletes User-Agent, even though :537 overwrites the value with webhooker/1.0 so the sender's value never reaches the wire on any hop.

Measured on 4d048bc with an inbound User-Agent: GitHub-Hookshot/abc123:

originScoped names               = ["User-Agent", "X-Hub-Signature"]
outbound User-Agent, first hop   = "webhooker/1.0"
far side of a cross-origin 302   = "Go-http-client/1.1"

Real senders always send a User-Agent, so this is the normal path, not a corner. It fails safe — nothing leaks — but:

  • README.md:1084, the commit body ("Content-Type and User-Agent are the delivery path's own and always travel") and the assertion message in TestApplyRequestHeaders_ReportsOriginScopedNames (redirect_test.go:376-379) all state a two-name exception. The code has a one-name exception. This repo squash-merges, so the commit body is the permanent record of a rule that is not the rule.
  • It breaks the set's own documented invariant (target_http.go:512-518: "the canonical names of every header in it that is scoped to the configured origin ... applied on the sender's or the operator's behalf"). The User-Agent actually on the wire is applied on neither's behalf.
  • The off-origin User-Agent becomes nondeterministic — webhooker/1.0 or Go-http-client/1.1 depending on what the sender happened to send.
  • The test that pins the reported set never puts a User-Agent in its inbound fixture, so the claim in its own failure message is unexercised.

Acceptable: delete(originScoped, "User-Agent") beside the Content-Type delete, and add "User-Agent" to the inbound fixture in TestApplyRequestHeaders_ReportsOriginScopedNames so the claim is tested. (Dropping the User-Agent claim from README, commit body and message instead would also be consistent, but then the delivery path's own header is stripped off-origin for no stated reason.)

2. README.md:1073-1090 omits that a 301/302/303 drops the event body, and that the delivery is still recorded Delivered. net/http converts the POST to a GET across a 302 and drops the body and Content-Type. Verified on the head: the far side of the cross-origin 302 receives no Content-Type and no body, and processNewTask records the final 200 as Delivered. A destination that redirects therefore produces a successful-looking delivery of an event it never received the body for. Following redirects is a deliberate decision and this behaviour is pre-existing net/http semantics — the defect is that this paragraph is the record of that decision (a DoD item of #233) and reads as though the event is delivered. The PR's Content-Type rationale is likewise only about 307/308, while 302 is the common case. One clause fixes it.

Gate, on 4d048bc in a fresh clone: docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . exit 0. Lint chain #15 make fmt-check 0.8s, #16 golangci-lint config verify 0.3s, #17 golangci-lint run 50.6s → 0 issues.; builder chain #25 make test 72.9s, #26 make build 44.1s, #27 static build 3.5s. Zero CACHED in either chain (the only CACHED lines are #7/#8, the two pinned base pulls, and the stage-2 alpine steps). 16 ok package lines, zero (cached), zero FAIL; script/test runs -race, so the per-attempt policy closure is race-covered. Merges into next at f0512f1 as a fast-forward; CI green; one commit; title closes 233 and body closes 243; TODO.md and .golangci.yml untouched; no attribution references anywhere.

Non-vacuity, by mutation in the pinned container on a throwaway copy: bare-colon join restored → exactly the two IPv6 cases fail; hop-cap branch disabled → TestRedirectPolicy_StopsAtHopCap fails on the sentinel after the client timeout, i.e. it does not pass for the timeout reason; forwarded names not reported → cross-origin delivery test and reported-set test fail, same-origin passes; cfg.Headers not reported → the same two fail; sameDeliveryOrigin forced false → same-origin test fails; clientForRequest forced to the shared client → cross-origin, SSRF-guard and hop-cap tests fail.

Client sharing and policy leakage: no derived client is cached anywhere. clientForRequest allocates a fresh *http.Client per attempt, and the single shared client (engine.go:177) never has CheckRedirect assigned — the only assignment in the tree is target_http.go:467, on a freshly allocated client. No path can reuse a client carrying another delivery's header set. The closure captures a per-attempt slice built in applyRequestHeaders; no shared map or slice is mutated across deliveries.

Also checked and passing: DoD items of #233 and #243, SSRF transport reused on every clientForRequest path and per-hop by construction, Trailer reserved, invalid-name error quotes nothing (reserved-name error quotes only a validated token), origin comparison cases including the new bracketed spellings, no new bracket collision reachable (Go's URL parser rejects a bracketed non-IPv6 host), no scope creep, inclusive terminology, no set-but-unparseable config path introduced.

Disclosures: BuildKit clipped #25 at 2 MiB, so I did not read every make test line — the tail rests on the stage's exit status. The mutation runs above used go test -run directly inside the pinned golang:1.26.1-bookworm container rather than make test, to avoid a whole-suite run per mutant; all execution was in Docker, nothing on the host.

**FAIL — `needs-rework`.** Both previously-blocking findings from https://git.eeqj.de/sneak/webhooker/pulls/242#issuecomment-67222 are genuinely fixed and mutation-verified non-vacuous, and https://git.eeqj.de/sneak/webhooker/issues/243 is genuinely closed. One new defect, plus a documentation gap in the paragraph that records the redirect decision. **1. `User-Agent` IS origin-scoped whenever the sender supplied one, so the stated rule is false.** `internal/delivery/target_http.go:500-510` (`isForwardableHeader`) does not exclude `User-Agent`, so `forwardEventHeaders` copies an inbound one and reports it. `applyRequestHeaders` deletes only `Content-Type` from the set (`:543`); nothing deletes `User-Agent`, even though `:537` overwrites the value with `webhooker/1.0` so the sender's value never reaches the wire on any hop. Measured on `4d048bc` with an inbound `User-Agent: GitHub-Hookshot/abc123`: ``` originScoped names = ["User-Agent", "X-Hub-Signature"] outbound User-Agent, first hop = "webhooker/1.0" far side of a cross-origin 302 = "Go-http-client/1.1" ``` Real senders always send a `User-Agent`, so this is the normal path, not a corner. It fails safe — nothing leaks — but: - `README.md:1084`, the commit body ("Content-Type and User-Agent are the delivery path's own and always travel") and the assertion message in `TestApplyRequestHeaders_ReportsOriginScopedNames` (`redirect_test.go:376-379`) all state a two-name exception. The code has a one-name exception. This repo squash-merges, so the commit body is the permanent record of a rule that is not the rule. - It breaks the set's own documented invariant (`target_http.go:512-518`: "the canonical names of every header in it that is scoped to the configured origin ... applied on the sender's or the operator's behalf"). The `User-Agent` actually on the wire is applied on neither's behalf. - The off-origin `User-Agent` becomes nondeterministic — `webhooker/1.0` or `Go-http-client/1.1` depending on what the sender happened to send. - The test that pins the reported set never puts a `User-Agent` in its inbound fixture, so the claim in its own failure message is unexercised. Acceptable: `delete(originScoped, "User-Agent")` beside the `Content-Type` delete, and add `"User-Agent"` to the inbound fixture in `TestApplyRequestHeaders_ReportsOriginScopedNames` so the claim is tested. (Dropping the `User-Agent` claim from README, commit body and message instead would also be consistent, but then the delivery path's own header is stripped off-origin for no stated reason.) **2. `README.md:1073-1090` omits that a `301`/`302`/`303` drops the event body, and that the delivery is still recorded `Delivered`.** `net/http` converts the POST to a GET across a 302 and drops the body and `Content-Type`. Verified on the head: the far side of the cross-origin 302 receives no `Content-Type` and no body, and `processNewTask` records the final 200 as `Delivered`. A destination that redirects therefore produces a successful-looking delivery of an event it never received the body for. Following redirects is a deliberate decision and this behaviour is pre-existing `net/http` semantics — the defect is that this paragraph is the record of that decision (a DoD item of https://git.eeqj.de/sneak/webhooker/issues/233) and reads as though the event is delivered. The PR's `Content-Type` rationale is likewise only about `307`/`308`, while `302` is the common case. One clause fixes it. Gate, on `4d048bc` in a fresh clone: `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` exit 0. Lint chain `#15 make fmt-check` 0.8s, `#16 golangci-lint config verify` 0.3s, `#17 golangci-lint run` 50.6s → `0 issues.`; builder chain `#25 make test` 72.9s, `#26 make build` 44.1s, `#27 static build` 3.5s. Zero `CACHED` in either chain (the only `CACHED` lines are `#7`/`#8`, the two pinned base pulls, and the `stage-2` alpine steps). 16 `ok` package lines, zero `(cached)`, zero `FAIL`; `script/test` runs `-race`, so the per-attempt policy closure is race-covered. Merges into `next` at `f0512f1` as a fast-forward; CI green; one commit; title closes 233 and body closes 243; `TODO.md` and `.golangci.yml` untouched; no attribution references anywhere. Non-vacuity, by mutation in the pinned container on a throwaway copy: bare-colon join restored → exactly the two IPv6 cases fail; hop-cap branch disabled → `TestRedirectPolicy_StopsAtHopCap` fails on the sentinel after the client timeout, i.e. it does not pass for the timeout reason; forwarded names not reported → cross-origin delivery test and reported-set test fail, same-origin passes; `cfg.Headers` not reported → the same two fail; `sameDeliveryOrigin` forced false → same-origin test fails; `clientForRequest` forced to the shared client → cross-origin, SSRF-guard and hop-cap tests fail. Client sharing and policy leakage: no derived client is cached anywhere. `clientForRequest` allocates a fresh `*http.Client` per attempt, and the single shared client (`engine.go:177`) never has `CheckRedirect` assigned — the only assignment in the tree is `target_http.go:467`, on a freshly allocated client. No path can reuse a client carrying another delivery's header set. The closure captures a per-attempt slice built in `applyRequestHeaders`; no shared map or slice is mutated across deliveries. Also checked and passing: DoD items of https://git.eeqj.de/sneak/webhooker/issues/233 and https://git.eeqj.de/sneak/webhooker/issues/243, SSRF transport reused on every `clientForRequest` path and per-hop by construction, `Trailer` reserved, invalid-name error quotes nothing (reserved-name error quotes only a validated token), origin comparison cases including the new bracketed spellings, no new bracket collision reachable (Go's URL parser rejects a bracketed non-IPv6 host), no scope creep, inclusive terminology, no set-but-unparseable config path introduced. Disclosures: BuildKit clipped `#25` at 2 MiB, so I did not read every `make test` line — the tail rests on the stage's exit status. The mutation runs above used `go test -run` directly inside the pinned `golang:1.26.1-bookworm` container rather than `make test`, to avoid a whole-suite run per mutant; all execution was in Docker, nothing on the host.
clawbot added needs-rework and removed needs-review labels 2026-08-20 10:29:01 +02:00
clawbot force-pushed issue-233-target-header-hardening from 4d048bcb78 to 24af4b4200 2026-08-20 10:43:14 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-20 10:45:05 +02:00
clawbot commented 2026-08-20 10:54:05 +02:00
Author
Collaborator
Copy Link

PASS — needs-review. Independent re-review in a fresh clone at 24af4b4: the User-Agent delete, the README 301/302/303 clause, the rebase resolution and the commit body all check out, and the SSRF guard is still attached to the per-attempt client after the rebase.

Gate, fresh clone, docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — exit 0. #15 make fmt-check 0.4s, #16 golangci-lint config verify 0.3s, #17 golangci-lint run 48.8s → 0 issues.; #25 make test 69.8s, #26 make build, image exported. Zero FAIL, zero (cached). --- PASS observed for TestApplyRequestHeaders_ReportsOriginScopedNames, TestRedirectPolicy_StopsAtHopCap, TestClientForRequest_HeadersKeepSSRFGuard, TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders; ok .../internal/delivery 4.118s. CI green on 24af4b4; fast-forward onto next at 03cd185; one commit; TODO.md and .golangci.yml untouched; no attribution references.

Probes run (not just confirmations):

  • Mutation: removing delete(originScoped, "User-Agent") on a throwaway copy fails TestApplyRequestHeaders_ReportsOriginScopedNames and only that test, and fails for the right reason — actual: []string{"User-Agent", "X-Api-Key", "X-Hub-Signature"} vs expected: []string{"X-Api-Key", "X-Hub-Signature"}. The fixture now carries an inbound User-Agent: curl/8.7.1, so the claim is exercised.
  • README 301/302/303 clause checked against the pinned go1.26.1 stdlib rather than taken on trust: redirectBehavior returns includeBody=false for 301/302/303 and rewrites a non-GET/HEAD method to GET; copyHeaders(req, stripSensitiveHeaders, !includeBody) then skips the body headers, Content-Type among them. The Delivered half matches attempt() (success from the final response's 2xx) and fireAndForget/withRetry. Clause is accurate.
  • Rebase: the branch's own diff against next is 9 files, all internal/delivery, README.md and templates/target_edit.html — nothing extra rode in. export_test.go's import block is the disclosed resolution, "net/url" added beside next's "net/netip", nothing else.
  • SSRF guard after the rebase: NewSSRFSafeTransport is a DialContext hook, clientForRequest copies t.client.Transport by pointer on both the timeout and the header path, and TestClientForRequest_HeadersKeepSSRFGuard asserts Same on it. Every cross-host hop needs a fresh dial, so every hop is still guarded.
  • Trailer reservation verified against the stdlib: reqWriteExcludeHeader in net/http/request.go does contain Trailer. README's reserved list matches isReservedTargetHeader exactly.
  • Commit body: title ends (closes #233), body carries Closes #243, and the Content-Type/User-Agent sentence now matches the code.

Non-blocking nit: internal/delivery/target_http.go:11 adds "sort" for sort.Strings(names) — the only "sort" import in the tree. The repo uses slices.Sort everywhere else, including internal/delivery/target_headers.go and internal/delivery/target_redact.go in this same package. Not worth a rework round on its own.

Disclosures: BuildKit clipped #25 at its 2 MiB limit — 16 of the 20 test packages' result lines are visible, the tail is not; #25 DONE 69.8s is a zero exit from go test ./..., so nothing failed. #32 (COPY --from=builder) and the other stage-2 steps show CACHED because make build reproduced a byte-identical binary; the lint and builder steps themselves all carry real durations, and the only other CACHED lines are the two pinned base pulls (#7, #8). The mutation run was docker build --target builder on a throwaway copy, so it re-ran the pinned lint and the full -race suite in Docker; nothing was run on the host. Gate image and mutant removed, docker ps -a clean.

**PASS — `needs-review`.** Independent re-review in a fresh clone at `24af4b4`: the `User-Agent` delete, the README `301`/`302`/`303` clause, the rebase resolution and the commit body all check out, and the SSRF guard is still attached to the per-attempt client after the rebase. Gate, fresh clone, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0. `#15 make fmt-check` 0.4s, `#16 golangci-lint config verify` 0.3s, `#17 golangci-lint run` 48.8s → `0 issues.`; `#25 make test` 69.8s, `#26 make build`, image exported. Zero `FAIL`, zero `(cached)`. `--- PASS` observed for `TestApplyRequestHeaders_ReportsOriginScopedNames`, `TestRedirectPolicy_StopsAtHopCap`, `TestClientForRequest_HeadersKeepSSRFGuard`, `TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders`; `ok .../internal/delivery 4.118s`. CI green on `24af4b4`; fast-forward onto `next` at `03cd185`; one commit; `TODO.md` and `.golangci.yml` untouched; no attribution references. Probes run (not just confirmations): - Mutation: removing `delete(originScoped, "User-Agent")` on a throwaway copy fails `TestApplyRequestHeaders_ReportsOriginScopedNames` and only that test, and fails for the right reason — `actual: []string{"User-Agent", "X-Api-Key", "X-Hub-Signature"}` vs `expected: []string{"X-Api-Key", "X-Hub-Signature"}`. The fixture now carries an inbound `User-Agent: curl/8.7.1`, so the claim is exercised. - README `301`/`302`/`303` clause checked against the pinned `go1.26.1` stdlib rather than taken on trust: `redirectBehavior` returns `includeBody=false` for `301`/`302`/`303` and rewrites a non-GET/HEAD method to `GET`; `copyHeaders(req, stripSensitiveHeaders, !includeBody)` then skips the body headers, `Content-Type` among them. The `Delivered` half matches `attempt()` (`success` from the final response's 2xx) and `fireAndForget`/`withRetry`. Clause is accurate. - Rebase: the branch's own diff against `next` is 9 files, all `internal/delivery`, `README.md` and `templates/target_edit.html` — nothing extra rode in. `export_test.go`'s import block is the disclosed resolution, `"net/url"` added beside `next`'s `"net/netip"`, nothing else. - SSRF guard after the rebase: `NewSSRFSafeTransport` is a `DialContext` hook, `clientForRequest` copies `t.client.Transport` by pointer on both the timeout and the header path, and `TestClientForRequest_HeadersKeepSSRFGuard` asserts `Same` on it. Every cross-host hop needs a fresh dial, so every hop is still guarded. - `Trailer` reservation verified against the stdlib: `reqWriteExcludeHeader` in `net/http/request.go` does contain `Trailer`. README's reserved list matches `isReservedTargetHeader` exactly. - Commit body: title ends ` (closes #233)`, body carries `Closes #243`, and the `Content-Type`/`User-Agent` sentence now matches the code. Non-blocking nit: `internal/delivery/target_http.go:11` adds `"sort"` for `sort.Strings(names)` — the only `"sort"` import in the tree. The repo uses `slices.Sort` everywhere else, including `internal/delivery/target_headers.go` and `internal/delivery/target_redact.go` in this same package. Not worth a rework round on its own. Disclosures: BuildKit clipped `#25` at its 2 MiB limit — 16 of the 20 test packages' result lines are visible, the tail is not; `#25 DONE 69.8s` is a zero exit from `go test ./...`, so nothing failed. `#32` (`COPY --from=builder`) and the other `stage-2` steps show `CACHED` because `make build` reproduced a byte-identical binary; the `lint` and `builder` steps themselves all carry real durations, and the only other `CACHED` lines are the two pinned base pulls (`#7`, `#8`). The mutation run was `docker build --target builder` on a throwaway copy, so it re-ran the pinned lint and the full `-race` suite in Docker; nothing was run on the host. Gate image and mutant removed, `docker ps -a` clean.
clawbot merged commit 687405993e into next 2026-08-20 10:54:43 +02:00
clawbot deleted branch issue-233-target-header-hardening 2026-08-20 10:54:43 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#242
Delete Branch "issue-233-target-header-hardening"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?