From de4219242646f13c047bbaa319e67f33dab865c0 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 15:38:35 +0100 Subject: [PATCH] refactor(mcp): restore errors.Is for the oversize-response sentinel Federation's tools/call oversize branch matched a string marker with strings.Contains instead of wrapping a sentinel. The stated reason was that no gate exercised this path under Yaegi, so errors.Is was unproven here. The caution was misplaced. errors.New returns a compiled *errors.errorString and fmt.Errorf a compiled *fmt.wrapError, so declaring the sentinel variable in interpreted code leaves the entire chain errors.Is walks compiled. Neither documented trap applies: the matchesSentinel trip-wire covers a plugin-declared error TYPE, and the comma-ok trap needs a plugin-declared interface. retry.go has matched errRequestBuildFailed this way through a multi-%w wrap since v0.2.0. The string check was also strictly worse: err.Error() is itself an interpreted-to-compiled dispatch, so it verified nothing extra, and matching on text that partly comes from the backend admits false positives a sentinel cannot have. Rather than settle this by argument, tools/yaegi-check now drives POST /mcp tools/call against a backend whose response exceeds the cap, and asserts the interpreted plugin reports "response too large". Breaking the match makes the gate fail with "upstream error", so the probe has teeth. This closes the objection permanently instead of answering it in a comment. Also in doBackendJSONRPC, check the HTTP status before the size cap. A backend failing with a large HTML error page now reports "upstream returned HTTP 500", which is actionable, rather than sending the operator after a payload-size problem that does not exist. Two comment corrections carried forward from review: the fan-out semaphore is not the same shape as limiter.spawnTokens (spawnTokens drops work via a default case, this one blocks and must never skip a server), and the shared fan-out deadline gives later batches the remaining budget, not a fresh one, so uniformly slow backends yield a partial list. --- mcp_federation.go | 80 +++++++++++++++++++--------- mcp_federation_test.go | 11 ++-- tools/yaegi-check/main.go | 106 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 28 deletions(-) diff --git a/mcp_federation.go b/mcp_federation.go index 25e3a6d..37dae6c 100644 --- a/mcp_federation.go +++ b/mcp_federation.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -98,19 +99,32 @@ const mcpBackendResponseMaxBytes = 4 << 20 // tool whose legitimate results sit between 4MiB and 10MiB. const mcpBackendCallResponseMaxBytes = maxRequestBytes -// mcpResponseTooLargeMarker is a fixed, unique substring doBackendJSONRPC -// embeds in its own error message when a backend response hits its -// call's maxBytes cap (security review round 2, 2026-08-22, important -// finding 4) — detected via strings.Contains, deliberately NOT an -// errors.Is-checked sentinel. matchesSentinel's own doc comment -// (limits.go) proves errors.Is safe under Yaegi only for chains reaching -// a call site this package's own yaegi-check harness actually exercises -// at runtime under the real interpreter — federation's tools/call path -// is not one of those exercised paths today, so a plain string check -// trades a little idiom for zero interpreter risk here, rather than -// relying on reasoning about compiled-vs-interpreted-origin values that -// nothing in the gate actually verifies for this specific path. -const mcpResponseTooLargeMarker = "mcp backend response too large" +// errMCPResponseTooLarge is the sentinel doBackendJSONRPC wraps when a +// backend response hits its call's maxBytes cap (security review round 2, +// 2026-08-22, important finding 4), letting the tools/call handler report +// "response too large" instead of a generic "upstream error". +// +// errors.Is is the correct match here, and it is Yaegi-safe. An earlier +// revision used a string marker plus strings.Contains out of caution; a +// harness on yaegi v0.16.1 (the version tools/yaegi-check pins, same +// stdlib.Symbols set) interpreting this shape found errors.Is correct +// through shallow, deep, and multi-%w wrapping, and correctly false for +// an unrelated error. The reason is that declaring the VARIABLE in +// interpreted code does not make its VALUE interpreted: errors.New +// returns a compiled *errors.errorString and fmt.Errorf a compiled +// *fmt.wrapError, so the whole chain errors.Is walks is compiled. This +// is the same shape retry.go:isTransient has run in production since +// v0.2.0, and errRequestBuildFailed (providers.go) is already matched +// this way through a multi-%w wrap. +// +// Neither documented Yaegi trap applies: matchesSentinel's TRIP-WIRE +// (limits.go) is about a plugin-declared error TYPE, which errors.New +// does not create, and the comma-ok trap needs a plugin-declared +// interface. A string check was also strictly worse than it looked — +// err.Error() is itself an interpreted-to-compiled method dispatch, so +// it verified nothing extra, and matching on text that partly comes +// from the backend admits false positives a sentinel cannot have. +var errMCPResponseTooLarge = errors.New("mcp backend response too large") // jsonrpcRequest is one JSON-RPC 2.0 request/notification, decoded from // the client's POST body and re-encoded (with a synthetic id) for this @@ -444,8 +458,8 @@ type mcpToolsListResult struct { // tools/list fan-out, mcpBackendCallResponseMaxBytes for tools/call — // their own doc comments cover why the two differ; security review round // 2, 2026-08-22, important finding 4). A response body that hits maxBytes -// is reported with mcpResponseTooLargeMarker in its message, not silently truncated into -// whatever partial (and likely invalid) JSON happened to fit. +// is reported by wrapping errMCPResponseTooLarge, not silently truncated +// into whatever partial (and likely invalid) JSON happened to fit. // // It is the one place that actually builds and sends an HTTP request for // this file; every @@ -518,12 +532,18 @@ func (g *Gateway) doBackendJSONRPC(ctx context.Context, targetURL, method string if err != nil { return nil, status, respSessionID, err } - if int64(len(respBody)) > maxBytes { - return nil, status, respSessionID, fmt.Errorf("%s: exceeds %d bytes", mcpResponseTooLargeMarker, maxBytes) - } + // Status is checked BEFORE the size cap: a backend that fails with a + // large HTML error page is diagnosed as "upstream returned HTTP 500", + // which is actionable, rather than as "too large", which sends the + // operator hunting a payload-size problem that does not exist. An + // oversized SUCCESS response still reports the cap, which is the case + // the cap exists for. if status < 200 || status >= 300 { return nil, status, respSessionID, fmt.Errorf("upstream returned HTTP %d", status) } + if int64(len(respBody)) > maxBytes { + return nil, status, respSessionID, fmt.Errorf("%w: exceeds %d bytes", errMCPResponseTooLarge, maxBytes) + } parsed, err := parseBackendJSONRPC(httpResp.Header.Get("Content-Type"), respBody) if err != nil { @@ -723,10 +743,12 @@ func lastSSEDataLine(body []byte) []byte { // mcpFederatedFanoutConcurrency bounds how many of names' backend servers // mcpFederatedToolsList's own fan-out below contacts at once — a -// buffered-channel semaphore, the same bounded-concurrency shape -// limiter.spawnTokens already uses for its own goroutine fan-out -// (limits.go), applied here to gate each goroutine's actual outbound HTTP -// work rather than its launch. Before this bound (security audit finding +// buffered-channel semaphore gating each goroutine's actual outbound HTTP +// work rather than its launch. It resembles limiter.spawnTokens +// (limits.go) only in using a buffered channel: spawnTokens is a +// non-blocking select with a `default:` that DROPS the work, whereas this +// one blocks until a slot frees and must never skip a server. Do not +// treat the two as interchangeable. Before this bound (security audit finding // 1a, 2026-08-22), one incoming tools/list request spawned one goroutine // per allowed server with NO cap at all: with the 11 production servers, // one client call could hold up to 11 concurrent connections and up to @@ -795,6 +817,18 @@ func (g *Gateway) mcpFederatedToolsList(w http.ResponseWriter, r *http.Request, // TestHandleMCPFederated_ToolsList_SlowBackend_BoundedByRequestContext), // so deriving fanoutCtx from r.Context() here preserves that same // property for the request's own deadline, on top of this one. + // + // The accepted trade: a later batch inherits the REMAINING budget, not + // a fresh one, so if every backend is slow enough to consume most of + // toolsListBackendTimeout on its own, servers in later batches are cut + // off and their tools are missing from the merged list. Measured: 11 + // servers at 11s each returns 8 of 11 tools after 20s, where a per- + // backend budget returned 11 of 11 after 11s. This is deliberate — + // the alternative is a worst case of ceil(11/8) x 20s = 60s for a + // single tools/list — and it degrades to a partial list plus a logged + // failure line per starved server, never a hung request. Real + // tools/list calls answer in well under a second (at 300ms/server all + // 11 return), so this path needs a pathological backend to trigger. fanoutCtx, fanoutCancel := context.WithTimeout(r.Context(), toolsListBackendTimeout) defer fanoutCancel() @@ -951,7 +985,7 @@ func (g *Gateway) mcpFederatedToolsCall(w http.ResponseWriter, r *http.Request, // specific message, not the generic "upstream error" a truncated- // then-unparsable body would otherwise produce (security review // round 2, 2026-08-22, important finding 4). - if strings.Contains(err.Error(), mcpResponseTooLargeMarker) { + if errors.Is(err, errMCPResponseTooLarge) { writeJSONRPCErrorResponse(w, req.ID, jsonrpcInternalError, "response too large") } else { writeJSONRPCErrorResponse(w, req.ID, jsonrpcInternalError, "upstream error") diff --git a/mcp_federation_test.go b/mcp_federation_test.go index 5bf0582..ed0e5a4 100644 --- a/mcp_federation_test.go +++ b/mcp_federation_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -1072,9 +1073,9 @@ func TestMcpBackendCall_NetworkFailure_NeverAttemptsHandshake(t *testing.T) { // fast, small-scale unit test of doBackendJSONRPC's own maxBytes // mechanism, using an artificially tiny cap so the test needs no // multi-megabyte payload: a backend response over the cap must be -// reported with mcpResponseTooLargeMarker in its message — -// distinguishable via strings.Contains from an ordinary network/parse -// failure — never silently truncated into a confusing parse error. +// reported by wrapping errMCPResponseTooLarge — distinguishable via +// errors.Is from an ordinary network/parse failure — never silently +// truncated into a confusing parse error. func TestMcpBackendCall_ResponseExceedsCap_ReturnsDistinctTooLargeError(t *testing.T) { const tinyCap = 64 big := strings.Repeat("x", tinyCap*4) // well past tinyCap once JSON-encoded @@ -1101,8 +1102,8 @@ func TestMcpBackendCall_ResponseExceedsCap_ReturnsDistinctTooLargeError(t *testi if err == nil { t.Fatal("want an error for a response exceeding the cap") } - if !strings.Contains(err.Error(), mcpResponseTooLargeMarker) { - t.Errorf("err = %v, want it to contain %q", err, mcpResponseTooLargeMarker) + if !errors.Is(err, errMCPResponseTooLarge) { + t.Errorf("err = %v, want it to wrap %v", err, errMCPResponseTooLarge) } } diff --git a/tools/yaegi-check/main.go b/tools/yaegi-check/main.go index ede1ebb..1261538 100644 --- a/tools/yaegi-check/main.go +++ b/tools/yaegi-check/main.go @@ -24,6 +24,7 @@ package main import ( "bufio" + "bytes" "context" "encoding/json" "fmt" @@ -226,6 +227,30 @@ func run() error { })) defer slowUpstream.Close() + // mcpProbeUpstream answers federation's outbound tools/call with a + // body deliberately larger than mcpBackendCallResponseMaxBytes, so + // exerciseHandler's POST /mcp probe below drives doBackendJSONRPC's + // oversize branch — and therefore its errors.Is(err, + // errMCPResponseTooLarge) match — under the REAL interpreter. + // + // This exists because a security review round objected that no gate + // exercised federation's tools/call path interpreted, which left + // errors.Is there justified by reasoning rather than by evidence. + // Reasoning is exactly what the Yaegi traps in this codebase have + // repeatedly defeated, so the path is now covered instead of argued. + mcpProbeUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"`)) + chunk := bytes.Repeat([]byte("x"), 1<<20) + for written := 0; written <= mcpProbeOversizeBytes; written += len(chunk) { + if _, writeErr := w.Write(chunk); writeErr != nil { + return + } + } + _, _ = w.Write([]byte(`"}]}}`)) + })) + defer mcpProbeUpstream.Close() + // builtinLookupContextTokens (review fix, SHOULD-6): read directly // out of the generated pricing_data_gen.go rather than hardcoding a // number, so this assertion self-updates across a `make @@ -252,6 +277,7 @@ func run() error { // runs correctly under Yaegi too, not just the config-override // one testDataWantModel already exercises. `"modelMeta":{"` + testDataWantModel + `":{"contextTokens":` + yaegiMetaContextTokens + `,"inputCostPerMTokMicroUsd":1250000,"outputCostPerMTokMicroUsd":10000000}},` + + `"mcpServers":{"` + mcpProbeServerName + `":{"url":"` + mcpProbeUpstream.URL + `"}},` + `"admin":{"enabled":true},"users":{"inline":[{"name":"tester","group":"default","apiKey":"` + testDataUserAPIKey + `"},{"name":"admin1","group":"default","apiKey":"` + attemptAccountingAdminAPIKey + `","admin":true}]}}` if err = json.Unmarshal([]byte(attemptAccountingOverride), cfgVal.Interface()); err != nil { return fmt.Errorf("decode attempt-accounting harness override into the interpreted Config: %w", err) @@ -298,6 +324,32 @@ func run() error { // the config-override path testDataWantModel already exercises. const builtinLookupModelID = "gpt-4o" +// mcpProbeServerName is the federated MCP server run() configures against +// mcpProbeUpstream, and mcpProbeToolName is a tool id carrying its +// "_" prefix so resolveFederatedTool routes POST /mcp's +// tools/call there. The tool needs no tools/list entry — resolution is +// pure prefix matching on the configured, group-allowed server names. +const ( + mcpProbeServerName = "probe" + mcpProbeToolName = mcpProbeServerName + "_big" +) + +// mcpProbeOversizeBytes is how much filler mcpProbeUpstream writes: over +// mcpBackendCallResponseMaxBytes (maxRequestBytes, 10MiB) so the response +// trips doBackendJSONRPC's cap. Kept as its own named value rather than +// importing the plugin's const, because this harness deliberately builds +// nothing from the plugin package at compile time — everything it asserts +// about the plugin must come from the interpreter. +const mcpProbeOversizeBytes = 11 << 20 + +// mcpTooLargeWantMessage is the exact JSON-RPC error message +// mcpFederatedToolsCall writes when errors.Is matches +// errMCPResponseTooLarge. "upstream error" here instead means errors.Is +// returned false under the interpreter for a chain that is true when +// compiled — which is precisely the class of Yaegi divergence this +// harness exists to catch, and which no compiled test can see. +const mcpTooLargeWantMessage = "response too large" + // readBuiltinContextTokens reads repoRoot/pricing_data_gen.go and // extracts modelID's own ContextTokens value directly out of the // generated source (review fix, SHOULD-6) — rather than hardcoding a @@ -401,6 +453,10 @@ func exerciseHandler(handler http.Handler, builtinContextTokens int) error { return err } + if err := exerciseFederatedTooLarge(handler); err != nil { + return err + } + unauthedReq := httptest.NewRequest(http.MethodGet, "/v1/models", nil) unauthedRec := httptest.NewRecorder() handler.ServeHTTP(unauthedRec, unauthedReq) @@ -439,6 +495,56 @@ func exerciseHandler(handler http.Handler, builtinContextTokens int) error { // a compiled concrete value's method set in Yaegi. matchesSentinel now // uses real errors.Is instead (its own doc comment in limits.go has the // full account); this harness is what caught the difference. +// exerciseFederatedTooLarge drives POST /mcp (federation's tools/call +// path) against mcpProbeUpstream, whose response deliberately exceeds +// mcpBackendCallResponseMaxBytes, and asserts the interpreted plugin +// answers with mcpTooLargeWantMessage rather than the generic "upstream +// error". +// +// What this actually pins is errors.Is under the interpreter. A security +// review round replaced that errors.Is with a strings.Contains marker on +// the grounds that no gate exercised this path interpreted — true at the +// time, and the honest objection. The reasoning for errors.Is is sound +// (errors.New and fmt.Errorf both return COMPILED values, so declaring +// the sentinel variable in interpreted code leaves the whole chain +// errors.Is walks compiled, which is why retry.go's isTransient has +// worked in production since v0.2.0), and a standalone yaegi v0.16.1 +// harness confirmed it. But this codebase's own history — matchesSentinel +// (limits.go) — is a case where sound reasoning about the reflect +// boundary was wrong and only an interpreted probe caught it. So the +// path is covered here rather than argued in a comment. +// +// A failure surfaces as "upstream error": errors.Is returning false +// interpreted for a chain that is true compiled. No `go test` row can +// see that difference. +func exerciseFederatedTooLarge(handler http.Handler) error { + body := `{"jsonrpc":"2.0","id":7,"method":"tools/call","params":{"name":"` + mcpProbeToolName + `","arguments":{}}}` + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + req.Header.Set("Authorization", "Bearer "+testDataUserAPIKey) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + return fmt.Errorf("POST /mcp tools/call: status = %d, want 200 (JSON-RPC reports the failure in the body, not the status), body=%s", rec.Code, rec.Body.String()) + } + + var resp struct { + Error *struct { + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + return fmt.Errorf("decode POST /mcp tools/call body: %w (body=%s)", err, rec.Body.String()) + } + if resp.Error == nil { + return fmt.Errorf("POST /mcp tools/call returned no JSON-RPC error, want %q — the probe upstream's oversized response should have tripped the cap: %s", mcpTooLargeWantMessage, rec.Body.String()) + } + if resp.Error.Message != mcpTooLargeWantMessage { + return fmt.Errorf("POST /mcp tools/call error message = %q, want %q — errors.Is(err, errMCPResponseTooLarge) did not match under the interpreter (mcp_federation.go)", resp.Error.Message, mcpTooLargeWantMessage) + } + return nil +} + func exerciseAttemptAccounting(handler http.Handler) error { chatReq := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader( `{"model":"`+testDataWantModel+`","messages":[{"role":"user","content":"hi"}]}`,