feat!: stateless Streamable HTTP by default; SSE kept as a deprecated -transport option - #15
feat!: stateless Streamable HTTP by default; SSE kept as a deprecated -transport option#15codebutler wants to merge 4 commits into
Conversation
grpcmcp keeps no state between requests. Every tool call is one unary gRPC request. The server now says so: it mints no session, it ignores an inbound Mcp-Session-Id, and it answers GET with 405. Any replica can serve any request, and a restart costs a client nothing. A session is optional in MCP 2025-03-26 through 2025-11-25, and MCP 2026-07-28 removes it. Declining it is correct under every revision this server accepts, and 2026-07-28 asks for exactly this shape. The transport now follows one input. Without -hostport the server uses stdio. With -hostport it serves Streamable HTTP at /mcp. The HTTP+SSE transport from MCP 2024-11-05, which the spec deprecated in 2025-03-26, is deleted. listChanged now follows the transport. Only stdio can deliver the notification, so only stdio declares the capability. A stateless HTTP server holds no client to notify, and declaring the capability there would advertise something this server cannot do. Add two checks that MCP requires and mcp-go does not implement: * An inbound Origin header gets 403. Only a browser sets Origin, and this server has no browser client, so the check needs no origin list. This guards against DNS rebinding. * An unsupported Mcp-Protocol-Version gets 400, and the body names the versions this server supports. mcp-go answered an unknown version with its own newest version, which told the client it agreed to something it never checked. Remove four flags: * -transport, because -hostport now selects the transport. * -short-names and -very-short-names, because the very short form falls back on a collision and is correct in every case. * -bearer, because a token on argv is visible to every process through ps. Use -bearer-env, which now exits when the named variable is empty. Add -instructions, which describes the server to the agent. Upgrade mcp-go from v0.54.1 to v0.57.0. Both carry protocol version 2025-11-25, so the upgrade changes no behavior. BREAKING CHANGE: -transport, -short-names, -very-short-names, and -bearer are removed, and tool names now use the very short form. A deployment that passes a removed flag will not start. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
43aa21a to
d6635a1
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates grpcmcp to serve only stateless Streamable HTTP when -hostport is set (and stdio otherwise), removing the legacy SSE transport and session-related behavior to align with newer MCP protocol revisions.
Changes:
- Serve stateless Streamable HTTP only (no sessions, ignore
Mcp-Session-Id,GET /mcpreturns 405) and drop the legacy SSE transport. - Add HTTP-layer guards: reject requests with
Origin(403) and reject unsupportedMcp-Protocol-Versionvalues (400 with supported versions listed). - Simplify tool naming to always choose the shortest unique name; adjust refresh logic and tests/docs accordingly.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| transport_test.go | Adds handler-level tests covering stateless HTTP behavior, Origin rejection, and protocol-version rejection. |
| refresh.go | Removes short/very-short naming options from refresh path; refresh now uses the unified tool namer. |
| README.md | Updates docs to reflect transport selection via -hostport, stateless HTTP behavior, and removed SSE/flags. |
| origin.go | Introduces middleware to reject browser Origin headers and unsupported MCP protocol versions. |
| main.go | Removes SSE transport/flags, adds -instructions, enforces bearer token via env, sets capabilities based on transport, and serves stateless Streamable HTTP behind the new middleware. |
| main_test.go | Removes the legacy SSE transport integration test. |
| main_fork_test.go | Updates tool-name tests to match the new “shortest unique name” behavior. |
| go.mod | Bumps github.com/mark3labs/mcp-go to v0.57.0. |
| go.sum | Updates checksums for the mcp-go upgrade. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline nits already flagged, I also checked whether the protocol-version check should validate the protocolVersion field in the initialize request body, not just the Mcp-Protocol-Version header — it shouldn't. That field is negotiated per the MCP spec, where a server may reply with a version it does support rather than erroring on the client's proposed one, so the header-only check (which applies to post-initialize requests) is correct as written.
Extended reasoning...
The candidate issue flagged that rejectUnsupportedProtocolVersion in origin.go only inspects the Mcp-Protocol-Version header and not the protocolVersion field inside an initialize request body. I read origin.go and mcp-go's initialize negotiation: the header only applies to requests made after a version has been negotiated (per spec, a client sends this header on every request following the initial handshake), while the initial initialize call negotiates protocolVersion via the body, where the spec explicitly permits the server to reply with a version it supports instead of erroring, letting the client decide whether to proceed. So mcp-go's body-level behavior described in the code comment ("answers an unknown one with the newest version it knows") is the spec-sanctioned negotiation path for initialize, not a gap this middleware needs to close — the middleware's job is the separate, stricter check on subsequent requests. I'm not approving given the PR's size and its security-relevant surface (Origin/DNS-rebinding check, protocol-version enforcement, bearer-env auth behavior, and the removal of the SSE transport and several flags), which warrants a human look regardless.
… null buildToolNamer incremented simpleNameCount for every filtered-in service, including ones that expose no tools. A same-named sibling that is all streaming then forced the full-path fallback even though ServiceName__MethodName was already unique. Count a service only when at least one of its methods is exposed. writeJSONRPCError omitted the id member. JSON-RPC 2.0 requires it, with null when the request id is unknown. Set id to null so the Origin and protocol-version error bodies match the spec and mcp-go's own shape. Co-authored-by: Eric Butler <eric@codebutler.com>
There was a problem hiding this comment.
Beyond the reported mcp-go DNS-rebinding regression, I also checked whether rejectUnsupportedProtocolVersion should validate the initialize request body's protocolVersion field in addition to the Mcp-Protocol-Version header — it's header-only by design (the header is what the spec defines for this check on non-initialize requests too), so this isn't a gap.
Extended reasoning...
Checked the candidate concern that rejectUnsupportedProtocolVersion in origin.go only inspects the Mcp-Protocol-Version HTTP header and never cross-checks the protocolVersion field inside an initialize request body. This isn't a real gap: the MCP spec's protocol-version negotiation for Streamable HTTP is header-based for every request after the first, and mcp-go's own initialize handling separately negotiates the body's protocolVersion during the handshake. The two checks operate on different fields for different purposes, so there's no missing validation here.
amarbakir
left a comment
amarbakir
left a comment
There was a problem hiding this comment.
I'd take a look at the Claude comments and try to run this locally to make sure it works as expected
c86b569 to
b404355
Compare
Stacked on #15. Boots the real binary against the example backend and exercises it over actual HTTP: initialize/tools-list/tools-call on Streamable HTTP, the 405/403/400 guards, and the SSE path (deprecation warning, event-stream response, Origin rejection). Complements the in-process unit tests in transport_test.go, which don't prove the binary actually boots and serves correctly end-to-end. Adds a real RPC (echo.v1.EchoService/Echo) to the example backend, built by hand the same way this repo's own tests build descriptors, so grpcmcp's reflection -> tools/call -> backend path gets a genuine round trip with a real argument and a real return value, not just health-check plumbing. Wire-level gRPC framing (length-prefixed unary request/response + a grpc-status trailer) is hand-rolled rather than built on connect-go's generic Handler API, since dynamicpb messages can't be plugged into connect's generic type parameters without a concrete Go type to instantiate. Wired into CI: runs after go test on every push/PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJKesYekqPnbBrhM3swNHs
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Stacked on #15. Boots the real binary against the example backend and exercises it over actual HTTP: initialize/tools-list/tools-call on Streamable HTTP, the 405/403/400 guards, and the SSE path (deprecation warning, event-stream response, Origin rejection). Complements the in-process unit tests in transport_test.go, which don't prove the binary actually boots and serves correctly end-to-end. Adds a real RPC (echo.v1.EchoService/Echo) to the example backend, built by hand the same way this repo's own tests build descriptors, so grpcmcp's reflection -> tools/call -> backend path gets a genuine round trip with a real argument and a real return value, not just health-check plumbing. Wire-level gRPC framing (length-prefixed unary request/response + a grpc-status trailer) is hand-rolled rather than built on connect-go's generic Handler API, since dynamicpb messages can't be plugged into connect's generic type parameters without a concrete Go type to instantiate. Wired into CI: runs after go test on every push/PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJKesYekqPnbBrhM3swNHs
b404355 to
e798cb8
Compare
There was a problem hiding this comment.
Beyond the inline finding on the -transport validation regression, I also checked two other things this run: rejectUnsupportedProtocolVersion's header-only check against the initialize exemption (the initialize request itself carries no Mcp-Protocol-Version header, so it correctly passes through) and the tier-3 tool-name fallback's non-injective ReplaceAll(".", "_") collision path (pre-existing behavior, not introduced by this diff). Neither is a new problem beyond what's flagged inline.
Extended reasoning...
This run's inline findings (main.go:217, a real regression where -transport validation now runs unconditionally and drops the "streamable-http" alias) are new since my last review of this PR and are already posted as an inline comment, so the review body should not restate them. All three issues from my prior review (main.go:112 simpleNameCount gating, origin.go:28 missing JSON-RPC id, main.go:427 missing WithDisableLocalhostProtection) are verified fixed in the current code: simpleNameCount is now gated on exposed, writeJSONRPCError now includes "id": nil, and both NewStreamableHTTPServer and NewSSEServer now pass the localhost-protection opt-outs. Given a finding is present this run, per the narrow exception I'm recording the two additional candidate issues that were specifically investigated and ruled out (protocol-version header/initialize exemption, and the pre-existing tier-3 name-collision fallback), without restating the inline findings themselves.
e798cb8 to
4b2b713
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches security-sensitive flows (bearer-token handling, DNS-rebinding/Origin checks, and HTTP transport selection) and the latest commit hasn't had human eyes on it yet, a human look would still be worthwhile.
What was reviewed: the restored -transport=sse path and its now-hostport-gated flag validation in main.go, the WithDisableLocalhostProtection/WithSSEDisableLocalhostProtection wiring added to both HTTP transports, and the new transport_flag_test.go/transport_test.go coverage for these paths. Also checked whether the legacy SSE handler should get the same Mcp-Protocol-Version check as Streamable HTTP — it currently doesn't, which appears intentional given the deprecated, optional nature of that transport rather than an oversight.
Extended reasoning...
Overview
This is the same PR I've reviewed on prior pushes. The latest commit (4b2b713, "restore -transport=sse as a deprecated, flag-gated option") makes three changes on top of the previously-reviewed state: it moves -transport validation so it only runs when -hostport is set (fixing a regression I flagged in my last review, where a stdio invocation with any -transport value would now exit(-1)); it restores -transport=sse as a deprecated legacy path, wrapped in rejectBrowserOrigin and passing WithSSEDisableLocalhostProtection(true); and it adds WithDisableLocalhostProtection(true) to the stateless Streamable HTTP server (fixing the mcp-go v0.57.0 default-on loopback DNS-rebinding check I flagged two reviews ago, which would have spuriously 403'd common local proxy/port-forward topologies). Both fixes are backed by new tests (transport_flag_test.go, and additions to transport_test.go covering the loopback-with-foreign-Host case and SSE Origin rejection). An earlier commit (0c44ecae866f, authored by a different party, not self-resolved by this bot) also independently fixed the simpleNameCount short-name-collision bug and the missing JSON-RPC id: null — both previously-flagged findings — via actual code changes rather than thread dismissal.
Security risks
The touched surface is inherently security-relevant: bearer-token sourcing (env-var only, now hard-fails on empty/unset rather than silently sending no auth), the Origin-header DNS-rebinding guard, the new Mcp-Protocol-Version validation, and now the interaction between grpcmcp's own Origin check and mcp-go's built-in (and now disabled) loopback-based DNS-rebinding guard. The latest commit's reasoning for disabling mcp-go's native guard — that rejectBrowserOrigin already covers every connection, loopback or not — is sound on its face, but it is exactly the kind of judgment call (trusting a custom re-implementation over a well-tested upstream default) that benefits from an independent human read, especially since it hasn't had one yet.
Level of scrutiny
High. This is a breaking change to transport selection and auth-flag handling, and the diff has already gone through several rounds of bot and human review with real bugs found and fixed at each step. That history is a point in the PR's favor, not a reason to lower scrutiny now — the fact that meaningful fixes kept surfacing on each pass argues for keeping a human in the loop on the newest code rather than rubber-stamping it once the bug-hunting system stops finding things.
Other factors
No new findings were produced by this run's bug hunt. The one candidate examined and ruled out (SSE not getting the same protocol-version check as Streamable HTTP) looks like a deliberate scope decision for a deprecated transport rather than a bug. A human reviewer (amarbakir) already approved an earlier revision of this PR, but that approval predates this latest commit, so the specific changes reviewed here are still unseen by a person.
-hostport alone now only selects HTTP vs stdio; -transport chooses which HTTP transport, defaulting to the stateless Streamable HTTP this PR introduces. -transport=sse brings back the legacy stateful transport this PR removed, so clients that cannot yet speak Streamable HTTP keep working. Selecting it prints a stderr deprecation notice. -transport is only validated (and only defaults meaningfully) when -hostport is set, matching how -tls-crt is already gated a few lines below -- a stdio invocation carrying a stale or unrelated -transport value is not rejected over a flag that does nothing for it. -transport=streamable-http is accepted as an alias for http, matching this server's behavior before -transport was removed by this PR. Both HTTP transports are wrapped in the Origin check (rejectBrowserOrigin), which covers DNS rebinding for every connection regardless of whether it arrived via loopback. mcp-go's own DNS-rebinding guard is narrower -- it only fires for loopback connections -- and default-on since the v0.57.0 bump; left as-is it 403s a real, common local-dev shape (kubectl port-forward through a proxy that preserves the external Host header), without adding any protection this server doesn't already have. Both transports now pass WithDisableLocalhostProtection / (SSE's equivalent) to turn it off. listChanged capability now follows serveHTTP-or-not, minus stateless HTTP specifically: stdio and SSE both hold a live client to notify, stateless HTTP does not. README updated to describe three transports instead of two. The post() test helper now closes each response body via t.Cleanup, per review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJKesYekqPnbBrhM3swNHs
4b2b713 to
3bb6eb1
Compare
SSE was only wrapped in rejectBrowserOrigin, so a client hitting --transport=sse with an unsupported Mcp-Protocol-Version silently got mcp-go's fallback instead of the documented 400 rejection that Streamable HTTP and the README both promise.
There was a problem hiding this comment.
I reviewed the latest commits and found no new bugs. All of the issues flagged in earlier rounds of this review (the short-name collision counting in buildToolNamer, the missing id in JSON-RPC error responses, the default-on DNS-rebinding check breaking common proxy topologies, and the unconditional -transport validation rejecting stdio invocations) have been fixed, and the SSE-transport protocol-version gap I flagged in my last review is now closed by the latest commit (e7d245e). Because this PR touches bearer-token handling, DNS-rebinding protections, and reintroduces a legacy transport (SSE) after the existing human approval, a human look at these newest commits would still be worthwhile.
Extended reasoning...
This is a zero-new-findings run, but the PR has a substantial history of prior findings across multiple review rounds, all of which I re-verified against the current HEAD. The four issues I previously raised (main.go:112 simpleNameCount over-counting non-exposing services; origin.go:28 missing JSON-RPC "id"; main.go:439 mcp-go v0.57.0's default-on Host-header DNS-rebinding check breaking common proxy topologies; main.go:217 unconditional -transport validation rejecting stdio invocations) were fixed in commit 0c44eca and 3bb6eb1. My most recent finding, that the reintroduced SSE branch (added in 3bb6eb1) only wrapped its handler in rejectBrowserOrigin and not rejectUnsupportedProtocolVersion, was fixed by the very latest commit e7d245e, which changes handler := rejectBrowserOrigin(sseSrv) to handler := rejectBrowserOrigin(rejectUnsupportedProtocolVersion(sseSrv)) — exactly the one-line fix I suggested.
I re-read origin.go and the relevant sections of main.go end-to-end to confirm these fixes are correct and complete: writeJSONRPCError now includes "id": nil; buildToolNamer only increments simpleNameCount when a service actually exposes at least one tool; both the stateless Streamable HTTP path and the newly reintroduced SSE path pass WithDisableLocalhostProtection/WithSSEDisableLocalhostProtection and are wrapped in both rejectBrowserOrigin and rejectUnsupportedProtocolVersion; and -transport is now validated only when -hostport is set, matching the existing -tls-crt gating pattern, with "streamable-http" retained as an accepted alias.
Security risk here remains real even with no new findings: this code governs bearer-token sourcing (env-only, exits if unset), DNS-rebinding defenses (both the Origin check and the disabling of mcp-go's narrower loopback-only check), and JSON-RPC/protocol-version negotiation on the network-facing HTTP paths. The PR also reintroduces a legacy SSE transport after a prior human "APPROVED" review (amarbakir, 2026-08-05) — that approval predates the SSE reintroduction (3bb6eb1, 2026-08-31) and its follow-up fix (e7d245e, 2026-09-02), so a human has not yet looked at this newer surface area. Given the security sensitivity, the amount of churn since the last human approval, and the general guidance to prefer deferring when in doubt, I chose defer over approve despite finding no new bugs myself.
I am not repeating my four prior inline findings' substance here, only confirming their resolution status as part of the verdict, per the "acknowledge progress" guidance. No new inline findings are being posted this run since the bug hunter reported none and I found nothing additional beyond what was already flagged.
defer
DanielPBak
changed the title
Why
grpcmcp keeps no state between requests. Every tool call is one unary gRPC call, so there is nothing worth carrying forward. It nonetheless minted sessions, because that is the
mcp-godefault.That default cost three things. Sessions accumulated with no sweeper, since the POST path registers a session and never unregisters it. Session IDs were never validated, so the machinery bought no correctness. And
tools/list_changedwas declared but cannot be delivered coherently by a multi-replica server, because each process refreshes on its own timer.MCP 2026-07-28 removed protocol-level sessions outright. This change moves grpcmcp to what it already was.
What changed
Stateless Streamable HTTP.
WithStateLess(true)andWithDisableStreaming(true). The server mints no session, ignores an inboundMcp-Session-Id, and answersGETwith 405. Any replica can serve any request, and a restart costs a client nothing.A session is optional in MCP 2025-03-26 through 2025-11-25 (the spec says a server MAY assign one), and removed in 2026-07-28. Declining it is correct under every revision this server accepts — it still speaks 2025-11-25 and older, statelessly — and 2026-07-28 asks for exactly this shape: ignore an inbound
Mcp-Session-Id, answerGETwith 405.-transportselects the HTTP transport; SSE is kept but deprecated. Without-hostport, stdio. With-hostport,-transport=http(the default, aliasstreamable-http) serves Streamable HTTP at/mcp.-transport=ssekeeps the legacy HTTP+SSE transport from MCP 2024-11-05 for clients that cannot yet speak Streamable HTTP — it prints a deprecation warning at startup (the transport is deprecated in the spec since 2025-03-26, formally classified so by 2026-07-28) and will be removed once no deployment needs it. This preserves upstream's transport surface instead of dropping it.listChangedfollows the transport.trueon stdio,falseon HTTP. A stateless server holds no client to notify, so declaring the capability there would advertise something it cannot do. The refresh loop still replaces the tool set, and clients see the new set on their nexttools/list.Two MCP requirements
mcp-godoes not implement. Both applied to Streamable HTTP and SSE:Originheader gets 403. Only a browser setsOrigin, and this server has no browser client, so the valid set is empty and the check needs no origin list. This is the DNS-rebinding guard the spec requires.Mcp-Protocol-Versiongets 400, with the supported versions named in the body.mcp-goanswered an unknown version with its own newest version instead, telling the client it had agreed to something it never checked.Flag surface. Removed
-short-names,-very-short-names, and-bearer. Added-instructions. Very-short tool naming with collision fallback is now the only behavior.-bearer-envis the sole token path and exits when the named variable is empty, rather than silently sending no auth — a token on argv is visible to every process on the host throughps.mcp-gov0.54.1 to v0.57.0. Both carry protocol version 2025-11-25, so the upgrade changes no behavior.Verification
Tests in
transport_test.goandtransport_flag_test.gocover the handler and flag wiring. The table below is the built binary against the example gRPC backend in this repo.tools/list, no initialize, no sessionMcp-Session-Idreturnedtools/callcarrying a stale session headerGET /mcpOriginMcp-Protocol-Version: 2025-06-18Mcp-Protocol-Version: 2026-07-28-transport=sse/sse, prints deprecation warning, same Origin/protocol-version guards-instructionsinitializeresultcapabilities.toolsover HTTP{}, nolistChangedBreaking changes
-short-names,-very-short-names, and-bearerare removed, and tool names now use the very short form. Go exits on an unknown flag, so any deployment passing a removed flag will fail to start. Update the invocation in the same step as the image.Deployments using SSE keep working via
-transport=sse, but should plan the move to Streamable HTTP — the SSE path is deprecated and slated for removal.The protocol-version check is the only change that can reject traffic which works today. It fails closed by design, and the error body names the versions to retry with.
🤖 Generated with Claude Code