From 5b4f3d575aee85e32b4e4537d8db48f7c972ec5b Mon Sep 17 00:00:00 2001 From: Andy Lo-A-Foe Date: Tue, 5 May 2026 19:50:37 +0200 Subject: [PATCH] fix(mcp): add allowlist and validation for dynamic headers Addresses security concerns raised in PR review: 1. Dynamic headers now require explicit allowlist per server: - New `dynamic_headers.allowed` config field lists permitted header names - Without allowlist, no dynamic headers are forwarded (secure by default) - Header names matched case-insensitively 2. Add limits to prevent abuse: - `max_count`: max headers per request (default: 10) - `max_value_len`: max value length (default: 4096) 3. Protect internal metadata in pico channel: - Reserved keys (platform, session_id, conn_id, etc.) cannot be overwritten by client-provided metadata Example config: ```json "dynamic_headers": { "allowed": ["X-Grafana-Service-Account-Token"], "max_count": 5, "max_value_len": 2048 } ``` Co-Authored-By: Claude Opus 4.5 --- pkg/channels/pico/pico.go | 20 ++++ pkg/channels/pico/pico_test.go | 29 +++++ pkg/config/config.go | 32 ++++++ pkg/mcp/manager.go | 46 +++++++- pkg/mcp/manager_test.go | 195 +++++++++++++++++++++++++++++++++ 5 files changed, 317 insertions(+), 5 deletions(-) diff --git a/pkg/channels/pico/pico.go b/pkg/channels/pico/pico.go index 87e13f701..14add26e9 100644 --- a/pkg/channels/pico/pico.go +++ b/pkg/channels/pico/pico.go @@ -44,6 +44,23 @@ var allowedInlineImageMIMETypes = map[string]struct{}{ "image/bmp": {}, } +// reservedRawKeys are internal metadata keys that cannot be overwritten by client-provided metadata. +// This prevents clients from spoofing internal identity or session information. +var reservedRawKeys = map[string]struct{}{ + "platform": {}, + "session_id": {}, + "conn_id": {}, + "message_id": {}, + "sender_id": {}, + "chat_id": {}, + "message_kind": {}, +} + +func isReservedRawKey(key string) bool { + _, reserved := reservedRawKeys[key] + return reserved +} + func outboundMessageIsThought(msg bus.OutboundMessage) bool { if len(msg.Context.Raw) == 0 { return false @@ -951,6 +968,9 @@ func (c *PicoChannel) handleMessageSend(pc *picoConn, msg PicoMessage) { if payloadMeta, ok := msg.Payload["metadata"].(map[string]any); ok { for k, v := range payloadMeta { if s, ok := v.(string); ok && s != "" { + if isReservedRawKey(k) { + continue + } metadata[k] = s } } diff --git a/pkg/channels/pico/pico_test.go b/pkg/channels/pico/pico_test.go index bbe73a222..73d6f2ed5 100644 --- a/pkg/channels/pico/pico_test.go +++ b/pkg/channels/pico/pico_test.go @@ -547,3 +547,32 @@ func newTestPicoWebSocket(t *testing.T) (*websocket.Conn, <-chan PicoMessage, fu defer resp.Body.Close() return clientConn, received, cleanup } + +func TestIsReservedRawKey(t *testing.T) { + reserved := []string{ + "platform", + "session_id", + "conn_id", + "message_id", + "sender_id", + "chat_id", + "message_kind", + } + for _, key := range reserved { + if !isReservedRawKey(key) { + t.Errorf("isReservedRawKey(%q) = false, want true", key) + } + } + + allowed := []string{ + "mcp:Authorization", + "mcp:X-Custom-Header", + "custom_field", + "user_data", + } + for _, key := range allowed { + if isReservedRawKey(key) { + t.Errorf("isReservedRawKey(%q) = true, want false", key) + } + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index c9d90e0f8..fae692bfb 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -953,6 +953,35 @@ func (c *SkillRegistryConfig) DecodeParam(target any) error { return json.Unmarshal(data, target) } +// DynamicHeadersConfig controls which headers can be forwarded from channel context to MCP servers. +// By default (when nil or empty), no dynamic headers are allowed - this is secure by default. +type DynamicHeadersConfig struct { + // Allowed is a list of header names that may be forwarded from channel context. + // Only headers explicitly listed here will be passed to this MCP server. + // Header names are matched case-insensitively. + Allowed []string `json:"allowed,omitempty"` + // MaxCount limits the number of dynamic headers per request (default: 10). + MaxCount int `json:"max_count,omitempty"` + // MaxValueLen limits the maximum length of each header value (default: 4096). + MaxValueLen int `json:"max_value_len,omitempty"` +} + +// GetMaxCount returns the max header count, defaulting to 10. +func (c *DynamicHeadersConfig) GetMaxCount() int { + if c == nil || c.MaxCount <= 0 { + return 10 + } + return c.MaxCount +} + +// GetMaxValueLen returns the max value length, defaulting to 4096. +func (c *DynamicHeadersConfig) GetMaxValueLen() int { + if c == nil || c.MaxValueLen <= 0 { + return 4096 + } + return c.MaxValueLen +} + // MCPServerConfig defines configuration for a single MCP server type MCPServerConfig struct { // Enabled indicates whether this MCP server is active @@ -975,6 +1004,9 @@ type MCPServerConfig struct { URL string `json:"url,omitempty"` // Headers are HTTP headers to send with requests (sse/http only) Headers map[string]string `json:"headers,omitempty"` + // DynamicHeaders controls which headers from channel context can be forwarded to this server. + // When nil or empty, no dynamic headers are allowed (secure by default). + DynamicHeaders *DynamicHeadersConfig `json:"dynamic_headers,omitempty"` } // MCPConfig defines configuration for all MCP servers diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index ef51544f2..bd9194fb9 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -23,8 +23,9 @@ import ( // headerTransport is an http.RoundTripper that adds custom headers to requests type headerTransport struct { - base http.RoundTripper - headers map[string]string + base http.RoundTripper + headers map[string]string + dynamicHeaders *config.DynamicHeadersConfig } func expandHomeCommandPath(command string) string { @@ -51,7 +52,8 @@ func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { req.Header.Set(key, value) } if dynamic := toolshared.MCPHeaders(req.Context()); len(dynamic) > 0 { - for key, value := range dynamic { + filtered := t.filterDynamicHeaders(dynamic) + for key, value := range filtered { req.Header.Set(key, value) } } @@ -62,6 +64,39 @@ func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { return base.RoundTrip(req) } +// filterDynamicHeaders applies the allowlist and limits from DynamicHeadersConfig. +// Returns nil if no dynamic headers config is set (secure by default). +func (t *headerTransport) filterDynamicHeaders(headers map[string]string) map[string]string { + if t.dynamicHeaders == nil || len(t.dynamicHeaders.Allowed) == 0 { + return nil + } + + allowedSet := make(map[string]struct{}, len(t.dynamicHeaders.Allowed)) + for _, h := range t.dynamicHeaders.Allowed { + allowedSet[strings.ToLower(h)] = struct{}{} + } + + maxCount := t.dynamicHeaders.GetMaxCount() + maxValueLen := t.dynamicHeaders.GetMaxValueLen() + + result := make(map[string]string) + count := 0 + for key, value := range headers { + if count >= maxCount { + break + } + if _, ok := allowedSet[strings.ToLower(key)]; !ok { + continue + } + if len(value) > maxValueLen { + value = value[:maxValueLen] + } + result[key] = value + count++ + } + return result +} + // loadEnvFile loads environment variables from a file in .env format // Each line should be in the format: KEY=value // Lines starting with # are comments @@ -384,8 +419,9 @@ func connectServer( sseTransport.HTTPClient = &http.Client{ Transport: &headerTransport{ - base: http.DefaultTransport, - headers: cfg.Headers, + base: http.DefaultTransport, + headers: cfg.Headers, + dynamicHeaders: cfg.DynamicHeaders, }, } if len(cfg.Headers) > 0 { diff --git a/pkg/mcp/manager_test.go b/pkg/mcp/manager_test.go index 79e91fb29..b11df13e8 100644 --- a/pkg/mcp/manager_test.go +++ b/pkg/mcp/manager_test.go @@ -643,6 +643,9 @@ func TestHeaderTransport_DynamicHeaders(t *testing.T) { transport := &headerTransport{ base: base, headers: map[string]string{"X-Static": "from-config"}, + dynamicHeaders: &config.DynamicHeadersConfig{ + Allowed: []string{"Authorization", "X-Custom"}, + }, } ctx := toolshared.WithMCPHeaders(context.Background(), map[string]string{ @@ -680,6 +683,9 @@ func TestHeaderTransport_DynamicOverridesStatic(t *testing.T) { transport := &headerTransport{ base: base, headers: map[string]string{"Authorization": "Bearer static"}, + dynamicHeaders: &config.DynamicHeadersConfig{ + Allowed: []string{"Authorization"}, + }, } ctx := toolshared.WithMCPHeaders(context.Background(), map[string]string{ @@ -727,6 +733,195 @@ func TestHeaderTransport_NoDynamicHeaders(t *testing.T) { } } +func TestHeaderTransport_NoAllowlist_BlocksDynamic(t *testing.T) { + captured := make(http.Header) + base := roundTripFunc(func(req *http.Request) (*http.Response, error) { + for k, v := range req.Header { + captured[k] = v + } + return &http.Response{StatusCode: 200, Body: http.NoBody}, nil + }) + + // No dynamicHeaders config = secure by default, blocks all dynamic headers + transport := &headerTransport{ + base: base, + headers: map[string]string{"X-Static": "val"}, + } + + ctx := toolshared.WithMCPHeaders(context.Background(), map[string]string{ + "Authorization": "Bearer should-be-blocked", + "X-Custom": "also-blocked", + }) + req, _ := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", nil) + + resp, err := transport.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip() error = %v", err) + } + resp.Body.Close() + + if got := captured.Get("X-Static"); got != "val" { + t.Errorf("X-Static = %q, want %q", got, "val") + } + if got := captured.Get("Authorization"); got != "" { + t.Errorf("Authorization should be blocked without allowlist, got %q", got) + } + if got := captured.Get("X-Custom"); got != "" { + t.Errorf("X-Custom should be blocked without allowlist, got %q", got) + } +} + +func TestHeaderTransport_AllowlistFilters(t *testing.T) { + captured := make(http.Header) + base := roundTripFunc(func(req *http.Request) (*http.Response, error) { + for k, v := range req.Header { + captured[k] = v + } + return &http.Response{StatusCode: 200, Body: http.NoBody}, nil + }) + + transport := &headerTransport{ + base: base, + dynamicHeaders: &config.DynamicHeadersConfig{ + Allowed: []string{"X-Allowed-Header"}, + }, + } + + ctx := toolshared.WithMCPHeaders(context.Background(), map[string]string{ + "X-Allowed-Header": "this-passes", + "X-Blocked-Header": "this-is-blocked", + "Authorization": "also-blocked", + }) + req, _ := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", nil) + + resp, err := transport.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip() error = %v", err) + } + resp.Body.Close() + + if got := captured.Get("X-Allowed-Header"); got != "this-passes" { + t.Errorf("X-Allowed-Header = %q, want %q", got, "this-passes") + } + if got := captured.Get("X-Blocked-Header"); got != "" { + t.Errorf("X-Blocked-Header should be blocked, got %q", got) + } + if got := captured.Get("Authorization"); got != "" { + t.Errorf("Authorization should be blocked, got %q", got) + } +} + +func TestHeaderTransport_AllowlistCaseInsensitive(t *testing.T) { + captured := make(http.Header) + base := roundTripFunc(func(req *http.Request) (*http.Response, error) { + for k, v := range req.Header { + captured[k] = v + } + return &http.Response{StatusCode: 200, Body: http.NoBody}, nil + }) + + transport := &headerTransport{ + base: base, + dynamicHeaders: &config.DynamicHeadersConfig{ + Allowed: []string{"X-Grafana-Token"}, // lowercase in allowlist + }, + } + + ctx := toolshared.WithMCPHeaders(context.Background(), map[string]string{ + "x-grafana-token": "token-value", // different case in request + }) + req, _ := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", nil) + + resp, err := transport.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip() error = %v", err) + } + resp.Body.Close() + + if got := captured.Get("X-Grafana-Token"); got != "token-value" { + t.Errorf("X-Grafana-Token = %q, want %q (case-insensitive match)", got, "token-value") + } +} + +func TestHeaderTransport_MaxCountLimit(t *testing.T) { + captured := make(http.Header) + base := roundTripFunc(func(req *http.Request) (*http.Response, error) { + for k, v := range req.Header { + captured[k] = v + } + return &http.Response{StatusCode: 200, Body: http.NoBody}, nil + }) + + transport := &headerTransport{ + base: base, + dynamicHeaders: &config.DynamicHeadersConfig{ + Allowed: []string{"X-Header-1", "X-Header-2", "X-Header-3"}, + MaxCount: 2, + }, + } + + ctx := toolshared.WithMCPHeaders(context.Background(), map[string]string{ + "X-Header-1": "val1", + "X-Header-2": "val2", + "X-Header-3": "val3", + }) + req, _ := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", nil) + + resp, err := transport.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip() error = %v", err) + } + resp.Body.Close() + + // Only 2 headers should be set (MaxCount=2), but we can't predict which due to map iteration order + count := 0 + for _, h := range []string{"X-Header-1", "X-Header-2", "X-Header-3"} { + if captured.Get(h) != "" { + count++ + } + } + if count != 2 { + t.Errorf("Expected exactly 2 headers due to MaxCount=2, got %d", count) + } +} + +func TestHeaderTransport_MaxValueLenLimit(t *testing.T) { + captured := make(http.Header) + base := roundTripFunc(func(req *http.Request) (*http.Response, error) { + for k, v := range req.Header { + captured[k] = v + } + return &http.Response{StatusCode: 200, Body: http.NoBody}, nil + }) + + transport := &headerTransport{ + base: base, + dynamicHeaders: &config.DynamicHeadersConfig{ + Allowed: []string{"X-Long-Header"}, + MaxValueLen: 10, + }, + } + + ctx := toolshared.WithMCPHeaders(context.Background(), map[string]string{ + "X-Long-Header": "this-value-is-way-too-long-and-should-be-truncated", + }) + req, _ := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", nil) + + resp, err := transport.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip() error = %v", err) + } + resp.Body.Close() + + got := captured.Get("X-Long-Header") + if len(got) != 10 { + t.Errorf("X-Long-Header length = %d, want 10 (truncated)", len(got)) + } + if got != "this-value" { + t.Errorf("X-Long-Header = %q, want %q", got, "this-value") + } +} + type roundTripFunc func(*http.Request) (*http.Response, error) func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {