Forwarded inbound event headers are not stripped on a cross-origin redirect, unlike operator-configured ones #243
Reference in New Issue
Block a user
Delete Branch "%!s()"
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?
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
AuthorizationandCookiecross-host and forwards everything else, so an inboundX-Hub-Signature,X-Api-Keyor 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/Cookieare 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:CheckRedirectthat #242 adds already holds the request, so the strip has somewhere obvious to live.Definition of done:
CheckRedirect, with a test asserting a cross-host 302 does not carry a forwarded inbound headerDecision: 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-Signaturesafe to hand to a host the operator never configured.Folded into the rework of #242 rather than dispatched separately: the strip lives in the
CheckRedirectthat PR adds, so a second branch would conflict with it by construction.Implementation requirements:
sameDeliveryOriginis false, in the sameCheckRedirect, using the same origin comparison — one rule, not two.Landed on
nextin6874059via #242. Closing manually — the squash commit body carriedCloses #243but the merge parsed only the title.Implemented as decided: forwarded inbound headers are stripped on any hop where
sameDeliveryOriginis false, in the sameCheckRedirectand under the same origin comparison as the operator-configured ones. The stripped set is derived from what the delivery path actually forwarded —applyRequestHeadersreturns 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-TypeandUser-Agentare excluded, being the delivery path's own rather than anyone else's:Content-Typemust survive so a body preserved across a307stays typed, andUser-Agentis 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.