Two places decided whether a request was TLS, by two different means,
and they disagreed.
The session cookie's Secure attribute was fixed at startup from
!Config.IsDev(). "dev" is the environment when WEBHOOKER_ENVIRONMENT is
unset, so a deployment terminating TLS at a proxy without also setting
the environment shipped the authentication cookie with no Secure
attribute -- on the same response as a CSRF cookie that had one. It
failed silently: everything kept working, so nothing prompted anyone to
look.
The CSRF middleware's per-request check compared X-Forwarded-Proto with
== "https" exactly, so "HTTPS", "https, http" and "https,https" all took
the plaintext path. Uppercase is legal for a case-insensitive token and
the comma forms are what a proxy chained behind another proxy emits by
appending rather than replacing. On that path gorilla/csrf stops
enforcing the strict Referer check on a site that genuinely is HTTPS.
Both now go through internal/reqtls.IsTLS, which folds case and takes
the leftmost comma-separated element -- the hop nearest the client, and
so the one a cookie's Secure attribute is about. A third package is
needed because internal/middleware already imports internal/session, so
session cannot import middleware back.
Per-request beat a startup warning for the session cookie because it
turned out to need no restructuring: gorilla/sessions gives every
session its own copy of the store's Options and renders the cookie from
that copy, and every session-cookie write here already goes through
Session.Save or Session.Regenerate, both of which hold the request. The
store's template Secure becomes true so that a write path added later
which forgets to track the transport fails visibly instead of silently
dropping Secure.
The flag tracks the transport in both directions rather than latching
on. Secure over plaintext is discarded by the browser without an error,
which would make a plain-HTTP local run impossible to log into -- and
would also void the deletion cookies in Destroy and Regenerate, leaving
a session the user just tried to end still live.
A third site that makes this decision, internal/handlers'
BaseURL construction, assigns the raw header straight into the URL
scheme. It is left alone here and filed separately.
#160 scrubbed the Sentry body, query, cookies, env and headers but kept
Request.URL, which the SDK builds from the concrete path. On the
receiver that path is /webhook/<uuid> in full — a write capability, not
an identifier: anyone holding it can inject events the operator's
targets then deliver. #146's "2xx and 5xx keep the concrete path" ruling
was reasoned about a log the operator owns and does not transfer to a
tracker with its own retention and access control.
The chi route pattern now replaces the path on every route, reached via
the request the SDK carries on hint.Context. Unconditional, because a
route-conditional rule leaks on any route someone forgets to add, and on
a static route the pattern is the path anyway. The fallback is never the
concrete path.
Also rewrites event.Transaction, which carries the same UUID on the
sibling dispatch and which the issue did not name. Tracing is off today,
so that half is a floor rather than a live fix — and it is why enabling
tracing later needs #185 first, or every transaction collapses into one
bucket.
Independently reviewed. The reviewer ran fourteen adversarial probes —
404 and 405 panics, panics in middleware before and after routing,
direct CaptureException, mounted subrouters, wildcards, tracing on and
off — and found no path where the concrete URL survives, and no third
field carrying it.
Merge note: the final round was a two-comment documentation fix on an
already-passed review, correcting a rationale that called the host
operator configuration when it is the client's Host header. I verified
that amend is comment-only myself rather than spending a fifth review
round on it.
r.FormValue falls back to the query string, so
POST /source/{id}/targets?url=<secret> created a working target from a
value carried on the request line — where proxy logs, browser history
and Referer all record it. Every form read is now r.PostFormValue,
including the login password and both password-change fields, which had
the same defect in a more acute form.
The Sentry leg needed more than the query string. sentryhttp attaches
the whole request to the scope, and ApplyToEvent copies the teed body
into Request.Data with no SendDefaultPII guard — so reading every field
from the body only pointed every credential this change protects at the
one field the first revision did not scrub. Body and query are now
redacted, Cookies and Env cleared, and Headers reduced to an allowlist,
because the SDK's own filter removes four names and would otherwise ship
X-Csrf-Token and the shared secrets senders put on the receiver route.
Also adds json:"-" to Target.Config, APIKey.Key and Setting.Value —
TargetView is the masking barrier for the HTML path only, and the first
handler to marshal a model would serialise a bearer token or the session
encryption key.
Independently reviewed three times. The second review found the Data
leak and proved it with a scratch module; the third disproved the
PR's own claim that BeforeSend gets no request, so the README now
records that redacting unconditionally is a deliberate choice rather
than a limitation — which is what makes #179 cheap to fix.
