From 93410a295bc50e40d621a3a2f58d03851058a124 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sun, 19 Jul 2026 10:31:51 +0200 Subject: [PATCH] L8: MCP client robustness (timeouts, names, terminal escapes) - internal/mcpclient/client.go: default timeout for caller-deadline-free calls; validate server names; reject tool names containing '__'; DefaultTimeout var for test overrides. - internal/mcpclient/client_test.go: tests for invalid server names, double underscore tool names, default timeout. - internal/mcpclient/testdata/main.go: FAKE_DELAY env for timeout tests. - cmd/odek/mcp_approval.go: sanitize tool descriptions in approval prompt. - cmd/odek/mcp_approval_test.go: sanitize tests. - docs/SECURITY.md + AGENTS.md: document L8 mitigations. --- AGENTS.md | 1 + cmd/odek/mcp_approval.go | 25 +++++++++- cmd/odek/mcp_approval_test.go | 15 ++++++ docs/SECURITY.md | 10 ++++ internal/mcpclient/client.go | 47 ++++++++++++++++++- internal/mcpclient/client_test.go | 72 +++++++++++++++++++++++++++++ internal/mcpclient/testdata/main.go | 8 ++++ 7 files changed, 175 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 363433e5..0293297d 100644 --- a/AGENTS.md +++ b/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 (`__`) 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 `__` 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. diff --git a/cmd/odek/mcp_approval.go b/cmd/odek/mcp_approval.go index b877d48b..f35fedec 100644 --- a/cmd/odek/mcp_approval.go +++ b/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 diff --git a/cmd/odek/mcp_approval_test.go b/cmd/odek/mcp_approval_test.go index d34ae249..c93779d0 100644 --- a/cmd/odek/mcp_approval_test.go +++ b/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) + } +} diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 8e597c1a..2221cd7c 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -634,6 +634,16 @@ Now: The per-turn audit log and divergence heuristic only inspected `tool` messages for the nonce'd `` 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 (`__`). 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. diff --git a/internal/mcpclient/client.go b/internal/mcpclient/client.go index 12b5e493..fa0ab80a 100644 --- a/internal/mcpclient/client.go +++ b/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) diff --git a/internal/mcpclient/client_test.go b/internal/mcpclient/client_test.go index e8e2244b..915cf7eb 100644 --- a/internal/mcpclient/client_test.go +++ b/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 { diff --git a/internal/mcpclient/testdata/main.go b/internal/mcpclient/testdata/main.go index ce6c7dbf..3952d76b 100644 --- a/internal/mcpclient/testdata/main.go +++ b/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{