From 24049c75d708c1682c79b56dc355453d392d15c3 Mon Sep 17 00:00:00 2001 From: King Star Date: Sun, 6 Sep 2026 22:33:30 +0800 Subject: [PATCH 1/3] test(vmcp): pin that the health path never opens a standalone GET stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #6497 reported a healthy Tableau MCP backend being marked unavailable because the vMCP health probe used a bare HTTP GET, which that backend rejects with 400 for lack of a session id. Current main no longer probes with GET: the health check is BackendClient.ListCapabilities, and newStreamableHTTPClient enables transport.WithContinuousListening — the standalone server->client SSE stream, the only GET vMCP makes — solely on the forwarding tools/call path. Nothing pinned that, so add the regression the issue describes: a real go-sdk stateful streamable-HTTP backend behind a handler that answers every GET with Tableau's 400, asserting that ListCapabilities both succeeds and issues no GET at all. Verified to have teeth: appending WithContinuousListening unconditionally in newStreamableHTTPClient makes the test fail on the GET assertion. --- pkg/vmcp/client/getreject_realbackend_test.go | 110 ++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 pkg/vmcp/client/getreject_realbackend_test.go diff --git a/pkg/vmcp/client/getreject_realbackend_test.go b/pkg/vmcp/client/getreject_realbackend_test.go new file mode 100644 index 0000000000..4be2ddeb3d --- /dev/null +++ b/pkg/vmcp/client/getreject_realbackend_test.go @@ -0,0 +1,110 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client + +import ( + "context" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + mcpmcp "github.com/stacklok/toolhive-core/mcpcompat/mcp" + mcpserver "github.com/stacklok/toolhive-core/mcpcompat/server" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// newGETRejectingEchoServer stands up the same real go-sdk streamable-HTTP +// backend as newRealEchoServer, stateful (Legacy), behind a handler that +// answers every HTTP GET with 400 and records the method of every request it +// receives. +// +// That is the Tableau MCP shape from issue #6497: a healthy backend that +// requires a session id issued by initialize, so a bare GET — which carries no +// session context — is rejected rather than upgraded to a standalone SSE +// stream. +func newGETRejectingEchoServer(t *testing.T, record func(method string)) *httptest.Server { + t.Helper() + + mcpSrv := mcpserver.NewMCPServer("get-rejecting-backend", "1.0.0") + mcpSrv.AddTool( + mcpmcp.NewTool("echo", + mcpmcp.WithDescription("Echoes the input back"), + mcpmcp.WithString("input", mcpmcp.Required()), + ), + func(_ context.Context, req mcpmcp.CallToolRequest) (*mcpmcp.CallToolResult, error) { + args, _ := req.Params.Arguments.(map[string]any) + input, _ := args["input"].(string) + return &mcpmcp.CallToolResult{Content: []mcpmcp.Content{mcpmcp.NewTextContent(input)}}, nil + }, + ) + + inner := mcpserver.NewStreamableHTTPServer(mcpSrv) + mux := http.NewServeMux() + mux.Handle("/mcp", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + record(r.Method) + if r.Method == http.MethodGet { + // Tableau MCP's response to a session-less GET. + http.Error(w, "Invalid or missing session ID", http.StatusBadRequest) + return + } + inner.ServeHTTP(w, r) + })) + ts := httptest.NewServer(mux) + t.Cleanup(ts.Close) + return ts +} + +// TestListCapabilities_GETRejectingBackendIsHealthy pins the health signal +// reported in issue #6497: a backend that rejects a bare HTTP GET must not be +// treated as unavailable. +// +// vMCP's health check is BackendClient.ListCapabilities (see +// health.NewHealthChecker), which reaches the backend over POST. GET is only +// ever the standalone server->client SSE stream, and in Streamable HTTP that +// stream is optional — a server without one answers 405 per spec, and one that +// requires a session id (Tableau MCP) answers 400. Neither says anything about +// whether the backend can serve MCP. +// +// The assertion is therefore two-sided: ListCapabilities must succeed against +// such a backend, AND it must not issue a GET at all. Only the tools/call +// forwarding path enables transport.WithContinuousListening (see +// newStreamableHTTPClient); a change that opened the standalone stream on the +// non-forwarding path would put every session-requiring backend back to +// "unavailable" and drop its tools from tools/list. +func TestListCapabilities_GETRejectingBackendIsHealthy(t *testing.T) { + t.Parallel() + + var mu sync.Mutex + var methods []string + srv := newGETRejectingEchoServer(t, func(method string) { + mu.Lock() + defer mu.Unlock() + methods = append(methods, method) + }) + + h := newProbeClient(t) + target := &vmcp.BackendTarget{ + WorkloadID: "get-rejecting-backend", + WorkloadName: "GET Rejecting Backend", + BaseURL: srv.URL + "/mcp", + TransportType: "streamable-http", + } + + caps, err := h.ListCapabilities(context.Background(), target) + require.NoError(t, err, + "a backend that rejects a bare GET is still a healthy MCP backend: the health check speaks MCP over POST") + require.NotNil(t, caps) + assert.Len(t, caps.Tools, 1) + assert.Equal(t, "echo", caps.Tools[0].Name) + + mu.Lock() + defer mu.Unlock() + require.NotEmpty(t, methods, "the backend must have been reached") + assert.NotContains(t, methods, http.MethodGet, + "the health path must not open a standalone SSE GET stream; only the tools/call forwarding path may") +} From f2f74710f136b5e15b5a68350dec5edbd5a0c840 Mon Sep 17 00:00:00 2001 From: King Star Date: Sun, 6 Sep 2026 22:39:38 +0800 Subject: [PATCH 2/3] fix(authz): accept JSON-RPC responses to server-initiated requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A client POSTing its answer to a server-initiated request — a ping result today, elicitation and sampling results as those land — got 400 "Invalid or malformed MCP request" whenever authorization was enabled. Clients treat that as a fatal transport error: VS Code tears the session down and retries with a new session id, so the connection flaps once per ping interval. parseMCPRequest handles only JSON-RPC requests and returns nil for a response, which the authz middleware could not tell apart from an unparseable body, so it took the malformed-body branch. Record in the parsing middleware that a body decoded as a JSON-RPC response or error, and let the authz middleware pass those through. A response names no method and reaches no tool, so there is nothing to authorize; the streamable-HTTP transport already answers 202 for it, as the spec requires. The content-type refusal ahead of this is untouched: a JSON-RPC body smuggled under text/plain is still rejected before parsing, and a genuinely malformed JSON body still gets the 400. Fixes #5009 --- pkg/authz/clientresponse_test.go | 56 ++++++++++++++++++++++++++++++++ pkg/authz/middleware.go | 9 +++++ pkg/mcp/parser.go | 35 ++++++++++++++++++++ 3 files changed, 100 insertions(+) create mode 100644 pkg/authz/clientresponse_test.go diff --git a/pkg/authz/clientresponse_test.go b/pkg/authz/clientresponse_test.go new file mode 100644 index 0000000000..3e631252b1 --- /dev/null +++ b/pkg/authz/clientresponse_test.go @@ -0,0 +1,56 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package authz + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/authz/authorizers/cedar" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" +) + +func TestMiddlewareAcceptsClientResponse(t *testing.T) { + t.Parallel() + + authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{ + Policies: []string{`permit(principal, action, resource);`}, + EntitiesJSON: `[]`, + }, "") + require.NoError(t, err) + + for _, tt := range []struct { + name string + body string + }{ + {"ping result", `{"jsonrpc":"2.0","id":1,"result":{}}`}, + {"error response", `{"jsonrpc":"2.0","id":2,"error":{"code":-32601,"message":"Method not found"}}`}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var handlerCalled bool + handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + handlerCalled = true + w.WriteHeader(http.StatusAccepted) + }) + middleware := mcpparser.ParsingMiddleware(Middleware(authorizer, handler, nil)) + + req, err := http.NewRequest(http.MethodPost, "/mcp", strings.NewReader(tt.body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + + rr := httptest.NewRecorder() + middleware.ServeHTTP(rr, req) + + assert.True(t, handlerCalled, "a client response must reach the transport") + assert.Equal(t, http.StatusAccepted, rr.Code) + }) + } +} diff --git a/pkg/authz/middleware.go b/pkg/authz/middleware.go index 50a37a5578..9ada6222e3 100644 --- a/pkg/authz/middleware.go +++ b/pkg/authz/middleware.go @@ -222,6 +222,15 @@ func Middleware(a authorizers.Authorizer, next http.Handler, passThroughTools ma // Get parsed MCP request from context (set by parsing middleware) parsedRequest := mcp.GetParsedMCPRequest(r.Context()) if parsedRequest == nil { + // A JSON-RPC response or error answers a request the SERVER + // initiated (ping, elicitation, sampling). It names no method and + // reaches no tool, so there is nothing to authorize, and the + // streamable-HTTP spec requires the transport to accept it with + // 202. Rejecting it tears the client's session down (#5009). + if mcp.IsClientResponse(r.Context()) { + next.ServeHTTP(w, r) + return + } // Non-JSON POSTs are already rejected by the early return above, // so a nil parsed request here means a malformed JSON body or a // missing parsing middleware. This branch is now only a diff --git a/pkg/mcp/parser.go b/pkg/mcp/parser.go index 2dd3d4f378..0b653c893e 100644 --- a/pkg/mcp/parser.go +++ b/pkg/mcp/parser.go @@ -26,6 +26,11 @@ type contextKey string const ( // MCPRequestContextKey is the context key for storing parsed MCP request data. MCPRequestContextKey contextKey = "mcp_request" + + // ClientResponseContextKey marks a POST body that decoded as a JSON-RPC + // response or error rather than a request, so downstream middleware can + // tell one apart from a body that failed to parse at all. + ClientResponseContextKey contextKey = "mcp_client_response" ) // ParsedMCPRequest contains the parsed MCP request information. @@ -125,6 +130,12 @@ func ParsingMiddleware(next http.Handler) http.Handler { // Parse the MCP request and store in context parsedRequest := parseMCPRequest(bodyBytes) + if parsedRequest == nil && isClientResponseBody(bodyBytes) { + // Not a request, but well-formed: it answers a request the server + // initiated. Record that so downstream middleware does not confuse + // it with an unparseable body. + r = r.WithContext(context.WithValue(r.Context(), ClientResponseContextKey, true)) + } if parsedRequest != nil { parsedRequest.MCPMethodHeader = r.Header.Get("Mcp-Method") parsedRequest.MCPNameHeader = r.Header.Get("Mcp-Name") @@ -258,6 +269,30 @@ func RequestHasJSONContentType(r *http.Request) bool { return strings.EqualFold(mediaType, "application/json") } +// isClientResponseBody reports whether bodyBytes is a well-formed JSON-RPC +// response or error, i.e. a client's answer to a server-initiated request +// (ping, elicitation, sampling) rather than a call the client is making. +func isClientResponseBody(bodyBytes []byte) bool { + if len(bodyBytes) == 0 { + return false + } + msg, err := jsonrpc2.DecodeMessage(bodyBytes) + if err != nil { + return false + } + _, ok := msg.(*jsonrpc2.Response) + return ok +} + +// IsClientResponse reports whether the request body decoded as a JSON-RPC +// response or error rather than a request. A nil result from +// [GetParsedMCPRequest] means either this or an unparseable body; callers that +// must distinguish the two use this. +func IsClientResponse(ctx context.Context) bool { + v, _ := ctx.Value(ClientResponseContextKey).(bool) + return v +} + // GetParsedMCPRequest retrieves the parsed MCP request from the request context. // Returns nil if no parsed request is available. func GetParsedMCPRequest(ctx context.Context) *ParsedMCPRequest { From 327f0d37269647f8317900f1cf87c071c1e9b345 Mon Sep 17 00:00:00 2001 From: King Star Date: Sun, 6 Sep 2026 22:42:08 +0800 Subject: [PATCH 3/3] Fix spelling flagged by codespell --- pkg/mcp/parser.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/mcp/parser.go b/pkg/mcp/parser.go index 0b653c893e..1080359ac3 100644 --- a/pkg/mcp/parser.go +++ b/pkg/mcp/parser.go @@ -133,7 +133,7 @@ func ParsingMiddleware(next http.Handler) http.Handler { if parsedRequest == nil && isClientResponseBody(bodyBytes) { // Not a request, but well-formed: it answers a request the server // initiated. Record that so downstream middleware does not confuse - // it with an unparseable body. + // it with an unparsable body. r = r.WithContext(context.WithValue(r.Context(), ClientResponseContextKey, true)) } if parsedRequest != nil { @@ -286,7 +286,7 @@ func isClientResponseBody(bodyBytes []byte) bool { // IsClientResponse reports whether the request body decoded as a JSON-RPC // response or error rather than a request. A nil result from -// [GetParsedMCPRequest] means either this or an unparseable body; callers that +// [GetParsedMCPRequest] means either this or an unparsable body; callers that // must distinguish the two use this. func IsClientResponse(ctx context.Context) bool { v, _ := ctx.Value(ClientResponseContextKey).(bool)