Send the chi route pattern to Sentry, not the concrete path (closes #179)
All checks were successful
check / check (push) Successful in 2m51s
All checks were successful
check / check (push) Successful in 2m51s
#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.
This commit was merged in pull request #181.
This commit is contained in:
101
README.md
101
README.md
@@ -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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user