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

117
internal/server/sentry.go Normal file
View File

@@ -0,0 +1,117 @@
package server
import (
"net/http"
"github.com/getsentry/sentry-go"
)
// sentryRedacted stands in for a withheld field on every event shipped
// to Sentry. It is a marker rather than an empty string so a reader
// can tell a suppressed value from an absent one.
const sentryRedacted = "(redacted)"
// sentryClientOptions builds the options the SDK is initialised with.
// It is its own function so a test can stand up a client wired exactly
// as production is, with only the transport swapped.
func sentryClientOptions(dsn, release string) sentry.ClientOptions {
return sentry.ClientOptions{
Dsn: dsn,
Release: release,
// Both hooks, because the SDK runs one for error events
// and the other for transactions.
BeforeSend: scrubSentryRequest,
BeforeSendTransaction: scrubSentryRequest,
}
}
// scrubSentryRequest strips client-supplied content from an event's
// request context before it leaves the process.
//
// sentryhttp attaches the whole *http.Request to the scope
// (sentryhttp.go:113), and Scope.ApplyToEvent fills the event's
// Request from it inside prepareEvent, which runs before this hook.
// Two of the fields it fills are copied with no SendDefaultPII guard:
//
// - QueryString, verbatim from r.URL.RawQuery.
// - Data, the first 10 KiB of the request body, teed off r.Body by
// SetRequest and filled precisely because the handlers call
// ParseForm.
//
// Since every form field in this service is read with PostFormValue,
// the body is the only place a credential is submitted: a target's
// destination URL, whose path segments are the bearer token, plus the
// login password and both password-change fields. None of that may
// reach a third-party service.
//
// This hook is a floor, not a default: the fields it clears stay
// cleared even if SendDefaultPII is ever turned on.
func scrubSentryRequest(
event *sentry.Event,
_ *sentry.EventHint,
) *sentry.Event {
if event == nil || event.Request == nil {
return event
}
req := event.Request
if req.QueryString != "" {
req.QueryString = sentryRedacted
}
if req.Data != "" {
req.Data = sentryRedacted
}
req.Cookies = ""
req.Env = nil
req.Headers = keptSentryHeaders(req.Headers)
return event
}
// keptSentryHeaders returns the subset of headers an event may carry
// off-host. Dropping by allowlist rather than by blocklist is what
// makes an unrecognised header safe: the SDK's own filter removes four
// names and passes everything else, so X-Csrf-Token — which
// gorilla/csrf accepts in place of the form field — and the shared
// secrets senders put on the receiver route (X-Gitlab-Token and the
// per-provider signature headers) would otherwise ship verbatim.
func keptSentryHeaders(headers map[string]string) map[string]string {
if len(headers) == 0 {
return headers
}
kept := make(map[string]string, len(headers))
for name, value := range headers {
if sentryKeepsHeader(name) {
kept[name] = value
}
}
return kept
}
// sentryKeepsHeader reports whether a request header is routing or
// content metadata rather than client-chosen payload. Referer is kept
// on the reasoning that it is browser-set, that this service emits
// only ?page= in its own links, and that Referrer-Policy is set to
// strict-origin-when-cross-origin. X-Request-Id ties the event to the
// local access log line, which holds the rest of the detail.
func sentryKeepsHeader(name string) bool {
switch http.CanonicalHeaderKey(name) {
case "Accept",
"Content-Length",
"Content-Type",
"Host",
"Origin",
"Referer",
"User-Agent",
"X-Request-Id":
return true
default:
return false
}
}