The /metrics method label is still unbounded: 300 requests with random method tokens mint 7,525 permanent series #261

New Issue

2026-08-24T01:33:03+02:00

clawbot commented

2026-08-24 01:33:03 +02:00

Found by the independent review of #258, measured against that branch's build — that is, AFTER the handler label was fixed.

method is recorded as r.Method verbatim, and net/http accepts any RFC 9110 token as a method. 300 requests to / using random 12-character method tokens took the scrape from 106 series to 7,631 — roughly 25 permanent series per distinct token, never evicted.

Unauthenticated, and / carries no rate limiter, so this is the same remote memory-exhaustion vector #254 was filed to close, reached through a different label dimension.

#254's definition of done was scoped to the handler label and is fully met by #258; that scoping was mine and it was too narrow. This issue closes the rest.

Definition of done

method is collapsed to the set chi actually routes, with anything else recorded as a fixed sentinel — mirroring the (unmatched) treatment #258 established for handler. Reuse that sentinel convention rather than inventing a second one.
Audit EVERY remaining label on every metric exposed by /metrics for attacker-controlled input, and state the result in the PR body. Two dimensions have now been found unbounded by two separate passes; assume a third until proven otherwise. code is a bounded status code and service is static, but verify rather than assume, and check the delivery metrics in internal/metrics/metrics.go as well as the HTTP middleware.
A test driving N distinct method tokens and asserting exactly one series results.
Legitimate methods must still be distinguishable — a fix that collapses GET and POST into one bucket destroys the metric. Positive control required.

Also in this unit, same README section

The aggregate http_requests_inflight{handler="(all)"} semantics #258 introduced are documented in its PR body and in code, but not in README's Metrics section — which is the very section that makes the cardinality argument for the delivery metrics. Document it there.

Verification

make check green.
Reproduce first on current next so the probe is known to work, then show it fixed: scrape, drive several hundred distinct method tokens, scrape again, series count flat.
Confirm real methods still produce distinct series.

Found by the independent review of https://git.eeqj.de/sneak/webhooker/pulls/258, measured against that branch's build — that is, AFTER the `handler` label was fixed. `method` is recorded as `r.Method` verbatim, and `net/http` accepts any RFC 9110 token as a method. 300 requests to `/` using random 12-character method tokens took the scrape from **106 series to 7,631** — roughly 25 permanent series per distinct token, never evicted. Unauthenticated, and `/` carries no rate limiter, so this is the same remote memory-exhaustion vector https://git.eeqj.de/sneak/webhooker/issues/254 was filed to close, reached through a different label dimension. #254's definition of done was scoped to the `handler` label and is fully met by #258; that scoping was mine and it was too narrow. This issue closes the rest. ## Definition of done - `method` is collapsed to the set chi actually routes, with anything else recorded as a fixed sentinel — mirroring the `(unmatched)` treatment #258 established for `handler`. Reuse that sentinel convention rather than inventing a second one. - Audit EVERY remaining label on every metric exposed by `/metrics` for attacker-controlled input, and state the result in the PR body. Two dimensions have now been found unbounded by two separate passes; assume a third until proven otherwise. `code` is a bounded status code and `service` is static, but verify rather than assume, and check the delivery metrics in `internal/metrics/metrics.go` as well as the HTTP middleware. - A test driving N distinct method tokens and asserting exactly one series results. - Legitimate methods must still be distinguishable — a fix that collapses GET and POST into one bucket destroys the metric. Positive control required. ## Also in this unit, same README section The aggregate `http_requests_inflight{handler="(all)"}` semantics #258 introduced are documented in its PR body and in code, but not in README's Metrics section — which is the very section that makes the cardinality argument for the delivery metrics. Document it there. ## Verification - `make check` green. - Reproduce first on current `next` so the probe is known to work, then show it fixed: scrape, drive several hundred distinct method tokens, scrape again, series count flat. - Confirm real methods still produce distinct series.

clawbot added this to the 1.0.0 milestone 2026-08-24 01:33:07 +02:00

clawbot commented

2026-08-24 01:35:13 +02:00

Plan:

Bound method in the same seam #258 added, routePatternRecorder in internal/middleware/metrics.go, by rewriting props.Method on both observation calls alongside props.ID. The recorder is renamed to reflect that it now bounds two dimensions rather than one.
The retained domain is the nine methods chi's router can match a route for (CONNECT, DELETE, GET, HEAD, OPTIONS, PATCH, POST, PUT, TRACE — chi's methodMap, which is unexported, so it is restated as a switch over the net/http constants). Anything else is a token chi answers 405 for and can never route, so it carries the sentinel.
Sentinel is the existing (unmatched) value, aliased rather than respelled: the same idea (a client-chosen token matching nothing this service registers) must not read as two different states in one scrape. Bound becomes 10 method values.
Tests: N distinct random method tokens collapsing to exactly one series; a positive control that GET/POST/PUT/DELETE/HEAD/OPTIONS stay distinct; a flat-series-count flood. Fail-first against the current code before the fix goes in.
Full label audit of every metric /metrics exposes (service, handler, method, code, the delivery target_type, and the default Go/process collectors) goes in the PR body, with how each was verified.
README's Metrics section gains the inbound HTTP metric labels and the http_requests_inflight{handler="(all)"} aggregate semantics.

No new metrics, no changes under internal/delivery or internal/database, no TODO.md edit.

Plan: - Bound `method` in the same seam #258 added, `routePatternRecorder` in `internal/middleware/metrics.go`, by rewriting `props.Method` on both observation calls alongside `props.ID`. The recorder is renamed to reflect that it now bounds two dimensions rather than one. - The retained domain is the nine methods chi's router can match a route for (`CONNECT`, `DELETE`, `GET`, `HEAD`, `OPTIONS`, `PATCH`, `POST`, `PUT`, `TRACE` — chi's `methodMap`, which is unexported, so it is restated as a `switch` over the `net/http` constants). Anything else is a token chi answers 405 for and can never route, so it carries the sentinel. - Sentinel is the existing `(unmatched)` value, aliased rather than respelled: the same idea (a client-chosen token matching nothing this service registers) must not read as two different states in one scrape. Bound becomes 10 method values. - Tests: N distinct random method tokens collapsing to exactly one series; a positive control that `GET`/`POST`/`PUT`/`DELETE`/`HEAD`/`OPTIONS` stay distinct; a flat-series-count flood. Fail-first against the current code before the fix goes in. - Full label audit of every metric `/metrics` exposes (`service`, `handler`, `method`, `code`, the delivery `target_type`, and the default Go/process collectors) goes in the PR body, with how each was verified. - README's Metrics section gains the inbound HTTP metric labels and the `http_requests_inflight{handler="(all)"}` aggregate semantics. No new metrics, no changes under `internal/delivery` or `internal/database`, no `TODO.md` edit.

clawbot referenced this issue

2026-08-24 01:40:37 +02:00

Milestone 1.0.0: internet-facing readiness #111

clawbot referenced this issue from a commit

2026-08-24 01:47:08 +02:00

Bound the /metrics method label (closes #261)

clawbot referenced a pull request that will close this issue

2026-08-24 01:47:50 +02:00

Bound the /metrics method label (closes #261) #264

clawbot commented

2026-08-24 01:48:12 +02:00

Done in #264 (branch issue-261-metrics-method-label, base next). Full audit table and evidence are in the PR body.

Built: method is now bounded in the same recorder seam that bounds handler. A method chi can route is kept verbatim; anything else carries the existing (unmatched) sentinel, aliased rather than respelled. Bound is 10 values. README's Metrics section gained the inbound HTTP metrics, each label's bound, and the aggregate http_requests_inflight{handler="(all)"} semantics.

Verified:

Reproduced on unmodified next at fd5966f first: a live instance went from 81 to 7,606 scrape lines under 300 random 12-character method tokens (the issue's 7,525-line delta). The same binary from this branch stayed flat at 331 -> 331.
New test failed against the unmodified recorder (300 tokens -> 601 series / 7,501 lines; 1,200 more -> 3,001 / 37,501) and passes with the fix (3 series / 26 lines, flat).
Positive control: all nine routable methods keep their own duration series, one sample each — confirmed both in test and in a live scrape.
Audit: every label on every metric /metrics exposes was enumerated from a live authenticated scrape, not from reading alone. code and service are confirmed safe by tracing where each is written. No third unbounded dimension found.
make check green with GOFLAGS=-count=1; lint clean in the Docker stage.

TODO.md untouched per #112.

Done in https://git.eeqj.de/sneak/webhooker/pulls/264 (branch `issue-261-metrics-method-label`, base `next`). Full audit table and evidence are in the PR body. Built: `method` is now bounded in the same recorder seam that bounds `handler`. A method chi can route is kept verbatim; anything else carries the existing `(unmatched)` sentinel, aliased rather than respelled. Bound is 10 values. README's Metrics section gained the inbound HTTP metrics, each label's bound, and the aggregate `http_requests_inflight{handler="(all)"}` semantics. Verified: - Reproduced on unmodified `next` at `fd5966f` first: a live instance went from 81 to 7,606 scrape lines under 300 random 12-character method tokens (the issue's 7,525-line delta). The same binary from this branch stayed flat at 331 -> 331. - New test failed against the unmodified recorder (300 tokens -> 601 series / 7,501 lines; 1,200 more -> 3,001 / 37,501) and passes with the fix (3 series / 26 lines, flat). - Positive control: all nine routable methods keep their own duration series, one sample each — confirmed both in test and in a live scrape. - Audit: every label on every metric `/metrics` exposes was enumerated from a live authenticated scrape, not from reading alone. `code` and `service` are confirmed safe by tracing where each is written. No third unbounded dimension found. - `make check` green with `GOFLAGS=-count=1`; lint clean in the Docker stage. `TODO.md` untouched per https://git.eeqj.de/sneak/webhooker/issues/112.

clawbot closed this issue

2026-08-24 02:03:19 +02:00

clawbot referenced this issue from a commit

2026-08-24 02:03:20 +02:00

Bound the /metrics method label (closes #261)

clawbot referenced this issue from a commit

2026-08-24 03:34:12 +02:00

Record landed 1.0.0 work and drop the removed signing roadmap in TODO.md

clawbot referenced this issue

2026-08-24 03:34:45 +02:00

Record landed 1.0.0 work and drop the removed signing roadmap in TODO.md #284

clawbot referenced this issue from a commit