diff --git a/README.md b/README.md index ce020c7..d1edf4b 100644 --- a/README.md +++ b/README.md @@ -1037,38 +1037,77 @@ 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 same hook rewrites the request URL. The SDK builds it as +`scheme://host/path` from the concrete path, which on the receiver +route is `/webhook/` in full — and that UUID is a write +capability, not an identifier: anyone holding it can post events this +service accepts and its targets then deliver. A tracker has its own +retention, access control and deletion policy, so the rule the access +log follows above does not carry across that boundary. What is sent is +the chi route pattern instead: `http://host/webhook/{uuid}`. -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 +The scheme and the host are kept, and everything else in the URL is +discarded rather than edited, so a future SDK version that starts +appending a query string cannot widen this. The scheme has to survive +for the reason given below. The host is whatever the request's `Host` +header carried — this service validates no hostname, so on a directly +exposed deployment a client sets it — and that same header is on the +allowlist above, so scrubbing the host out of the URL would withhold +nothing that is not sent anyway. + +The body, the query string and the URL are all handled on every route +rather than filtered by route. For the URL that is also what keeps the +event locatable: an error event is grouped by its exception and stack +trace, not by its URL, so replacing the path with the pattern costs no +grouping and the pattern still names the route in the UI. And an +unconditional rule cannot leak on a route somebody forgets to add to +it, which a route-conditional one can. For the body there is a second +reason: nothing debuggable is lost, because 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. + +The route is reachable from the hook only on the error dispatch. +`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. The +transaction dispatch has no such request: a finished span captures +with a nil hint, which the client replaces with an empty one, so +`BeforeSendTransaction` sees no context at all. Tracing is off in this +service, so no transaction event is produced today, but the hook is +installed on both dispatches as a floor. + +Where the pattern is out of reach — the transaction dispatch, an event +captured outside the router, or a request that matched no route — the +fallback is never the concrete path. The path becomes the literal +`/(redacted)`, so the URL reads `http://host/(redacted)`; a URL the +rewrite cannot parse into a scheme is withheld whole. A transaction +event additionally carries the SDK's own `METHOD /path` name, built +from the concrete path as well; it is rewritten on the same terms, to +`POST /webhook/{uuid}` where the pattern is known and `POST +/(redacted)` where it is not. + +The headers are an allowlist for the same reason the rules above are +unconditional: 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, route pattern, 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 the scheme of the +retained URL, because the SDK derives that 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. +That is what the rewrite above preserves it for, and it is why +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. diff --git a/internal/server/sentry.go b/internal/server/sentry.go index af795aa..5ad9314 100644 --- a/internal/server/sentry.go +++ b/internal/server/sentry.go @@ -2,8 +2,11 @@ package server import ( "net/http" + "net/url" + "strings" "github.com/getsentry/sentry-go" + "github.com/go-chi/chi" ) // sentryRedacted stands in for a withheld field on every event shipped @@ -11,6 +14,12 @@ import ( // can tell a suppressed value from an absent one. const sentryRedacted = "(redacted)" +// sentryRedactedPath is what stands in for the request path when the +// route pattern is not reachable. It is deliberately not the concrete +// path: on the receiver route that path carries the entrypoint UUID, +// which is a write capability rather than an identifier. +const sentryRedactedPath = "/" + sentryRedacted + // 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. @@ -44,18 +53,43 @@ func sentryClientOptions(dsn, release string) sentry.ClientOptions { // login password and both password-change fields. None of that may // reach a third-party service. // +// URL is the third such field. NewRequest builds it as +// scheme://host/path (interfaces.go:183), and on the receiver route +// that path is /webhook/ in full — a write capability, not an +// identifier. It is rebuilt here from the chi route pattern, on every +// route, keeping the scheme and the host. +// // 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, + hint *sentry.EventHint, ) *sentry.Event { - if event == nil || event.Request == nil { + if event == nil { + return event + } + + pattern := sentryRoutePattern(hint) + + // Only transaction events carry a Transaction name, and the SDK + // builds it from the concrete path too (sentryhttp.go:105 via + // tracing.go:553). Rewritten on the same terms. + if event.Transaction != "" { + event.Transaction = sentryTransactionName( + event.Transaction, pattern, + ) + } + + if event.Request == nil { return event } req := event.Request + if req.URL != "" { + req.URL = sentryRouteURL(req.URL, pattern) + } + if req.QueryString != "" { req.QueryString = sentryRedacted } @@ -71,6 +105,91 @@ func scrubSentryRequest( return event } +// sentryRoutePattern returns the chi route pattern for the request the +// hint carries, or "" when it is not reachable. +// +// The request is reachable on the error dispatch only. sentryhttp's +// recover path calls RecoverWithContext with the request on the +// context under sentry.RequestContextKey (sentryhttp.go:124-125), and +// the client copies that context onto the hint (client.go:484-485) +// before handing it to BeforeSend (client.go:631). chi's routing +// context is a pointer placed on the request context before the +// middleware chain runs (chi mux.go:84) and filled in as the mux +// routes, so by the time a handler panics it names the matched route. +// +// The transaction dispatch has no such request: Span.doFinish calls +// hub.CaptureEvent (tracing.go:356), which passes a nil hint that the +// client replaces with an empty one (client.go:620-622). The pattern +// is therefore always "" there, and the callers fall back. +func sentryRoutePattern(hint *sentry.EventHint) string { + if hint == nil || hint.Context == nil { + return "" + } + + req, ok := hint.Context.Value( + sentry.RequestContextKey, + ).(*http.Request) + if !ok || req == nil { + return "" + } + + rctx := chi.RouteContext(req.Context()) + if rctx == nil { + return "" + } + + // Empty when no route matched, which is the fallback case too. + return rctx.RoutePattern() +} + +// sentryRouteURL rebuilds an event's request URL with the route +// pattern in place of the concrete path. +// +// The scheme is load-bearing and is kept: the SDK derives it from +// r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" +// (interfaces.go:180), byte for byte the predicate +// internal/middleware/csrf.go uses, so it is the CSRF TLS decision and +// the reason dropping X-Forwarded-Proto from the header allowlist +// costs nothing. The host is parsed.Host of the SDK's +// scheme://r.Host/path, so it is whatever the client's Host header +// carried: this service validates no hostname. It is kept because that +// same header is on the allowlist, so scrubbing it here would withhold +// nothing that is not sent anyway. +// +// Everything else in the URL is discarded rather than edited, so a +// future SDK that starts appending a query string cannot widen this. +func sentryRouteURL(rawURL, pattern string) string { + parsed, err := url.Parse(rawURL) + if err != nil || parsed.Scheme == "" { + // Not a shape this can safely take apart. + return sentryRedacted + } + + if pattern == "" { + pattern = sentryRedactedPath + } + + return parsed.Scheme + "://" + parsed.Host + pattern +} + +// sentryTransactionName rebuilds the SDK's "METHOD /path" transaction +// name with the route pattern in place of the concrete path. Method is +// kept for the same reason Request.Method is: net/http admits only a +// bounded token there. A name in any other shape is withheld whole, +// since nothing can be said about which part of it is a path. +func sentryTransactionName(name, pattern string) string { + method, _, found := strings.Cut(name, " ") + if !found { + return sentryRedacted + } + + if pattern == "" { + pattern = sentryRedactedPath + } + + return method + " " + pattern +} + // 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 diff --git a/internal/server/sentry_test.go b/internal/server/sentry_test.go index 6ebb756..9b7148e 100644 --- a/internal/server/sentry_test.go +++ b/internal/server/sentry_test.go @@ -13,12 +13,13 @@ import ( "github.com/getsentry/sentry-go" sentryhttp "github.com/getsentry/sentry-go/http" + "github.com/go-chi/chi" "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 +// The four 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 ( @@ -33,6 +34,12 @@ const ( // sentryHeaderMarker rides X-Csrf-Token, which gorilla/csrf // accepts in place of the form field. sentryHeaderMarker = "QQSENTRYHEADERMARKERQQ" + + // sentryReceiverUUID is the entrypoint identifier in the path of + // a receiver request. It is a write capability: anyone holding + // it can POST events this service accepts and its targets then + // deliver, so it may not reach a third-party tracker. + sentryReceiverUUID = "6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d" ) // sentryKeptUserAgent is a non-secret header value planted so the @@ -58,19 +65,41 @@ func (c *captureTransport) SendEvent(event *sentry.Event) { 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. +// sentryCase drives one request through the real sentryhttp middleware +// inside a real chi router and returns the events 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. +// Routing through a chi mux is load-bearing, not decoration. chi puts +// its routing context on the request context before the middleware +// chain runs and fills it in as it matches, so a hand-built request +// carries no route pattern at all and could not distinguish the hook +// working from the hook falling back. // -// 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 { +// This is also 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. +type sentryCase struct { + // scrub selects whether the production BeforeSend hooks are + // installed, so the same path shows both what the SDK collects + // and what survives. + scrub bool + + // tracing enables the transaction dispatch, which the service + // leaves off. With it on, a served request produces a + // transaction event through BeforeSendTransaction. + tracing bool + + // panics selects the error dispatch, via BeforeSend. + panics bool + + // request builds the request to serve, given the client whose + // hub it must carry. + request func(*sentry.Client) *http.Request +} + +func (c sentryCase) capture(t *testing.T) []*sentry.Event { t.Helper() transport := &captureTransport{} @@ -80,57 +109,107 @@ func captureThroughSentryHTTP(t *testing.T, scrub bool) *sentry.Event { ) opts.Transport = transport - if !scrub { + if !c.scrub { opts.BeforeSend = nil opts.BeforeSendTransaction = nil } + if c.tracing { + opts.EnableTracing = true + opts.TracesSampleRate = 1.0 + } + 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() + c.router().ServeHTTP(httptest.NewRecorder(), c.request(client)) - panic("boom") - }), - ) - - handler.ServeHTTP( - httptest.NewRecorder(), - sentryLoginRequest(client), - ) - - require.Len(t, transport.events, 1) - - return transport.events[0] + return transport.events } -// sentryLoginRequest builds the password POST the capture above drives, -// with a credential planted in the body, the query and a header. +// router mirrors setupGlobalMiddleware's ordering over the two route +// patterns these tests need: a recovering middleware first, then the +// sentryhttp handler registered with Use and Repanic set, exactly as +// routes.go registers it. The local recover stands in for chi's +// middleware.Recoverer, which holds that slot in production; it is +// here only to keep panic stacks out of the test output. +func (c sentryCase) router() http.Handler { + handler := func(_ http.ResponseWriter, r *http.Request) { + // This call is what drains the body tee and fills the + // buffer. Its success is asserted by the unscrubbed case + // below, which sees the body in the event. + _ = r.ParseForm() + + if c.panics { + panic("boom") + } + } + + router := chi.NewRouter() + router.Use(recoveringMiddleware) + router.Use( + sentryhttp.New(sentryhttp.Options{Repanic: true}).Handle, + ) + router.HandleFunc("/pages/login", handler) + router.HandleFunc("/webhook/{uuid}", handler) + + return router +} + +func recoveringMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + defer func() { _ = recover() }() + + next.ServeHTTP(w, r) + }, + ) +} + +// sentryLoginRequest builds the password POST most cases drive, 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 := sentryRequest( + client, + "/pages/login?url=https://hooks.slack.com/services/"+ + sentryQueryMarker, + form.Encode(), + ) + + req.Header.Set("X-Csrf-Token", sentryHeaderMarker) + + return req +} + +// sentryReceiverRequest builds a POST to the receiver route, whose +// concrete path carries the entrypoint capability. +func sentryReceiverRequest(client *sentry.Client) *http.Request { + return sentryRequest( + client, "/webhook/"+sentryReceiverUUID, "payload=hello", + ) +} + +func sentryRequest( + client *sentry.Client, + target, body string, +) *http.Request { 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()), + target, + strings.NewReader(body), ) 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 @@ -146,15 +225,27 @@ func marshalEvent(t *testing.T, event *sentry.Event) string { return string(encoded) } +// onlyEvent asserts a single event was captured and returns it. +func onlyEvent(t *testing.T, events []*sentry.Event) *sentry.Event { + t.Helper() + + require.Len(t, events, 1) + require.NotNil(t, events[0].Request) + + return events[0] +} + // 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. +// raw query, the CSRF header and the concrete request path, none of +// which SendDefaultPII=false suppresses. func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) { t.Parallel() - event := captureThroughSentryHTTP(t, false) - require.NotNil(t, event.Request) + event := onlyEvent(t, sentryCase{ + panics: true, + request: sentryLoginRequest, + }.capture(t)) assert.Contains( t, event.Request.Data, sentryBodyMarker, @@ -165,6 +256,18 @@ func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) { assert.Contains( t, marshalEvent(t, event), sentryHeaderMarker, ) + + receiver := onlyEvent(t, sentryCase{ + panics: true, + request: sentryReceiverRequest, + }.capture(t)) + + assert.Contains( + t, receiver.Request.URL, sentryReceiverUUID, + "the SDK is expected to build Request.URL from the "+ + "concrete path; if it no longer does, the route "+ + "pattern rewrite's premise changed", + ) } // TestSentryScrub_RedactsTheCapturedRequest is the regression test: no @@ -173,8 +276,11 @@ func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) { func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) { t.Parallel() - event := captureThroughSentryHTTP(t, true) - require.NotNil(t, event.Request) + event := onlyEvent(t, sentryCase{ + scrub: true, + panics: true, + request: sentryLoginRequest, + }.capture(t)) encoded := marshalEvent(t, event) @@ -189,16 +295,45 @@ func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) { assert.Empty(t, event.Request.Env) } +// TestSentryScrub_ReplacesTheCapabilityPathWithTheRoutePattern is the +// regression test for the receiver URL: the entrypoint UUID is a write +// capability and may not reach the tracker, while the route it names +// must still be readable there. +func TestSentryScrub_ReplacesTheCapabilityPathWithTheRoutePattern( + t *testing.T, +) { + t.Parallel() + + event := onlyEvent(t, sentryCase{ + scrub: true, + panics: true, + request: sentryReceiverRequest, + }.capture(t)) + + assert.NotContains( + t, marshalEvent(t, event), sentryReceiverUUID, + ) + assert.Equal( + t, "http://example.com/webhook/{uuid}", event.Request.URL, + ) +} + // TestSentryScrub_KeepsTheRoutingContext checks the hook does not cost -// the debugging signal: the route, the method and the metadata headers -// still identify what failed. +// the debugging signal: the route, its scheme and host, the method and +// the metadata headers still identify what failed. On a static route +// the pattern is the path, so the URL is unchanged there. func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) { t.Parallel() - event := captureThroughSentryHTTP(t, true) - require.NotNil(t, event.Request) + event := onlyEvent(t, sentryCase{ + scrub: true, + panics: true, + request: sentryLoginRequest, + }.capture(t)) - assert.Contains(t, event.Request.URL, "/pages/login") + assert.Equal( + t, "http://example.com/pages/login", event.Request.URL, + ) assert.Equal(t, http.MethodPost, event.Request.Method) assert.Equal( t, @@ -212,6 +347,127 @@ func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) { ) } +// TestSentryScrub_RedactsTheTransactionDispatch covers the other hook. +// Span.doFinish captures with a nil hint, so BeforeSendTransaction +// gets one with no context and no request: the route pattern is out of +// reach and both the URL and the SDK-built transaction name have to +// fall back. Tracing is off in this service, so no transaction event +// is produced today; the hook is a floor against that changing. +func TestSentryScrub_RedactsTheTransactionDispatch(t *testing.T) { + t.Parallel() + + events := sentryCase{ + scrub: true, + tracing: true, + request: sentryReceiverRequest, + }.capture(t) + + event := onlyEvent(t, events) + require.Equal(t, "transaction", event.Type) + + assert.NotContains( + t, marshalEvent(t, event), sentryReceiverUUID, + ) + assert.Equal( + t, "http://example.com/(redacted)", event.Request.URL, + ) + assert.Equal(t, "POST /(redacted)", event.Transaction) +} + +// TestSentryScrub_TransactionDispatchIsUnscrubbedWithoutTheHook pins +// that dispatch's premise the same way, since it is the one the +// service does not exercise today. +func TestSentryScrub_TransactionDispatchIsUnscrubbedWithoutTheHook( + t *testing.T, +) { + t.Parallel() + + event := onlyEvent(t, sentryCase{ + tracing: true, + request: sentryReceiverRequest, + }.capture(t)) + + require.Equal(t, "transaction", event.Type) + assert.Contains(t, event.Request.URL, sentryReceiverUUID) + assert.Contains(t, event.Transaction, sentryReceiverUUID) +} + +// TestSentryScrub_FallsBackWithoutARoutePattern covers every way the +// pattern can be missing. None of them may fall back to the concrete +// path, and all of them keep the scheme, which is the CSRF TLS +// decision. +func TestSentryScrub_FallsBackWithoutARoutePattern(t *testing.T) { + t.Parallel() + + concrete := "https://example.com/webhook/" + sentryReceiverUUID + + // A request with no chi routing context on it at all, which is + // what an event captured outside the router would carry. + unrouted := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, concrete, nil, + ) + + for name, hint := range map[string]*sentry.EventHint{ + "no hint": nil, + "no context": {}, + "no request": {Context: context.Background()}, + "unrouted request": { + Context: context.WithValue( + context.Background(), + sentry.RequestContextKey, + unrouted, + ), + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + event := sentry.NewEvent() + event.Request = &sentry.Request{URL: concrete} + event.Transaction = "POST /webhook/" + + sentryReceiverUUID + + scrubbed := server.ScrubSentryRequestForTest( + event, hint, + ) + require.NotNil(t, scrubbed) + + assert.Equal( + t, + "https://example.com/(redacted)", + scrubbed.Request.URL, + ) + assert.Equal( + t, "POST /(redacted)", scrubbed.Transaction, + ) + assert.NotContains( + t, + marshalEvent(t, scrubbed), + sentryReceiverUUID, + ) + }) + } +} + +// TestSentryScrub_WithholdsUnparseableValues covers the shapes the +// rewrite cannot take apart. Withholding them whole is the safe +// answer, since nothing can be said about which part is a path. +func TestSentryScrub_WithholdsUnparseableValues(t *testing.T) { + t.Parallel() + + event := sentry.NewEvent() + event.Request = &sentry.Request{ + URL: "/webhook/" + sentryReceiverUUID, + } + event.Transaction = "/webhook/" + sentryReceiverUUID + + scrubbed := server.ScrubSentryRequestForTest(event, nil) + require.NotNil(t, scrubbed) + + assert.Equal(t, "(redacted)", scrubbed.Request.URL) + assert.Equal(t, "(redacted)", scrubbed.Transaction) +} + // TestSentryScrub_ToleratesEventsWithoutARequest covers the events the // hook sees outside an HTTP handler, where no request is attached. func TestSentryScrub_ToleratesEventsWithoutARequest(t *testing.T) { @@ -223,5 +479,6 @@ func TestSentryScrub_ToleratesEventsWithoutARequest(t *testing.T) { require.NotNil(t, scrubbed) assert.Nil(t, scrubbed.Request) + assert.Empty(t, scrubbed.Transaction) assert.Nil(t, server.ScrubSentryRequestForTest(nil, nil)) }