From 7d1a189f2e6b38537c7da0033bc3c809b82e702a Mon Sep 17 00:00:00 2001 From: Molecule AI Infra-SRE Date: Sun, 10 May 2026 09:01:51 +0000 Subject: [PATCH] fix(mcp): scrub err.Error() from JSON-RPC error messages (OFFSEC-001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace all three err.Error() leaks in mcp.go with constant strings, consistent with the same fix applied to 22 other files in PRs #1193/1206/1219/#168. - Call handler (line ~329): "parse error: " + err.Error() → "parse error" - dispatchRPC params unmarshal (line ~417): "invalid params: " + err.Error() → "invalid parameters" - dispatchRPC tool call (line ~422): err.Error() → "tool call failed" + log.Printf server-side for forensics Routes protected by WorkspaceAuth (C1) and MCPRateLimiter (C2) — this is defence-in-depth per OFFSEC-001 / #259. Tests added: - TestMCPHandler_Call_MalformedJSON_ReturnsConstantParseError - TestMCPHandler_dispatchRPC_InvalidParams_ReturnsConstantMessage - TestMCPHandler_dispatchRPC_UnknownTool_ReturnsConstantMessage - TestMCPHandler_dispatchRPC_InvalidParams_ArrayInsteadOfObject Co-Authored-By: Claude Opus 4.7 --- workspace-server/internal/handlers/mcp.go | 11 +- .../internal/handlers/mcp_test.go | 123 ++++++++++++++++++ 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/workspace-server/internal/handlers/mcp.go b/workspace-server/internal/handlers/mcp.go index 44290487..3065ca4a 100644 --- a/workspace-server/internal/handlers/mcp.go +++ b/workspace-server/internal/handlers/mcp.go @@ -28,6 +28,7 @@ import ( "database/sql" "encoding/json" "fmt" + "log" "net/http" "os" "time" @@ -326,7 +327,7 @@ func (h *MCPHandler) Call(c *gin.Context) { if err := c.ShouldBindJSON(&req); err != nil { c.JSON(http.StatusBadRequest, mcpResponse{ JSONRPC: "2.0", - Error: &mcpRPCError{Code: -32700, Message: "parse error: " + err.Error()}, + Error: &mcpRPCError{Code: -32700, Message: "parse error"}, }) return } @@ -414,12 +415,16 @@ func (h *MCPHandler) dispatchRPC(ctx context.Context, workspaceID string, req mc Arguments map[string]interface{} `json:"arguments"` } if err := json.Unmarshal(req.Params, ¶ms); err != nil { - base.Error = &mcpRPCError{Code: -32602, Message: "invalid params: " + err.Error()} + base.Error = &mcpRPCError{Code: -32602, Message: "invalid parameters"} return base } text, err := h.dispatch(ctx, workspaceID, params.Name, params.Arguments) if err != nil { - base.Error = &mcpRPCError{Code: -32000, Message: err.Error()} + // Log full error server-side for forensics; return constant string + // to client per OFFSEC-001 / #259. WorkspaceAuth required — caller + // already authenticated, so this is defence-in-depth. + log.Printf("mcp: tool call failed workspace=%s tool=%s: %v", workspaceID, params.Name, err) + base.Error = &mcpRPCError{Code: -32000, Message: "tool call failed"} return base } base.Result = map[string]interface{}{ diff --git a/workspace-server/internal/handlers/mcp_test.go b/workspace-server/internal/handlers/mcp_test.go index 6ede0c2c..dbad430a 100644 --- a/workspace-server/internal/handlers/mcp_test.go +++ b/workspace-server/internal/handlers/mcp_test.go @@ -1024,3 +1024,126 @@ func TestIsPrivateOrMetadataIP_PublicAllowed(t *testing.T) { } } } + +// TestMCPHandler_Call_MalformedJSON returns constant parse-error message. +// Per OFFSEC-001 / #259: err.Error() must not leak struct field names or +// JSON library internals in JSON-RPC error.message. +func TestMCPHandler_Call_MalformedJSON_ReturnsConstantParseError(t *testing.T) { + h, _ := newMCPHandler(t) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + // Valid JSON-RPC 2.0 envelope but JSON body is malformed. + c.Request = httptest.NewRequest("POST", "/", bytes.NewBuffer([]byte("not valid json{]["))) + c.Request.Header.Set("Content-Type", "application/json") + + h.Call(c) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String()) + } + var resp mcpResponse + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("response is not valid JSON: %v", err) + } + if resp.Error == nil { + t.Fatal("expected JSON-RPC error, got nil") + } + // Message must be a constant — no err.Error() content. + if resp.Error.Message != "parse error" { + t.Errorf("error message should be constant 'parse error', got: %q", resp.Error.Message) + } + // Code must be -32700 (Parse error). + if resp.Error.Code != -32700 { + t.Errorf("error code should be -32700, got: %d", resp.Error.Code) + } +} + +// TestMCPHandler_dispatchRPC_InvalidParams returns constant message. +// Per OFFSEC-001 / #259: err.Error() from json.Unmarshal must not be +// returned in JSON-RPC error.message. +func TestMCPHandler_dispatchRPC_InvalidParams_ReturnsConstantMessage(t *testing.T) { + h, _ := newMCPHandler(t) + + // Valid JSON-RPC but params is a string (not an object) — invalid for tools/call. + w := mcpPost(t, h, "ws-1", map[string]interface{}{ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": "not an object", // string instead of object — json.Unmarshal fails + }) + + var resp mcpResponse + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("response is not valid JSON: %v", err) + } + if resp.Error == nil { + t.Fatal("expected JSON-RPC error, got nil") + } + // Message must be a constant — no JSON library error content. + if resp.Error.Message != "invalid parameters" { + t.Errorf("error message should be constant 'invalid parameters', got: %q", resp.Error.Message) + } + if resp.Error.Code != -32602 { + t.Errorf("error code should be -32602 (Invalid params), got: %d", resp.Error.Code) + } +} + +// TestMCPHandler_dispatchRPC_UnknownTool returns constant tool-failed message. +// Per OFFSEC-001 / #259: dispatch errors must not leak workspace IDs or +// internal paths. Note: this test exercises the dispatch path through +// dispatchRPC since dispatch is package-private. +func TestMCPHandler_dispatchRPC_UnknownTool_ReturnsConstantMessage(t *testing.T) { + h, _ := newMCPHandler(t) + + // Valid params shape but tool name does not exist. + w := mcpPost(t, h, "ws-1", map[string]interface{}{ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": map[string]interface{}{ + "name": "nonexistent_tool_xyz", + "arguments": map[string]interface{}{}, + }, + }) + + var resp mcpResponse + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("response is not valid JSON: %v", err) + } + if resp.Error == nil { + t.Fatal("expected JSON-RPC error for unknown tool, got nil") + } + // Message must be a constant — no "unknown tool: nonexistent_tool_xyz" leak. + if resp.Error.Message != "tool call failed" { + t.Errorf("error message should be constant 'tool call failed', got: %q", resp.Error.Message) + } + if resp.Error.Code != -32000 { + t.Errorf("error code should be -32000 (Server error), got: %d", resp.Error.Code) + } +} + +// TestMCPHandler_dispatchRPC_InvalidParams_NilParams covers the edge case +// where params is present but not an object (e.g. an array). json.Unmarshal +// into the params struct fails, and we assert the constant error message. +func TestMCPHandler_dispatchRPC_InvalidParams_ArrayInsteadOfObject(t *testing.T) { + h, _ := newMCPHandler(t) + + w := mcpPost(t, h, "ws-1", map[string]interface{}{ + "jsonrpc": "2.0", + "id": 3, + "method": "tools/call", + "params": []interface{}{"one", "two"}, // array instead of object + }) + + var resp mcpResponse + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("response is not valid JSON: %v", err) + } + if resp.Error == nil { + t.Fatal("expected JSON-RPC error, got nil") + } + if resp.Error.Message != "invalid parameters" { + t.Errorf("error message should be constant 'invalid parameters', got: %q", resp.Error.Message) + } +}