Enabling /metrics lets an unauthenticated client grow the process without bound, and publishes live entrypoint UUIDs #254
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/webhooker#254
Reference in New Issue
Block a user
The metrics middleware labels the
handlerdimension with the literal request path instead of the chi route pattern, so every distinct/webhook/<anything>mints a permanent new label set.Measured during the deployability audit against a live instance with
METRICS_USERNAME/METRICS_PASSWORDset:handlerlabels, RSS 143 MB.Roughly 26 permanent series and 3.5 KB of scrape output per distinct path, retained for the life of the process.
Rate limiting does not bound this. 45,025 of the new series carry
code="429"— the metrics recorder is global middleware and runs BEFORE the route-level rate limiter, so rejected requests still mint series. The aggregate limit (1,200/min/IP) therefore caps this at roughly 31,000 new series per minute, indefinitely, from a single address.Control: the same 6,000-distinct-path load against a metrics-DISABLED instance produced no retained growth (RSS 227 MB to 41 MB after GC).
Second effect, independent of the cardinality problem: live entrypoint UUIDs appear verbatim in the scrape as
handler="/webhook/aaaaaaaa-bbbb-...". The entrypoint UUID is the receiver's only credential. Authenticated URL path segments leak the same way (handler="/source/SECRET-PATH-SEGMENT/logs").Metrics are default-off, which is the only reason this is not already exploitable. But #209 shipped delivery metrics precisely so an operator can alert on delivery failures, so the expected production configuration turns this on — at which point a public unauthenticated endpoint becomes a remote memory-exhaustion vector.
This is the last surviving instance of the class fixed for Sentry in #179. The access log in the very same request path already does it correctly, emitting
"url":"/webhook/{uuid}".Definition of done
handlerlabel carries the chi route pattern, not the concrete path.chi.RouteContext(r.Context()).RoutePattern()./metricsoutput./webhook/<uuid>paths produce exactly ONEhandler="/webhook/{uuid}"label set.code="429"), since those were the majority of the leaked series and they take a different path through the middleware stack.Verification
make checkgreen.Plan.
The
handlerlabel comes fromstd.Handler("", ...)inMiddleware.Metrics: with an empty handler id,go-http-metricssubstitutesreporter.URLPath(), i.e. the concrete path. The pattern is not available at that point — the metrics recorder is global middleware and chi only populatesRouteContext.RoutePattern()duringrouteHTTP, after the whole global chain has been entered — so passing the pattern as the handler id is not possible.What is available:
Middleware.Measurecapturesreporter.Context()(the request context, which already holds the*chi.Contextpointer that routing mutates in place) and passes it to every recorder call. The duration and size observations happen in adefer, afternext()— exactly where the access log reads the pattern inaccessLogURL, and the same place the Sentry scrub reads it.So: wrap the Prometheus recorder in a decorator that rewrites
props.IDfromchi.RouteContext(ctx).RoutePattern()at record time.std.Handlerand its response-writer interceptor stay untouched, so status and byte capture are unaffected. Because it records after the whole chain returns, requests rejected by the route-levelReceiverRateLimit(thecode="429"majority) resolve the pattern too.Unmatched routes: empty pattern collapses to the existing
unmatchedRoutesentinel(unmatched), the same value the access log already uses.http_requests_inflightis the one metric that cannot carry the pattern: it is incremented before routing and decremented after, so a pattern-derived label would unbalance the gauge. It gets a fixedhandler="(all)"— one series, total concurrent requests. Called out in the PR body.Tests: a chi router mirroring the real ordering (global metrics middleware, route-level rate limiter on
/webhook/{uuid}), asserting N distinct UUIDs produce exactly onehandlerlabel value, for both the 200 and the 429 path, plus an unmatched-path flood. Plus the live scrape probe from the issue, before and after, reported in the PR.Built in #258 (branch
issue-254-metrics-route-pattern, basenext).A
metrics.Recorderdecorator rewritesprops.IDfromchi.RouteContext(ctx).RoutePattern()at record time, which is after the wrapped handler returns and therefore after routing — the pattern cannot be supplied as the library's handler id, since the recorder is global middleware andMeasurefixes the id up front. Unmatched routes carry the existing(unmatched)sentinel.http_requests_inflightcannot carry a pattern (incremented before routing, decremented after) and gets a fixed aggregate label; details in the PR body.Verified. Reproduced the leak on the parent commit first: 107 series / 12,899 bytes to 78,132 series / 10,655,997 bytes / 3,002
handlerlabels after 3,000 unauthenticated POSTs to distinct invented UUIDs, unchanged on re-scrape. Same probe on the fixed build: 106 to 181 series, 20,907 bytes, 4 handler labels; a second 3,000-path flood left it at 181 — flat. Zero UUID-shaped strings anywhere in the scrape. 4,800 of the driven requests were rejected 429 and all of them landed onhandler="/webhook/{uuid}", 25 series total. A separate unmatched-route flood (250 paths, two shapes) settled at 206 series and did not move.make checkgreen withGOFLAGS=-count=1, lint in Docker,0 issues.Six new tests ininternal/middleware/metrics_test.go; five were confirmed to fail against the unfixed recorder before being kept.