From d82df6b8ee61db35f316b0630d1718513dbd1aab Mon Sep 17 00:00:00 2001 From: Sacha Al Himdani Date: Thu, 23 Apr 2026 11:35:00 +0200 Subject: [PATCH] Sanitize MCP errors to avoid leaking internal details Signed-off-by: Sacha Al Himdani --- e2e/internal/testutil/mcp.go | 14 +++++ e2e/mcp/asset_test.go | 23 +++++++ e2e/mcp/risk_test.go | 20 ++++++ e2e/mcp/vendor_test.go | 38 ++++++++++++ go.mod | 2 +- go.sum | 4 +- pkg/server/api/mcp/mcputils/recovery.go | 82 ++++++++++++++++--------- pkg/server/api/mcp/v1/v1_handler.go | 4 +- 8 files changed, 152 insertions(+), 35 deletions(-) diff --git a/e2e/internal/testutil/mcp.go b/e2e/internal/testutil/mcp.go index 3e6222529..0ef1e3d6d 100644 --- a/e2e/internal/testutil/mcp.go +++ b/e2e/internal/testutil/mcp.go @@ -204,6 +204,20 @@ func (mc *MCPClient) CallTool(toolName string, args map[string]any) *MCPToolResu return &toolResult } +// CallToolExpectToolError invokes an MCP tool and expects a tool-level error +// (isError: true in the result). It returns the error text content. +func (mc *MCPClient) CallToolExpectToolError(toolName string, args map[string]any) string { + tr := mc.CallTool(toolName, args) + require.True(mc.t, tr.IsError, "expected tool %s to return isError", toolName) + require.NotEmpty(mc.t, tr.Content, "tool %s returned no content", toolName) + + var text string + err := json.Unmarshal(tr.Content[0].Text, &text) + require.NoError(mc.t, err, "cannot unmarshal error text for %s", toolName) + + return text +} + // CallToolInto invokes an MCP tool and unmarshals the first text content into dest. func (mc *MCPClient) CallToolInto(toolName string, args map[string]any, dest any) { tr := mc.CallTool(toolName, args) diff --git a/e2e/mcp/asset_test.go b/e2e/mcp/asset_test.go index 8c0bcc531..55fe8c17c 100644 --- a/e2e/mcp/asset_test.go +++ b/e2e/mcp/asset_test.go @@ -92,4 +92,27 @@ func TestMCP_Asset_CRUD(t *testing.T) { "id": addResult.Asset.ID, }, &deleteResult) assert.Equal(t, addResult.Asset.ID, deleteResult.DeletedAssetID) + + // Get deleted asset returns sanitized not-found error + msg := mc.CallToolExpectToolError("getAsset", map[string]any{ + "id": addResult.Asset.ID, + }) + assert.Equal(t, "resource not found", msg) +} + +func TestMCP_Asset_PermissionDenied(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + orgID := owner.GetOrganizationID().String() + viewer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + viewerMC := testutil.NewMCPClient(t, viewer) + + msg := viewerMC.CallToolExpectToolError("addAsset", map[string]any{ + "organizationId": orgID, + "name": factory.SafeName("Asset"), + "amount": 1, + "assetType": "VIRTUAL", + "dataTypesStored": "PII", + }) + assert.Contains(t, msg, "permission denied") } diff --git a/e2e/mcp/risk_test.go b/e2e/mcp/risk_test.go index 2e2b955e6..1fe4686b4 100644 --- a/e2e/mcp/risk_test.go +++ b/e2e/mcp/risk_test.go @@ -86,4 +86,24 @@ func TestMCP_Risk_CRUD(t *testing.T) { "id": addResult.Risk.ID, }, &deleteResult) assert.Equal(t, addResult.Risk.ID, deleteResult.DeletedRiskID) + + // Get deleted risk returns sanitized not-found error + msg := mc.CallToolExpectToolError("getRisk", map[string]any{ + "id": addResult.Risk.ID, + }) + assert.Equal(t, "resource not found", msg) +} + +func TestMCP_Risk_PermissionDenied(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + orgID := owner.GetOrganizationID().String() + viewer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + viewerMC := testutil.NewMCPClient(t, viewer) + + msg := viewerMC.CallToolExpectToolError("addRisk", map[string]any{ + "organizationId": orgID, + "name": factory.SafeName("Risk"), + }) + assert.Contains(t, msg, "permission denied") } diff --git a/e2e/mcp/vendor_test.go b/e2e/mcp/vendor_test.go index b6a6ecda3..35c1efdb5 100644 --- a/e2e/mcp/vendor_test.go +++ b/e2e/mcp/vendor_test.go @@ -76,4 +76,42 @@ func TestMCP_Vendor_CRUD(t *testing.T) { "id": addResult.Vendor.ID, }, &deleteResult) assert.Equal(t, addResult.Vendor.ID, deleteResult.DeletedVendorID) + + // Update deleted vendor returns sanitized not-found error + msg := mc.CallToolExpectToolError("updateVendor", map[string]any{ + "id": addResult.Vendor.ID, + "name": "Should Fail", + }) + assert.Equal(t, "resource not found", msg) +} + +func TestMCP_Vendor_ValidationError(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + mc := testutil.NewMCPClient(t, owner) + orgID := owner.GetOrganizationID().String() + + msg := mc.CallToolExpectToolError("addVendor", map[string]any{ + "organizationId": orgID, + "name": "", + }) + assert.Contains(t, msg, "name") + assert.NotContains(t, msg, "pq:") + assert.NotContains(t, msg, "sql:") +} + +func TestMCP_Vendor_PermissionDenied(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + orgID := owner.GetOrganizationID().String() + viewer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + viewerMC := testutil.NewMCPClient(t, viewer) + + msg := viewerMC.CallToolExpectToolError("addVendor", map[string]any{ + "organizationId": orgID, + "name": factory.SafeName("Vendor"), + }) + assert.Contains(t, msg, "permission denied") + assert.NotContains(t, msg, "pq:") + assert.NotContains(t, msg, "sql:") } diff --git a/go.mod b/go.mod index 03bfb5eb5..76f73f25c 100644 --- a/go.mod +++ b/go.mod @@ -39,7 +39,7 @@ require ( go.gearno.de/x/ref v0.0.0-20260216110753-a700c951377c go.opentelemetry.io/otel v1.43.0 go.opentelemetry.io/otel/trace v1.43.0 - go.probo.inc/mcpgen v0.0.0-20260223192226-386b1fbe6184 + go.probo.inc/mcpgen v0.0.0-20260428172408-1496ba9b4619 golang.org/x/crypto v0.50.0 golang.org/x/image v0.39.0 golang.org/x/oauth2 v0.36.0 diff --git a/go.sum b/go.sum index d1ec0c9c3..d1896db5c 100644 --- a/go.sum +++ b/go.sum @@ -372,8 +372,8 @@ go.opentelemetry.io/otel/trace v1.43.0 h1:BkNrHpup+4k4w+ZZ86CZoHHEkohws8AY+WTX09 go.opentelemetry.io/otel/trace v1.43.0/go.mod h1:/QJhyVBUUswCphDVxq+8mld+AvhXZLhe+8WVFxiFff0= go.opentelemetry.io/proto/otlp v1.10.0 h1:IQRWgT5srOCYfiWnpqUYz9CVmbO8bFmKcwYxpuCSL2g= go.opentelemetry.io/proto/otlp v1.10.0/go.mod h1:/CV4QoCR/S9yaPj8utp3lvQPoqMtxXdzn7ozvvozVqk= -go.probo.inc/mcpgen v0.0.0-20260223192226-386b1fbe6184 h1:LH9uUR10Nt6ixEAtr7M8Drg97Ei519WEWM1mSWLkrhQ= -go.probo.inc/mcpgen v0.0.0-20260223192226-386b1fbe6184/go.mod h1:HunWQGqLdMocExJh4tWaX7p+uRZ9GlKvBvOXHaFW6vM= +go.probo.inc/mcpgen v0.0.0-20260428172408-1496ba9b4619 h1:LHOdoF7kYRXFtSP97eWpF1dIf0dBLrunRLOeU/pXt9c= +go.probo.inc/mcpgen v0.0.0-20260428172408-1496ba9b4619/go.mod h1:HunWQGqLdMocExJh4tWaX7p+uRZ9GlKvBvOXHaFW6vM= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.yaml.in/yaml/v2 v2.4.4 h1:tuyd0P+2Ont/d6e2rl3be67goVK4R6deVxCUX5vyPaQ= diff --git a/pkg/server/api/mcp/mcputils/recovery.go b/pkg/server/api/mcp/mcputils/recovery.go index 78210e4bc..39fad2729 100644 --- a/pkg/server/api/mcp/mcputils/recovery.go +++ b/pkg/server/api/mcp/mcputils/recovery.go @@ -20,49 +20,73 @@ import ( "fmt" "runtime/debug" - "github.com/modelcontextprotocol/go-sdk/mcp" "go.gearno.de/kit/log" + mcpgenmcp "go.probo.inc/mcpgen/mcp" + "go.probo.inc/probo/pkg/coredata" "go.probo.inc/probo/pkg/iam" + "go.probo.inc/probo/pkg/validator" ) -func RecoveryMiddleware(logger *log.Logger) func(mcp.MethodHandler) mcp.MethodHandler { - return func(next mcp.MethodHandler) mcp.MethodHandler { - return func(ctx context.Context, method string, req mcp.Request) (result mcp.Result, err error) { - defer func() { - if r := recover(); r != nil { - err = convertPanicToError(ctx, logger, r) - result = nil - } - }() - - result, err = next(ctx, method, req) - return +// NewRecoverFunc returns a RecoverFunc for the generated MCP server that +// classifies panics into safe client-facing errors and logs unknown errors. +func NewRecoverFunc(logger *log.Logger) mcpgenmcp.RecoverFunc { + return func(ctx context.Context, r any) error { + if r == nil { + logger.ErrorCtx(ctx, "nil panic in MCP tool handler") + return fmt.Errorf("internal server error") } - } -} -func convertPanicToError(ctx context.Context, logger *log.Logger, panicValue any) error { - if panicValue == nil { - logger.ErrorCtx(ctx, "nil panic in MCP method handler") + if err, ok := r.(error); ok { + return sanitizeError(ctx, logger, err) + } + + logger.ErrorCtx( + ctx, + "unexpected panic in MCP tool handler", + log.Any("panic", r), + log.String("stack", string(debug.Stack())), + ) + return fmt.Errorf("internal server error") } +} +// sanitizeError classifies known error types and returns a clear message for +// those. Unknown errors are logged and replaced with a generic internal error +// to avoid leaking implementation details to the client. +func sanitizeError(ctx context.Context, logger *log.Logger, err error) error { var permissionDeniedErr *iam.ErrInsufficientPermissions - if errTyped, ok := panicValue.(error); ok && errors.As(errTyped, &permissionDeniedErr) { - return fmt.Errorf("permission denied: %s", permissionDeniedErr.Error()) + if errors.As(err, &permissionDeniedErr) { + return fmt.Errorf("permission denied") } - if err, ok := panicValue.(error); ok { - return err + var assumptionRequiredErr *iam.ErrAssumptionRequired + if errors.As(err, &assumptionRequiredErr) { + return fmt.Errorf("assumption required") } - // Log unexpected panics with stack trace - logger.ErrorCtx( - ctx, - "unexpected panic in MCP method handler", - log.Any("panic", panicValue), - log.String("stack", string(debug.Stack())), - ) + if errors.Is(err, coredata.ErrResourceNotFound) { + return fmt.Errorf("resource not found") + } + if errors.Is(err, coredata.ErrResourceAlreadyExists) { + return fmt.Errorf("resource already exists") + } + + if errors.Is(err, coredata.ErrResourceInUse) { + return fmt.Errorf("resource is in use") + } + + var validationErrors validator.ValidationErrors + if errors.As(err, &validationErrors) { + return validationErrors + } + + var validationError *validator.ValidationError + if errors.As(err, &validationError) { + return validationError + } + + logger.ErrorCtx(ctx, "internal error in MCP tool handler", log.Error(err)) return fmt.Errorf("internal server error") } diff --git a/pkg/server/api/mcp/v1/v1_handler.go b/pkg/server/api/mcp/v1/v1_handler.go index edd3a5037..ba6d2ba0e 100644 --- a/pkg/server/api/mcp/v1/v1_handler.go +++ b/pkg/server/api/mcp/v1/v1_handler.go @@ -50,11 +50,9 @@ func NewMux(logger *log.Logger, proboSvc *probo.Service, iamSvc *iam.Service, ac logger: logger, } - mcpServer := server.New(resolver) + mcpServer := server.New(resolver, mcpgenmcp.WithRecoverFunc(mcputils.NewRecoverFunc(logger))) - // Add panic recovery middleware to handle panics in goroutines spawned by MCP SDK mcpServer.AddReceivingMiddleware(mcputils.LoggingMiddleware(logger)) - mcpServer.AddReceivingMiddleware(mcputils.RecoveryMiddleware(logger)) getServer := func(r *http.Request) *mcp.Server { return mcpServer } eventStore := mcp.NewMemoryEventStore(nil)