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

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

New Issue
Closed
opened 2026-08-20 08:34:50 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-20 08:34:50 +02:00
Collaborator
Copy Link
Copy Source

Raised during the review of #242, which strips OPERATOR-CONFIGURED target headers on a redirect hop that leaves the configured origin. That PR's scope was exactly that, per #233, so this is a follow-up rather than a defect in it.

The inbound event headers the same delivery path forwards are still governed only by Go's defaults on a redirect. Go withholds Authorization and Cookie cross-host and forwards everything else, so an inbound X-Hub-Signature, X-Api-Key or similar follows a 302 to a host the operator never configured.

Both the reviewer and the implementer judged this NOT a blocker, and I agree: the headers are sender-controlled rather than operator credentials, Authorization/Cookie are already withheld, and #228 scrubs the inbound signature secret before it is stored or forwarded at all. Filing it anyway, for three reasons the reviewer gave:

  1. The asymmetry is now baked into the code and absent from the README. An operator who reads "configured headers are dropped off-origin" will reasonably infer the forwarded ones are too.
  2. "Sender-controlled" is not "harmless". Inbound webhooks routinely carry signature headers, and scrubbing a secret BEFORE STORAGE is a different property from NOT FORWARDING IT ACROSS A REDIRECT.
  3. It is cheap to close — the same CheckRedirect that #242 adds already holds the request, so the strip has somewhere obvious to live.

Definition of done:

  • decide, and record here, whether forwarded inbound headers should be stripped off-origin as well, or deliberately preserved
  • if stripped: implement it in the existing CheckRedirect, with a test asserting a cross-host 302 does not carry a forwarded inbound header
  • if preserved: the README says so explicitly, next to the sentence about configured headers, so the asymmetry is stated rather than inferred
  • either way the README must not leave a reader to guess which of the two rules applies to which headers
Raised during the review of https://git.eeqj.de/sneak/webhooker/pulls/242, which strips OPERATOR-CONFIGURED target headers on a redirect hop that leaves the configured origin. That PR's scope was exactly that, per https://git.eeqj.de/sneak/webhooker/issues/233, so this is a follow-up rather than a defect in it. The inbound event headers the same delivery path forwards are still governed only by Go's defaults on a redirect. Go withholds `Authorization` and `Cookie` cross-host and forwards everything else, so an inbound `X-Hub-Signature`, `X-Api-Key` or similar follows a 302 to a host the operator never configured. Both the reviewer and the implementer judged this NOT a blocker, and I agree: the headers are sender-controlled rather than operator credentials, `Authorization`/`Cookie` are already withheld, and https://git.eeqj.de/sneak/webhooker/pulls/228 scrubs the inbound signature secret before it is stored or forwarded at all. Filing it anyway, for three reasons the reviewer gave: 1. The asymmetry is now baked into the code and absent from the README. An operator who reads "configured headers are dropped off-origin" will reasonably infer the forwarded ones are too. 2. "Sender-controlled" is not "harmless". Inbound webhooks routinely carry signature headers, and scrubbing a secret BEFORE STORAGE is a different property from NOT FORWARDING IT ACROSS A REDIRECT. 3. It is cheap to close — the same `CheckRedirect` that https://git.eeqj.de/sneak/webhooker/pulls/242 adds already holds the request, so the strip has somewhere obvious to live. Definition of done: - decide, and record here, whether forwarded inbound headers should be stripped off-origin as well, or deliberately preserved - if stripped: implement it in the existing `CheckRedirect`, with a test asserting a cross-host 302 does not carry a forwarded inbound header - if preserved: the README says so explicitly, next to the sentence about configured headers, so the asymmetry is stated rather than inferred - either way the README must not leave a reader to guess which of the two rules applies to which headers
clawbot added this to the 1.0.0 milestone 2026-08-20 08:34:50 +02:00
clawbot commented 2026-08-20 09:55:36 +02:00
Author
Collaborator
Copy Link
Copy Source

Decision: strip. Forwarded inbound headers get the same off-origin rule as operator-configured ones. The asymmetry is not defensible to an operator reading the README, and "sender-controlled" does not make an inbound X-Hub-Signature safe to hand to a host the operator never configured.

Folded into the rework of #242 rather than dispatched separately: the strip lives in the CheckRedirect that PR adds, so a second branch would conflict with it by construction.

Implementation requirements:

  • Strip forwarded inbound headers on any hop where sameDeliveryOrigin is false, in the same CheckRedirect, using the same origin comparison — one rule, not two.
  • The set stripped must be derived from what the delivery path actually forwarded, not a hardcoded name list; a hardcoded list silently fails open on the next header added to the forward set.
  • Test: a cross-host 302 does not carry a forwarded inbound header, and a same-origin 302 still does.
  • README states one rule covering both header classes; the reader must not have to infer which applies to which.
Decision: **strip**. Forwarded inbound headers get the same off-origin rule as operator-configured ones. The asymmetry is not defensible to an operator reading the README, and "sender-controlled" does not make an inbound `X-Hub-Signature` safe to hand to a host the operator never configured. Folded into the rework of https://git.eeqj.de/sneak/webhooker/pulls/242 rather than dispatched separately: the strip lives in the `CheckRedirect` that PR adds, so a second branch would conflict with it by construction. Implementation requirements: - Strip forwarded inbound headers on any hop where `sameDeliveryOrigin` is false, in the same `CheckRedirect`, using the same origin comparison — one rule, not two. - The set stripped must be derived from what the delivery path actually forwarded, not a hardcoded name list; a hardcoded list silently fails open on the next header added to the forward set. - Test: a cross-host 302 does not carry a forwarded inbound header, and a same-origin 302 still does. - README states one rule covering both header classes; the reader must not have to infer which applies to which.
clawbot commented 2026-08-20 10:55:01 +02:00
Author
Collaborator
Copy Link
Copy Source

Landed on next in 6874059 via #242. Closing manually — the squash commit body carried Closes #243 but the merge parsed only the title.

Implemented as decided: forwarded inbound headers are stripped on any hop where sameDeliveryOrigin is false, in the same CheckRedirect and under the same origin comparison as the operator-configured ones. The stripped set is derived from what the delivery path actually forwarded — applyRequestHeaders returns the canonical names it applied on the sender's or operator's behalf — so a header added to the forward set is covered without a second edit.

Content-Type and User-Agent are excluded, being the delivery path's own rather than anyone else's: Content-Type must survive so a body preserved across a 307 stays typed, and User-Agent is overwritten before the request goes out, so the sender's never reaches the wire.

Verified independently: a cross-host 302 does not carry a forwarded X-Hub-Signature, a same-origin 302 still does, and both mutation-check clean. README states one rule covering both header classes.

Landed on `next` in `6874059` via https://git.eeqj.de/sneak/webhooker/pulls/242. Closing manually — the squash commit body carried `Closes #243` but the merge parsed only the title. Implemented as decided: forwarded inbound headers are stripped on any hop where `sameDeliveryOrigin` is false, in the same `CheckRedirect` and under the same origin comparison as the operator-configured ones. The stripped set is derived from what the delivery path actually forwarded — `applyRequestHeaders` returns the canonical names it applied on the sender's or operator's behalf — so a header added to the forward set is covered without a second edit. `Content-Type` and `User-Agent` are excluded, being the delivery path's own rather than anyone else's: `Content-Type` must survive so a body preserved across a `307` stays typed, and `User-Agent` is overwritten before the request goes out, so the sender's never reaches the wire. Verified independently: a cross-host 302 does not carry a forwarded `X-Hub-Signature`, a same-origin 302 still does, and both mutation-check clean. README states one rule covering both header classes.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#243