L8: MCP client robustness (timeouts, names, terminal escapes)

AGENTS.md

@@ -182,6 +182,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **odek self-invocation gate** (`internal/danger/classifier.go`) — any shell stage whose program basename is `odek` is classified as `system_write`. This prevents a prompt-injected agent from reaching the human-gated trust mutations (`odek memory promote`, `odek memory extended confirm`, `odek skill promote --force`) through the shell tool and flipping its own taint gates.
 - **MCP inputSchema hardening** (`cmd/odek/mcp_approval.go`) — every string in an MCP tool's `inputSchema` is recursively guard-scanned for injection patterns; schemas larger than 256 KiB are rejected; the interactive approval prompt shows a SHA-256 hash and byte size of the schema so operators can detect changes.
 - **MCP tool batch classification** (`internal/loop/loop.go`) — MCP tools (`<server>__<tool>`) are classified as `unknown` by `classifyToolCall`, so the batch approval gate shows them and untrusted sub-agents force them to `deny`.
+- **MCP client robustness** (`internal/mcpclient/client.go`, `cmd/odek/mcp_approval.go`) — MCP calls use a default timeout when the caller supplies no deadline; server names and tool names are validated (tool names are rejected if they contain `__` to prevent collisions with odek's `<server>__<tool>` namespace); the interactive approval prompt sanitises tool descriptions to strip ANSI escape sequences and terminal control characters.
 - **Sub-agent trust defaults + delegate_tasks gate** (`cmd/odek/subagent.go`, `internal/loop/loop.go`) — a missing `trust_level` in `delegate_tasks` defaults to `untrusted`; `delegate_tasks` itself is classified as `system_write` so it requires explicit approval before spawning child processes.
 - **Memory add/replace pipe-to-shell filter** (`internal/memory/memory.go`) — `AddFact` and `ReplaceFact` now run `FactLooksUnsafe` in addition to the general guard scan, blocking agent-driven planting of download-and-execute facts.
 - **Skill learn-loop provenance propagation** (`internal/skills/learnloop.go`) — conversation-extracted suggestions and LLM-enhanced suggestions both retain the session's `SkillProvenance`, so tainted sessions cannot produce clean-looking auto-saved skills.

cmd/odek/mcp_approval.go

@@ -11,6 +11,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -243,7 +244,7 @@ func approveMCPToolsWithTTY(projectDir, serverName string, cfg mcpclient.ServerC
 
 		fmt.Fprintf(stdout, "\nMCP server %q wants to register tool %q\n", serverName, def.Name)
 		if def.Description != "" {
-			fmt.Fprintf(stdout, "  description: %s\n", truncateDescription(def.Description, 200))
+			fmt.Fprintf(stdout, "  description: %s\n", sanitizeTerminal(truncateDescription(def.Description, 200)))
 		}
 		fmt.Fprintf(stdout, "  schema: sha256:%s (%d bytes)\n", schemaHash, schemaSize)
 		fmt.Fprintf(stdout, "Approve? [y/N] ")
@@ -344,6 +345,28 @@ func truncateDescription(desc string, max int) string {
 	return desc[:max-3] + "..."
 }
 
+// sanitizeTerminal removes ANSI escape sequences and replaces other terminal
+// control characters with a replacement character so a malicious MCP server
+// cannot disguise an approval prompt with cursor movement or colour codes.
+func sanitizeTerminal(s string) string {
+	// Strip ANSI escape sequences: ESC [ ... m (and similar).
+	ansi := regexp.MustCompile("\x1b\\[[0-9;]*[a-zA-Z]")
+	s = ansi.ReplaceAllString(s, "")
+	// Replace remaining control characters (except tab/newline) with �.
+	var b strings.Builder
+	for _, r := range s {
+		switch {
+		case r == '\t' || r == '\n' || r == '\r':
+			b.WriteRune(r)
+		case r < 0x20 || r == 0x7f:
+			b.WriteRune('�')
+		default:
+			b.WriteRune(r)
+		}
+	}
+	return b.String()
+}
+
 // maxMCPSchemaBytes caps the serialized JSON schema size for a single MCP tool.
 // Schemas are part of the model's tool catalogue, so an oversized schema can be
 // used for prompt stuffing. Real-world MCP schemas are typically small; 256 KiB

cmd/odek/mcp_approval_test.go

@@ -301,3 +301,18 @@ func TestApproveMCPTools_OversizeSchemaSkipped(t *testing.T) {
 		t.Errorf("approved %d tools, want 0 (oversized schema should be skipped)", len(got))
 	}
 }
+
+func TestSanitizeTerminal_StripsANSIAndControlChars(t *testing.T) {
+	// ANSI red colour + cursor-up sequence, then a bell and normal text.
+	input := "\x1b[31m\x1b[2A\x07normal"
+	got := sanitizeTerminal(input)
+	if strings.Contains(got, "\x1b") {
+		t.Errorf("ANSI escapes should be stripped, got: %q", got)
+	}
+	if strings.Contains(got, "\x07") {
+		t.Errorf("control characters should be removed, got: %q", got)
+	}
+	if !strings.Contains(got, "normal") {
+		t.Errorf("normal text should be preserved, got: %q", got)
+	}
+}

docs/SECURITY.md

@@ -634,6 +634,16 @@ Now:
 The per-turn audit log and divergence heuristic only inspected `tool` messages for the nonce'd `<untrusted_content_*>` wrapper. @-references, `--ctx` files, and Web-UI attachments were also wrapped before entering the user message, but:
 
 - `cmd/odek/refs.go` called `wrapUntrusted` with `context.Background()`, so `recordIngest` found no active recorder.
+
+### 39k. MCP client robustness (timeouts, name validation, terminal output)
+
+Three MCP client hardening fixes close availability and spoofing issues:
+
+1. **Request timeout** — `internal/mcpclient/client.go` declared `DefaultTimeout` but never applied it. A hung MCP server would block `Discover` or `CallTool` indefinitely. The timeout is now applied automatically when the caller does not supply a context deadline.
+
+2. **Server-name validation** — MCP server names are used as the prefix in registered tool names (`<server>__<tool>`). Names are now validated to be non-empty, ≤ 64 characters, ASCII letters/digits/underscore/hyphen only, and must not contain `__`. This prevents invalid API identifiers from killing every LLM turn and blocks the collision where server `a` + tool `b__c` produces the same effective name as server `a__b` + tool `c`. Tool names are also rejected if they contain `__`.
+
+3. **Terminal-sanitized approval prompt** — the interactive MCP tool approval prompt printed the server-supplied description verbatim. A malicious server could hide cursor movement or colour codes in the description to disguise what was being approved. Descriptions are now passed through `sanitizeTerminal`, which strips ANSI escape sequences and replaces other control characters before printing.
 - `cmd/odek/serve.go` resolved @-refs and attachments before attaching the per-session ingest recorder.
 - `recordTurnAudit` only scanned `tool` messages, so `ingested_untrusted` stayed false for these vectors.
 - The enriched prompt (including injected resource literals) was passed as the "user message" to the divergence check, making attacker resources count as user-mentioned and disabling the heuristic.

internal/mcpclient/client.go

@@ -44,8 +44,7 @@ import (
 // ── Protocol Constants ──────────────────────────────────────────────────
 
 const (
-	ProtocolVersion    = "2025-03-26"
-	DefaultTimeout     = 30 * time.Second
+	ProtocolVersion = "2025-03-26"
 	// maxMCPResponseLine caps the size of a single JSON-RPC response line
 	// from an MCP server. A malicious or broken server that emits a huge
 	// line without a newline would otherwise be buffered entirely in memory
@@ -54,6 +53,10 @@ const (
 	maxMCPResponseLine = 10 << 20 // 10 MiB
 )
 
+// DefaultTimeout bounds each MCP request when the caller does not supply a
+// context deadline. It is a var so tests can temporarily lower it.
+var DefaultTimeout = 30 * time.Second
+
 // ── JSON-RPC Types ─────────────────────────────────────────────────────
 
 // request is a JSON-RPC 2.0 request sent to the MCP server.
@@ -119,6 +122,9 @@ func validateToolName(name string) error {
 	if len(name) > 64 {
 		return fmt.Errorf("tool name %q exceeds 64 characters", name)
 	}
+	if strings.Contains(name, "__") {
+		return fmt.Errorf("tool name %q cannot contain %q", name, "__")
+	}
 	for _, r := range name {
 		switch {
 		case r >= 'a' && r <= 'z':
@@ -192,9 +198,38 @@ type Client struct {
 	pending map[int]chan callResponse // routes responses to waiting callers
 }
 
+// validateName checks that an MCP server or tool name is safe to use as part
+// of an odek tool identifier. Names must be non-empty, ≤64 chars, contain only
+// ASCII letters/digits/underscore/hyphen, and must not contain the double-
+// underscore separator used to qualify tool names.
+func validateName(kind, name string) error {
+	if name == "" {
+		return fmt.Errorf("%s name cannot be empty", kind)
+	}
+	if len(name) > 64 {
+		return fmt.Errorf("%s name %q too long (%d > 64)", kind, name, len(name))
+	}
+	if strings.Contains(name, "__") {
+		return fmt.Errorf("%s name %q cannot contain %q", kind, name, "__")
+	}
+	for i, r := range name {
+		isLetter := (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z')
+		isDigit := r >= '0' && r <= '9'
+		if isLetter || isDigit || r == '_' || r == '-' {
+			continue
+		}
+		return fmt.Errorf("%s name %q contains invalid character %q at position %d", kind, name, r, i)
+	}
+	return nil
+}
+
 // New spawns an MCP server process and returns a client connected to it.
 // The server process is started immediately and cleaned up on Close().
 func New(name string, cfg ServerConfig) (*Client, error) {
+	if err := validateName("server", name); err != nil {
+		return nil, err
+	}
+
 	cmd := exec.Command(cfg.Command, cfg.Args...)
 
 	// Apply env overrides. Always build a sanitized environment so MCP children
@@ -430,6 +465,14 @@ func (c *Client) call(ctx context.Context, method string, params json.RawMessage
 	if ctx == nil {
 		ctx = context.Background()
 	}
+	// Bound the request with the package default timeout unless the caller
+	// already supplied a deadline. A hung MCP server must not deadlock the
+	// agent loop or startup discovery.
+	if _, ok := ctx.Deadline(); !ok {
+		var cancel context.CancelFunc
+		ctx, cancel = context.WithTimeout(ctx, DefaultTimeout)
+		defer cancel()
+	}
 
 	// Assign unique ID and register a response channel.
 	respCh := make(chan callResponse, 1)

internal/mcpclient/client_test.go

@@ -504,6 +504,78 @@ func TestClient_CloseWithoutDiscover(t *testing.T) {
 	}
 }
 
+// ── L-8 regression tests ────────────────────────────────────────────────
+
+func TestNew_RejectsInvalidServerName(t *testing.T) {
+	cases := []struct {
+		name string
+	}{
+		{""},
+		{"has space"},
+		{"has__double"},
+		{"tool?name"},
+		{"a" + strings.Repeat("x", 64)},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := New(tc.name, ServerConfig{Command: "echo"})
+			if err == nil {
+				t.Fatal("expected error for invalid server name")
+			}
+		})
+	}
+}
+
+func TestDiscover_RejectsToolWithDoubleUnderscore(t *testing.T) {
+	client, err := New("valid", ServerConfig{
+		Command: fakeServerPath(t),
+		Env: map[string]string{
+			"FAKE_TOOLS": `[{"name":"a__b","description":"bad"}]`,
+		},
+	})
+	if err != nil {
+		t.Fatalf("New: %v", err)
+	}
+	defer client.Close()
+
+	_, err = client.Discover(context.Background())
+	if err == nil {
+		t.Fatal("expected Discover to reject tool name containing __")
+	}
+}
+
+func TestCall_DefaultTimeout(t *testing.T) {
+	// A client whose read loop never answers must time out via DefaultTimeout.
+	client, err := New("timeout", ServerConfig{
+		Command: fakeServerPath(t),
+		Env: map[string]string{
+			"FAKE_DELAY": "10s",
+			"FAKE_TOOLS": `[{"name":"x","description":"y"}]`,
+		},
+	})
+	if err != nil {
+		t.Fatalf("New: %v", err)
+	}
+	defer client.Close()
+
+	// Restore DefaultTimeout after the test.
+	orig := DefaultTimeout
+	DefaultTimeout = 100 * time.Millisecond
+	defer func() { DefaultTimeout = orig }()
+
+	start := time.Now()
+	_, err = client.Discover(context.Background())
+	if err == nil {
+		t.Fatal("expected timeout error")
+	}
+	if !contains(err.Error(), "context deadline exceeded") {
+		t.Errorf("expected context deadline exceeded, got: %v", err)
+	}
+	if time.Since(start) > 2*time.Second {
+		t.Errorf("timeout took too long: %v", time.Since(start))
+	}
+}
+
 // ── Helpers ────────────────────────────────────────────────────────────
 
 func contains(s, substr string) bool {

internal/mcpclient/testdata/main.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"strings"
+	"time"
 )
 
 type request struct {
@@ -31,6 +32,7 @@ type rpcError struct {
 var (
 	toolsJSON    = os.Getenv("FAKE_TOOLS")
 	errorOnCall  = os.Getenv("FAKE_ERROR_ON_CALL")
+	delayStr     = os.Getenv("FAKE_DELAY")
 )
 
 func main() {
@@ -46,6 +48,12 @@ func main() {
 			continue
 		}
 
+		if delayStr != "" {
+			if d, err := time.ParseDuration(delayStr); err == nil {
+				time.Sleep(d)
+			}
+		}
+
 		switch req.Method {
 		case "initialize":
 			initResult, _ := json.Marshal(map[string]any{