mirror of
https://github.com/lukaszraczylo/traefik-llmgateway.git
synced 2026-10-12 04:54:45 +00:00
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.
This commit is contained in:
+21
-2
@@ -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 }()
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user