Compare commits

1 Commits

Author SHA1 Message Date
3127b4e5cc Read form fields from the POST body only (closes #160)
All checks were successful
check / check (push) Successful in 2m48s
internal/handlers/source_management.go read the target destination
with r.FormValue, which falls back to the URL query string when the
field is absent from the body. So

    POST /source/{id}/targets?url=https://hooks.slack.com/services/T/B/S

created a working target from a value carried on the request line,
where logs, proxies, Referer headers and error trackers record it.
That is the remaining ingress path of the credential-exposure class
the render, delivery-error and log-line paths were each closed for.

Every form read in these handlers is now r.PostFormValue, so no
query-string value can populate stored configuration or be taken as a
credential. The one deliberate query read, `page` on the authenticated
pagination links, is untouched: it uses r.URL.Query().Get already.

The access log no longer carries the query on any branch, so the log
half of the report is already mitigated; the Sentry half is not, and
making the body the only place these fields are read from aims every
credential at Sentry's request context. The SDK attaches the request
to every captured event, and SendDefaultPII=false does not cover all
of what it copies: Scope.SetRequest tees the first 10 KiB of the body
into a buffer that ParseForm then fills, and Scope.ApplyToEvent copies
both that buffer and r.URL.RawQuery into the event with no guard,
before BeforeSend runs.

So the BeforeSend hook replaces the query string and the body with a
marker, drops cookies and the remote-address environment, and reduces
the headers to an allowlist. The body is replaced on every route
rather than filtered by route, and that is a choice rather than a
limitation: sentryhttp's recover path puts the request on the context
it hands to RecoverWithContext, the SDK carries that context through
to BeforeSend as hint.Context, and chi's RoutePattern is reachable
from it. Redacting unconditionally is still the right call. Every
handler reads its fields with PostFormValue, so the body is exactly
where the credentials are; the one route whose body is genuine signal
is the receiver, and that 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 headers need an allowlist because the SDK's own filter removes
four names and passes everything else, including X-Csrf-Token and the
shared secrets senders put on the receiver route. Scheme, host, path,
method and X-Request-Id stay, which is what names the failing route
and ties it to the access log line. Nothing dropped is needed to debug
a CSRF rejection: Origin and Referer are kept, and the TLS decision is
already in the retained URL, whose scheme sentry-go derives from the
same r.TLS-or-X-Forwarded-Proto predicate the CSRF middleware uses to
pick its handler.

Second barrier, for the JSON path that does not exist yet: the fields
that hold a credential are tagged json:"-" so the first handler to
marshal a model cannot serialise one. Target.Config holds the
incoming-webhook URL, APIKey.Key is a bearer token, and Setting.Value
holds the session encryption key. delivery.TargetView remains the
masking barrier for the HTML path, which is unaffected.
2026-08-17 23:47:48 +00:00

View File

@@ -1034,20 +1034,40 @@ reduces the headers to a fixed allowlist — `Accept`, `Content-Length`,
`Content-Type`, `Host`, `Origin`, `Referer`, `User-Agent` and `Content-Type`, `Host`, `Origin`, `Referer`, `User-Agent` and
`X-Request-Id`. `X-Request-Id`.
The body is replaced rather than filtered because the hook cannot tell The body is replaced on every route rather than filtered by route, and
which route it is on: the SDK hands `BeforeSend` no request, so a that is a choice rather than a limitation: the route is reachable from
route-conditional rule would have to guess, and an unrecognised route the hook. `sentryhttp`'s recover path puts the request on the context
must not leak. Nothing debuggable is lost by it. Every handler reads it hands to `RecoverWithContext`, and the SDK carries that context
its fields with `PostFormValue`, so the body is exactly where the through to `BeforeSend` as `hint.Context`, so
credentials are — the target destination URL, the login password, both `hint.Context.Value(sentry.RequestContextKey)` yields the live request
password-change fields — and on the receiver route, the one route and chi's `RoutePattern()` yields the matched pattern off it. There
whose body is genuine signal, that body is already stored on the event are two reasons to redact unconditionally anyway. Nothing debuggable
and served from the UI. The headers are an allowlist for the same is lost:
reason: the SDK's own filter removes four names and passes everything every handler reads its fields with `PostFormValue`, so the body is
else, which would ship `X-CSRF-Token` and the shared secrets senders exactly where the credentials are — the target destination URL, the
put on the receiver route. What survives still names the failing login password, both password-change fields — and the one route whose
route — scheme, host, path, method — and `X-Request-Id` ties the event body is genuine signal is the receiver, whose body is already stored
to the local access log line that holds the rest. 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 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
`Sentry-Trace`/`Baggage` are already reflected in the event's trace
context.
The remaining client-supplied fields are truncated rather than dropped, The remaining client-supplied fields are truncated rather than dropped,
each to a fixed budget: 512 bytes for `url`, `useragent` and `referer`, each to a fixed budget: 512 bytes for `url`, `useragent` and `referer`,