Read form fields from the POST body only (closes #160)
All checks were successful
check / check (push) Successful in 2m55s

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 rather than filtered
by route because the SDK hands the hook no request to identify the
route with, and an unrecognised route must not leak; nothing is lost,
since the receiver route's body is already stored on the event and
served from the UI. 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.

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.
This commit is contained in:
2026-08-17 22:49:25 +00:00
parent 992b3c68f5
commit 0598f1dc04
13 changed files with 755 additions and 27 deletions

View File

@@ -1022,6 +1022,33 @@ buy the same amplification as an invented path. Nothing debuggable is
lost: `page`, on the authenticated pagination links, is the only query
parameter this service reads.
Client-supplied request content does not leave the host by the other
route either. The Sentry SDK attaches the request to every event it
captures, independently of the access log, and `SendDefaultPII=false`
does not cover all of what it copies: the raw query string and the
first 10 KiB of the request body are both taken unconditionally, the
body precisely because these handlers call `ParseForm`. A `BeforeSend`
hook therefore replaces the query string and the body with
`(redacted)`, drops cookies and the remote-address environment, and
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 rather than filtered because the hook cannot tell
which route it is on: the SDK hands `BeforeSend` no request, so a
route-conditional rule would have to guess, and an unrecognised route
must not leak. Nothing debuggable is lost by it. 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 on the receiver route, the one route
whose body is genuine signal, that body is already stored on the event
and served from the UI. The headers are an allowlist for the same
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.
The remaining client-supplied fields are truncated rather than dropped,
each to a fixed budget: 512 bytes for `url`, `useragent` and `referer`,
128 for `request_id` (chi passes an inbound `X-Request-Id` header