From a357b9f609cfbcf8ed3cbd3b42fcf66bf953d640 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 9 Sep 2026 19:22:58 -0400 Subject: [PATCH] fix(mcp): restrict the remote transport to the protocol era pad can serve (TASK-2977) (#1310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): restrict the remote transport to the era pad can serve (TASK-2977) mcp-go 1.0 implements the stateless protocol core from 2026-07-28 — no handshake, no sessions, per-request identity in _meta — and its Streamable HTTP transport advertises EVERY revision it implements by default, serving both eras concurrently on one endpoint and deciding the era per request. pad's construction site passed no version restriction, so the library bump alone had main answering modern-era traffic through server/discover while pad://_meta/version still published 2025-11-25 as the maximum revision this server can negotiate. NOT A PRODUCTION DEFECT, checked rather than assumed: app.getpad.dev and mcp.getpad.dev both report commit 0e2cb06a built 2026-08-31, nine days before the mcp-go 1.0 merge, and 0.58 has no 2026-07-28 constant at all. The gap is in main, ahead of a hand deploy, so this lands before it can become real. Beyond the mismatch: pad_set_workspace pins a session default workspace and the stateless era has no session for that pin to live in. So the modern era is not something pad happens not to advertise, it is something pad is not known to be able to serve. Establishing what it would take is step 2 of this item; the honest advertisement meanwhile is the era pad was built and tested against. The set is DERIVED from mcp.LegacyProtocolVersions(), the SDK's own answer to "which revisions use the handshake", so a future SDK adding a legacy revision includes it and one adding a modern revision excludes it, with no edit here. A hand-written list would silently mean the wrong thing after either bump — the same shape as the defect being closed. The option set moved into mcpserver.NewRemoteTransport so a test can drive what cmd/pad actually constructs. An advertised set is only correct if the option is PASSED, and a test building its own transport would vouch for the option and not for the binding (CONVE-19). Four tests, and the second is what makes the first mean anything: - a well-formed 2026-07-28 server/discover against pad's transport is refused with code -32022, data.requested naming the version and data.supported carrying exactly the legacy four. Asserting "an error came back" would also pass on a transport that had simply broken. - the identical bytes against an UNRESTRICTED mcp-go transport are SERVED, with 2026-07-28 among supportedVersions. Negative control: it is what says the refusal comes from pad's option rather than from a malformed request or a changed library default. Both requests carry the Mcp-Method header the modern era requires, so a refusal cannot be about headers. - initialize still negotiates 2025-11-25 — the restriction must not break the era pad actually serves. - AdvertisedMCPProtocolVersion equals the newest served revision, which is the half TestAdvertisedProtocolVersion structurally cannot see: it pins the literal to what the HANDSHAKE answers, and the modern era has no handshake. Sweep: meta.go's two comments described the handshake cap as the whole story. One construction site only — pad-cloud is an OAuth/billing layer and builds no transport, so the pad binary in cloud mode is the single place this is decided. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * test(mcp): the handshake test measured less than its comment claimed (TASK-2977) A mutation found this in my own instrument. Restricting the advertised list to a version that EXCLUDES 2025-11-25 leaves the legacy-handshake test green, so that test cannot be evidence that the restriction preserved the era pad serves — which is exactly what its comment said it was. The mechanism, read rather than inferred from the green: initialize is answered by MCPServer through mcp.NegotiateLegacyVersion, which consults LATEST_LEGACY_PROTOCOL_VERSION and never the transport's list. The two are independent. So the comment now says what the test measures (the legacy path works) and what it does not (that the restriction preserved it), and the independence is pinned as its own subtest: a transport advertising only 2025-06-18 still answers initialize with 2025-11-25. If that ever fails, the handshake has become coupled to the advertised list and the restriction has become able to refuse legacy clients — the moment this file needs a different test. Nothing about the fix changes; the claim about the evidence does. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn --- cmd/pad/cmd_server.go | 22 +-- internal/mcp/meta.go | 22 ++- internal/mcp/transport.go | 71 +++++++++ internal/mcp/transport_test.go | 269 +++++++++++++++++++++++++++++++++ 4 files changed, 366 insertions(+), 18 deletions(-) create mode 100644 internal/mcp/transport.go create mode 100644 internal/mcp/transport_test.go diff --git a/cmd/pad/cmd_server.go b/cmd/pad/cmd_server.go index 30ac954c..3ffc029d 100644 --- a/cmd/pad/cmd_server.go +++ b/cmd/pad/cmd_server.go @@ -40,7 +40,6 @@ import ( "github.com/PerpetualSoftware/pad/internal/watchevents" "github.com/PerpetualSoftware/pad/internal/webhooks" "github.com/google/uuid" - mcptransport "github.com/mark3labs/mcp-go/server" "github.com/redis/go-redis/v9" ) @@ -385,21 +384,14 @@ func serveCmd() *cobra.Command { // session-id behave exactly as they did under the original // WithStateLess(true) setup. Codex review on PR #400 round 1 // caught the gauge-stays-at-zero gap. - streamable := mcptransport.NewStreamableHTTPServer( + // The option set lives in mcpserver.NewRemoteTransport so a + // test can drive the real thing: the advertised protocol + // versions are only correct if the option is actually passed, + // and a test constructing its own transport would vouch for + // the option rather than for this binding (TASK-2977). + streamable := mcpserver.NewRemoteTransport( mcpSrv.MCP(), - mcptransport.WithEndpointPath("/mcp"), - mcptransport.WithSessionIdManager(&padMCPGenerateOnlySessionIDManager{}), - // mcp-go v0.56 turns on DNS-rebinding protection by - // default: a request whose accept socket is loopback but - // whose Host header is non-loopback gets a 403. pad-cloud's - // mcp.getpad.dev vhost sits behind a reverse proxy that - // forwards to this process over 127.0.0.1 while preserving - // the original Host, so the default would reject every real - // request. Disable it to keep the pre-v0.56 behaviour — the - // browser-driven rebinding threat it guards against doesn't - // apply here: this transport only mounts in cloud mode and - // every request is Bearer/OAuth-authenticated. - mcptransport.WithDisableLocalhostProtection(true), + &padMCPGenerateOnlySessionIDManager{}, ) // TASK-1120: optional env-driven overrides for the // mcp-active-sessions tracker. Both default to the diff --git a/internal/mcp/meta.go b/internal/mcp/meta.go index 84b1b65a..cd656451 100644 --- a/internal/mcp/meta.go +++ b/internal/mcp/meta.go @@ -51,6 +51,15 @@ type MetaPayload struct { // So the advertised revision is a claim pad makes deliberately. Moving // it means reading the new revision's delta against this server's // surface first; a library bump must not move it on its own. + // + // The claim is also ENFORCED now, not merely documented (TASK-2977). The + // handshake cap is not the whole story: mcp-go 1.0's Streamable HTTP + // transport serves both eras on one endpoint and advertises the modern one + // through server/discover by default, so pinning this literal while + // leaving the transport unrestricted would have published one maximum here + // and a higher one on the wire. ServedProtocolVersions restricts the + // transport to the handshake era, and a test ties its newest entry to this + // constant, so the two cannot drift apart in either direction. MCPProtocolVersion string `json:"mcp_protocol_version"` } @@ -65,9 +74,16 @@ type MetaPayload struct { // upper bound, not any specific session. // AdvertisedMCPProtocolVersion is the MCP wire protocol revision pad claims // to negotiate. It is deliberately a literal rather than a library constant — -// see MetaPayload.MCPProtocolVersion — and TestAdvertisedProtocolVersion pins -// it to what the library's handshake actually answers, so a bump that moves -// one and not the other fails instead of shipping a false claim. +// see MetaPayload.MCPProtocolVersion. +// +// Two tests hold it in place, and they answer different questions. +// TestAdvertisedProtocolVersion pins it to what the library's HANDSHAKE +// answers. That is necessary and not sufficient: the era introduced in +// 2026-07-28 has no handshake, so a handshake test cannot see the transport +// advertising it through server/discover. +// TestAdvertisedRevisionMatchesWhatTheTransportServes closes that half by +// pinning this constant to the newest revision ServedProtocolVersions allows +// the transport to advertise (TASK-2977). const AdvertisedMCPProtocolVersion = "2025-11-25" func BuildMetaPayload(padVersion string) MetaPayload { diff --git a/internal/mcp/transport.go b/internal/mcp/transport.go new file mode 100644 index 00000000..f8f92702 --- /dev/null +++ b/internal/mcp/transport.go @@ -0,0 +1,71 @@ +package mcp + +import ( + "github.com/mark3labs/mcp-go/mcp" + "github.com/mark3labs/mcp-go/server" +) + +// ServedProtocolVersions is the set of MCP wire protocol revisions pad's +// remote transport advertises: the LEGACY era only, meaning every revision +// that still uses the initialize/initialized handshake. +// +// WHY THIS IS RESTRICTED AT ALL (TASK-2977). mcp-go 1.0 implements the +// stateless protocol core introduced in 2026-07-28 — no handshake, no +// sessions, per-request identity in `_meta` — and its Streamable HTTP +// transport advertises EVERY revision it implements by default, serving both +// eras concurrently on one endpoint and deciding the era per request. So the +// library bump alone would have had pad advertising the modern era through +// server/discover, while pad://_meta/version publishes 2025-11-25 as the +// maximum revision this server can negotiate. That mismatch is the defect: +// the advertisement is the promise a client acts on, and nothing here has +// been read against the modern revision, let alone tested. +// +// It is also not merely a documentation gap. pad_set_workspace pins a session +// default workspace, and the stateless era has no sessions for that pin to +// live in — so the modern era is not something pad happens not to advertise, +// it is something pad is not known to be able to serve. Establishing what it +// would take is a separate unit; until then the honest advertisement is the +// era pad was built against and is tested against. +// +// DERIVED, NOT LISTED, and that is load-bearing. mcp.LegacyProtocolVersions() +// is the SDK's own answer to "which revisions use the handshake", so a future +// SDK that adds another legacy revision includes it here automatically and one +// that adds another modern revision excludes it automatically. A hand-written +// list would silently mean the wrong thing after either bump, which is the +// shape of the defect this function exists to close. +func ServedProtocolVersions() []string { + return mcp.LegacyProtocolVersions() +} + +// NewRemoteTransport builds the Streamable HTTP transport pad serves at /mcp +// in cloud mode. +// +// It lives here rather than inline at the call site so the option set is +// reachable from a test: the advertised protocol versions are only correct if +// the option is actually PASSED, and a test that constructs its own transport +// vouches for the option and not for the binding (team CONVE-19). +// +// sessionIDs is the caller's session-id manager. It is a parameter because the +// generate-only manager pad uses exists for the active-sessions tracker in +// cmd/pad, and the reason it is not mcp-go's WithStateLess(true) is documented +// there. +func NewRemoteTransport(srv *server.MCPServer, sessionIDs server.SessionIdManager) *server.StreamableHTTPServer { + return server.NewStreamableHTTPServer( + srv, + server.WithEndpointPath("/mcp"), + server.WithSessionIdManager(sessionIDs), + // See ServedProtocolVersions: without this the transport advertises + // the stateless 2026-07-28 core through server/discover. + server.WithStreamableHTTPProtocolVersions(ServedProtocolVersions()...), + // mcp-go v0.56 turns on DNS-rebinding protection by default: a request + // whose accept socket is loopback but whose Host header is non-loopback + // gets a 403. pad-cloud's mcp.getpad.dev vhost sits behind a reverse + // proxy that forwards to this process over 127.0.0.1 while preserving + // the original Host, so the default would reject every real request. + // Disable it to keep the pre-v0.56 behaviour — the browser-driven + // rebinding threat it guards against doesn't apply here: this transport + // only mounts in cloud mode and every request is Bearer/OAuth + // authenticated. + server.WithDisableLocalhostProtection(true), + ) +} diff --git a/internal/mcp/transport_test.go b/internal/mcp/transport_test.go new file mode 100644 index 00000000..521361c0 --- /dev/null +++ b/internal/mcp/transport_test.go @@ -0,0 +1,269 @@ +package mcp + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "slices" + "strings" + "testing" + + mcpspec "github.com/mark3labs/mcp-go/mcp" + "github.com/mark3labs/mcp-go/server" +) + +// generateOnlySessionIDs mirrors cmd/pad's padMCPGenerateOnlySessionIDManager +// closely enough for a transport test: Generate returns a value, Validate +// accepts anything. The real one is in package main and unreachable from here; +// what is under test is the option set, not the manager. +type generateOnlySessionIDs struct{} + +func (generateOnlySessionIDs) Generate() string { return "test-session" } +func (generateOnlySessionIDs) Validate(string) (bool, error) { return false, nil } +func (generateOnlySessionIDs) Terminate(string) (bool, error) { return false, nil } + +// postJSONRPC drives one JSON-RPC request through a Streamable HTTP transport +// and returns the decoded envelope. Modern-era requests are identified by the +// Mcp-Protocol-Version header, which is how the transport decides the era. +func postJSONRPC(t *testing.T, h http.Handler, protocolVersion, method, body string) map[string]any { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if protocolVersion != "" { + req.Header.Set(mcpspec.HeaderProtocolVersion, protocolVersion) + } + // The modern era requires the JSON-RPC method to be mirrored in a header + // (SEP-2243) so gateways can route without parsing bodies. Set it whenever + // the caller supplied one, so a request that reaches the version gate is + // well-formed in every OTHER respect — otherwise a refusal could be about + // the headers and the test would not know. + if method != "" { + req.Header.Set(mcpspec.HeaderMethod, method) + } + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + raw := rec.Body.String() + // A Streamable HTTP response may arrive as SSE; take the first data frame. + if strings.HasPrefix(strings.TrimSpace(raw), "event:") || strings.Contains(raw, "\ndata: ") { + for _, line := range strings.Split(raw, "\n") { + if after, ok := strings.CutPrefix(line, "data: "); ok { + raw = after + break + } + } + } + var envelope map[string]any + if err := json.Unmarshal([]byte(strings.TrimSpace(raw)), &envelope); err != nil { + t.Fatalf("status %d, body %q: %v", rec.Code, rec.Body.String(), err) + } + return envelope +} + +func modernDiscover(t *testing.T, h http.Handler) map[string]any { + t.Helper() + return postJSONRPC(t, h, mcpspec.ProtocolVersion20260728, string(mcpspec.MethodServerDiscover), modernDiscoverBody) +} + +// modernDiscoverBody is a well-formed 2026-07-28 server/discover request: the +// protocol version, client identity and client capabilities all travel in +// _meta, because the modern era has no handshake to carry them. +const modernDiscoverBody = `{ + "jsonrpc": "2.0", "id": 1, "method": "server/discover", + "params": {"_meta": { + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "io.modelcontextprotocol/clientInfo": {"name": "test", "version": "0.0.0"}, + "io.modelcontextprotocol/clientCapabilities": {} + }} +}` + +// TestRemoteTransportRefusesTheModernEra is the reason TASK-2977 exists. +// mcp-go 1.0's Streamable HTTP transport advertises and serves every revision +// it implements by default, including the stateless 2026-07-28 core, deciding +// the era per request — so the library bump alone would have had pad answering +// modern-era traffic it has never been read against, while pad://_meta/version +// still published 2025-11-25 as its maximum. +// +// It drives the CONSTRUCTOR cmd/pad calls, not a transport built here, because +// the advertised set is only correct if the option is actually passed, and a +// test constructing its own transport vouches for the option rather than for +// the binding (team CONVE-19). +// +// The refusal is a typed one and the assertions read it: code -32022, and a +// data.supported list the client can negotiate down from. That is the shape a +// well-behaved client acts on, so asserting only "the response was an error" +// would pass on a transport that had simply broken. +func TestRemoteTransportRefusesTheModernEra(t *testing.T) { + transport := NewRemoteTransport(server.NewMCPServer("pad-test", "0.0.0"), generateOnlySessionIDs{}) + + env := modernDiscover(t, transport) + + errObj, ok := env["error"].(map[string]any) + if !ok { + t.Fatalf("a 2026-07-28 server/discover was SERVED, not refused: %v — pad has "+ + "not been read against the stateless era, and pad_set_workspace pins "+ + "session state that era does not have", env) + } + if code, _ := errObj["code"].(float64); int(code) != -32022 { + t.Errorf("refused with code %v, want -32022 (unsupported protocol version) — "+ + "a different code means it was refused for a different reason and this "+ + "test is not measuring the version gate", errObj["code"]) + } + data, ok := errObj["data"].(map[string]any) + if !ok { + t.Fatalf("refusal carries no data: %v", errObj) + } + if got := data["requested"]; got != mcpspec.ProtocolVersion20260728 { + t.Errorf("refusal names requested=%v, want %s", got, mcpspec.ProtocolVersion20260728) + } + supported := toStrings(t, data["supported"]) + if slices.Contains(supported, mcpspec.ProtocolVersion20260728) { + t.Errorf("the refusal offers %s among supported versions: %v", + mcpspec.ProtocolVersion20260728, supported) + } + if want := mcpspec.LegacyProtocolVersions(); !slices.Equal(supported, want) { + t.Errorf("offers %v, want %v — the set is DERIVED from the SDK's own "+ + "handshake-era list, so a future revision lands on the right side of "+ + "this line without an edit here", supported, want) + } +} + +// TestUnrestrictedTransportWouldServeTheModernEra is the NEGATIVE CONTROL for +// the test above, and without it that test proves little: a green there is +// equally consistent with the option working and with the request being +// malformed in some way that would be refused by any transport. +// +// This builds the transport WITHOUT pad's option — a bare mcp-go 1.0 default — +// and sends the identical bytes. It must be SERVED, and the served answer must +// advertise the modern era. +func TestUnrestrictedTransportWouldServeTheModernEra(t *testing.T) { + bare := server.NewStreamableHTTPServer( + server.NewMCPServer("pad-test", "0.0.0"), + server.WithEndpointPath("/mcp"), + server.WithSessionIdManager(generateOnlySessionIDs{}), + server.WithDisableLocalhostProtection(true), + ) + + env := modernDiscover(t, bare) + + result, ok := env["result"].(map[string]any) + if !ok { + t.Fatalf("an UNRESTRICTED mcp-go transport refused the same request: %v — if "+ + "the library default has changed, or this request is malformed, then "+ + "the refusal asserted above is not evidence that pad's option did "+ + "anything", env) + } + if got := toStrings(t, result["supportedVersions"]); !slices.Contains(got, mcpspec.ProtocolVersion20260728) { + t.Errorf("an unrestricted transport advertised %v, without %s — the default "+ + "this option exists to override may no longer be the default", + got, mcpspec.ProtocolVersion20260728) + } +} + +func toStrings(t *testing.T, v any) []string { + t.Helper() + raw, ok := v.([]any) + if !ok { + t.Fatalf("not a list: %#v", v) + } + out := make([]string, 0, len(raw)) + for _, item := range raw { + s, ok := item.(string) + if !ok { + t.Fatalf("not a string in %#v", v) + } + out = append(out, s) + } + return out +} + +// TestRemoteTransportStillAnswersTheLegacyHandshake covers the era pad +// actually serves: a client that opens with initialize negotiates 2025-11-25, +// which is what every pad MCP client does today and what pad://_meta/version +// advertises. +// +// WHAT THIS DOES NOT MEASURE, established by mutation rather than assumed. The +// handshake is INDEPENDENT of the transport's advertised list: initialize is +// answered by MCPServer through mcp.NegotiateLegacyVersion, which consults +// LATEST_LEGACY_PROTOCOL_VERSION and never the transport. Restricting the list +// to a version that excludes 2025-11-25 leaves this test green — measured, and +// asserted directly by the subtest below. So this test says the legacy path +// works; it is NOT evidence that the restriction preserved it, and the earlier +// draft of this comment claimed it was. +// +// That independence is itself worth pinning: a future reader restricting the +// advertised list in the belief that it gates the handshake would be wrong in +// a way nothing else here would catch. +func TestRemoteTransportStillAnswersTheLegacyHandshake(t *testing.T) { + transport := NewRemoteTransport(server.NewMCPServer("pad-test", "0.0.0"), generateOnlySessionIDs{}) + + env := postJSONRPC(t, transport, "", "", `{ + "jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": { + "protocolVersion": "2025-11-25", + "capabilities": {}, + "clientInfo": {"name": "test", "version": "0.0.0"} + } + }`) + + result, ok := env["result"].(map[string]any) + if !ok { + t.Fatalf("initialize returned no result: %v", env) + } + if got := result["protocolVersion"]; got != mcpspec.ProtocolVersion20251125 { + t.Errorf("initialize negotiated %v, want %s", got, mcpspec.ProtocolVersion20251125) + } + + // The independence, asserted. A transport advertising ONLY 2025-06-18 + // still answers initialize with 2025-11-25, because the handshake never + // consults the transport's list. If this ever starts failing, the two have + // been wired together and the restriction has become able to break legacy + // clients — which is the moment this file needs a different test. + t.Run("the handshake ignores the advertised list", func(t *testing.T) { + narrow := server.NewStreamableHTTPServer( + server.NewMCPServer("pad-test", "0.0.0"), + server.WithEndpointPath("/mcp"), + server.WithSessionIdManager(generateOnlySessionIDs{}), + server.WithStreamableHTTPProtocolVersions(mcpspec.ProtocolVersion20250618), + server.WithDisableLocalhostProtection(true), + ) + env := postJSONRPC(t, narrow, "", "", `{ + "jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": { + "protocolVersion": "2025-11-25", + "capabilities": {}, + "clientInfo": {"name": "test", "version": "0.0.0"} + } + }`) + result, ok := env["result"].(map[string]any) + if !ok { + t.Fatalf("initialize returned no result: %v", env) + } + if got := result["protocolVersion"]; got != mcpspec.ProtocolVersion20251125 { + t.Errorf("negotiated %v, want %s — the handshake has become coupled to the "+ + "transport's advertised list, so the restriction can now refuse "+ + "legacy clients", got, mcpspec.ProtocolVersion20251125) + } + }) +} + +// TestAdvertisedRevisionMatchesWhatTheTransportServes closes the loop the +// original defect ran through: pad://_meta/version publishes a maximum +// negotiable revision, and until now nothing tied that literal to the set the +// transport advertises. TestAdvertisedProtocolVersion pins it to what the +// HANDSHAKE answers, which cannot see the modern era at all — the era has no +// handshake. +func TestAdvertisedRevisionMatchesWhatTheTransportServes(t *testing.T) { + served := ServedProtocolVersions() + if len(served) == 0 { + t.Fatal("ServedProtocolVersions is empty; the transport would advertise every revision") + } + // Newest first, per ValidProtocolVersions' documented order. + if newest := served[0]; newest != AdvertisedMCPProtocolVersion { + t.Errorf("the transport's newest advertised revision is %s but "+ + "AdvertisedMCPProtocolVersion says %s — pad://_meta/version and "+ + "server/discover must not disagree about what this server can negotiate", + newest, AdvertisedMCPProtocolVersion) + } +}