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)
+	// Configuration fields (JSON stored based on type).
-	Config string `gorm:"type:text" json:"config"` // JSON configuration
+	//
+	// 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")
+		// PostFormValue, not FormValue: the credential must come
-		password := r.FormValue("password")
+		// 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"),
+			// PostFormValue, not FormValue: the credential must
-			r.FormValue("new_password"),
+			// come from the body, never from the query string.
-			r.FormValue("confirm_password"),
+			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")
+		name := r.PostFormValue("name")
-		description := r.FormValue("description")
+		description := r.PostFormValue("description")
-		retentionStr := r.FormValue("retention_days")
+		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"))
+	// Every field here is read with PostFormValue, not FormValue.
-	targetURL := r.FormValue("url")
+	// FormValue falls back to the query string, which would let
-	maxRetriesStr := r.FormValue("max_retries")
+	// `POST /source/{id}/targets?url=https://hooks.slack.com/...`
-	expiry := r.FormValue("expiry")
+	// 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{
+	err := sentry.Init(sentryClientOptions(
-		Dsn: s.params.Config.SentryDSN,
+		s.params.Config.SentryDSN,
-		Release: fmt.Sprintf(
+		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