Skip to content

fix(telemetry): bound mcp method metric label cardinality - #6268

Closed
Nashon-Steffen wants to merge 1 commit into
mainfrom
nashon/telemetry-method-label-allowlist
Closed

fix(telemetry): bound mcp method metric label cardinality#6268
Nashon-Steffen wants to merge 1 commit into
mainfrom
nashon/telemetry-method-label-allowlist

Conversation

@Nashon-Steffen

Copy link
Copy Markdown
Contributor

Summary

The telemetry middleware writes the JSON-RPC method field into metric label values before validation. The field is parsed from the raw request body, so an unauthenticated caller controls it. The in-process metric registry grows without limit as new values arrive (CWE-770 / CWE-400).

This is observable in production: a public docs-MCP endpoint already carries labels from internet scanners that map to no MCP method - debug_traceTransaction, txpool_content, User.filter, web.Login. Each distinct value creates 17 time series (a 15-bucket histogram plus _sum and _count).

Reported against toolhive-doc-mcp.stacklok.com (2026-08-08), good-faith testing.

Change

  • Add pkg/telemetry/method_allowlist.go: a bounded allowlist of MCP method names and a sanitizeMCPMethod helper.
    • Known method -> recorded verbatim.
    • Any other parsed method -> other sentinel (keeps the "an unregistered method was attempted" signal without recording its value).
    • Empty/unparsed method -> unknown (existing behavior).
  • Apply the sanitizer to the two metric label sites in middleware.go: mcp_method (request counter and duration) and mcp.method.name (operation duration).

What is intentionally not changed

  • Span attributes keep the raw method. Traces are sampled and ephemeral. They do not aggregate into a registry, and the OTEL MCP semantic conventions want the real method name on the span.
  • Control flow keeps the real method. The tools/call and prompts/get branches and the "method could not be determined" warning still switch on the parsed value. Both methods are in the allowlist, so labels stay consistent.

Known follow-up

Tool-name labels (gen_ai.tool.name, mcp_resource_id) remain unbounded for a valid tools/call carrying an arbitrary tool name. The parser leaves ResourceID empty for unregistered methods, so the observed scanner traffic does not reach those labels. Bounding tool names needs the registered tool set at middleware time and is left as follow-up.

Testing

  • pkg/telemetry/method_allowlist_test.go - table tests for the sanitizer, including the four scanner method names from the report and a cardinality-collapse assertion.
  • pkg/telemetry/middleware_test.go - end-to-end case: a bogus method through the middleware surfaces mcp.method.name="other" with no tool label leak.
  • go test ./pkg/telemetry/, go vet, gofmt -l all clean.

🤖 Generated with Claude Code

The JSON-RPC method field is parsed from the raw request body and written
into metric label values before validation. An unauthenticated caller
controls that field. The metric registry grew without limit as new values
arrived. A live public endpoint already carried labels from internet
scanners that map to no MCP method (debug_traceTransaction, txpool_content,
User.filter, web.Login). Each distinct value created 17 time series.

Add a bounded allowlist of MCP method names. Record a known method
verbatim. Collapse any other parsed method to an "other" sentinel. Keep
"unknown" for a request with no parsed method. This bounds the mcp_method
and mcp.method.name label cardinality regardless of input. It keeps the
signal that an unregistered method was attempted.

Span attributes keep the raw method. Traces are sampled and ephemeral, so
they do not grow a registry, and the OTEL MCP semconv wants the real name.

Tool-name labels (gen_ai.tool.name, mcp_resource_id) stay unbounded for a
valid tools/call with an arbitrary name. The parser leaves ResourceID
empty for unregistered methods, so the observed scanner traffic does not
reach those labels. Bounding tool names needs the registered tool set and
is tracked as follow-up.

Refs: CWE-770, CWE-400. Unauthenticated diagnostics report, 2026-08-08.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Aug 10, 2026
@Nashon-Steffen

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate.

A deeper assessment showed the method-name bounding here is already in flight in #5956, which adds IsKnownMethod and bounds mcp.method.name / http.request.method to the semconv-standard _OTHER sentinel (this PR used a non-standard other). #5956 also deletes the legacy mcp_method label that this PR edited.

Method names are the closed surface. The remaining unbounded surface is tracked in #6169 (tool/prompt names via ResourceID, blocked on the resolved tool/prompt set). Separately, label-value length is unbounded (method taken verbatim, capped only by the 8 MB body limit, retained for process lifetime under cumulative aggregation) - a truncation fix for that is worth its own PR and is independent of the #6169 blocker.

Deferring to #5956 and #6169.

@Nashon-Steffen
Nashon-Steffen deleted the nashon/telemetry-method-label-allowlist branch August 10, 2026 19:18
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.72%. Comparing base (f66d0f5) to head (eb5f70c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6268      +/-   ##
==========================================
- Coverage   72.73%   72.72%   -0.01%     
==========================================
  Files         742      743       +1     
  Lines       77299    77336      +37     
==========================================
+ Hits        56221    56245      +24     
- Misses      17115    17127      +12     
- Partials     3963     3964       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant