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"}]}`,