From 5e84a4143c0c4756d289271fa1e2634aacdb03c4 Mon Sep 17 00:00:00 2001 From: james-crowley <42618816+james-crowley@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:22:30 -0400 Subject: [PATCH 1/2] Apply output_sanitisation to direct routing mode makeDirectModeHandler forwarded upstream results without calling applyOutputSanitisation or spotlightForwarded, so redact, block, strip_control_chars and spotlight_untrusted never applied to /mcp/all. Call both from the direct handler the same way the call_tool_* handlers do: sanitise the raw result before the activity classification and truncation, and spotlight after truncation, refreshing the recorded response text. Fixes #1683 --- .../server/direct_output_sanitisation_test.go | 74 +++++++++++++++++++ internal/server/mcp_routing.go | 16 ++++ internal/server/scope_fixture_test.go | 6 ++ 3 files changed, 96 insertions(+) create mode 100644 internal/server/direct_output_sanitisation_test.go diff --git a/internal/server/direct_output_sanitisation_test.go b/internal/server/direct_output_sanitisation_test.go new file mode 100644 index 000000000..9376e6657 --- /dev/null +++ b/internal/server/direct_output_sanitisation_test.go @@ -0,0 +1,74 @@ +package server + +import ( + "context" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/security" +) + +// Issue #1683: direct routing mode (/mcp/all, server__tool names) is the fifth +// upstream dispatch path and must honour output_sanitisation like call_tool_* +// and code_execution do. Each test drives the handler mcp-go actually +// registered (directRoutingFixture) against a real in-process upstream that +// returns a secret. + +func directSanitisationFixture(t *testing.T, cfg *config.OutputSanitisationConfig, body string) *directRoutingFixture { + t.Helper() + f := newDirectRoutingFixture(t, directServerSpec{server: "github", tools: []toolSpec{readSpec("get_secret")}}) + f.proxy.config.OutputSanitisation = cfg + f.proxy.sanitisationDetector = security.NewDetector(config.DefaultSensitiveDataDetectionConfig()) + f.ups["github"].okBody.Store(&body) + return f +} + +func directText(t *testing.T, r *mcp.CallToolResult) string { + t.Helper() + require.NotEmpty(t, r.Content) + return r.Content[0].(mcp.TextContent).Text +} + +func TestDirectModeSanitisation_RedactMasksSecret(t *testing.T) { + f := directSanitisationFixture(t, sanCfg("redact", false, false), "token "+awsKeyFixture+" end") + res := f.call(t, context.Background(), "github", "get_secret") + require.False(t, res.IsError, directText(t, res)) + assert.NotContains(t, directText(t, res), awsKeyFixture, "the agent must not receive the raw secret") + assert.Contains(t, directText(t, res), "token ", "non-secret text survives") +} + +func TestDirectModeSanitisation_BlockRefusesCriticalResponse(t *testing.T) { + f := directSanitisationFixture(t, sanCfg("block", false, false), awsKeyFixture) + res := f.call(t, context.Background(), "github", "get_secret") + assert.True(t, res.IsError, "a critical detection in block mode must block") + assert.Contains(t, directText(t, res), "blocked by sanitisation policy") + assert.NotContains(t, directText(t, res), awsKeyFixture) +} + +func TestDirectModeSanitisation_SpotlightWrapsUntrustedText(t *testing.T) { + f := directSanitisationFixture(t, sanCfg("spotlight", true, false), "hello") + res := f.call(t, context.Background(), "github", "get_secret") + require.False(t, res.IsError, directText(t, res)) + text := directText(t, res) + assert.Contains(t, text, "untrusted:github/get_secret") + assert.Contains(t, text, "hello") +} + +func TestDirectModeSanitisation_StripRemovesControlSequences(t *testing.T) { + f := directSanitisationFixture(t, sanCfg("spotlight", false, true), "plain\x1b[31mred\x1b[0m") + res := f.call(t, context.Background(), "github", "get_secret") + require.False(t, res.IsError, directText(t, res)) + assert.NotContains(t, directText(t, res), "\x1b[") +} + +func TestDirectModeSanitisation_DefaultOptOutForwardsUnchanged(t *testing.T) { + f := directSanitisationFixture(t, config.DefaultOutputSanitisationConfig(), "token "+awsKeyFixture) + res := f.call(t, context.Background(), "github", "get_secret") + require.False(t, res.IsError, directText(t, res)) + assert.Equal(t, "token "+awsKeyFixture, directText(t, res), + "with no output_sanitisation configured the response is forwarded byte for byte") +} diff --git a/internal/server/mcp_routing.go b/internal/server/mcp_routing.go index e451c4688..8d5ea70a8 100644 --- a/internal/server/mcp_routing.go +++ b/internal/server/mcp_routing.go @@ -739,6 +739,15 @@ func (p *MCPProxyServer) makeDirectModeHandler(entry *directCatalogEntry) mcpser return mcp.NewToolResultError(fmt.Sprintf("Error calling %s:%s: %v", serverName, toolName, err)), nil } + // Spec 054 Track B (pre-forward): redact secrets, strip control sequences + // or block on a critical detection, exactly as the call_tool_* handlers + // do. Direct mode previously skipped this, so output_sanitisation never + // applied to /mcp/all. Applied to the RAW result before the activity + // classification and truncation below; a non-nil result means blocked. + if blockResult := p.applyOutputSanitisation(ctx, serverName, toolName, requestID, directContentTrust, result); blockResult != nil { + return blockResult, nil + } + // Determine tool variant for activity logging toolVariant := contracts.DeriveCallWith(annotations) @@ -812,6 +821,13 @@ func (p *MCPProxyServer) makeDirectModeHandler(entry *directCatalogEntry) mcpser forwarded = mcp.NewToolResultText(responseText) } + // Spec 054 Track B (post-forward): spotlight untrusted text in + // source-identifying delimiters. Lossless, applied after truncation; + // responseText is refreshed so the activity record matches what the + // agent receives. + p.spotlightForwarded(serverName, toolName, directContentTrust, forwarded) + responseText = forwardedText(forwarded, responseText) + // Emit completion activity (success, or error when the upstream itself // reported one — issue #935). // Spec 069 A1: pre-truncation sizes; result was measured before the truncation loop above. diff --git a/internal/server/scope_fixture_test.go b/internal/server/scope_fixture_test.go index 54c1bfcea..adb45ef0a 100644 --- a/internal/server/scope_fixture_test.go +++ b/internal/server/scope_fixture_test.go @@ -102,6 +102,9 @@ type countingUpstream struct { // errBody, when set, makes every tool answer an upstream isError result // carrying that text (an approved upstream failing on its own). errBody atomic.Pointer[string] + // okBody, when set, replaces the default "ok" text of a successful answer + // (for tests that need the upstream to return a particular payload). + okBody atomic.Pointer[string] } // serve registers one more tool on the running stub upstream, for tests @@ -130,6 +133,9 @@ func (u *countingUpstream) serve(spec toolSpec) { if body := u.errBody.Load(); body != nil { return mcp.NewToolResultError(*body), nil } + if body := u.okBody.Load(); body != nil { + return mcp.NewToolResultText(*body), nil + } return mcp.NewToolResultText("ok"), nil }) } From b91c6c1cf803729262625482d4e74b3bcfb5e000 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 20 Oct 2026 07:44:54 +0300 Subject: [PATCH 2/2] docs(output-sanitisation): direct routing mode is now covered Direct mode (/mcp/all, server__tool) now runs redact/block/strip/spotlight (#1683), so drop it from the not-covered list. --- docs/features/output-sanitisation.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/features/output-sanitisation.md b/docs/features/output-sanitisation.md index 0b6433034..ac05ae5db 100644 --- a/docs/features/output-sanitisation.md +++ b/docs/features/output-sanitisation.md @@ -37,7 +37,8 @@ Spotlighting wraps untrusted text as: ### Scope and limits - **Default behavior:** nothing is redacted or blocked. Sensitive-data detection still runs and flags records in the activity log, but it does so after the call is recorded and does not change what the agent receives. -- **Two paths are not covered.** (1) **Direct routing mode** (`routing_mode: direct`, tools exposed as `server__tool`) never invokes sanitisation, so `redact` and `block` do not apply to those calls; a matching secret in a direct-mode text response is forwarded unchanged (detection still flags it in the activity log). Sanitisation applies to the `call_tool_*` wrapper path and to `code_execution` tool calls. (2) **Upstream error responses** (`isError: true`) bypass `redact` and `block`, so a secret echoed in an upstream error result is forwarded as-is, even with `response_action: block`. +- **Covered paths.** Sanitisation applies to the `call_tool_*` wrapper path, to direct routing mode (`/mcp/all`, or `routing_mode: direct`, with tools exposed as `server__tool`), and to `code_execution` tool calls. +- **Upstream error responses are not covered.** Upstream error responses (`isError: true`) bypass `redact` and `block`, so a secret echoed in an upstream error result is forwarded as-is, even with `response_action: block`. - **Text responses only.** `redact` and `block` act on text content in successful tool responses on the covered paths. Image, audio and embedded-resource content is never modified, and tool **arguments** sent to an upstream server are not covered. - **Pattern-based.** Only values matching the detector's patterns are redacted, and `block` triggers only on a **critical** detection. Redaction replaces the matched span; a multi-line secret such as a PEM private key may leave surrounding lines (in a local check with a synthetic key, only the `BEGIN` line was replaced). Well-known documentation example values (for instance the AWS documentation example access key) are classed as likely examples by the detector and were forwarded unredacted in a local check, while the accompanying secret key was masked. Treat redaction as risk reduction, not a guarantee that no secret reaches the agent. - **A canary check you can run locally:** point a scratch instance at a fixture server that returns a fake AWS key or private key and compare the response under the default config, `response_action: redact` and `response_action: block`. Use fake values only.