Bound client-controlled metric label length - #6279
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6279 +/- ##
==========================================
+ Coverage 72.82% 72.84% +0.02%
==========================================
Files 743 743
Lines 77647 77654 +7
==========================================
+ Hits 56544 56565 +21
+ Misses 17133 17121 -12
+ Partials 3970 3968 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Nashon-Steffen
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: security, test-coverage, general-code-quality
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | mcp_resource_id not asserted in integration test |
10/10 | MEDIUM | Fix |
Overall
This PR is a focused security bug fix adding a truncateLabelValue helper to cap client-controlled metric label values at 128 bytes with UTF-8 rune-boundary awareness, preventing memory exhaustion in Prometheus/OTLP cumulative metric readers. The approach is technically sound: the 128-byte cap is applied consistently at all 6 identified metric label sites (mcp_method, mcp_resource_id, tool, mcp.method.name, gen_ai.tool.name, gen_ai.prompt.name), spans intentionally retain the full untruncated value per OTEL MCP semconv, and the truncation logic is correct including the idempotent double-truncation path. The regression guard test uses real SDK instrumentation rather than mocks, making it genuinely load-bearing.
One fixable gap — unanimous across all three review agents — is that the integration test's post-loop assertions don't verify that mcp_resource_id was actually observed, creating a latent false-negative for that label. This is a one-line fix. No other findings crossed the consensus threshold.
Generated with Claude Code
Cumulative metric readers (Prometheus, OTLP PeriodicReader) keep every distinct attribute set resident for the process lifetime. The OTEL SDK's 2000-series cap bounds the count of attribute sets but not the byte length of any single label value. The parsed MCP method, tool, and prompt names are taken verbatim from the request and bounded only by the 8 MB body limit, so a single label value can approach 8 MB and stay resident until the process exits. A small burst of large values is enough to OOM a pod. Add truncateLabelValue, capping client-controlled metric label values at 128 bytes on a UTF-8 rune boundary with a truncation marker, and apply it to every client-controlled label site in the telemetry middleware. Span attributes keep the full value on purpose: spans are sampled and ephemeral and the OTEL MCP semconv wants the real value. Part of #6271 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The integration test verified gen_ai.tool.name and tool were observed but never confirmed mcp_resource_id was, leaving a false-negative gap: if that label key were dropped or renamed, the test would pass silently. Assert it was checked alongside the others. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
db6a561 to
f7563ec
Compare
Summary
Cumulative metric readers (Prometheus reader, OTLP
PeriodicReader) keep every distinct attribute set resident for the process lifetime — there is no TTL, LRU, or scrape-driven reset. The OpenTelemetry Go SDK caps attribute sets at 2000 per instrument, which bounds series count, but nothing bounds the byte length of any single label value.The parsed MCP method, tool, and prompt names are taken verbatim from the client request (
pkg/mcp/parser.go,Method: req.Method), bounded only by the 8 MB request body cap. So a single metric label value can approach 8 MB and stay resident until the process exits. Roughly 64 such requests retain ~512 MB permanently — enough to OOM a typical pod, and it OOM-loops if the attack repeats after restart. The metric is recorded after the response, so a backend 404/500 or a Cedar 403 still records; only an auth 401 short-circuits, so this is reachable on intentionally-unauthenticated deployments (e.g. a public docs server).This is Finding B of #6271.
truncateLabelValue, capping a client-controlled metric label value at 128 bytes, clamped on a UTF-8 rune boundary (never emits invalid UTF-8) with a trailing...marker.pkg/telemetry/middleware.go:mcp_method,mcp_resource_id,tool,mcp.method.name,gen_ai.tool.name,gen_ai.prompt.name. Hardcoded values (transport,jsonrpc.protocol.version) are left alone.Part of #6271
Type of change
Test plan
task test)task lint-fix)New tests in
pkg/telemetry/middleware_test.go:TestTruncateLabelValue— under/at/over the cap, empty string, and multi-byte rune-boundary clamping.TestHTTPMiddleware_TruncatesMetricLabelsKeepsSpanFull— an oversized tool name is truncated on every metric label it reaches, while the span retains the full value.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Does this introduce a user-facing change?
Metric label values for client-controlled MCP method, tool, and prompt names are now truncated to 128 bytes (with a trailing
...) on metrics. Traces are unaffected and retain the full value.Special notes for reviewers
Generated with Claude Code