Telemetry + diagnostics hardening: bound label length (OOM), overflow lock, tool/prompt cardinality, diagnostics bypass, /health fingerprint

[Nashon-Steffen]

Summary

An external good-faith report against a toolhive-doc-mcp deployment pointed at
the telemetry label code and the proxy diagnostics endpoints. A follow-up code
assessment refuted the report's headline mechanism but confirmed several real
defects in the same area. This issue breaks the confirmed defects into
actionable steps.

Scope note. This issue tracks the code fixes only. It does not decide severity,
CVSS, or advisory-vs-issue. The reported live asset carries a build fingerprint
(0.11.0, commit 08ba97f8...) that matches no stacklok/toolhive build, so
disposition against a specific deployment is a separate question.

Correction that frames the rest

The report claimed "the metric registry grows without limit." That is false on
current main. The OpenTelemetry Go SDK caps attribute sets at 2000 per
instrument by default since sdk/metric v1.44.0. ToolHive pins v1.45.0
(go.mod), so series count is hard-capped. ToolHive crossed that line at
430b9cdc8, first released in v0.29.2. The findings below are what remains
real once that cap is accounted for.

The 2000 cap is an inherited transitive default. ToolHive sets no views, no
attribute filters, and no explicit limit of its own. A downgrade or an explicit
unlimited limit would silently remove the protection. See finding C for the
regression test.

---

Finding B - unbounded label value length leads to memory exhaustion

Priority: highest value per unit of effort.

The 2000 cap bounds series count. It does not bound the length of a label
value. Both the Prometheus reader and the OTLP PeriodicReader aggregate
cumulatively, so every distinct attribute set stays resident until the process
exits. There is no TTL, no LRU, and no scrape-driven reset.

The parsed MCP method is taken verbatim with no truncation
(pkg/mcp/parser.go:323, Method: req.Method). The only bound is the request
body cap (pkg/bodylimit/middleware.go:36, DefaultMaxRequestBodySize = 8 MB).
So a single label value can approach 8 MB and stay resident for the process
lifetime. Roughly 64 requests at 8 MB each retain about 512 MB permanently. That
is enough to OOM a typical pod, and it OOM-loops if the attack repeats after
restart.

Reachability. The metric is recorded after the response, so a backend 404 or
500 still records. An authz (Cedar) 403 still records, because authz sits inside
telemetry. Only an auth 401 short-circuits first, so this is reachable only on
intentionally-unauthenticated deployments (which a public docs server is).

Label sites that take client-controlled strings, pkg/telemetry/middleware.go:

• :697 mcp_method
• :698 mcp_resource_id (on the common attribute set for requestCounter
  and requestDuration, so every request; set from initialize ->
  clientInfo.name, a valid method)
• :727 tool
• :743 mcp.method.name
• :768 gen_ai.tool.name
• :772 gen_ai.prompt.name

Steps

1. Add a truncateLabelValue(s string) string helper. Cap at 64 to 128 bytes.
   Clamp on a UTF-8 boundary. Keep a short marker on truncation, for example a
   trailing ....
2. Apply it to every client-controlled label value at the sites above. Do not
   apply it to hardcoded values (jsonrpc.protocol.version, transport).
3. Keep the raw value on spans. Spans are sampled and ephemeral, so they do not
   grow a registry, and the OTEL MCP semconv wants the real value.
4. Add a unit test that a label value over the cap is truncated, and a test that
   confirms the span keeps the full value.

Acceptance

• No metric label value can exceed the cap regardless of request body size.
• A burst of large method values does not grow retained memory beyond the cap
  times the series count.

This step does not depend on the resolved tool or prompt set, so it is
independent of the #6169 blocker. Ship it first.

---

Finding C - overflow lock leads to permanent observability denial

Once 1999 attribute sets exist on an instrument, every new set collapses into
otel.metric.overflow=true permanently
(go.opentelemetry.io/otel/sdk/metric/internal/aggregate/limit.go). Existing
sets keep recording. New ones never get their own series again until restart.

Practical attack. About 2000 cheap junk requests early in a process life consume
all slots. The per-method and per-tool breakdown is then destroyed until
restart. Sums and counts stay correct, so dashboards keep rendering and the
failure is silent. For a security-monitoring product this defeats the exact
signal the telemetry exists to provide.

C has no independent code fix. It closes transitively once the distinct label
values are bounded to a small known set. That is finding B (truncation, which
merges near-identical junk) plus finding D (sentinel for unrecognized method,
tool, and prompt names). The standalone deliverable is proof that junk cannot
exhaust the budget.

Steps

1. Land B and D first.
2. Add a regression test. Send many requests with distinct unrecognized method,
   tool, and prompt names. Assert the recognized set still gets its own series
   afterward, so junk did not consume the slot budget.

Acceptance

• After a burst of distinct junk requests, a subsequent recognized method still
  records under its own label value, not the overflow sentinel.

---

Finding D - unbounded client-controlled label values (method, tool, prompt)

This is the hygiene root cause. Two parts, at different states.

Method names and HTTP method. Bounded to the semconv _OTHER sentinel by
#5956 (adds IsKnownMethod, deletes the legacy mcp_method label and the
toolhive_mcp_* twins). #5956 is open but currently CONFLICTING.

Tool and prompt names. Set from parsedMCP.ResourceID (params.name
verbatim) and not validated against the server resolved set. Tracked by #6169.
#6169 is blocked by design, because the telemetry middleware sees a parsed
request, not the backend capability list. Bounding needs the resolved set
threaded into telemetry, or name resolution before the metric records.

Steps

1. Rebase and land Standardize proxy + vMCP metrics to stacklok.*, delete legacy twins #5956 for the method-name and HTTP-method bound.
2. Implement Bound client-controlled tool/prompt name cardinality on per-request metrics #6169 for tool and prompt names. Pick one design: thread the
   resolved tool and prompt set into telemetry, or resolve the name before the
   metric records, then map unresolved names to a bounded sentinel. Keep the raw
   name on the span.

Acceptance

• No per-request metric carries a client-controlled method, tool, or prompt name
  unbounded. Unrecognized values record a bounded sentinel.

---

Finding E - diagnostics endpoints bypass the middleware chain

/health and /metrics are registered as explicit mux paths. Go ServeMux
longest-match means they always beat the / catch-all that carries the
middleware chain. So they stay unauthenticated even on a fully OIDC-configured
deployment, and they bypass body limits, rate limiting, and audit.

• pkg/transport/proxy/transparent/transparent_proxy.go:1316 (/health),
  :1323 (/metrics), :1341 (/), :1355 (Handler: mux)
• pkg/transport/proxy/streamable/streamable_proxy.go:271,275
• pkg/transport/proxy/httpsse/http_proxy.go:288,292
• pkg/vmcp/server/server.go:596-599, under the comment
  "Optional Prometheus metrics endpoint (unauthenticated)"

The deployed listener is external. defaultProxyHost = "0.0.0.0"
(cmd/thv-operator/controllers/mcpserver_runconfig.go). The CLI default is
127.0.0.1 (cmd/thv/app/run_flags.go). Both endpoints are GET-only, which
limits the practical impact of the bypass. /metrics is off by default at every
layer, so this applies when metrics are enabled.

Constraint. /health must stay reachable without auth for Kubernetes liveness
and readiness probes. So the fix is not "put /health behind auth."

Steps

1. Bind diagnostics to a separate internal listener, on its own host and port
   that deployments do not route to the internet. Move /metrics there. This is
   the reporter's original recommendation and matches the sibling
   registry-server pattern.
2. Keep /health reachable for probes, but make it minimal per finding F.
3. Confirm the split with a test. Assert /metrics is not served on the main
   application listener.

Acceptance

• /metrics is not reachable on the internet-facing listener.
• /health stays reachable for probes and carries no sensitive fields.

Note. NewInternalServer does not exist in this repo today. There is one host
and port per proxy and no option to bind diagnostics separately. This step adds
that option. It is larger than a config change.

---

Finding F - /health discloses a build fingerprint

pkg/healthcheck/healthcheck.go:85 sets Version: versions.GetVersionInfo()
unconditionally, with no flag and no redaction. pkg/versions/version.go
supplies version, commit, build date, runtime.Version(), and GOOS/GOARCH.
The transparent proxy has no off switch for the /health path.

ToolHive already made the opposite call twice:

• pkg/vmcp/server/server.go:911-919 returns only {"status":"ok"}, with a
  security comment that it exposes no version information to prevent disclosure.
• pkg/api/v1/healthcheck.go returns 204 No Content.

The proxies did not follow that decision. Aligning them needs no new listener
and no deployment change. Version disclosure has limited intelligence value for
an open-source project, so the strongest argument here is internal consistency.

Steps

1. Drop VersionInfo from the unauthenticated /health response body, or gate
   it behind an explicit opt-in that defaults off.
2. Match the minimal shape already used by vMCP and the v1 API.
3. Update the healthcheck tests for the minimal body.

Acceptance

• The unauthenticated /health response carries no version, commit, build date,
  Go version, or platform by default.

---

Suggested sequencing

1. B (truncation). Highest value, self-contained, unblocks nothing else.
2. F (drop fingerprint). Small, self-contained.
3. D method half. Rebase and land Standardize proxy + vMCP metrics to stacklok.*, delete legacy twins #5956.
4. E (internal listener). Larger, covers /metrics exposure.
5. D tool/prompt half. Bound client-controlled tool/prompt name cardinality on per-request metrics #6169, needs the design decision.
6. C regression test. After B and D land.

Also worth adding, referenced by #6169 but currently missing: a cardinality
warning in docs/observability.md.

Related

• Standardize proxy + vMCP metrics to stacklok.*, delete legacy twins #5956 - method-name and HTTP-method bound (open, needs rebase). Covers part of D.
• Bound client-controlled tool/prompt name cardinality on per-request metrics #6169 - tool and prompt name cardinality (open, blocked). Covers part of D.
• fix(telemetry): bound mcp method metric label cardinality #6268 - closed as a duplicate of Standardize proxy + vMCP metrics to stacklok.*, delete legacy twins #5956.
• GHSA-grwg-v9p7-76m2 - unbounded request bodies exhaust memory. Same class as B.
• GHSA-hfrv-94x5-85p2 - unauthenticated-by-default proxy design, relevant to E reachability.

[Nashon-Steffen]

Status update

B and F are merged. E remains deferred and C/D remain dependency/design-blocked follow-ups.

Finding	Status	PR / Notes
B – bound label length (OOM)	✅ Merged	#6279 — truncateLabelValue (128-byte cap, UTF-8 rune-boundary clamp, ... marker) at all 6 client-controlled metric label sites; spans keep the full value.
F – /health build fingerprint	✅ Merged	#6280 — removed Version from HealthResponse, aligning proxies with vMCP/v1-API redaction; status/transport/MCP status kept for probes.
E – diagnostics bypass middleware	⏸️ Deferred	Introduces a new in-process internal-listener pattern across all four proxy/server types (transparent, streamable, httpsse, vMCP) plus the config/factory/runner/CLI plumbing — ~500–900 lines / 12–15 files, well past the 400-line/10-file PR limit. Needs a split-PR plan before implementation.
C – overflow-lock regression test	🔜 Follow-up	Per the issue, depends on B and D landing first. B has landed; once D lands, add the "junk cannot exhaust the 2000-series budget" regression test.
D – method/tool/prompt cardinality	🔜 Follow-up	Method half = rebase & land #5956 (still open); tool/prompt half = #6169 (design-blocked). Not standalone PRs from this pass.

Both merged PRs used Part of #6271 (no auto-close), so this issue stays open to track E, C, and D. Suggested next step: agree a split strategy for E, then sequence C after D lands.

Generated with Claude Code