Read form fields from the POST body only (closes #160)

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.

README.md

@@ -1022,6 +1022,53 @@ 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 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 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,
 each to a fixed budget: 512 bytes for `url`, `useragent` and `referer`,
 128 for `request_id` (chi passes an inbound `X-Request-Id` header

internal/database/model_apikey.go

@@ -2,12 +2,16 @@ package database
 
 import "time"
 
-// APIKey represents an API key for a user
+// APIKey represents an API key for a user.
+//
+// Key is a bearer credential, so it is never marshalled with the
+// model. A creation handler that has to show it once returns it in its
+// own response type.
 type APIKey struct {
 	BaseModel
 
 	UserID      string     `gorm:"type:uuid;not null"   json:"userId"`
-	Key         string     `gorm:"uniqueIndex;not null" json:"key"`
+	Key         string     `gorm:"uniqueIndex;not null" json:"-"`
 	Description string     `json:"description"`
 	LastUsedAt  *time.Time `json:"lastUsedAt,omitempty"`
 

internal/database/model_secrets_test.go

@@ -0,0 +1,107 @@
+package database_test
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// keptField is a non-secret value planted alongside each secret, so
+// the assertions below cannot pass by the model marshalling to nothing.
+const keptField = "keepme"
+
+// marshalModel encodes a model the way a future JSON handler would.
+func marshalModel(t *testing.T, v any) string {
+	t.Helper()
+
+	encoded, err := json.Marshal(v)
+	require.NoError(t, err)
+
+	return string(encoded)
+}
+
+// TestModelsDoNotMarshalTheirSecrets pins the barrier for the JSON
+// path. The /api/v1 route group exists and is empty; delivery's
+// TargetView masks the credential for the HTML path only, so without
+// these tags the first handler that marshals a model serialises the
+// secret with it. Each field below is a live credential:
+//
+//   - Target.Config holds an incoming-webhook URL whose path segments
+//     are the bearer token.
+//   - APIKey.Key is a bearer token outright.
+//   - Setting.Value holds the session encryption key.
+//   - User.Password holds the Argon2 hash, and was already tagged.
+func TestModelsDoNotMarshalTheirSecrets(t *testing.T) {
+	t.Parallel()
+
+	const marker = "QQMODELMARKERQQ"
+
+	cases := []struct {
+		name  string
+		model any
+	}{
+		{
+			name: "target config",
+			model: database.Target{
+				Name:   keptField,
+				Type:   database.TargetTypeSlack,
+				Config: `{"webhookUrl":"https://h/s/` + marker + `"}`,
+			},
+		},
+		{
+			name: "api key",
+			model: database.APIKey{
+				Description: keptField,
+				Key:         marker,
+			},
+		},
+		{
+			name: "setting value",
+			model: database.Setting{
+				Key:   keptField,
+				Value: marker,
+			},
+		},
+		{
+			name: "user password hash",
+			model: database.User{
+				Username: keptField,
+				Password: marker,
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			encoded := marshalModel(t, tc.model)
+
+			assert.NotContains(t, encoded, marker)
+			assert.Contains(t, encoded, keptField)
+		})
+	}
+}
+
+// TestWebhookMarshalsNoTargetConfig covers the nested case: a webhook
+// marshalled with its targets preloaded must not carry the credential
+// through the association either.
+func TestWebhookMarshalsNoTargetConfig(t *testing.T) {
+	t.Parallel()
+
+	const marker = "QQNESTEDMARKERQQ"
+
+	encoded := marshalModel(t, database.Webhook{
+		Name: keptField,
+		Targets: []database.Target{{
+			Name:   "slack",
+			Config: `{"webhookUrl":"https://h/s/` + marker + `"}`,
+		}},
+	})
+
+	assert.NotContains(t, encoded, marker)
+	assert.Contains(t, encoded, keptField)
+}

internal/database/model_setting.go

@@ -4,5 +4,8 @@ package database
 // Used for auto-generated values like the session encryption key.
 type Setting struct {
 	Key string `gorm:"primaryKey" json:"key"`
-	Value string `gorm:"type:text;not null" json:"value"`
+
+	// Value holds the session encryption key, so it is never
+	// marshalled with the model.
+	Value string `gorm:"type:text;not null" json:"-"`
 }

internal/database/model_target.go

@@ -20,8 +20,14 @@ type Target struct {
 	Type      TargetType `gorm:"not null"           json:"type"`
 	Active    bool       `gorm:"default:true"       json:"active"`
 
-	// Configuration fields (JSON stored based on type)
-	Config string `gorm:"type:text" json:"config"` // JSON configuration
+	// Configuration fields (JSON stored based on type).
+	//
+	// json:"-" because the blob holds the target's credential — a
+	// Slack incoming-webhook URL, or an http destination whose path
+	// segments are the secret. delivery.TargetView is the masking
+	// barrier for the HTML path; this tag is the barrier for any
+	// handler that marshals the model itself.
+	Config string `gorm:"type:text" json:"-"` // JSON configuration
 
 	// For HTTP targets (max_retries=0 means fire-and-forget,
 	// >0 enables retries with backoff)

internal/handlers/auth.go

@@ -39,8 +39,10 @@ func (h *Handlers) HandleLoginSubmit() http.HandlerFunc {
 			return
 		}
 
-		username := r.FormValue("username")
-		password := r.FormValue("password")
+		// PostFormValue, not FormValue: the credential must come
+		// from the body, never from the query string.
+		username := r.PostFormValue("username")
+		password := r.PostFormValue("password")
 
 		// Validate input
 		if username == "" || password == "" {

internal/handlers/profile.go

@@ -44,9 +44,11 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc {
 		successMessage, errorMessage, handled := h.applyPasswordChange(
 			w,
 			sessionUsername,
-			r.FormValue("current_password"),
-			r.FormValue("new_password"),
-			r.FormValue("confirm_password"),
+			// PostFormValue, not FormValue: the credential must
+			// come from the body, never from the query string.
+			r.PostFormValue("current_password"),
+			r.PostFormValue("new_password"),
+			r.PostFormValue("confirm_password"),
 		)
 		if !handled {
 			return

internal/handlers/source_management.go

@@ -227,9 +227,9 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
 			return
 		}
 
-		name := r.FormValue("name")
-		description := r.FormValue("description")
-		retentionStr := r.FormValue("retention_days")
+		name := r.PostFormValue("name")
+		description := r.PostFormValue("description")
+		retentionStr := r.PostFormValue("retention_days")
 
 		if name == "" {
 			w.WriteHeader(http.StatusBadRequest)
@@ -509,7 +509,7 @@ func (h *Handlers) applyWebhookEdit(
 ) {
 	// The body size cap is enforced by the MaxBodySize middleware,
 	// which runs before CSRF parses the form.
-	name := r.FormValue("name")
+	name := r.PostFormValue("name")
 	if name == "" {
 		data := map[string]any{
 			tmplKeyWebhook: webhook,
@@ -523,12 +523,12 @@ func (h *Handlers) applyWebhookEdit(
 	}
 
 	webhook.Name = name
-	webhook.Description = r.FormValue("description")
+	webhook.Description = r.PostFormValue("description")
 
 	// An empty field falls back to the stored value, so submitting the
 	// form without touching retention leaves the policy alone.
 	retentionDays, retErr := parseRetentionDays(
-		r.FormValue("retention_days"), webhook.RetentionDays,
+		r.PostFormValue("retention_days"), webhook.RetentionDays,
 	)
 	if retErr != nil {
 		data := map[string]any{
@@ -950,7 +950,7 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc {
 			return
 		}
 
-		description := r.FormValue("description")
+		description := r.PostFormValue("description")
 
 		entrypoint := &database.Entrypoint{
 			WebhookID:   webhook.ID,
@@ -1020,11 +1020,18 @@ func (h *Handlers) processTargetCreate(
 ) {
 	// The body size cap is enforced by the MaxBodySize middleware,
 	// which runs before CSRF parses the form.
-	name := r.FormValue("name")
-	targetType := database.TargetType(r.FormValue("type"))
-	targetURL := r.FormValue("url")
-	maxRetriesStr := r.FormValue("max_retries")
-	expiry := r.FormValue("expiry")
+	//
+	// Every field here is read with PostFormValue, not FormValue.
+	// FormValue falls back to the query string, which would let
+	// `POST /source/{id}/targets?url=https://hooks.slack.com/...`
+	// configure a target from a value the request line carries — and
+	// the request line, unlike the body, is what logs, proxies,
+	// Referer headers and error trackers record.
+	name := r.PostFormValue("name")
+	targetType := database.TargetType(r.PostFormValue("type"))
+	targetURL := r.PostFormValue("url")
+	maxRetriesStr := r.PostFormValue("max_retries")
+	expiry := r.PostFormValue("expiry")
 
 	if name == "" {
 		http.Error(

internal/handlers/target_create_query_test.go

@@ -0,0 +1,206 @@
+package handlers_test
+
+import (
+	"bytes"
+	"context"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/go-chi/chi"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/middleware"
+)
+
+// targetSecretSegments are the path segments of an incoming-webhook
+// URL. For Slack, Discord and Teams the path IS the bearer credential,
+// so this string must not reach storage or the access log by way of
+// the request line.
+const targetSecretSegments = "T00000000/B00000000/QQTARGETSECRETQQ"
+
+// targetSecretURL is a destination whose secret lives in its path. It
+// uses a literal public address rather than a hostname so the SSRF
+// check resolves nothing: with a hostname, a sandbox without DNS would
+// reject the URL for the wrong reason and the test would pass even
+// with the defect reintroduced.
+const targetSecretURL = "https://93.184.216.34/services/" +
+	targetSecretSegments
+
+// targetsForWebhook returns every target stored against a webhook.
+func targetsForWebhook(
+	t *testing.T,
+	db *database.Database,
+	webhookID string,
+) []database.Target {
+	t.Helper()
+
+	var targets []database.Target
+
+	require.NoError(
+		t,
+		db.DB().Where("webhook_id = ?", webhookID).
+			Find(&targets).Error,
+	)
+
+	return targets
+}
+
+// postTargetCreate drives HandleTargetCreate through the production
+// access-log middleware and a chi route, so the logged url field is
+// produced exactly as it ships, and returns the recorder plus the
+// captured log.
+func postTargetCreate(
+	t *testing.T,
+	env *sourceTestEnv,
+	webhookID string,
+	query string,
+	form url.Values,
+) (*httptest.ResponseRecorder, string) {
+	t.Helper()
+
+	logBuf := new(bytes.Buffer)
+	mw := middleware.NewForTest(
+		slog.New(slog.NewJSONHandler(
+			logBuf, &slog.HandlerOptions{Level: slog.LevelInfo},
+		)),
+		&config.Config{Environment: config.EnvironmentDev},
+		nil,
+	)
+
+	router := chi.NewRouter()
+	router.Use(mw.Logging())
+	router.Post(
+		"/source/{sourceID}/targets",
+		env.handlers.HandleTargetCreate(),
+	)
+
+	target := "/source/" + webhookID + "/targets"
+	if query != "" {
+		target += "?" + query
+	}
+
+	body := ""
+	if form != nil {
+		body = form.Encode()
+	}
+
+	req := httptest.NewRequestWithContext(
+		context.Background(),
+		http.MethodPost,
+		target,
+		strings.NewReader(body),
+	)
+	req.Header.Set(
+		"Content-Type", "application/x-www-form-urlencoded",
+	)
+
+	for _, c := range env.cookies {
+		req.AddCookie(c)
+	}
+
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	return w, logBuf.String()
+}
+
+// TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget is the
+// regression test for the ingress leak. r.FormValue falls back to the
+// query string when a field is absent from the POST body, so
+//
+//	POST /source/{id}/targets?url=https://hooks.slack.com/services/...
+//
+// with an empty url field used to create a working target from a value
+// carried on the request line — where logs, proxies, Referer headers
+// and error trackers record it. The handler reads the body only, so
+// the request is rejected for a missing URL and stores nothing.
+//
+// name and type are sent in the BODY on purpose: the request has to
+// get past those two validations for the assertion to be about the url
+// read specifically.
+func TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+	webhook := seedWebhookWithRetention(t, env.db, 30)
+
+	body := url.Values{}
+	body.Set("name", "leaky")
+	body.Set("type", string(database.TargetTypeSlack))
+
+	w, logged := postTargetCreate(
+		t, env, webhook.ID,
+		"url="+url.QueryEscape(targetSecretURL),
+		body,
+	)
+
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+
+	targets := targetsForWebhook(t, env.db, webhook.ID)
+	assert.Empty(
+		t, targets,
+		"a query-string value must not populate a target config",
+	)
+
+	assert.NotContains(t, logged, targetSecretSegments)
+	assert.NotContains(t, logged, "93.184.216.34")
+	assert.NotEmpty(t, logged, "the access log line must still be written")
+}
+
+// TestHandleTargetCreate_BodyURLStillCreatesTheTarget is the positive
+// control for the test above: the rejection has to come from where the
+// value was read, not from the handler being broken.
+func TestHandleTargetCreate_BodyURLStillCreatesTheTarget(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+	webhook := seedWebhookWithRetention(t, env.db, 30)
+
+	body := url.Values{}
+	body.Set("name", "legit")
+	body.Set("type", string(database.TargetTypeSlack))
+	body.Set("url", targetSecretURL)
+
+	w, logged := postTargetCreate(t, env, webhook.ID, "", body)
+
+	assert.Equal(t, http.StatusSeeOther, w.Code)
+
+	targets := targetsForWebhook(t, env.db, webhook.ID)
+	require.Len(t, targets, 1)
+	assert.Contains(t, targets[0].Config, targetSecretSegments)
+
+	// The body carried the credential, so the access log must still
+	// not have it: the log records the request line only.
+	assert.NotContains(t, logged, targetSecretSegments)
+}
+
+// TestHandleTargetCreate_QueryStringCannotSupplyNameOrType covers the
+// rest of the converted reads on this handler in one request: with an
+// empty body, nothing the query carries is visible to it.
+func TestHandleTargetCreate_QueryStringCannotSupplyNameOrType(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+	webhook := seedWebhookWithRetention(t, env.db, 30)
+
+	w, _ := postTargetCreate(
+		t, env, webhook.ID,
+		"name=leaky&type=slack&max_retries=9&expiry=30d&url="+
+			url.QueryEscape(targetSecretURL),
+		url.Values{},
+	)
+
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Contains(t, w.Body.String(), "Name is required")
+	assert.Empty(t, targetsForWebhook(t, env.db, webhook.ID))
+}

internal/server/export_test.go

@@ -4,6 +4,7 @@ import (
 	"log/slog"
 	"net/http"
 
+	"github.com/getsentry/sentry-go"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/middleware"
@@ -13,6 +14,25 @@ import (
 // build requests that sit exactly at, below, and above it.
 const MaxFormBodySizeForTest = maxFormBodySize
 
+// ScrubSentryRequestForTest exposes the BeforeSend hook that
+// enableSentry installs, so a test can assert on what it leaves in an
+// event without standing up a Sentry client.
+func ScrubSentryRequestForTest(
+	event *sentry.Event,
+	hint *sentry.EventHint,
+) *sentry.Event {
+	return scrubSentryRequest(event, hint)
+}
+
+// SentryClientOptionsForTest exposes the exact options enableSentry
+// initialises the SDK with, so a test can capture events through the
+// production hook wiring rather than a hand-built equivalent.
+func SentryClientOptionsForTest(
+	dsn, release string,
+) sentry.ClientOptions {
+	return sentryClientOptions(dsn, release)
+}
+
 // NewRouterForTest builds the real route tree via SetupRoutes with
 // the supplied middleware and handlers, bypassing the fx lifecycle
 // and the HTTP listener. Tests use it so that route-group middleware

internal/server/sentry.go

@@ -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
+	}
+}

internal/server/sentry_test.go

@@ -0,0 +1,227 @@
+package server_test
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/getsentry/sentry-go"
+	sentryhttp "github.com/getsentry/sentry-go/http"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/server"
+)
+
+// The three markers below are the credentials a captured event could
+// carry off-host, one per field of sentry.Request that the SDK fills
+// from the request without a SendDefaultPII guard.
+const (
+	// sentryBodyMarker is submitted as a form value. Since every
+	// handler reads its fields with PostFormValue, the body is the
+	// only place a password or a target URL is ever supplied.
+	sentryBodyMarker = "QQSENTRYBODYMARKERQQ"
+
+	// sentryQueryMarker rides the request line.
+	sentryQueryMarker = "T00000000/B00000000/QQSENTRYQUERYMARKERQQ"
+
+	// sentryHeaderMarker rides X-Csrf-Token, which gorilla/csrf
+	// accepts in place of the form field.
+	sentryHeaderMarker = "QQSENTRYHEADERMARKERQQ"
+)
+
+// sentryKeptUserAgent is a non-secret header value planted so the
+// assertions below cannot pass by the event carrying no headers at
+// all.
+const sentryKeptUserAgent = "webhooker-test-agent"
+
+// captureTransport records events instead of shipping them, so a test
+// sees exactly the payload the SDK would have put on the wire.
+type captureTransport struct {
+	mu     sync.Mutex
+	events []*sentry.Event
+}
+
+func (c *captureTransport) Configure(sentry.ClientOptions) {}
+
+func (c *captureTransport) Flush(time.Duration) bool { return true }
+
+func (c *captureTransport) SendEvent(event *sentry.Event) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	c.events = append(c.events, event)
+}
+
+// captureThroughSentryHTTP panics inside a form handler wrapped in the
+// real sentryhttp middleware and returns the event the SDK produced.
+//
+// This is the only construction path on which Request.Data appears:
+// sentryhttp calls Scope.SetRequest, which tees r.Body into a 10 KiB
+// buffer, ParseForm drains the tee, and Scope.ApplyToEvent copies the
+// buffer into the event inside prepareEvent — before BeforeSend runs.
+// A hand-built sentry.NewRequest never reads the body and so cannot
+// regress-test any of it.
+//
+// scrub selects whether the production BeforeSend hooks are installed,
+// so the same path shows both what the SDK collects and what survives.
+func captureThroughSentryHTTP(t *testing.T, scrub bool) *sentry.Event {
+	t.Helper()
+
+	transport := &captureTransport{}
+
+	opts := server.SentryClientOptionsForTest(
+		"https://public@sentry.invalid/1", "webhooker-test",
+	)
+	opts.Transport = transport
+
+	if !scrub {
+		opts.BeforeSend = nil
+		opts.BeforeSendTransaction = nil
+	}
+
+	client, err := sentry.NewClient(opts)
+	require.NoError(t, err)
+
+	handler := sentryhttp.New(sentryhttp.Options{}).Handle(
+		http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
+			// This call is what drains the tee and fills the
+			// buffer. Its success is asserted by the unscrubbed
+			// case below, which sees the body in the event.
+			_ = r.ParseForm()
+
+			panic("boom")
+		}),
+	)
+
+	handler.ServeHTTP(
+		httptest.NewRecorder(),
+		sentryLoginRequest(client),
+	)
+
+	require.Len(t, transport.events, 1)
+
+	return transport.events[0]
+}
+
+// sentryLoginRequest builds the password POST the capture above drives,
+// with a credential planted in the body, the query and a header.
+func sentryLoginRequest(client *sentry.Client) *http.Request {
+	form := url.Values{}
+	form.Set("username", "admin")
+	form.Set("password", sentryBodyMarker)
+
+	req := httptest.NewRequestWithContext(
+		sentry.SetHubOnContext(
+			context.Background(),
+			sentry.NewHub(client, sentry.NewScope()),
+		),
+		http.MethodPost,
+		"/pages/login?url=https://hooks.slack.com/services/"+
+			sentryQueryMarker,
+		strings.NewReader(form.Encode()),
+	)
+
+	req.Header.Set(
+		"Content-Type", "application/x-www-form-urlencoded",
+	)
+	req.Header.Set("X-Csrf-Token", sentryHeaderMarker)
+	req.Header.Set("User-Agent", sentryKeptUserAgent)
+
+	return req
+}
+
+// marshalEvent encodes an event the way the transport does.
+func marshalEvent(t *testing.T, event *sentry.Event) string {
+	t.Helper()
+
+	encoded, err := json.Marshal(event)
+	require.NoError(t, err)
+
+	return string(encoded)
+}
+
+// TestSentryScrub_SDKCollectsTheRequestUnscrubbed pins the premise the
+// hook exists for. Without it the SDK ships the whole POST body, the
+// raw query and the CSRF header, none of which SendDefaultPII=false
+// suppresses.
+func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
+	t.Parallel()
+
+	event := captureThroughSentryHTTP(t, false)
+	require.NotNil(t, event.Request)
+
+	assert.Contains(
+		t, event.Request.Data, sentryBodyMarker,
+		"the SDK is expected to collect the POST body; if it no "+
+			"longer does, the scrub hook's premise changed",
+	)
+	assert.Contains(t, event.Request.QueryString, sentryQueryMarker)
+	assert.Contains(
+		t, marshalEvent(t, event), sentryHeaderMarker,
+	)
+}
+
+// TestSentryScrub_RedactsTheCapturedRequest is the regression test: no
+// byte of any planted credential may survive into the marshalled event
+// that leaves the process.
+func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) {
+	t.Parallel()
+
+	event := captureThroughSentryHTTP(t, true)
+	require.NotNil(t, event.Request)
+
+	encoded := marshalEvent(t, event)
+
+	assert.NotContains(t, encoded, sentryBodyMarker)
+	assert.NotContains(t, encoded, sentryQueryMarker)
+	assert.NotContains(t, encoded, sentryHeaderMarker)
+	assert.NotContains(t, encoded, "hooks.slack.com")
+
+	assert.Equal(t, "(redacted)", event.Request.Data)
+	assert.Equal(t, "(redacted)", event.Request.QueryString)
+	assert.Empty(t, event.Request.Cookies)
+	assert.Empty(t, event.Request.Env)
+}
+
+// TestSentryScrub_KeepsTheRoutingContext checks the hook does not cost
+// the debugging signal: the route, the method and the metadata headers
+// still identify what failed.
+func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) {
+	t.Parallel()
+
+	event := captureThroughSentryHTTP(t, true)
+	require.NotNil(t, event.Request)
+
+	assert.Contains(t, event.Request.URL, "/pages/login")
+	assert.Equal(t, http.MethodPost, event.Request.Method)
+	assert.Equal(
+		t,
+		sentryKeptUserAgent,
+		event.Request.Headers["User-Agent"],
+	)
+	assert.Equal(
+		t,
+		"application/x-www-form-urlencoded",
+		event.Request.Headers["Content-Type"],
+	)
+}
+
+// TestSentryScrub_ToleratesEventsWithoutARequest covers the events the
+// hook sees outside an HTTP handler, where no request is attached.
+func TestSentryScrub_ToleratesEventsWithoutARequest(t *testing.T) {
+	t.Parallel()
+
+	scrubbed := server.ScrubSentryRequestForTest(
+		sentry.NewEvent(), nil,
+	)
+
+	require.NotNil(t, scrubbed)
+	assert.Nil(t, scrubbed.Request)
+	assert.Nil(t, server.ScrubSentryRequestForTest(nil, nil))
+}

internal/server/server.go

@@ -141,14 +141,14 @@ func (s *Server) enableSentry() {
 		return
 	}
 
-	err := sentry.Init(sentry.ClientOptions{
-		Dsn: s.params.Config.SentryDSN,
-		Release: fmt.Sprintf(
+	err := sentry.Init(sentryClientOptions(
+		s.params.Config.SentryDSN,
+		fmt.Sprintf(
 			"%s-%s",
 			s.params.Globals.Appname,
 			s.params.Globals.Version,
 		),
-	})
+	))
 	if err != nil {
 		s.log.Error("sentry init failure", "error", err)
 		// Don't use fatal since we still want the service to run