Uh oh!
There was an error while loading. Please reload this page.
feat(mcp): support protocol revision 2026-07-28 alongside the legacy era - #219
Open
christianromeni wants to merge 5 commits into
Open
feat(mcp): support protocol revision 2026-07-28 alongside the legacy era#219christianromeni wants to merge 5 commits into
christianromeni wants to merge 5 commits into
Conversation
The 2026-07-28 revision removes the initialize handshake and protocol sessions, makes server/discover mandatory, and moves required per-request data into _meta plus mandatory headers. None of that is backward compatible, so both eras have to run side by side on one endpoint. The protocol version is resolved once at the edge (Negotiate) and every era difference lives behind ServerDialect/ClientDialect. Envelope, Result and CallRequest are era-neutral, so no caller branches on the era. Dispatch keys off the negotiated dialect, never off the version in the request body, which is attacker-controlled. VoidLLM now speaks both eras in all three roles: as an MCP server, as an MCP client, and as a transparent intermediary. Highlights: - Streaming pass-through with a real response stream instead of full buffering, with an idle timeout and a byte cap. - Legacy sessions are tracked per (org, api key) and fail closed, so a caller can never relay another tenant's session upstream. - Mcp-Param-* headers are relayed uninterpreted as the spec requires; more than the supported number rejects the request rather than silently forwarding a subset. - x-mcp-header annotations are honoured on the client side. A tool whose annotations violate the constraints is excluded from tools/list on its own, never taking the rest of the list with it. - The tool cache honours CacheableResult ttlMs and cacheScope, clamped against both an upstream freezing an entry and an upstream forcing a fetch per request. private results stay in memory and never reach the database. - The health checker probes through the shared transport, so it speaks whatever era the upstream speaks instead of a hardcoded payload. - protocol_version per server, settable via API, YAML and the UI, for the case where era detection gets it wrong. No prompt or response content, tool arguments, _meta payloads, upstream error text or session identifiers are logged or persisted anywhere on these paths.
The first review rounds ran against a working tree where the new core files were untracked, so git diff never showed them. This is what a review of the complete diff turned up. - The buffered client path read only the first SSE event, so an upstream that sends a progress notification before its result made ListTools return an empty tool list with no error at all. It now parses SSE properly: blank-line separated events, multi-line data joined, comments skipped, notifications passed over, and the response matched by its JSON-RPC id. - An empty credential is no longer sent. A failed decrypt used to leave the token empty and still put "Authorization: Bearer " on the wire; the transport cache now refuses to build such a transport at all, and both request paths fail closed if one reaches them anyway. - A session the caller never receives is no longer recorded. An upstream marking Mcp-Session-Id hop-by-hop via Connection had the session entered into the registry while it was correctly stripped from the response, which let it push out the caller's real sessions. - A tool listing whose cache visibility is absent or unrecognised is no longer written to the database. Only an explicit public scope, or a legacy upstream that offers no hint at all, is persisted. - Changing a server's OAuth issuer, client id or scopes now rebuilds its transport, and any auth- or transport-affecting change invalidates that server's cached tool listing instead of serving it under the new credential. - An x-mcp-header annotation on a node that carries $ref is rejected. The declared type is read locally and $ref is never resolved, so the type the header would be mirrored under is not actually known. - Envelope.Name is populated for every name-bearing method, not just tools/call, via the same helper both other call sites already use. - A response over the size limit is rejected rather than silently truncated to a valid-looking prefix. - The MCP tool call counter's label is named for what it has always carried: the JSON-RPC method, not a tool name. Known and deliberately left open: MCPToolCallDurationSeconds carries the same mislabelled dimension, the 202 and redirect paths record a session before the response headers are filtered, and RefreshServer can still publish a result fetched before an invalidation.
…eader The origin check compared the Origin header against the request's own Host. DNS rebinding gives an attacker control of both: the browser sends Host: attacker.example and Origin: http://attacker.example, the domain resolves to 127.0.0.1, the two agree, and the request was allowed through. The check could not catch the attack it existed for. Origin validation is now explicit-allow, the same posture this project already takes for model access. A configured allowed_origins list is the only authority. With no list configured, a built-in localhost allowlist applies (localhost, 127.0.0.1, [::1], any or no port, http or https). The request's Host is never consulted. Requests without an Origin header are unaffected, so SDK and other non-browser clients keep working exactly as before. This is a breaking change for anyone serving the MCP endpoints to a browser under a real domain: that domain now has to be listed in allowed_origins. VoidLLM warns at startup when the list is empty, because it binds every interface and cannot tell whether it is reachable from anywhere but loopback. Verified against the official MCP conformance suite: the dns-rebinding-protection scenario failed both halves before and passes both after. Running the full legacy suite through VoidLLM as a transparent intermediary in front of the reference server now differs from the same suite run directly against that server in exactly one check, this one, where VoidLLM is the stricter of the two.
RefreshServer fetched outside the lock and then published unconditionally, so a refresh started before a credential change could put its stale- credential listing back after the invalidation had already removed it, making the invalidation pointless. Each server ID now carries a generation that every invalidation bumps; a refresh captures it before fetching and drops its result, cache and store alike, if it no longer matches. entryFor was checked and does not have this problem: it holds the write lock across its own fetch, so an invalidation cannot interleave. Session recording is now derived from the response the caller actually receives rather than from the upstream's headers. Reading Mcp-Session-Id back off the outgoing response makes "only record what we hand out" true by construction instead of by remembering to repeat the hop-by-hop check, and it also covers the 202 and redirect paths, which returned before the mirroring step and recorded a session the caller never saw. The MCP tool call duration histogram's label is renamed to method, matching the counter's rename and what both have always carried. A sweep over every label list and WithLabelValues call found no further instance. Noted, outside this repo: the public site's configuration docs still document the counter's old label name.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Seven items, from an independent review of the complete branch. A tool listing rehydrated from the database carries neither its cache hint nor its header-parameter bindings, because the store keeps only the tools themselves. Such an entry is now always stale on first access regardless of the configured fallback TTL. Previously, with tool_cache_ttl set to 0, it was marked as never expiring and so was never refreshed, which meant an annotated tool was called without its required Mcp-Param-* header for the lifetime of the process, silently. Two windows could bring an old tool listing back after a credential rotation. Transports are now reloaded before the tool cache is invalidated, so a concurrent miss can no longer pick up the old transport and publish under it; and the store write is fenced by its own lock and a second generation check, so an invalidation can no longer be undone by a refresh that was already past its first check. Neither takes I/O under the cache lock. Over-long Mcp-Param-* values are rejected rather than silently dropped, on both the transparent path and the client path, matching what the count limit already did. Silently dropping one made the request's headers and body disagree, which the upstream would answer with a header mismatch the caller could not explain. The tools/list decode error is no longer passed inward: the JSON library quotes a window of the source bytes on a syntax error, and that error reached a log through Code Mode. The same class was found and fixed in the OAuth discovery and token paths. Excluded tool names are truncated before they are logged. A session recorded by a response that arrived after its server was deactivated can no longer survive a quick reactivation. Removed server IDs are tombstoned, and a tombstone is cleared only by reactivation. The built-in server now validates Mcp-Param-* headers against the body before running a tool, with numeric comparison for integers, as the spec requires of any server that processes the body. Bindings are computed once at registration; a tool whose schema violates the annotation constraints fails registration, and that failure is fatal to startup rather than silently leaving the tool unregistered. Unknown method and tool names are truncated and escaped before they are echoed in an error message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The 2026-07-28 revision removes the
initializehandshake and protocol-levelsessions, makes
server/discovermandatory, and moves required per-request datainto
_metaplus mandatory headers. None of that is backward compatible, so botheras have to run side by side on one endpoint.
The protocol version is resolved once at the edge and every era difference lives
behind
ServerDialect/ClientDialect.Envelope,ResultandCallRequestareera-neutral, so no caller branches on the era. Dispatch keys off the negotiated
dialect, never off the version in the request body, which is attacker-controlled.
VoidLLM speaks both eras in all three roles: as an MCP server, as an MCP client,
and as a transparent intermediary.
Breaking change
Origin validation is now explicit-allow. It previously compared the
Originheader against the request's own
Host— a check DNS rebinding defeats trivially,since the attacker controls both. A configured
allowed_originslist is now theonly authority; with no list configured, a built-in localhost allowlist applies.
Anyone serving the MCP endpoints to a browser under a real domain has to list that
domain in
allowed_origins. Requests without anOriginheader are unaffected, soSDK and other non-browser clients are not impacted. VoidLLM warns at startup when
the list is empty.
Also in here
with an idle timeout and a byte cap.
relay another tenant's session upstream.
Mcp-Param-*headers relayed uninterpreted as the spec requires; more than thesupported number rejects the request rather than silently forwarding a subset.
x-mcp-headerannotations honoured client-side. A tool whose annotations violatethe constraints is excluded from
tools/liston its own.CacheableResultttlMsandcacheScope, clamped againstboth an upstream freezing an entry and one forcing a fetch per request.
privateresults stay in memory and never reach the database.
the upstream speaks instead of a hardcoded payload.
protocol_versionper server, settable via API, YAML and the UI, for the casewhere era detection gets it wrong.
Verification
go build,go vet,gofmtclean. Full suite green sequentially;-racegreen oninternal/mcpandinternal/api/admin.Validated against the official MCP conformance suite
(
@modelcontextprotocol/conformance). Running its full legacy suite through VoidLLMas a transparent intermediary in front of the reference server differs from the same
suite run directly against that server in exactly one of 32 checks:
dns-rebinding-protection, where VoidLLM is the stricter of the two. Everything elseis identical, including session handling against a stateful upstream, SSE with
multiple streams, resource subscribe/unsubscribe and error propagation.
The suite has no server scenarios for 2026-07-28 yet, so the modern era is covered by
this repository's own tests only.
Known and deliberately open
is not built. Each era works on its own; only the crossing is missing.
ToolCache.entryForholds the write lock across the upstream fetch.Server.RegisterToolstores the caller'sToolwithout cloning it.ListToolsdoes not follownextCursor.tool_nameis truncated on a byte boundary for the usage event.