From 8bc54d15b583549e9778fac4cb46a5db6d739dcc Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 13:55:26 +0100 Subject: [PATCH] fix(mcp): recover panics in tools/list fan-out goroutines Fixes finding 2 from the 2026-08-22 security audit: the per-server goroutines in mcpFederatedToolsList carried only defer wg.Done(), unlike every other off-request goroutine in this package (limiter.spawn, modelRegistry.refreshProvider/captureModelMetadata), which all guard against a panic crashing the whole shared Traefik process. This codebase has repeatedly hit interpreter-level panics (registry.go reflect.Set, cache.go CFG index-out-of-range, providers.go errors.As), so an unrecovered panic here is not hypothetical. Add the same defer-recover pattern used at the other three sites: a panicking backend now degrades to "that server failed" and is logged, exactly like an HTTP error from it, instead of taking down the process. --- mcp_federation.go | 23 +++++++++++- mcp_federation_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/mcp_federation.go b/mcp_federation.go index d4ae414..45e7782 100644 --- a/mcp_federation.go +++ b/mcp_federation.go @@ -703,7 +703,7 @@ const mcpFederatedFanoutConcurrency = 4 // replaces used, and the one pugbot's mcpclient and agentkit have already // persisted into their own tool-id databases (e.g. // "brave-search_brave_web_search" — verified live). A server that errors, -// times out, or returns an unparsable/invalid result is logged +// times out, panics, or returns an unparsable/invalid result is logged // and skipped, not surfaced as a whole-call failure — UNLESS every single // attempted server failed, in which case an empty tools list would // misleadingly look like "this caller's group has no MCP access" — see @@ -712,7 +712,7 @@ const mcpFederatedFanoutConcurrency = 4 // countTargetRequests (limits.go) attributes one request to EVERY server // ATTEMPTED here, in ONE batched call after wg.Wait() — "attempted", not // "reached" or "succeeded": a server this gateway dialed and got a -// response (or a timeout, or a connection refusal) from still had +// response (or a timeout, a connection refusal, or a panic) from still had // a real request sent to it and a real slot of this gateway's outbound // capacity spent on it, which is what these counters exist to track. A // single federated tools/list call can move several targets' own @@ -730,6 +730,25 @@ func (g *Gateway) mcpFederatedToolsList(w http.ResponseWriter, r *http.Request, wg.Add(1) go func(name string) { defer wg.Done() + // Unrecovered-panic guard (security audit finding 2, + // 2026-08-22): this goroutine runs off the request's own + // goroutine, so a panic here has no ServeHTTP caller to + // unwind into and would crash the whole shared Traefik + // process — the same reasoning limiter.spawn (limits.go) and + // modelRegistry.refreshProvider/captureModelMetadata + // (registry.go) already apply to their own off-request + // goroutines. A panicking backend must degrade to "that + // server failed", exactly like an HTTP error from it, never + // a process-wide crash. + defer func() { + if rec := recover(); rec != nil { + g.logf("federated tools/list: server %q panicked: %v", name, rec) + mu.Lock() + failed = append(failed, name) + mu.Unlock() + } + }() + sem <- struct{}{} defer func() { <-sem }() diff --git a/mcp_federation_test.go b/mcp_federation_test.go index 065dd32..dd19b17 100644 --- a/mcp_federation_test.go +++ b/mcp_federation_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "slices" + "strings" "sync" "sync/atomic" "testing" @@ -1308,6 +1309,88 @@ func TestHandleMCPFederated_ToolsList_FanoutBoundedConcurrency(t *testing.T) { } } +// --- security audit finding 2: unrecovered panic in the fan-out --- + +// panicOnHostRoundTripper panics for any request to panicHost — simulating +// an unrecovered interpreter-level panic reached deep inside one backend's +// own call path (mcpBackendCall -> doBackendJSONRPC -> g.targetClient.Do) +// — and delegates every other request to next unchanged. This exercises +// the REAL code path a genuine Yaegi interpreter panic would take, +// through the actual HTTP client the gateway uses, rather than adding any +// test-only hook to production code. +type panicOnHostRoundTripper struct { + next http.RoundTripper + panicHost string +} + +func (rt panicOnHostRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + if r.URL.Host == rt.panicHost { + panic("simulated interpreter-level panic mid-flight") + } + return rt.next.RoundTrip(r) +} + +// TestHandleMCPFederated_ToolsList_PanickingBackendDegradesNotCrashes is +// the regression for the missing recover() in mcpFederatedToolsList's own +// fan-out goroutine (security audit finding 2, 2026-08-22): that goroutine +// runs off the request's own goroutine, so an unrecovered panic there has +// no ServeHTTP caller to unwind into and would crash the whole shared +// Traefik process, not just fail one backend. Reaching the end of this +// test at all is part of the proof: if the recover() were missing, beta's +// panicking RoundTrip call below would already have crashed the test +// binary — exactly the registry.go precedent this fix follows +// (TestModelRegistry_RefreshProvider_AdapterPanics_RecoversAndReleasesInFlight). +func TestHandleMCPFederated_ToolsList_PanickingBackendDegradesNotCrashes(t *testing.T) { + alpha := newMockJSONRPCServer(t, []mcpTool{{Name: "lookup"}}) + beta := newMockJSONRPCServer(t, []mcpTool{{Name: "search"}}) // never actually reached; RoundTrip panics first + cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + gw, ok := h.(*Gateway) + if !ok { + t.Fatal("handler is not *Gateway") + } + betaURL, err := betaHost(beta.srv.URL) + if err != nil { + t.Fatalf("parse beta URL: %v", err) + } + gw.targetClient.Transport = panicOnHostRoundTripper{panicHost: betaURL, next: http.DefaultTransport} + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + var got jsonrpcResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if got.Error != nil { + t.Fatalf("error = %+v, want nil (a panicking server degrades the aggregate, it does not fail the call)", got.Error) + } + var result mcpToolsListResult + if err := json.Unmarshal(got.Result, &result); err != nil { + t.Fatalf("decode result: %v", err) + } + if len(result.Tools) != 1 || result.Tools[0].Name != "alpha_lookup" { + t.Errorf("tools = %+v, want exactly alpha_lookup (beta panicked, skipped)", result.Tools) + } +} + +// betaHost extracts the host:port a RoundTripper sees on r.URL.Host for +// requests aimed at rawURL — httptest.Server URLs are already in that +// exact "http://host:port" form, so this just strips the scheme. +func betaHost(rawURL string) (string, error) { + const prefix = "http://" + if !strings.HasPrefix(rawURL, prefix) { + return "", fmt.Errorf("unexpected test server URL shape: %q", rawURL) + } + return strings.TrimPrefix(rawURL, prefix), nil +} + // --- security audit finding 1b: tools/list weighted by backend count --- // TestHandleMCPFederated_ToolsList_WeightedByBackendCount_RejectsWhenOverBudget