diff --git a/pkg/transport/bridge_test.go b/pkg/transport/bridge_test.go index 0d0b2a8e39..c76d0f1292 100644 --- a/pkg/transport/bridge_test.go +++ b/pkg/transport/bridge_test.go @@ -45,6 +45,46 @@ func toolNamesOnServer(t *testing.T, srv *server.MCPServer) []string { return names } +// resourceNamesOnServer returns the URIs of the resources currently registered +// on the given local MCP server, by issuing a synthetic resources/list request +// through the shim's HandleMessage dispatch path. Mirrors toolNamesOnServer; +// call only after the bridge has fully stopped (run() returned). +func resourceNamesOnServer(t *testing.T, srv *server.MCPServer) []string { + t.Helper() + resp := srv.HandleMessage(context.Background(), []byte(`{"jsonrpc":"2.0","method":"resources/list","id":1}`)) + jr, ok := resp.(mcp.JSONRPCResponse) + require.True(t, ok, "resources/list should return a JSONRPCResponse, got %T", resp) + buf, err := json.Marshal(jr.Result) + require.NoError(t, err) + var lr mcp.ListResourcesResult + require.NoError(t, json.Unmarshal(buf, &lr)) + uris := make([]string, 0, len(lr.Resources)) + for _, res := range lr.Resources { + uris = append(uris, res.URI) + } + return uris +} + +// promptNamesOnServer returns the names of the prompts currently registered on +// the given local MCP server, by issuing a synthetic prompts/list request +// through the shim's HandleMessage dispatch path. Mirrors toolNamesOnServer; +// call only after the bridge has fully stopped (run() returned). +func promptNamesOnServer(t *testing.T, srv *server.MCPServer) []string { + t.Helper() + resp := srv.HandleMessage(context.Background(), []byte(`{"jsonrpc":"2.0","method":"prompts/list","id":1}`)) + jr, ok := resp.(mcp.JSONRPCResponse) + require.True(t, ok, "prompts/list should return a JSONRPCResponse, got %T", resp) + buf, err := json.Marshal(jr.Result) + require.NoError(t, err) + var lp mcp.ListPromptsResult + require.NoError(t, json.Unmarshal(buf, &lp)) + names := make([]string, 0, len(lp.Prompts)) + for _, p := range lp.Prompts { + names = append(names, p.Name) + } + return names +} + func containsTool(names []string, want string) bool { for _, n := range names { if n == want { @@ -208,6 +248,226 @@ func TestBridge_ToolsListChanged_TriggersReSync(t *testing.T) { "beta must be present after the re-sync, got %v", names) } +// noopResourceHandler is a stand-in resource handler; the bridge re-fetch +// tests never read resources, they only assert the advertised set. +func noopResourceHandler(_ context.Context, _ mcp.ReadResourceRequest) ([]mcp.ResourceContents, error) { + return nil, nil +} + +// noopPromptHandler is a stand-in prompt handler; the bridge re-fetch tests +// never fetch prompts, they only assert the advertised set. +func noopPromptHandler(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return nil, nil +} + +// TestBridge_ResourcesListChanged_TriggersReSync verifies the bridge's +// notifications/resources/list_changed re-fetch fix: when the upstream +// backend's resource set changes after the bridge has connected, the upstream +// emits a resources/list_changed notification, the bridge's OnNotification +// handler re-runs forwardAll, and the newly added resource appears on the +// bridge's local stdio server. +// +// Mirrors TestBridge_ToolsListChanged_TriggersReSync: the mutation is driven +// through the per-session overlay (SessionWithResources.SetSessionResources). +// forwardAll re-fetches tools, resources, resource templates, and prompts on +// ANY *_list_changed notification, so the existing tools/list counter +// (listToolsCounter, wired via hooks.AddBeforeListTools) remains the +// race-free readiness/re-fetch signal even though this test mutates +// resources, not tools. +// +//nolint:paralleltest // Swaps process-global os.Stdin; cannot run in parallel. +func TestBridge_ResourcesListChanged_TriggersReSync(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // --- upstream backend with an initial resource "res://alpha" --- + counter := &listToolsCounter{} + holder := &sessionHolder{} + hooks := &server.Hooks{} + hooks.AddOnRegisterSession(func(_ context.Context, s server.ClientSession) { holder.set(s) }) + hooks.AddBeforeListTools(counter.hook()) + + backend := server.NewMCPServer( + "backend", "1.0", + // WithToolCapabilities is declared (though no tool is registered) so the + // backend unambiguously serves tools/list and fires the BeforeListTools + // hook — the counter is the readiness signal for the resource-triggered + // re-sync, since forwardAll re-fetches tools on ANY *_list_changed. + server.WithToolCapabilities(true), + server.WithResourceCapabilities(true, true), + server.WithHooks(hooks), + ) + backend.AddResource(mcp.Resource{URI: "res://alpha", Name: "alpha"}, noopResourceHandler) + + httpSrv := server.NewStreamableHTTPServer(backend) + ts := httptest.NewServer(httpSrv) + t.Cleanup(ts.Close) + + // --- bridge pointing at the backend (streamable-http) --- + bridge, err := NewStdioBridge("test", ts.URL, types.TransportTypeStreamableHTTP) + require.NoError(t, err) + + // Swap os.Stdin ONLY for a pipe so ServeStdio does not touch the real + // process stdin and can be unblocked at teardown by closing the write end. + origIn := os.Stdin + pipeR, pipeW, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { + os.Stdin = origIn + _ = pipeR.Close() + _ = pipeW.Close() + }) + os.Stdin = pipeR + + bridge.Start(ctx) + t.Cleanup(func() { + _ = pipeW.Close() + bridge.Shutdown() + }) + + // Step 1: wait for the bridge to connect and run its initial forwardAll + // (counter >= 1 from the tools/list call forwardAll always issues). + require.Eventually(t, func() bool { + return holder.get() != nil && counter.count.Load() >= 1 + }, 5*time.Second, 50*time.Millisecond, + "bridge did not complete initial forwardAll (session=%v, listCalls=%d)", + holder.get() != nil, counter.count.Load()) + + // Step 2: mutate the upstream resource set — add "res://beta" via the + // per-session overlay. SetSessionResources syncs onto the live go-sdk + // server bound to this session, which emits + // notifications/resources/list_changed to the bridge's upstream client. + backendSession := holder.get() + swr, ok := backendSession.(server.SessionWithResources) + require.True(t, ok, "backend session must implement SessionWithResources") + swr.SetSessionResources(map[string]server.ServerResource{ + "res://alpha": {Resource: mcp.Resource{URI: "res://alpha", Name: "alpha"}, Handler: noopResourceHandler}, + "res://beta": {Resource: mcp.Resource{URI: "res://beta", Name: "beta"}, Handler: noopResourceHandler}, + }) + + // Step 3: wait for the re-fetch. forwardAll re-fetches tools on every + // *_list_changed notification (not just tools/list_changed), so the + // second tools/list call is the race-free signal that the resync ran. + require.Eventually(t, func() bool { + return counter.count.Load() >= 2 + }, 5*time.Second, 50*time.Millisecond, + "bridge did not re-run forwardAll after resources/list_changed (listCalls=%d)", + counter.count.Load()) + + // Step 4: stop the bridge, then read bridge.srv. The re-sync forwardAll runs + // on the upstream client's OnNotification goroutine, not run(); Shutdown's + // b.up.Close() joins that goroutine before returning, which is the + // happens-before edge that makes reading bridge.srv here race-free. + _ = pipeW.Close() + bridge.Shutdown() + + uris := resourceNamesOnServer(t, bridge.srv) + assert.Contains(t, uris, "res://alpha", "res://alpha must be present after the re-fetch (forwardAll is additive)") + assert.Contains(t, uris, "res://beta", "res://beta must be present after the re-sync") +} + +// TestBridge_PromptsListChanged_TriggersReSync verifies the bridge's +// notifications/prompts/list_changed re-fetch fix: when the upstream +// backend's prompt set changes after the bridge has connected, the upstream +// emits a prompts/list_changed notification, the bridge's OnNotification +// handler re-runs forwardAll, and the newly added prompt appears on the +// bridge's local stdio server. +// +// Mirrors TestBridge_ToolsListChanged_TriggersReSync/ +// TestBridge_ResourcesListChanged_TriggersReSync: the mutation is driven +// through the per-session overlay (SessionWithPrompts.SetSessionPrompts), and +// readiness/re-fetch is observed via the tools/list counter for the same +// race-free reason (forwardAll always re-fetches tools too). +// +//nolint:paralleltest // Swaps process-global os.Stdin; cannot run in parallel. +func TestBridge_PromptsListChanged_TriggersReSync(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // --- upstream backend with an initial prompt "alpha" --- + counter := &listToolsCounter{} + holder := &sessionHolder{} + hooks := &server.Hooks{} + hooks.AddOnRegisterSession(func(_ context.Context, s server.ClientSession) { holder.set(s) }) + hooks.AddBeforeListTools(counter.hook()) + + backend := server.NewMCPServer( + "backend", "1.0", + // WithToolCapabilities is declared (though no tool is registered) so the + // backend unambiguously serves tools/list and fires the BeforeListTools + // hook — the counter is the readiness signal for the prompt-triggered + // re-sync, since forwardAll re-fetches tools on ANY *_list_changed. + server.WithToolCapabilities(true), + server.WithPromptCapabilities(true), + server.WithHooks(hooks), + ) + backend.AddPrompt(mcp.NewPrompt("alpha"), noopPromptHandler) + + httpSrv := server.NewStreamableHTTPServer(backend) + ts := httptest.NewServer(httpSrv) + t.Cleanup(ts.Close) + + // --- bridge pointing at the backend (streamable-http) --- + bridge, err := NewStdioBridge("test", ts.URL, types.TransportTypeStreamableHTTP) + require.NoError(t, err) + + // Swap os.Stdin ONLY for a pipe so ServeStdio does not touch the real + // process stdin and can be unblocked at teardown by closing the write end. + origIn := os.Stdin + pipeR, pipeW, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { + os.Stdin = origIn + _ = pipeR.Close() + _ = pipeW.Close() + }) + os.Stdin = pipeR + + bridge.Start(ctx) + t.Cleanup(func() { + _ = pipeW.Close() + bridge.Shutdown() + }) + + // Step 1: wait for the bridge to connect and run its initial forwardAll. + require.Eventually(t, func() bool { + return holder.get() != nil && counter.count.Load() >= 1 + }, 5*time.Second, 50*time.Millisecond, + "bridge did not complete initial forwardAll (session=%v, listCalls=%d)", + holder.get() != nil, counter.count.Load()) + + // Step 2: mutate the upstream prompt set — add "beta" via the per-session + // overlay. SetSessionPrompts syncs onto the live go-sdk server bound to + // this session, which emits notifications/prompts/list_changed to the + // bridge's upstream client. + backendSession := holder.get() + swp, ok := backendSession.(server.SessionWithPrompts) + require.True(t, ok, "backend session must implement SessionWithPrompts") + swp.SetSessionPrompts(map[string]server.ServerPrompt{ + "alpha": {Prompt: mcp.NewPrompt("alpha"), Handler: noopPromptHandler}, + "beta": {Prompt: mcp.NewPrompt("beta"), Handler: noopPromptHandler}, + }) + + // Step 3: wait for the re-fetch, observed via the tools/list counter + // (forwardAll re-fetches tools on every *_list_changed notification). + require.Eventually(t, func() bool { + return counter.count.Load() >= 2 + }, 5*time.Second, 50*time.Millisecond, + "bridge did not re-run forwardAll after prompts/list_changed (listCalls=%d)", + counter.count.Load()) + + // Step 4: stop the bridge, then read bridge.srv. The re-sync forwardAll runs + // on the upstream client's OnNotification goroutine, not run(); Shutdown's + // b.up.Close() joins that goroutine before returning, which is the + // happens-before edge that makes reading bridge.srv here race-free. + _ = pipeW.Close() + bridge.Shutdown() + + names := promptNamesOnServer(t, bridge.srv) + assert.Contains(t, names, "alpha", "alpha must be present after the re-fetch (forwardAll is additive)") + assert.Contains(t, names, "beta", "beta must be present after the re-sync") +} + // TestBridge_ProgressAndLoggingNotifications_ForwardedByShim guards the premise // the bridge's notification forwarding depends on: the mcpcompat client the // bridge uses must deliver upstream notifications/progress and diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index 119de0c829..357f0ee351 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -708,10 +708,17 @@ func wrapBackendError(err error, backendID string, operation string) error { return fmt.Errorf("%w: failed to %s for backend %s: %v", vmcp.ErrAuthenticationFailed, operation, backendID, err) } - // ErrLegacySSEServer is returned for any 4xx (except 401) on initialize POST. - // This includes 403 (auth rejection) and 404/405 (endpoint not found/method not allowed). - // We cannot distinguish auth failures from routing errors without the raw status code, - // so we surface a clear message and classify as backend unavailable to allow recovery. + // ErrLegacySSEServer is toolhive-core's sentinel for a 4xx (except 401) on + // initialize POST against the "sse" transport type, where the SDK client + // cannot distinguish an auth rejection from a legacy SSE-only server without + // the raw status code. Under the streamable-http transport this arm is dead: + // mcp-go's streamable-HTTP client surfaces a generic HTTP status error for a + // 403 on initialize (e.g. "request failed with status 403"), not this + // sentinel, so that case falls through to the string-based classification + // below and still maps to ErrBackendUnavailable (see + // TestRegression_403OnInitialize_LegacySSEFallback). This arm only fires for + // the sse transport type; it is kept for backend targets still configured + // with legacy SSE. if errors.Is(err, transport.ErrLegacySSEServer) { const legacyMsg = "server rejected MCP initialize — possible auth rejection or legacy SSE-only server" return fmt.Errorf("%w: failed to %s for backend %s (%s): %v", diff --git a/pkg/vmcp/client/schema_ingestion_regression_test.go b/pkg/vmcp/client/schema_ingestion_regression_test.go new file mode 100644 index 0000000000..60fd0c017e --- /dev/null +++ b/pkg/vmcp/client/schema_ingestion_regression_test.go @@ -0,0 +1,167 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/mcpcompat/client" + mcptransport "github.com/stacklok/toolhive-core/mcpcompat/client/transport" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// TestRegression_ToolSchemaFidelity_PreservesCompositors pins that the +// ListCapabilities ingestion path preserves top-level JSON Schema compositor +// keywords (e.g. "oneOf") on a tool's input schema, and does not fabricate a +// spurious "type" field for a schema that has none at the top level (e.g. a +// oneOf-only schema). +// +// It is currently blocked: toolhive-core's mcp.ToolArgumentsSchema (backing +// mcp.Tool.InputSchema's UnmarshalJSON) only captures $defs/type/properties/ +// required/additionalProperties. A raw "oneOf" in the wire JSON is silently +// dropped during the SDK's own JSON->mcp.Tool unmarshal — before +// conversion.ConvertToolInputSchema ever sees it — and MarshalJSON +// unconditionally re-emits "type": tas.Type, fabricating "type":"" for a tool +// whose schema legitimately omits a top-level type. See #5976 (toolhive-core +// Tool.UnmarshalJSON) for the upstream fix; unskip this test in the +// toolhive-core bump PR that lands it. +func TestRegression_ToolSchemaFidelity_PreservesCompositors(t *testing.T) { + t.Parallel() + t.Skip("blocked on #5976 (toolhive-core Tool.UnmarshalJSON); unskip in the toolhive-core bump PR") + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + body, err := io.ReadAll(r.Body) + if err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } + + var req jsonRPCRequest + if err := json.Unmarshal(body, &req); err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } + + switch req.Method { + case "initialize": + resp := jsonRPCResponse{ + JSONRPC: "2.0", + ID: req.ID, + Result: json.RawMessage(`{ + "protocolVersion": "2024-11-05", + "capabilities": {"tools": {}}, + "serverInfo": {"name": "schema-fidelity-backend", "version": "1.0.0"} + }`), + } + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(resp) + + case "tools/list": + // tool-one has a top-level "oneOf" compositor and no top-level "type". + // tool-two has only properties/required, also no top-level "type". + resp := jsonRPCResponse{ + JSONRPC: "2.0", + ID: req.ID, + Result: json.RawMessage(`{ + "tools": [ + { + "name": "tool-one", + "description": "a tool whose schema is a oneOf of two shapes", + "inputSchema": { + "oneOf": [ + {"type": "object", "properties": {"a": {"type": "string"}}, "required": ["a"]}, + {"type": "object", "properties": {"b": {"type": "string"}}, "required": ["b"]} + ] + } + }, + { + "name": "tool-two", + "description": "a tool whose schema has properties/required but no top-level type", + "inputSchema": { + "properties": {"c": {"type": "string"}}, + "required": ["c"] + } + } + ] + }`), + } + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(resp) + + default: + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(jsonRPCResponse{ + JSONRPC: "2.0", + ID: req.ID, + Result: json.RawMessage(`{}`), + }) + } + })) + t.Cleanup(srv.Close) + + h := &httpBackendClient{ + clientFactory: func(ctx context.Context, target *vmcp.BackendTarget, _ bool) (*client.Client, error) { + c, err := client.NewStreamableHttpClient( + target.BaseURL, + mcptransport.WithHTTPTimeout(30*time.Second), + ) + if err != nil { + return nil, err + } + if err := c.Start(ctx); err != nil { + return nil, err + } + return c, nil + }, + } + + target := &vmcp.BackendTarget{ + WorkloadID: "schema-fidelity-backend", + WorkloadName: "Schema Fidelity Backend", + BaseURL: srv.URL, + TransportType: "streamable-http", + } + + caps, err := h.ListCapabilities(context.Background(), target) + require.NoError(t, err) + require.Len(t, caps.Tools, 2) + + byName := make(map[string]vmcp.Tool, len(caps.Tools)) + for _, tool := range caps.Tools { + byName[tool.Name] = tool + } + + toolOne, ok := byName["tool-one"] + require.True(t, ok, "tool-one must be present") + assert.Contains(t, toolOne.InputSchema, "oneOf", + "tool-one's projected InputSchema must still contain the oneOf compositor") + assertNoFabricatedEmptyType(t, toolOne.InputSchema, "tool-one") + + toolTwo, ok := byName["tool-two"] + require.True(t, ok, "tool-two must be present") + assertNoFabricatedEmptyType(t, toolTwo.InputSchema, "tool-two") +} + +// assertNoFabricatedEmptyType asserts that schema does not contain a "type" +// key with an empty string value — the fabrication produced by +// ToolArgumentsSchema.MarshalJSON unconditionally re-emitting "type": tas.Type +// for a schema that never had a top-level type in the original wire JSON. +func assertNoFabricatedEmptyType(t *testing.T, schema map[string]any, toolName string) { + t.Helper() + if typ, ok := schema["type"]; ok { + assert.NotEqual(t, "", typ, + "%s's projected InputSchema must not fabricate an empty top-level \"type\"", toolName) + } +} diff --git a/pkg/vmcp/errors_test.go b/pkg/vmcp/errors_test.go new file mode 100644 index 0000000000..55517c19b7 --- /dev/null +++ b/pkg/vmcp/errors_test.go @@ -0,0 +1,164 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package vmcp + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestIsAuthenticationError pins IsAuthenticationError's exact matching +// boundary (§13): it must recognize the specific phrase patterns the +// function checks for (e.g. "401 unauthorized", "unauthorized (401)", +// "403 forbidden", "authorization required", "access denied") but must NOT +// fire on a bare status code or a bare keyword with no surrounding context — +// otherwise unrelated errors that merely mention "401" or "unauthorized" in +// passing would be misclassified as authentication failures. +func TestIsAuthenticationError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want bool + }{ + // --- negatives: bare substrings and unrelated errors must not match --- + { + name: "nil error", + err: nil, + want: false, + }, + { + name: "bare 401 with no surrounding phrase", + err: errors.New("401"), + want: false, + }, + { + name: "bare unauthorized with no surrounding phrase", + err: errors.New("unauthorized"), + want: false, + }, + { + name: "unrelated error that merely contains 401 as a numeric substring", + err: errors.New("error 4010 occurred"), + want: false, + }, + { + name: "hostname that merely contains 401", + err: errors.New("http://backend401.example.com"), + want: false, + }, + { + name: "connection refused", + err: errors.New("connection refused"), + want: false, + }, + { + name: "request timeout", + err: errors.New("request timeout"), + want: false, + }, + { + name: "404 not found", + err: errors.New("404 not found"), + want: false, + }, + { + name: "500 internal server error", + err: errors.New("500 internal server error"), + want: false, + }, + { + // Pins against accidental loosening of the "authorization required" + // matcher: "field 'authorization' required" must NOT match. The + // matcher looks for the contiguous substring "authorization + // required"; a future change allowing arbitrary whitespace between + // the words would silently regress this. + name: "validation message with 'authorization' and 'required' separated", + err: errors.New("field 'authorization' required"), + want: false, + }, + + // --- positives: recognized phrase patterns --- + { + name: "authentication failed", + err: errors.New("authentication failed"), + want: true, + }, + { + name: "Authentication Failed (case-insensitive)", + err: errors.New("Authentication Failed"), + want: true, + }, + { + name: "authentication error phrase", + err: errors.New("authentication error: bad token"), + want: true, + }, + { + name: "401 Unauthorized phrase", + err: errors.New("401 Unauthorized"), + want: true, + }, + { + name: "unauthorized (401) reversed phrase (mcp-go ErrUnauthorized form)", + err: errors.New("unauthorized (401)"), + want: true, + }, + { + name: "403 forbidden phrase", + err: errors.New("403 forbidden"), + want: true, + }, + { + name: "HTTP 401 phrase", + err: errors.New("HTTP 401"), + want: true, + }, + { + name: "HTTP 403 phrase", + err: errors.New("HTTP 403"), + want: true, + }, + { + name: "status code 401 phrase", + err: errors.New("status code 401"), + want: true, + }, + { + name: "status code 403 phrase", + err: errors.New("status code 403"), + want: true, + }, + { + name: "request unauthenticated phrase", + err: errors.New("request unauthenticated"), + want: true, + }, + { + name: "request unauthorized phrase", + err: errors.New("request unauthorized"), + want: true, + }, + { + name: "authorization required phrase", + err: errors.New("authorization required"), + want: true, + }, + { + name: "access denied phrase", + err: errors.New("access denied"), + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, IsAuthenticationError(tt.err)) + }) + } +} diff --git a/pkg/vmcp/health/checker_test.go b/pkg/vmcp/health/checker_test.go index 49bd2b6453..7ca479e8b0 100644 --- a/pkg/vmcp/health/checker_test.go +++ b/pkg/vmcp/health/checker_test.go @@ -419,58 +419,11 @@ func TestCategorizeError(t *testing.T) { } } -func TestIsAuthenticationError(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - err error - expectErr bool - }{ - // Positive cases - {name: "authentication failed", err: errors.New("authentication failed"), expectErr: true}, - {name: "Authentication Failed (uppercase)", err: errors.New("Authentication Failed"), expectErr: true}, - {name: "authentication error", err: errors.New("authentication error: bad token"), expectErr: true}, - {name: "401 unauthorized", err: errors.New("401 unauthorized"), expectErr: true}, - {name: "403 forbidden", err: errors.New("403 forbidden"), expectErr: true}, - {name: "HTTP 401", err: errors.New("HTTP 401"), expectErr: true}, - {name: "HTTP 403", err: errors.New("HTTP 403"), expectErr: true}, - {name: "status code 401", err: errors.New("status code 401"), expectErr: true}, - {name: "status code 403", err: errors.New("status code 403"), expectErr: true}, - {name: "request unauthenticated", err: errors.New("request unauthenticated"), expectErr: true}, - {name: "request unauthorized", err: errors.New("request unauthorized"), expectErr: true}, - {name: "access denied", err: errors.New("access denied"), expectErr: true}, - - // mcp-go ErrUnauthorized format: "unauthorized (401)" (reversed order vs "401 unauthorized") - {name: "unauthorized (401) - mcp-go ErrUnauthorized format", err: errors.New("unauthorized (401)"), expectErr: true}, - - // Negative cases - should NOT be detected as auth errors - {name: "connection refused", err: errors.New("connection refused"), expectErr: false}, - {name: "timeout", err: errors.New("request timeout"), expectErr: false}, - {name: "generic error", err: errors.New("something went wrong"), expectErr: false}, - {name: "404 not found", err: errors.New("404 not found"), expectErr: false}, - {name: "500 internal server error", err: errors.New("500 internal server error"), expectErr: false}, - {name: "hostname with 401", err: errors.New("http://backend401.example.com"), expectErr: false}, - // Pin against accidental loosening of the "authorization required" - // substring matcher: a validation message of the form "field - // 'authorization' required" must not be misclassified as an auth - // failure. The current matcher uses the contiguous substring - // "authorization required" (one space, no punctuation), so this - // string does not match — but a future loosening (e.g. allowing - // any whitespace) would silently regress. - {name: "validation message containing 'authorization' and 'required'", err: errors.New("field 'authorization' required"), expectErr: false}, - {name: "nil error", err: nil, expectErr: false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - result := vmcp.IsAuthenticationError(tt.err) - assert.Equal(t, tt.expectErr, result) - }) - } -} +// NOTE: IsAuthenticationError is exhaustively tested in its owning package at +// pkg/vmcp/errors_test.go (TestIsAuthenticationError). It was previously +// re-tested here, but a classifier owned by pkg/vmcp belongs under test there, +// not in the health package that merely consumes it (see .claude/rules/testing.md +// "Test Scope"). func TestIsTimeoutError(t *testing.T) { t.Parallel() diff --git a/pkg/vmcp/server/dns_rebinding_regression_test.go b/pkg/vmcp/server/dns_rebinding_regression_test.go new file mode 100644 index 0000000000..b043ab8cb1 --- /dev/null +++ b/pkg/vmcp/server/dns_rebinding_regression_test.go @@ -0,0 +1,114 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/server/sessionmanager" +) + +// TestRegression_DNSRebinding_RejectsForeignHostByDefault pins that the vMCP +// server does NOT pass go-sdk's WithDisableLocalhostProtection option, so a +// Streamable HTTP server bound to a loopback listener retains go-sdk's default +// DNS-rebinding protection: a POST whose Host header names a non-localhost +// value is rejected with 403 before the request reaches the MCP dispatcher. +// +// Only toolhive-core (mcpcompat/server) exercises this behaviour directly +// today; this test pins it at the vMCP integration point so a future change +// that starts threading WithDisableLocalhostProtection(true) into the Serve +// path's streamableOpts (server.go Handler) regresses loudly instead of +// silently reopening the DNS-rebinding hole. +// +// The positive control (httptest's own loopback Host, which the client sets +// automatically) must NOT be rejected, so the negative case cannot pass +// vacuously (e.g. if the whole POST path were broken). +func TestRegression_DNSRebinding_RejectsForeignHostByDefault(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + factory, _ := newToolSessionFactory(t, ctrl, nil) + fc := &fakeCore{} + + srv, err := Serve(context.Background(), fc, &ServerConfig{ + SessionTTL: time.Minute, + SessionManagerConfig: &sessionmanager.FactoryConfig{Base: factory}, + BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = srv.Stop(context.Background()) }) + + handler, err := srv.Handler(context.Background()) + require.NoError(t, err) + ts := httptest.NewServer(handler) + t.Cleanup(ts.Close) + + initBody, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": map[string]any{ + "protocolVersion": "2025-06-18", + "capabilities": map[string]any{}, + "clientInfo": map[string]any{"name": "dns-rebinding-test", "version": "1.0"}, + }, + }) + require.NoError(t, err) + + tests := []struct { + name string + host string // empty means "leave the client-assigned default (loopback) Host" + wantStatus int + }{ + { + name: "foreign Host header is rejected (DNS-rebinding protection)", + host: "evil.example.com", + wantStatus: http.StatusForbidden, + }, + { + name: "loopback Host header is accepted (positive control)", + // Leave req.Host at its client-assigned default (the loopback + // listener's own address) so this case cannot pass vacuously if the + // POST path itself were broken. + host: "", + wantStatus: http.StatusOK, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, ts.URL+"/mcp", bytes.NewReader(initBody)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if tt.host != "" { + // Set the Host FIELD (not a header) — go-sdk's DNS-rebinding check + // inspects http.Request.Host, which net/http populates from the + // request line / Host header on the wire. Setting Header.Set("Host", + // ...) would not exercise the same code path. + req.Host = tt.host + } + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, tt.wantStatus, resp.StatusCode, + "POST with Host=%q must yield status %d", tt.host, tt.wantStatus) + }) + } +} diff --git a/pkg/vmcp/server/sessionmanager/horizontal_scaling_integration_test.go b/pkg/vmcp/server/sessionmanager/horizontal_scaling_integration_test.go index 9535abe3a6..23fc831a67 100644 --- a/pkg/vmcp/server/sessionmanager/horizontal_scaling_integration_test.go +++ b/pkg/vmcp/server/sessionmanager/horizontal_scaling_integration_test.go @@ -42,9 +42,19 @@ func newUnauthenticatedAuthRegistry(t *testing.T) vmcpauth.OutgoingAuthRegistry return reg } -// newSharedRedisStorage creates a RedisSessionDataStorage pointing at mr. -// The storage is closed via t.Cleanup. +// newSharedRedisStorage creates a RedisSessionDataStorage pointing at mr with +// a long (1h) TTL, suitable for tests that are not exercising TTL expiry +// itself. The storage is closed via t.Cleanup. func newSharedRedisStorage(t *testing.T, mr *miniredis.Miniredis) transportsession.DataStorage { + t.Helper() + return newSharedRedisStorageWithTTL(t, mr, time.Hour) +} + +// newSharedRedisStorageWithTTL is like newSharedRedisStorage but lets the +// caller control the sliding-window TTL, so tests can pin TTL-refresh and +// TTL-expiry behaviour with a short duration combined with mr.FastForward. +// The storage is closed via t.Cleanup. +func newSharedRedisStorageWithTTL(t *testing.T, mr *miniredis.Miniredis, ttl time.Duration) transportsession.DataStorage { t.Helper() storage, err := transportsession.NewRedisSessionDataStorage( context.Background(), @@ -52,7 +62,7 @@ func newSharedRedisStorage(t *testing.T, mr *miniredis.Miniredis) transportsessi Addr: mr.Addr(), }, "test:vmcp:session:", - time.Hour, + ttl, ) require.NoError(t, err) t.Cleanup(func() { _ = storage.Close() }) diff --git a/pkg/vmcp/server/sessionmanager/sliding_ttl_regression_test.go b/pkg/vmcp/server/sessionmanager/sliding_ttl_regression_test.go new file mode 100644 index 0000000000..bbd7b5678a --- /dev/null +++ b/pkg/vmcp/server/sessionmanager/sliding_ttl_regression_test.go @@ -0,0 +1,86 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package sessionmanager + +import ( + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// slidingTTLRegressionTTL is the short sliding-window TTL used by the +// regression tests below, paired with miniredis.FastForward to deterministically +// simulate elapsed time without slowing down the test suite. +const slidingTTLRegressionTTL = 200 * time.Millisecond + +// slidingTTLRegressionEpsilon is added to/subtracted from +// slidingTTLRegressionTTL when fast-forwarding, so assertions are not flaky +// against exact-boundary timing. +const slidingTTLRegressionEpsilon = 50 * time.Millisecond + +// --------------------------------------------------------------------------- +// Regression tests: sliding-TTL renewal on session-manager Validate (§14) +// --------------------------------------------------------------------------- + +// TestRegression_Validate_SlidingTTL_SurvivesUnderActivity pins that +// Manager.Validate's storage.Load (Redis GETEX) renews the session's TTL, so +// a session under regular activity never expires even though the total +// elapsed time far exceeds the configured TTL. This is the positive half of +// the sliding-TTL pair; TestRegression_Validate_NoActivity_ExpiresAfterTTL is +// the negative half proving the fast-forward actually simulates expiry +// (without it, this test could pass vacuously if renewal were broken). +func TestRegression_Validate_SlidingTTL_SurvivesUnderActivity(t *testing.T) { + t.Parallel() + + mr := miniredis.RunT(t) + storage := newSharedRedisStorageWithTTL(t, mr, slidingTTLRegressionTTL) + backend := startMCPBackend(t, "backend-alpha", "echo") + sm := newTestManagerWithSharedStorage(t, storage, []*vmcp.Backend{backend}) + + sessionID := createSession(t, sm, nil) + + // Repeatedly validate the session, advancing the fake clock by just under + // the TTL between each call. If Validate's Load renews the TTL (via + // GETEX), the session must never be reported as terminated, even though + // the cumulative elapsed time is many multiples of the configured TTL. + const iterations = 5 + for i := range iterations { + mr.FastForward(slidingTTLRegressionTTL - slidingTTLRegressionEpsilon) + + isTerminated, err := sm.Validate(sessionID) + require.NoError(t, err, "iteration %d: Validate must not error", i) + require.False(t, isTerminated, + "iteration %d: session must survive under repeated activity (sliding TTL renewal)", i) + } +} + +// TestRegression_Validate_NoActivity_ExpiresAfterTTL pins that a session +// which receives no Validate (or other Load) calls does expire once the +// sliding-window TTL elapses. Paired with +// TestRegression_Validate_SlidingTTL_SurvivesUnderActivity so the survival +// test cannot pass vacuously (e.g. if TTL enforcement were disabled +// entirely, the survival test alone would still pass). +func TestRegression_Validate_NoActivity_ExpiresAfterTTL(t *testing.T) { + t.Parallel() + + mr := miniredis.RunT(t) + storage := newSharedRedisStorageWithTTL(t, mr, slidingTTLRegressionTTL) + backend := startMCPBackend(t, "backend-alpha", "echo") + sm := newTestManagerWithSharedStorage(t, storage, []*vmcp.Backend{backend}) + + sessionID := createSession(t, sm, nil) + + // No Validate/Load calls occur here — advance the fake clock straight past + // the TTL. + mr.FastForward(slidingTTLRegressionTTL + slidingTTLRegressionEpsilon) + + isTerminated, err := sm.Validate(sessionID) + require.NoError(t, err) + assert.True(t, isTerminated, "session with no activity must be reported terminated once the TTL elapses") +}