Send the chi route pattern to Sentry, not the concrete path (closes #179)
All checks were successful
check / check (push) Successful in 2m49s

The Sentry SDK builds Request.URL as scheme://host/path from the
concrete path, which on the receiver route is /webhook/<uuid> in full.
That UUID is a write capability, not an identifier: anyone holding it
can post events this service accepts and its targets then deliver. A
third-party tracker has its own retention, access control and deletion
policy, so the rule the local access log follows does not carry across
that boundary.

The BeforeSend hook now rebuilds the URL from the chi route pattern,
on every route rather than by route list, since a route-conditional
rule leaks on any route someone forgets to add. Scheme and host are
kept and everything else in the URL is discarded rather than edited:
the scheme is the CSRF TLS decision the header allowlist relies on,
and the host is whatever the client's Host header carried, which the
allowlisted Host header already sends anyway.

The pattern is reachable only on the error dispatch, where sentryhttp
puts the request on the context the client copies onto the hint. A
finished span captures with a nil hint, so BeforeSendTransaction sees
no context; there and wherever else the pattern is missing the path
falls back to the literal /(redacted), never to the concrete path, and
a URL that will not parse into a scheme is withheld whole. A
transaction event's SDK-built "METHOD /path" name carries the same
capability and is rewritten on the same terms.
This commit is contained in:
2026-08-18 00:16:23 +00:00
parent 76725cffc4
commit fb5a203896
3 changed files with 496 additions and 81 deletions

101
README.md
View File

@@ -1037,38 +1037,77 @@ reduces the headers to a fixed allowlist — `Accept`, `Content-Length`,
`Content-Type`, `Host`, `Origin`, `Referer`, `User-Agent` and
`X-Request-Id`.
The body is replaced on every route rather than filtered by route, and
that is a choice rather than a limitation: the route is reachable from
the hook. `sentryhttp`'s recover path puts the request on the context
it hands to `RecoverWithContext`, and the SDK carries that context
through to `BeforeSend` as `hint.Context`, so
`hint.Context.Value(sentry.RequestContextKey)` yields the live request
and chi's `RoutePattern()` yields the matched pattern off it. There
are two reasons to redact unconditionally anyway. Nothing debuggable
is lost:
every handler reads its fields with `PostFormValue`, so the body is
exactly where the credentials are — the target destination URL, the
login password, both password-change fields — and the one route whose
body is genuine signal is the receiver, whose body is already stored
on the event and served from the UI, so a tracker is not where anyone
reads it. And an unconditional rule cannot leak on a route somebody
forgets to add to it, which a route-conditional one can.
The same hook rewrites the request URL. The SDK builds it as
`scheme://host/path` from the concrete path, which on the receiver
route is `/webhook/<uuid>` in full — and that UUID is a write
capability, not an identifier: anyone holding it can post events this
service accepts and its targets then deliver. A tracker has its own
retention, access control and deletion policy, so the rule the access
log follows above does not carry across that boundary. What is sent is
the chi route pattern instead: `http://host/webhook/{uuid}`.
The headers are an allowlist for that second reason: the SDK's own
filter removes four names and passes everything else, which would ship
`X-CSRF-Token` and the shared secrets senders put on the receiver
route. What survives still names the failing route — scheme, host,
path, method — and `X-Request-Id` ties the event to the local access
log line that holds the rest. Nothing dropped is needed for the
likeliest use, debugging a CSRF rejection. Its three inputs are the
TLS decision, `Origin` and `Referer`; the latter two are kept, and the
first is already in the retained URL, because the SDK derives that
URL's scheme from `r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"`
byte for byte the predicate `internal/middleware/csrf.go` uses to
choose between the `csrf.Secure(true)` and `csrf.Secure(false)`
handlers. So dropping `X-Forwarded-Proto` costs nothing. The dropped
provider headers (`X-GitHub-Event`, `X-Gitlab-Event` and the like) are
real signal but are recorded locally on the event, and
The scheme and the host are kept, and everything else in the URL is
discarded rather than edited, so a future SDK version that starts
appending a query string cannot widen this. The scheme has to survive
for the reason given below. The host is whatever the request's `Host`
header carried — this service validates no hostname, so on a directly
exposed deployment a client sets it — and that same header is on the
allowlist above, so scrubbing the host out of the URL would withhold
nothing that is not sent anyway.
The body, the query string and the URL are all handled on every route
rather than filtered by route. For the URL that is also what keeps the
event locatable: an error event is grouped by its exception and stack
trace, not by its URL, so replacing the path with the pattern costs no
grouping and the pattern still names the route in the UI. And an
unconditional rule cannot leak on a route somebody forgets to add to
it, which a route-conditional one can. For the body there is a second
reason: nothing debuggable is lost, because every handler reads its
fields with `PostFormValue`, so the body is exactly where the
credentials are — the target destination URL, the login password, both
password-change fields — and the one route whose body is genuine
signal is the receiver, whose body is already stored on the event and
served from the UI, so a tracker is not where anyone reads it.
The route is reachable from the hook only on the error dispatch.
`sentryhttp`'s recover path puts the request on the context it hands
to `RecoverWithContext`, and the SDK carries that context through to
`BeforeSend` as `hint.Context`, so
`hint.Context.Value(sentry.RequestContextKey)` yields the live request
and chi's `RoutePattern()` yields the matched pattern off it. The
transaction dispatch has no such request: a finished span captures
with a nil hint, which the client replaces with an empty one, so
`BeforeSendTransaction` sees no context at all. Tracing is off in this
service, so no transaction event is produced today, but the hook is
installed on both dispatches as a floor.
Where the pattern is out of reach — the transaction dispatch, an event
captured outside the router, or a request that matched no route — the
fallback is never the concrete path. The path becomes the literal
`/(redacted)`, so the URL reads `http://host/(redacted)`; a URL the
rewrite cannot parse into a scheme is withheld whole. A transaction
event additionally carries the SDK's own `METHOD /path` name, built
from the concrete path as well; it is rewritten on the same terms, to
`POST /webhook/{uuid}` where the pattern is known and `POST
/(redacted)` where it is not.
The headers are an allowlist for the same reason the rules above are
unconditional: the SDK's own filter removes four names and passes
everything else, which would ship `X-CSRF-Token` and the shared
secrets senders put on the receiver route. What survives still names
the failing route — scheme, host, route pattern, method — and
`X-Request-Id` ties the event to the local access log line that holds
the rest. Nothing dropped is needed for the likeliest use, debugging a
CSRF rejection. Its three inputs are the TLS decision, `Origin` and
`Referer`; the latter two are kept, and the first is the scheme of the
retained URL, because the SDK derives that scheme from
`r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"` — byte
for byte the predicate `internal/middleware/csrf.go` uses to choose
between the `csrf.Secure(true)` and `csrf.Secure(false)` handlers.
That is what the rewrite above preserves it for, and it is why
dropping `X-Forwarded-Proto` costs nothing. The dropped provider
headers (`X-GitHub-Event`, `X-Gitlab-Event` and the like) are real
signal but are recorded locally on the event, and
`Sentry-Trace`/`Baggage` are already reflected in the event's trace
context.