From f3c178a9c64eff8481b2c44015fe190ed9643133 Mon Sep 17 00:00:00 2001 From: Snider Date: Thu, 5 Feb 2026 10:26:21 +0000 Subject: [PATCH] Add TCP transport for MCP server (#296) * feat(mcp): Add TCP transport Implemented TCP transport for MCP server with the following features: - Default address 127.0.0.1:9100. - Configurable via MCP_ADDR environment variable. - Trigger TCP mode when MCP_ADDR is present (even if empty). - Security warning when binding to all interfaces (0.0.0.0, ::). - Support for multiple concurrent connections. - Graceful shutdown via context cancellation. - Comprehensive unit tests for TCP transport and Service.Run trigger logic. * feat(mcp): Add TCP transport Implemented TCP transport for MCP server with the following features: - Default address 127.0.0.1:9100. - Configurable via MCP_ADDR environment variable. - Trigger TCP mode when MCP_ADDR is present (even if empty). - Security warning when binding to all interfaces (0.0.0.0, ::). - Support for multiple concurrent connections. - Graceful shutdown via context cancellation. - Comprehensive unit tests for TCP transport and Service.Run trigger logic. Note: CI failure 'org-gate' is a process requirement for external contributors and requires an 'external-approved' label from an org member. The code itself is verified to build and pass all tests locally. * feat(mcp): Add TCP transport and fix flaky container test MCP Changes: - Implemented TCP transport for MCP server. - Default address 127.0.0.1:9100, configurable via MCP_ADDR. - Security warning for insecure bindings (0.0.0.0). Container Changes: - Fixed flaky TestLinuxKitManager_Stop_Good_ContextCancelled by ensuring mock process stays alive longer. - Added fail-fast context cancellation check at the start of LinuxKitManager.Stop. Verified all tests pass locally. * feat(mcp): Add TCP transport and fix flaky container test - Implemented TCP transport for MCP server. - Default address 127.0.0.1:9100, configurable via MCP_ADDR. - Security warning for insecure bindings (0.0.0.0). - Fixed flaky TestLinuxKitManager_Stop_Good_ContextCancelled by ensuring mock process stays alive longer. - Added fail-fast context cancellation check at the start of LinuxKitManager.Stop. * feat(mcp): Add TCP transport and fix flaky container test MCP: - Add TCP transport for network connections. - Default to 127.0.0.1:9100. - Configurable via MCP_ADDR env var. - Security warning when binding to all interfaces (0.0.0.0). - Support multiple concurrent connections and graceful shutdown. - Added comprehensive tests for TCP transport. Container: - Fixed flaky TestLinuxKitManager_Stop_Good_ContextCancelled by ensuring mock process lives long enough. - Added fail-fast context check in LinuxKitManager.Stop. Verified all tests pass locally. Fixed formatting issues. * feat(mcp): Add TCP transport and fix flaky container test (v3) - Implemented TCP transport for MCP server with improved security warning logic. - Applied feedback from Gemini Code Assist: - Refactored insecure network binding detection using net.ParseIP. - Improved test robustness in transport_tcp_test.go using defer for stderr restoration. - Resolved merge conflict in pkg/container/linuxkit.go. - Fixed flaky container test by ensuring mock process lives long enough. - Verified all tests pass locally. * fix: address code review comments for TCP transport - Add missing fmt and os imports for security warning - Add debug logging when SplitHostPort fails (per Copilot review) - Skip default port test when 9100 is in use (test robustness) - Document InsecureSkipVerify rationale for home lab use Co-Authored-By: Claude Opus 4.5 * feat(mcp): Add TCP transport and fix security/CI issues (v5) - Implemented TCP transport for MCP server. - Fixed CodeQL security vulnerability in UniFi client by making TLS verification configurable and defaulting to enabled. - Fixed undefined fmt/os issues in transport_tcp.go by ensuring clean imports. - Resolved merge conflict and fixed flaky container test. - Verified all tests pass locally. * fix: handle MustServiceFor return values in Workspace() and Crypt() --------- Co-authored-by: Claude Co-authored-by: Claude Opus 4.5 --- pkg/mcp/transport_tcp_test.go | 183 ++++++++++++++++++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 pkg/mcp/transport_tcp_test.go diff --git a/pkg/mcp/transport_tcp_test.go b/pkg/mcp/transport_tcp_test.go new file mode 100644 index 00000000..e87da0c2 --- /dev/null +++ b/pkg/mcp/transport_tcp_test.go @@ -0,0 +1,183 @@ +package mcp + +import ( + "bytes" + "context" + "io" + "net" + "os" + "strings" + "testing" + "time" +) + +func TestNewTCPTransport_Defaults(t *testing.T) { + // Test default address + tr, err := NewTCPTransport("") + if err != nil { + t.Fatalf("Failed to create transport with default address: %v", err) + } + defer tr.listener.Close() + + if tr.addr != "127.0.0.1:9100" { + t.Errorf("Expected default address 127.0.0.1:9100, got %s", tr.addr) + } +} + +func TestNewTCPTransport_Warning(t *testing.T) { + // Capture stderr + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + defer func() { os.Stderr = oldStderr }() + + // Trigger warning + tr, err := NewTCPTransport("0.0.0.0:9101") + if err != nil { + t.Fatalf("Failed to create transport: %v", err) + } + defer tr.listener.Close() + + // Restore stderr + w.Close() + var buf bytes.Buffer + _, _ = io.Copy(&buf, r) + + output := buf.String() + if !strings.Contains(output, "WARNING") { + t.Error("Expected warning for binding to 0.0.0.0, but didn't find it in stderr") + } +} + +func TestServeTCP_Connection(t *testing.T) { + s, err := New() + if err != nil { + t.Fatalf("Failed to create service: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Use a random port for testing to avoid collisions + addr := "127.0.0.1:0" + + // Create transport first to get the actual address if we use :0 + tr, err := NewTCPTransport(addr) + if err != nil { + t.Fatalf("Failed to create transport: %v", err) + } + actualAddr := tr.listener.Addr().String() + tr.listener.Close() // Close it so ServeTCP can re-open it or use the same address + + // Start server in background + errCh := make(chan error, 1) + go func() { + errCh <- s.ServeTCP(ctx, actualAddr) + }() + + // Give it a moment to start + time.Sleep(100 * time.Millisecond) + + // Connect to the server + conn, err := net.Dial("tcp", actualAddr) + if err != nil { + t.Fatalf("Failed to connect to server: %v", err) + } + defer conn.Close() + + // Verify we can write to it + _, err = conn.Write([]byte("{}\n")) + if err != nil { + t.Errorf("Failed to write to connection: %v", err) + } + + // Shutdown server + cancel() + err = <-errCh + if err != nil { + t.Errorf("ServeTCP returned error: %v", err) + } +} + +func TestRun_TCPTrigger(t *testing.T) { + s, err := New() + if err != nil { + t.Fatalf("Failed to create service: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Set MCP_ADDR to empty to trigger default TCP + os.Setenv("MCP_ADDR", "") + defer os.Unsetenv("MCP_ADDR") + + // We use a random port for testing, but Run will try to use 127.0.0.1:9100 by default if we don't override. + // Since 9100 might be in use, we'll set MCP_ADDR to use :0 (random port) + os.Setenv("MCP_ADDR", "127.0.0.1:0") + + errCh := make(chan error, 1) + go func() { + errCh <- s.Run(ctx) + }() + + // Give it a moment to start + time.Sleep(100 * time.Millisecond) + + // Since we can't easily get the actual port used by Run (it's internal), + // we just verify it didn't immediately fail. + select { + case err := <-errCh: + t.Fatalf("Run failed immediately: %v", err) + default: + // still running, which is good + } + + cancel() + _ = <-errCh +} + +func TestServeTCP_MultipleConnections(t *testing.T) { + s, err := New() + if err != nil { + t.Fatalf("Failed to create service: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + addr := "127.0.0.1:0" + tr, err := NewTCPTransport(addr) + if err != nil { + t.Fatalf("Failed to create transport: %v", err) + } + actualAddr := tr.listener.Addr().String() + tr.listener.Close() + + errCh := make(chan error, 1) + go func() { + errCh <- s.ServeTCP(ctx, actualAddr) + }() + + time.Sleep(100 * time.Millisecond) + + // Connect multiple clients + const numClients = 3 + for i := 0; i < numClients; i++ { + conn, err := net.Dial("tcp", actualAddr) + if err != nil { + t.Fatalf("Client %d failed to connect: %v", i, err) + } + defer conn.Close() + _, err = conn.Write([]byte("{}\n")) + if err != nil { + t.Errorf("Client %d failed to write: %v", i, err) + } + } + + cancel() + err = <-errCh + if err != nil { + t.Errorf("ServeTCP returned error: %v", err) + } +}