Operator-set target headers: credential headers survive a cross-host redirect, Trailer is not reserved, and an invalid header name error can quote a secret #233

New Issue

2026-08-20T07:24:32+02:00

clawbot commented

2026-08-20 07:24:32 +02:00

Three findings from the review of #229, which shipped the first way to configure per-target request headers. None blocked that merge; all three are small and belong to the feature it introduced.

Milestoned to 1.0.0 because the first item can leak an operator credential to a third party, and the feature that creates the surface is itself part of 1.0.

1. Credential headers survive a cross-host redirect. internal/delivery/engine.go:169 builds the delivery client with no CheckRedirect. Go's default strips Authorization and Cookie on a cross-host redirect but forwards everything else — so X-Api-Key, PRIVATE-TOKEN, X-Auth-Token and friends follow a 302 to a host the operator never configured. The client predates #229, but before it there was no way to set such a header at all, so the surface is new.

Exploitability is low: it needs an open redirect at a destination the operator already trusted, and the dial-time SSRF guard still blocks private hosts. It is a real credential egress path nonetheless.

This one needs a DECISION, not a reflex. CheckRedirect: http.ErrUseLastResponse stops following redirects entirely, which is a behaviour change for any destination that legitimately redirects. The alternative is to follow but strip configured headers on a cross-host hop. Pick one, say why in the PR, and document it.

2. Trailer is missing from isReservedTargetHeader (internal/delivery/target_headers.go:59-71). Go's reqWriteExcludeHeader (net/http/request.go:99-105) drops Trailer from the written request, so an operator can set it, it is accepted and stored, and it provably never reaches the wire — exactly the silently-ignored-header failure the rejection list exists to prevent. The other four names plus User-Agent were verified correct against applyRequestHeaders, and Content-Type is correctly NOT reserved because cfg.Headers is applied after it and wins. One-line fix.

3. A stated safety property does not hold. internal/delivery/target_headers.go:113-124 — the invalid-header-name error quotes rawName, which is the text before the FIRST colon. For input X-Auth-Token abc:def, rawName is X-Auth-Token abc and the 400 body renders header name must be a valid HTTP token: "X-Auth-Token abc". TestParseTargetHeaders_ErrorsNeverQuoteAValue misses it because it puts the secret AFTER the colon.

Exposure is negligible — the operator's own submission, in their own browser, not logged, stored or sent to Sentry. But "no error quotes a header value" is an asserted invariant with a test named after it, and the invariant is false. Fix the error or fix the claim.

Definition of done:

a decision recorded on redirect handling, implemented, with a test asserting a cross-host 302 does not carry a configured credential header
Trailer rejected, with a test asserting a Trailer: line is refused
the invalid-name error cannot echo any part of a header value, and TestParseTargetHeaders_ErrorsNeverQuoteAValue is extended to the before-the-colon case so it would have caught this
README's Target section documents the reserved-header list and the 300s timeout ceiling, neither of which is currently written down

Three findings from the review of https://git.eeqj.de/sneak/webhooker/pulls/229, which shipped the first way to configure per-target request headers. None blocked that merge; all three are small and belong to the feature it introduced. Milestoned to `1.0.0` because the first item can leak an operator credential to a third party, and the feature that creates the surface is itself part of 1.0. **1. Credential headers survive a cross-host redirect.** `internal/delivery/engine.go:169` builds the delivery client with no `CheckRedirect`. Go's default strips `Authorization` and `Cookie` on a cross-host redirect but forwards everything else — so `X-Api-Key`, `PRIVATE-TOKEN`, `X-Auth-Token` and friends follow a 302 to a host the operator never configured. The client predates https://git.eeqj.de/sneak/webhooker/pulls/229, but before it there was no way to set such a header at all, so the surface is new. Exploitability is low: it needs an open redirect at a destination the operator already trusted, and the dial-time SSRF guard still blocks private hosts. It is a real credential egress path nonetheless. This one needs a DECISION, not a reflex. `CheckRedirect: http.ErrUseLastResponse` stops following redirects entirely, which is a behaviour change for any destination that legitimately redirects. The alternative is to follow but strip configured headers on a cross-host hop. Pick one, say why in the PR, and document it. **2. `Trailer` is missing from `isReservedTargetHeader`** (`internal/delivery/target_headers.go:59-71`). Go's `reqWriteExcludeHeader` (`net/http/request.go:99-105`) drops `Trailer` from the written request, so an operator can set it, it is accepted and stored, and it provably never reaches the wire — exactly the silently-ignored-header failure the rejection list exists to prevent. The other four names plus `User-Agent` were verified correct against `applyRequestHeaders`, and `Content-Type` is correctly NOT reserved because `cfg.Headers` is applied after it and wins. One-line fix. **3. A stated safety property does not hold.** `internal/delivery/target_headers.go:113-124` — the invalid-header-name error quotes `rawName`, which is the text before the FIRST colon. For input `X-Auth-Token abc:def`, `rawName` is `X-Auth-Token abc` and the 400 body renders `header name must be a valid HTTP token: "X-Auth-Token abc"`. `TestParseTargetHeaders_ErrorsNeverQuoteAValue` misses it because it puts the secret AFTER the colon. Exposure is negligible — the operator's own submission, in their own browser, not logged, stored or sent to Sentry. But "no error quotes a header value" is an asserted invariant with a test named after it, and the invariant is false. Fix the error or fix the claim. Definition of done: - a decision recorded on redirect handling, implemented, with a test asserting a cross-host 302 does not carry a configured credential header - `Trailer` rejected, with a test asserting a `Trailer:` line is refused - the invalid-name error cannot echo any part of a header value, and `TestParseTargetHeaders_ErrorsNeverQuoteAValue` is extended to the before-the-colon case so it would have caught this - README's Target section documents the reserved-header list and the 300s timeout ceiling, neither of which is currently written down

clawbot added this to the 1.0.0 milestone 2026-08-20 07:24:32 +02:00

clawbot referenced this issue from a commit

2026-08-20 08:21:13 +02:00

Harden operator-set target headers (closes #233)

clawbot referenced a pull request that will close this issue

2026-08-20 08:21:53 +02:00

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

clawbot referenced this issue

2026-08-20 08:34:50 +02:00

Forwarded inbound event headers are not stripped on a cross-origin redirect, unlike operator-configured ones #243

clawbot referenced this issue from a commit

2026-08-20 10:13:25 +02:00