sneak/webhooker
1
0
Fork 0
Code Issues 62 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-24 01:33:03 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-24 01:33:03 +02:00
Collaborator
Copy Link

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
Author
Collaborator
Copy Link

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 commented 2026-08-24 01:48:12 +02:00
Author
Collaborator
Copy Link

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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#261
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?