Skip to content

Commit 3853a6d

Browse files
committed
fix: harden otel_middleware against malformed input and telemetry leaks
- extract_trace_context returns None on a malformed carrier instead of letting the propagator's TypeError fail the request - params validation failures set the sanitized wire message as span status instead of recording the pydantic error (which carries client input values) - jsonrpc.request.id is str() on client spans to match the server span
1 parent 186bf6e commit 3853a6d

5 files changed

Lines changed: 52 additions & 5 deletions

File tree

src/mcp/server/runner.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,10 @@ async def wrapped(
128128
except MCPError as e:
129129
span.set_status(StatusCode.ERROR, e.error.message)
130130
raise
131+
except ValidationError:
132+
# Mirror the sanitized wire response; pydantic messages carry client input.
133+
span.set_status(StatusCode.ERROR, "Invalid request parameters")
134+
raise
131135
except Exception as e:
132136
span.record_exception(e)
133137
span.set_status(StatusCode.ERROR, str(e))

src/mcp/shared/_otel.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,13 @@ def inject_trace_context(meta: dict[str, Any]) -> None:
4040
inject(meta)
4141

4242

43-
def extract_trace_context(meta: dict[str, Any]) -> Context:
44-
"""Extract W3C trace context from a `_meta` dict."""
45-
return extract(meta)
43+
def extract_trace_context(meta: dict[str, Any]) -> Context | None:
44+
"""Extract W3C trace context from a `_meta` dict.
45+
46+
Returns `None` when the carrier is malformed; telemetry parsing must
47+
never fail the request it annotates.
48+
"""
49+
try:
50+
return extract(meta)
51+
except (TypeError, ValueError):
52+
return None

src/mcp/shared/jsonrpc_dispatcher.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ async def send_raw_request(
334334
with otel_span(
335335
span_name,
336336
kind=SpanKind.CLIENT,
337-
attributes={"mcp.method.name": method, "jsonrpc.request.id": request_id},
337+
attributes={"mcp.method.name": method, "jsonrpc.request.id": str(request_id)},
338338
):
339339
# Inject W3C trace context into _meta (SEP-414). With a no-op
340340
# tracer this writes nothing, but `_meta` itself is still

src/mcp/shared/session.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ async def send_request(
218218
with otel_span(
219219
span_name,
220220
kind=SpanKind.CLIENT,
221-
attributes={"mcp.method.name": request.method, "jsonrpc.request.id": request_id},
221+
attributes={"mcp.method.name": request.method, "jsonrpc.request.id": str(request_id)},
222222
):
223223
# Inject W3C trace context into _meta (SEP-414).
224224
meta: dict[str, Any] = request_data.setdefault("params", {}).setdefault("_meta", {})

tests/server/test_runner.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,42 @@ async def test_otel_trace_context_propagates_client_to_server(server: SrvT, span
790790
assert client_span.context is not None and server_span.context is not None
791791
assert server_span.parent.span_id == client_span.context.span_id
792792
assert server_span.context.trace_id == client_span.context.trace_id
793+
assert client_span.attributes is not None and server_span.attributes is not None
794+
assert client_span.attributes["jsonrpc.request.id"] == server_span.attributes["jsonrpc.request.id"]
795+
796+
797+
@pytest.mark.anyio
798+
async def test_otel_middleware_malformed_traceparent_degrades_to_no_parent(server: SrvT, spans: SpanCapture):
799+
"""A non-string traceparent in `_meta` must not fail the request; the
800+
server span simply gets no parent."""
801+
802+
def break_traceparent(next_on_request: OnRequest) -> OnRequest:
803+
async def wrapped(dctx: DispatchContext[Any], method: str, params: Any) -> dict[str, Any]:
804+
mangled = {"_meta": {"traceparent": 123}} if method == "tools/list" else params
805+
return await next_on_request(dctx, method, mangled)
806+
807+
return wrapped
808+
809+
async with connected_runner(server, dispatch_middleware=[break_traceparent, otel_middleware]) as (client, _):
810+
spans.clear()
811+
await client.send_raw_request("tools/list", None)
812+
[server_span] = [s for s in spans.finished() if s.kind == SpanKind.SERVER]
813+
assert server_span.parent is None
814+
815+
816+
@pytest.mark.anyio
817+
async def test_otel_middleware_validation_failure_sets_sanitized_status(server: SrvT, spans: SpanCapture):
818+
"""Malformed params set the sanitized wire message as span status and do
819+
not record the pydantic exception (it carries client input)."""
820+
async with connected_runner(server, dispatch_middleware=[otel_middleware]) as (client, _):
821+
spans.clear()
822+
with pytest.raises(MCPError) as exc:
823+
await client.send_raw_request("tools/call", {"name": 123})
824+
assert exc.value.error.code == INVALID_PARAMS
825+
[span] = [s for s in spans.finished() if s.kind == SpanKind.SERVER]
826+
assert span.status.status_code == StatusCode.ERROR
827+
assert span.status.description == "Invalid request parameters"
828+
assert not span.events
793829

794830

795831
@pytest.mark.anyio

0 commit comments

Comments
 (0)