rohit/probo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Sanitize MCP errors to avoid leaking internal details

Browse Source 
Signed-off-by: Sacha Al Himdani <sacha@getprobo.com>
This commit is contained in:
Sacha Al Himdani
2026-04-23 11:35:00 +02:00
parent 4f88b15f6d
commit d82df6b8ee
8 changed files with 152 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
e2e/internal/testutil/mcp.go
View File

@@ -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)

23
e2e/mcp/asset_test.go
View File

@@ -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")
}

20
e2e/mcp/risk_test.go
View File

@@ -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")
}

38
e2e/mcp/vendor_test.go
View File

@@ -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:")
}

2
go.mod
View File

@@ -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

4
go.sum
View File

@@ -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=

82
pkg/server/api/mcp/mcputils/recovery.go
View File

@@ -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")
}

4
pkg/server/api/mcp/v1/v1_handler.go
View File

@@ -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)