Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/features/output-sanitisation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
74 changes: 74 additions & 0 deletions internal/server/direct_output_sanitisation_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
16 changes: 16 additions & 0 deletions internal/server/mcp_routing.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions internal/server/scope_fixture_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
})
}
Expand Down
Loading