From 2ef52bbac2e2a597d7ef16f32b02f4d83ad43cdb Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 23 Aug 2026 23:47:04 +0000 Subject: [PATCH] Bound the /metrics method label (closes #261) The `method` label was recorded as `r.Method` verbatim. net/http accepts any RFC 9110 token as a method and passes it through, so the label was bounded at nothing: 300 requests to `/` carrying random 12-character method tokens took a live scrape from 81 lines to 7,606, unauthenticated and on a route with no rate limiter. This is the same remote memory-exhaustion vector the handler label carried, reached through a second dimension. The fix goes in the same recorder seam that bounds the handler label, which is renamed to reflect that it now bounds both. A method the router can route is kept verbatim, so real methods stay distinguishable; anything else carries the existing `(unmatched)` sentinel, deliberately the same spelling rather than a second one for the same idea. The bound is ten values. The retained set is restated against the net/http constants because chi's own methodMap is unexported. A token outside it can only ever produce chi's 405, so folding those together loses nothing a scrape could have used. README's Metrics section gains the inbound HTTP metrics, the bound on each of their labels, and the aggregate `http_requests_inflight{handler="(all)"}` semantics. --- README.md | 34 +++ internal/middleware/export_test.go | 4 + internal/middleware/metrics.go | 67 ++++- internal/middleware/metrics_method_test.go | 309 +++++++++++++++++++++ internal/middleware/metrics_test.go | 41 ++- 5 files changed, 436 insertions(+), 19 deletions(-) create mode 100644 internal/middleware/metrics_method_test.go diff --git a/README.md b/README.md index 486233a..43b9565 100644 --- a/README.md +++ b/README.md @@ -1677,6 +1677,40 @@ gauge. The outcome counters move only after the status change has been written, so a transition the database rejected is never reported as an outcome that happened. +#### Inbound HTTP metrics + +The middleware records three more on the same registry: + +| Metric | Type | Labels | +| ------ | ---- | ------ | +| `http_request_duration_seconds` | histogram | `service`, `handler`, `method`, `code` | +| `http_response_size_bytes` | histogram | `service`, `handler`, `method`, `code` | +| `http_requests_inflight` | gauge | `service`, `handler` | + +Two of those labels are written once per request from bytes the client +chose, so both are bounded to something this service registers: + +- `handler` is the chi route pattern — `/webhook/{uuid}`, never the + concrete path. A request matching no route carries `(unmatched)`, + and no entrypoint UUID ever reaches a label. +- `method` is the request method when the router can route it, and + `(unmatched)` otherwise. `net/http` accepts any RFC 9110 token as a + method, so the raw value bounds the label at nothing; the nine chi + matches routes for stay distinguishable, and a token that could only + ever have produced a 405 does not get a series of its own. + +The other two are not request-controlled: `code` is the status one of +this service's own handlers wrote, and `service` is a fixed empty +string. + +`http_requests_inflight` is deliberately aggregate — its `handler` is +always `(all)`, one series counting the requests in flight across the +whole service. The gauge is incremented before routing and decremented +after the handler returns, and the route pattern exists only between +those two moments, so labelling it by pattern would increment one +series and decrement another, leaving every pattern permanently off by +the number of requests it served. + ### Rate Limiting Global blanket rate limiting middleware (e.g., a per-IP throttle shared diff --git a/internal/middleware/export_test.go b/internal/middleware/export_test.go index d0033d4..b75e467 100644 --- a/internal/middleware/export_test.go +++ b/internal/middleware/export_test.go @@ -26,6 +26,10 @@ const UnmatchedRouteConst = unmatchedRoute // inflight gauge. const InflightHandlerConst = inflightHandler +// UnmatchedMethodConst exposes the sentinel that stands in for a +// method the router can never route. +const UnmatchedMethodConst = unmatchedMethod + // NewLoggingResponseWriterForTest wraps newLoggingResponseWriter // for use in external test packages. func NewLoggingResponseWriterForTest( diff --git a/internal/middleware/metrics.go b/internal/middleware/metrics.go index 14d36a2..6ac39be 100644 --- a/internal/middleware/metrics.go +++ b/internal/middleware/metrics.go @@ -26,6 +26,15 @@ import ( // counting the requests in flight across the whole service. const inflightHandler = "(all)" +// unmatchedMethod is the `method` label for a request whose method +// the router can never route. +// +// It is deliberately the same sentinel as unmatchedRoute rather than +// a spelling of its own: both stand for a client-chosen token that +// matched nothing this service registers, and giving one idea two +// spellings would read in a scrape as two different unmatched states. +const unmatchedMethod = unmatchedRoute + // routePatternID is the `handler` label for a request: the chi route // pattern, never the concrete path. // @@ -50,9 +59,45 @@ func routePatternID(ctx context.Context) string { return unmatchedRoute } -// routePatternRecorder wraps a go-http-metrics recorder and replaces -// the handler id on every observation with the request's route -// pattern. +// methodID is the `method` label for a request: the request method +// when the router can route it, and the unmatched sentinel otherwise. +// +// net/http accepts any RFC 9110 token as a method and hands it +// through verbatim, so the raw method is client-chosen bytes and +// bounds the label at nothing — the same unauthenticated +// series-minting the handler label carried, reached through a second +// dimension. What bounds it is the set chi's router will match a +// route for: its methodMap, which is unexported, so it is restated +// here against the net/http constants it is built from. A token +// outside that set can only ever produce chi's 405, so folding every +// one of them onto a single series loses no information a scrape +// could have used, while the nine methods that can reach a handler +// stay distinguishable. +// +// chi.RegisterMethod would extend the router's set at runtime; this +// service never calls it, and a caller that started to would have to +// extend this switch with it. +func methodID(method string) string { + switch method { + case http.MethodConnect, + http.MethodDelete, + http.MethodGet, + http.MethodHead, + http.MethodOptions, + http.MethodPatch, + http.MethodPost, + http.MethodPut, + http.MethodTrace: + return method + default: + return unmatchedMethod + } +} + +// boundedLabelRecorder wraps a go-http-metrics recorder and replaces +// the request-controlled labels on every observation with bounded +// ones: the handler id becomes the request's route pattern, and the +// method becomes one the router can route. // // This is the seam that makes the pattern usable at all. The metrics // middleware is global (see Server.setupGlobalMiddleware), so it is @@ -71,29 +116,31 @@ func routePatternID(ctx context.Context) string { // Those never reach a handler, but chi has already matched the route // by the time the limiter runs, so their 429s land on the pattern // like any other response. -type routePatternRecorder struct { +type boundedLabelRecorder struct { inner httpmetrics.Recorder } -func (r routePatternRecorder) ObserveHTTPRequestDuration( +func (r boundedLabelRecorder) ObserveHTTPRequestDuration( ctx context.Context, props httpmetrics.HTTPReqProperties, duration time.Duration, ) { props.ID = routePatternID(ctx) + props.Method = methodID(props.Method) r.inner.ObserveHTTPRequestDuration(ctx, props, duration) } -func (r routePatternRecorder) ObserveHTTPResponseSize( +func (r boundedLabelRecorder) ObserveHTTPResponseSize( ctx context.Context, props httpmetrics.HTTPReqProperties, sizeBytes int64, ) { props.ID = routePatternID(ctx) + props.Method = methodID(props.Method) r.inner.ObserveHTTPResponseSize(ctx, props, sizeBytes) } -func (r routePatternRecorder) AddInflightRequests( +func (r boundedLabelRecorder) AddInflightRequests( ctx context.Context, props httpmetrics.HTTPProperties, quantity int, @@ -102,7 +149,7 @@ func (r routePatternRecorder) AddInflightRequests( r.inner.AddInflightRequests(ctx, props, quantity) } -var _ httpmetrics.Recorder = routePatternRecorder{} +var _ httpmetrics.Recorder = boundedLabelRecorder{} // Metrics returns middleware that records Prometheus HTTP metrics on // the default registry, which is the one the /metrics route gathers. @@ -119,14 +166,14 @@ func metricsMiddleware( rec httpmetrics.Recorder, ) func(http.Handler) http.Handler { mdlw := ghmm.New(ghmm.Config{ - Recorder: routePatternRecorder{inner: rec}, + Recorder: boundedLabelRecorder{inner: rec}, }) return func(next http.Handler) http.Handler { // The handler id is unmatchedRoute rather than "" so that // the client-chosen URL path never enters the metrics // pipeline at all: an empty id is the library's signal to - // substitute it. routePatternRecorder overwrites this value + // substitute it. boundedLabelRecorder overwrites this value // on every observation, so it is reachable only if that // decorator is removed — in which case the metrics collapse // to one series instead of leaking again. diff --git a/internal/middleware/metrics_method_test.go b/internal/middleware/metrics_method_test.go new file mode 100644 index 0000000..3a47cde --- /dev/null +++ b/internal/middleware/metrics_method_test.go @@ -0,0 +1,309 @@ +package middleware_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promhttp" + dto "github.com/prometheus/client_model/go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/middleware" +) + +const ( + // metricsProbeMethods is how many distinct invented method tokens + // each cardinality assertion drives. The measurement on the issue + // took 300 tokens from 106 exposition lines to 7,631 — about 25 + // permanent lines per token, never reclaimed — so a probe of this + // size puts a regression thousands of lines over the bound rather + // than leaving it to a rounding argument. + metricsProbeMethods = 300 + + // probeMethodLen is how many characters each invented method + // token carries, matching the 12 the issue measured with. + probeMethodLen = 12 + + // methodLabel is the label these tests are about. + methodLabel = "method" +) + +// realMethods is the positive control's domain: the methods chi's +// router can match a route for, every one of which a client +// legitimately sends and every one of which must keep a series of its +// own. Bounding the label by collapsing these into one bucket would +// destroy the metric it is meant to protect. +func realMethods() []string { + return []string{ + http.MethodConnect, http.MethodDelete, http.MethodGet, + http.MethodHead, http.MethodOptions, http.MethodPatch, + http.MethodPost, http.MethodPut, http.MethodTrace, + } +} + +// methodProbePath returns the one receiver path a method probe +// targets. Holding the path fixed leaves the method as the only +// dimension varying, so any series growth a probe produces is the +// method label's and nothing else's. +func methodProbePath() string { + return "/webhook/" + uuid.NewString() +} + +// inventedMethods returns n distinct RFC 9110 method tokens that no +// router will ever match: uppercase hex from a fresh UUID, which is +// both the shape and the length an unauthenticated flood would send. +// net/http accepts any token as a method, so every one of these +// reaches the metrics pipeline exactly as a real method does. +func inventedMethods(n int) []string { + methods := make([]string, 0, n) + + for range n { + token := strings.ToUpper( + strings.ReplaceAll(uuid.NewString(), "-", ""), + ) + methods = append(methods, token[:probeMethodLen]) + } + + return methods +} + +// driveMethods sends one request per supplied method to a single +// fixed path. +func driveMethods( + t *testing.T, + h http.Handler, + path string, + methods []string, +) map[int]int { + t.Helper() + + probes := make([]probe, 0, len(methods)) + + for _, m := range methods { + probes = append(probes, probe{method: m, path: path}) + } + + return drive(t, h, probes) +} + +// methodLabels returns the set of distinct `method` values across +// every gathered series that carries the label at all. The inflight +// gauge does not carry it, and so contributes nothing rather than an +// empty-string member. +func methodLabels(families []*dto.MetricFamily) map[string]struct{} { + seen := make(map[string]struct{}) + + for _, fam := range families { + for _, m := range fam.GetMetric() { + for _, pair := range m.GetLabel() { + if pair.GetName() == methodLabel { + seen[pair.GetValue()] = struct{}{} + } + } + } + } + + return seen +} + +// scrapeLines renders the registry through the same promhttp handler +// /metrics is mounted on and counts the sample lines it produced. +// +// This is the quantity the issue measured and the one a Prometheus +// server pays for on every scrape: one histogram label set is a +// single gathered series but around 25 lines of exposition, which is +// why 300 method tokens cost thousands of lines rather than hundreds. +func scrapeLines(t *testing.T, reg *prometheus.Registry) int { + t.Helper() + + h := promhttp.HandlerFor(reg, promhttp.HandlerOpts{}) + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/metrics", nil, + ) + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + require.Equal(t, http.StatusOK, w.Code) + + lines := 0 + + for line := range strings.SplitSeq(w.Body.String(), "\n") { + if line == "" || strings.HasPrefix(line, "#") { + continue + } + + lines++ + } + + return lines +} + +// TestMetrics_MethodSentinelIsTheRouteSentinel pins the convention +// rather than the mechanism. An unroutable method and an unmatched +// path are the same fact — a client-chosen token matching nothing +// this service registers — so they carry one spelling. Two spellings +// would read in a scrape as two different unmatched states. +func TestMetrics_MethodSentinelIsTheRouteSentinel(t *testing.T) { + t.Parallel() + + assert.Equal( + t, + middleware.UnmatchedRouteConst, + middleware.UnmatchedMethodConst, + "the unmatched sentinel must have exactly one spelling", + ) +} + +// TestMetrics_InventedMethodsMintOneLabelSet is the direct assertion +// the issue asks for: N requests carrying N distinct invented method +// tokens must produce exactly ONE method label. Before the fix this +// produced N of them, on an unauthenticated route with no rate +// limiter. +func TestMetrics_InventedMethodsMintOneLabelSet(t *testing.T) { + t.Parallel() + + h, reg := metricsTestRouter(t, generousReceiverLimit) + + methods := inventedMethods(metricsProbeMethods) + + codes := driveMethods(t, h, methodProbePath(), methods) + require.Equal( + t, metricsProbeMethods, codes[http.StatusMethodNotAllowed], + "every invented token should have been unroutable", + ) + + labels := methodLabels(gatherMetrics(t, reg)) + + // Asserted on the count rather than on the set, so that a + // regression reports one number instead of dumping every token it + // minted. + distinct := len(labels) + + assert.Equal( + t, 1, distinct, + "invented methods must collapse onto one label", + ) + assert.Contains( + t, keys(labels), middleware.UnmatchedMethodConst, + "that one label must be the unmatched sentinel", + ) + + // The scrape must not republish the tokens it was driven with + // either: a label that merely looks bounded while still echoing + // client bytes is the same defect wearing a different name. + echoed := 0 + + for _, m := range methods { + for label := range labels { + if strings.Contains(label, m) { + echoed++ + } + } + } + + assert.Equal( + t, 0, echoed, + "invented method tokens reached the metrics labels", + ) +} + +// TestMetrics_MethodSeriesCountIsFlatUnderAFlood reproduces the +// measurement on the issue in miniature: scrape, drive several +// hundred distinct method tokens, scrape again, and require the +// second scrape to be no larger than the first. The first batch +// establishes every label set the route can produce; a flood five +// times its size must land on exactly those. +func TestMetrics_MethodSeriesCountIsFlatUnderAFlood(t *testing.T) { + t.Parallel() + + h, reg := metricsTestRouter(t, generousReceiverLimit) + + path := methodProbePath() + + driveMethods(t, h, path, inventedMethods(metricsProbeMethods)) + seededSeries := seriesCount(gatherMetrics(t, reg)) + seededLines := scrapeLines(t, reg) + + driveMethods(t, h, path, inventedMethods(metricsProbeMethods*4)) + floodedSeries := seriesCount(gatherMetrics(t, reg)) + floodedLines := scrapeLines(t, reg) + + t.Logf( + "after %d invented methods: %d series, %d lines; "+ + "after %d more: %d series, %d lines", + metricsProbeMethods, seededSeries, seededLines, + metricsProbeMethods*4, floodedSeries, floodedLines, + ) + + assert.Equal( + t, seededSeries, floodedSeries, + "a flood of invented methods must not mint series", + ) + assert.Equal( + t, seededLines, floodedLines, + "a flood of invented methods must not grow the scrape", + ) +} + +// TestMetrics_RealMethodsStayDistinct is the positive control. The +// bound is worth nothing if it is bought by flattening the metric: +// every method the router can route must still carry a series of its +// own, one sample each, under the route pattern it was sent to. +func TestMetrics_RealMethodsStayDistinct(t *testing.T) { + t.Parallel() + + h, reg := metricsTestRouter(t, generousReceiverLimit) + + methods := realMethods() + + codes := driveMethods(t, h, methodProbePath(), methods) + require.Equal( + t, len(methods), codes[http.StatusNotFound], + "every real method should have reached the receiver", + ) + + families := gatherMetrics(t, reg) + + want := make(map[string]struct{}, len(methods)) + for _, m := range methods { + want[m] = struct{}{} + } + + assert.Equal( + t, want, methodLabels(families), + "real methods must remain distinguishable", + ) + + // Appearing somewhere in the scrape is not enough: each method + // must own its duration series, holding the one sample it sent. + observed := 0 + + for _, fam := range families { + if !strings.HasSuffix(fam.GetName(), "request_duration_seconds") { + continue + } + + for _, m := range fam.GetMetric() { + observed++ + + assert.Equal( + t, receiverRoutePattern, + labelValue(m, "handler"), + ) + assert.Equal( + t, uint64(1), + m.GetHistogram().GetSampleCount(), + "method %q shares a series", + labelValue(m, methodLabel), + ) + } + } + + assert.Equal( + t, len(methods), observed, + "one duration series per routable method", + ) +} diff --git a/internal/middleware/metrics_test.go b/internal/middleware/metrics_test.go index 39776f2..d19c30c 100644 --- a/internal/middleware/metrics_test.go +++ b/internal/middleware/metrics_test.go @@ -97,20 +97,24 @@ func metricsTestRouter( return r, reg } -// drivePaths sends one POST per supplied path and returns how many -// responses carried each status code. -func drivePaths( - t *testing.T, - h http.Handler, - paths []string, -) map[int]int { +// probe is one request a cardinality assertion sends. Both label +// dimensions that have leaked are request-controlled — the path and +// the method — so both vary here and one driver sends them. +type probe struct { + method string + path string +} + +// drive sends every probe and returns how many responses carried each +// status code. +func drive(t *testing.T, h http.Handler, probes []probe) map[int]int { t.Helper() codes := make(map[int]int) - for _, p := range paths { + for _, p := range probes { req := httptest.NewRequestWithContext( - t.Context(), http.MethodPost, p, nil, + t.Context(), p.method, p.path, nil, ) w := httptest.NewRecorder() h.ServeHTTP(w, req) @@ -120,6 +124,25 @@ func drivePaths( return codes } +// drivePaths sends one POST per supplied path. +func drivePaths( + t *testing.T, + h http.Handler, + paths []string, +) map[int]int { + t.Helper() + + probes := make([]probe, 0, len(paths)) + + for _, p := range paths { + probes = append( + probes, probe{method: http.MethodPost, path: p}, + ) + } + + return drive(t, h, probes) +} + // receiverPaths returns n distinct /webhook/ paths, each naming a // fresh UUID exactly as an unauthenticated flood would. func receiverPaths(n int) []string {