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

r.FormValue falls back to the query string, so
POST /source/{id}/targets?url=<secret> created a working target from a
value carried on the request line — where proxy logs, browser history
and Referer all record it. Every form read is now r.PostFormValue,
including the login password and both password-change fields, which had
the same defect in a more acute form.

The Sentry leg needed more than the query string. sentryhttp attaches
the whole request to the scope, and ApplyToEvent copies the teed body
into Request.Data with no SendDefaultPII guard — so reading every field
from the body only pointed every credential this change protects at the
one field the first revision did not scrub. Body and query are now
redacted, Cookies and Env cleared, and Headers reduced to an allowlist,
because the SDK's own filter removes four names and would otherwise ship
X-Csrf-Token and the shared secrets senders put on the receiver route.

Also adds json:"-" to Target.Config, APIKey.Key and Setting.Value —
TargetView is the masking barrier for the HTML path only, and the first
handler to marshal a model would serialise a bearer token or the session
encryption key.

Independently reviewed three times. The second review found the Data
leak and proved it with a scratch module; the third disproved the
PR's own claim that BeforeSend gets no request, so the README now
records that redacting unconditionally is a deliberate choice rather
than a limitation — which is what makes #179 cheap to fix.

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