forked from molecule-ai/molecule-core
fix(mcp): scrub err.Error() from JSON-RPC error messages (OFFSEC-001)
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 <noreply@anthropic.com>
This commit is contained in:
parent
08a929c740
commit
7d1a189f2e
@ -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{}{
|
||||
|
||||
@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user