Skip to content

mcp: say that the connection-scoped roots cache relies on one transport being one caller #98

Description

@carldebilly

Under mcp serve, native roots are cached for the life of the connection (McpRootsScope.Connection). That is correct, but it rests on an assumption the repository never states: one transport is one caller. A reviewer read the code, could not find that assumption anywhere, and filed a P1 for cross-caller disclosure — see #71 (thread on McpClientRootsService.cs:184). The finding is not a defect, but it was a reasonable reading, and it cost a round of analysis to refute.

What is true

The mechanism the reviewer described is real. On 2026-07-28 over mcp serve, the connection cache does fill, and a later request does read it without consulting its own request.Server. Measured:

era=2026-07-28  supported=True  current=[file:///ga]  get=[file:///ga]  roundTrips=1

This corrected a belief of mine: I had assumed sessionless requests never populate that cache, on the strength of DestinationBoundMcpServer.ClientCapabilities being null in HttpServerSessionMode.Stateless. That is an HTTP-path fact and does not carry to RunAsync.

Why it is not exploitable

One ITransport is one peer by construction, not by convention:

  • it exposes a single ChannelReader<JsonRpcMessage> created with SingleReader = true, and one read loop;
  • messages carry no source discriminatorJsonRpcMessageContext has no peer member;
  • there is no multiplexing primitive below the HTTP layer. Multiplexing lives in ModelContextProtocol.AspNetCore, where each StreamableHttpSession owns its own transport and its own McpServer.

A host merging two clients onto one pipe would collide their JSON-RPC request IDs long before roots leaked. Every shape that legitimately carries several callers already uses the reused BuildMcpServerOptions() pattern, which is McpRootsScope.Request and is guarded by Given_McpSharedServerOptions.

Why no enforcement is possible

Checked exhaustively; there is no per-caller key on this path:

candidate on stdio / StreamServerTransport
Server.SessionId null in every revision — 2026-07-28 removed session IDs (SEP-2567)
MessageContext.User populated only by the ASP.NET Core transports; unconditionally null here
Server.ClientInfo / ClientCapabilities client-asserted capability data, not identity
Services per-connection, or a per-request scope
request.Server fresh DestinationBoundMcpServer per message
_meta no protocol-reserved identity key

The only latent seam is TransportBase.SessionId, whose setter is protected — a custom transport could populate it, if enforcement ever becomes wanted.

Work

  1. State single-peer as a requirement, with its reason. In docs/mcp-transports.md Scenario A, at the TransportFactory sample: the returned transport serves exactly one client, because messages on one transport carry no source discriminator. Point hosts wanting several clients at the "Multi-session custom transports" pattern just below — already the right advice, merely unlabelled as the alternative.
  2. Isolation table: the Connection (mcp serve only) row is per transport, and per-caller only because one transport is one caller.
  3. TransportFactory XML doc on the MCP options type: carry the one-client requirement where a host reads it while writing the bridge.
  4. Comment the assumption at the cache — near CreateSessionContext(McpRootsScope.Connection) in McpServerHandler.RunAsync, including that no per-caller key exists to validate against, so the next reader does not go looking.
  5. Guard the cost model in Given_McpConcurrentSessions.cs: on 2026-07-28 under mcp serve, two tool calls on one connection must cost exactly one roots/list. Reuse BuildCountingRootsClientOptions and StartSessionAsync. Today's behaviour becomes deliberate, and a later switch to request scope turns it red rather than drifting.

Rejected

Request-scoped roots for sessionless requests under mcp serve (the reviewer's own remedy). It would cost one roots/list per request for the single peer that path serves, to defend a topology the transport contract already forbids.

Related: #70 — both concern where a scope boundary falls.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions