diff --git a/README.md b/README.md index 7a2a892..124fd68 100644 --- a/README.md +++ b/README.md @@ -2551,10 +2551,23 @@ Where the pattern is out of reach — the transaction dispatch, an event capture 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 /h/{uuid}` where the pattern is known and -`POST /(redacted)` where it is not. +withheld whole. + +A transaction event also carries a name, which the SDK builds as `METHOD /path` +from the concrete path and gives the transaction source `url`. Sentry groups +transactions by name, so withholding it would put every request in the service +into one group per method, such as `POST /(redacted)`. Inside each Sentry +handler, a second middleware renames the transaction once the request has been +routed, to the method and the route pattern (`POST /h/{uuid}`), and sets its +source to `route`. The hook keeps a name whose source is `route` and rewrites +any other to `METHOD /(redacted)`. A path that matches nothing at the top level +has no pattern, so its transaction keeps the SDK's name and the hook withholds +it. Inside a route group, a path that matches no route in the group, and a +request the group's own middleware turns away before routing (a CSRF refusal, +the redirect to `/pages/login`), are named after the group's pattern, such as +`GET /pages/*` or `POST /hook/{sourceID}/*`, and the hook keeps that name. The +URL on a transaction event still falls back to `http://host/(redacted)`, because +the hook has no request there. 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 @@ -3175,7 +3188,8 @@ Applied to all routes in this order: 7. **Recoverer** — Panic recovery: one `ERROR` record through `internal/logger` and a `500` 8. **Sentry** — Error reporting to Sentry (if `SENTRY_DSN` is set; configured - with `Repanic: true` so panics still reach Recoverer) + with `Repanic: true` so panics still reach Recoverer), and inside it the + naming of the request's Sentry transaction after its route pattern Recoverer sits seventh rather than first, and both neighbours are the reason. It runs **inside** everything that observes the response, so the `500` it writes @@ -3190,9 +3204,9 @@ set a header or start a timer. Each admin page route group (`/pages`, `/user/*`, `/settings`, `/hooks`, `/hook/*`) starts with its own **Recoverer** and, if `SENTRY_DSN` is set, its -own **Sentry** error reporting. That Recoverer answers a panic with the `500` -error page in the normal layout; the global one keeps the plain-text `500` for -every other route. +own **Sentry** error reporting and transaction naming. That Recoverer answers a +panic with the `500` error page in the normal layout; the global one keeps the +plain-text `500` for every other route. Additionally, form endpoints (`/pages`, `/user/*`, `/settings`, `/hooks`, `/hook/*`) apply a **MaxBodySize** middleware that limits POST/PUT/PATCH request diff --git a/internal/server/export_test.go b/internal/server/export_test.go index 957f4dd..3a3717e 100644 --- a/internal/server/export_test.go +++ b/internal/server/export_test.go @@ -91,6 +91,24 @@ func NewRouterForTest( return s.router } +// NewRouterWithSentryForTest is NewRouterForTest with the sentryhttp +// handler registered, as a set SENTRY_DSN does in production. +func NewRouterWithSentryForTest( + t *testing.T, + log *logger.Logger, + cfg *config.Config, + mw *middleware.Middleware, + h *handlers.Handlers, +) http.Handler { + t.Helper() + + s := newServerForTest(t, log, cfg, mw, h) + s.sentryEnabled.Store(true) + s.SetupRoutes() + + return s.router +} + // ListenAddrForTest exposes the address the HTTP listener binds for // a given Config, so the rendering of host and port — IPv6 // bracketing above all — can be pinned without standing up a diff --git a/internal/server/routes.go b/internal/server/routes.go index 40e7113..d7210ab 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -90,8 +90,8 @@ func (s *Server) setupGlobalMiddleware() { // recoverPanics installs on r the recoverer, answering a panic with // page (a plain 500 when page is nil), and inside it the Sentry error -// reporting (if SENTRY_DSN is set). Repanic is true so panics still -// bubble up to the recoverer. +// reporting and transaction naming (if SENTRY_DSN is set). Repanic is +// true so panics still bubble up to the recoverer. // // Each admin page route group installs its own, with the error page, // as its first middleware. A panic there is logged, reported and @@ -105,6 +105,11 @@ func (s *Server) recoverPanics(r chi.Router, page http.Handler) { Repanic: true, }) r.Use(sentryHandler.Handle) + // Installed with every sentryhttp handler rather than once + // globally: in an admin page route group, the group's own + // handler finishes the transaction as the group returns, so + // a rename further out would come too late. + r.Use(nameSentryTransaction) } } diff --git a/internal/server/sentry.go b/internal/server/sentry.go index 086c3a6..fdd2dbd 100644 --- a/internal/server/sentry.go +++ b/internal/server/sentry.go @@ -1,6 +1,7 @@ package server import ( + "context" "net/http" "net/url" "strings" @@ -71,10 +72,15 @@ func scrubSentryRequest( pattern := sentryRoutePattern(hint) - // Only transaction events carry a Transaction name, and the SDK + // Only transaction events carry a Transaction name. sentryhttp // builds it from the concrete path too (sentryhttp.go:105 via - // tracing.go:553). Rewritten on the same terms. - if event.Transaction != "" { + // tracing.go:553) and marks it SourceURL, so it is rewritten on + // the same terms. Only a name nameSentryTransaction set from the + // route pattern, which it marks SourceRoute, is kept. + namedByRoute := event.TransactionInfo != nil && + event.TransactionInfo.Source == sentry.SourceRoute + + if event.Transaction != "" && !namedByRoute { event.Transaction = sentryTransactionName( event.Transaction, pattern, ) @@ -120,7 +126,8 @@ func scrubSentryRequest( // 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. +// is therefore always "" there, and the callers fall back. A routed +// transaction is named by nameSentryTransaction instead. func sentryRoutePattern(hint *sentry.EventHint) string { if hint == nil || hint.Context == nil { return "" @@ -142,6 +149,52 @@ func sentryRoutePattern(hint *sentry.EventHint) string { return rctx.RoutePattern() } +// nameSentryTransaction renames the request's Sentry transaction to +// "METHOD ", such as "POST /h/{uuid}". sentryhttp names +// it after the concrete path, which scrubSentryRequest withholds; +// since Sentry groups transactions by name, every transaction in the +// service would otherwise land in one group per method, such as +// "POST /(redacted)". +// +// It has to run inside the sentryhttp handler, where the transaction +// is on the request context. It renames on the way out, once chi has +// routed the request, and does so in a defer so that a request whose +// handler panics is named too. The context is taken when the defer is +// set up, before routing, but the chi routing context on it is a +// pointer that chi fills in as it routes. +func nameSentryTransaction(next http.Handler) http.Handler { + return http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + defer setSentryTransactionName(r.Context(), r.Method) + + next.ServeHTTP(w, r) + }, + ) +} + +// setSentryTransactionName does the rename for nameSentryTransaction. +// A path that matches nothing at the top level has no pattern; its +// name is left as the SDK set it, for scrubSentryRequest to withhold. +// Inside a route group, a path that matches no route in the group, or +// a request the group's own middleware turns away before routing, has +// the group's pattern, such as "/pages/*", and is named after it. +func setSentryTransactionName(ctx context.Context, method string) { + transaction := sentry.TransactionFromContext(ctx) + rctx := chi.RouteContext(ctx) + + if transaction == nil || rctx == nil { + return + } + + pattern := rctx.RoutePattern() + if pattern == "" { + return + } + + transaction.Name = method + " " + pattern + transaction.Source = sentry.SourceRoute +} + // sentryRouteURL rebuilds an event's request URL with the route // pattern in place of the concrete path. // diff --git a/internal/server/sentry_test.go b/internal/server/sentry_test.go index 86e5cb0..cf1dcdc 100644 --- a/internal/server/sentry_test.go +++ b/internal/server/sentry_test.go @@ -16,6 +16,7 @@ import ( "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/server" ) @@ -130,11 +131,15 @@ func (c sentryCase) capture(t *testing.T) []*sentry.Event { // router mirrors the one ordering these tests depend on, over the two // route patterns they need: a recovering middleware outside, then the // sentryhttp handler registered with Use and Repanic set, exactly as -// routes.go orders the two. The bare recover stands in for -// Middleware.Recoverer, which holds that outer slot in production; it -// is here only to keep panic stacks out of the test output. That the -// production one really does catch what sentryhttp re-raises is -// pinned separately, by TestSentryStillSeesAPanic. +// routes.go orders the two. It leaves out nameSentryTransaction, +// which routes.go installs inside the sentryhttp handler, so a +// transaction captured here keeps the SDK's name and shows what the +// hook does with a transaction that missed the rename. The bare +// recover stands in for Middleware.Recoverer, which holds that outer +// slot in production; it is here only to keep panic stacks out of the +// test output. That the production one really does catch what +// sentryhttp re-raises is pinned separately, by +// TestSentryStillSeesAPanic. func (c sentryCase) router() http.Handler { handler := func(_ http.ResponseWriter, r *http.Request) { // This call is what drains the body tee and fills the @@ -353,8 +358,9 @@ func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) { // 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. +// fall back. This router has no nameSentryTransaction, so this is the +// floor under a transaction that missed the rename; +// TestSentryTransaction_NamedByRoute covers the routes as they ship. func TestSentryScrub_RedactsTheTransactionDispatch(t *testing.T) { t.Parallel() @@ -376,6 +382,83 @@ func TestSentryScrub_RedactsTheTransactionDispatch(t *testing.T) { assert.Equal(t, "POST /(redacted)", event.Transaction) } +// TestSentryTransaction_NamedByRoute serves requests through the real +// route tree with tracing on, which the service leaves off. A routed +// request's transaction is named after its route pattern, so Sentry +// can group transactions by route, and the entrypoint UUID in the +// receiver path appears nowhere in the event. A path that matches +// nothing at the top level has no pattern, and its name is still +// withheld; one that matches no route inside a route group is named +// after the group's pattern. +func TestSentryTransaction_NamedByRoute(t *testing.T) { + t.Parallel() + + // newTestEnv leaves the receiver rate limit at zero, which + // refuses every request before it reaches the receiver. + env := newTestEnvWithConfig(t, &config.Config{ + DataDir: t.TempDir(), + Environment: config.EnvironmentDev, + ReceiverRateLimit: 10, + }) + + router := server.NewRouterWithSentryForTest( + t, env.log, env.cfg, env.mw, env.hnd, + ) + + for _, tc := range []struct { + method, path, want string + }{ + { + http.MethodPost, + "/h/" + sentryReceiverUUID, + "POST /h/{uuid}", + }, + // An admin page route group, which installs its own + // sentryhttp handler. + {http.MethodGet, "/pages/login", "GET /pages/login"}, + {http.MethodGet, "/pages/unrouted", "GET /pages/*"}, + { + http.MethodPost, + "/h/" + sentryReceiverUUID + "/unrouted", + "POST /(redacted)", + }, + } { + t.Run(tc.want, func(t *testing.T) { + t.Parallel() + + transport := &captureTransport{} + + opts := server.SentryClientOptionsForTest( + "https://public@sentry.invalid/1", + "webhooker-test", + ) + opts.Transport = transport + opts.EnableTracing = true + opts.TracesSampleRate = 1.0 + + client, err := sentry.NewClient(opts) + require.NoError(t, err) + + req := httptest.NewRequestWithContext( + sentry.SetHubOnContext( + context.Background(), + sentry.NewHub(client, sentry.NewScope()), + ), + tc.method, tc.path, nil, + ) + router.ServeHTTP(httptest.NewRecorder(), req) + + event := onlyEvent(t, transport.events) + require.Equal(t, "transaction", event.Type) + + assert.Equal(t, tc.want, event.Transaction) + assert.NotContains( + t, marshalEvent(t, event), sentryReceiverUUID, + ) + }) + } +} + // TestSentryScrub_TransactionDispatchIsUnscrubbedWithoutTheHook pins // that dispatch's premise the same way, since it is the one the // service does not exercise today.