From 9e773f08c9de7b792cde10047c028190d60bf905 Mon Sep 17 00:00:00 2001 From: ashwinhegde19 Date: Fri, 10 Jul 2026 20:51:47 +0530 Subject: [PATCH] fix(config): enforce MCP trust boundary so project config cannot override user disable A user-level MCP server disable is now sticky. The project config (a cloned repo's ./.zero/config.json) cannot re-enable a server the user disabled, and cannot disable a server the user explicitly enabled. The previous merge just assigned base.Disabled = next.Disabled, so a repo that set disabled:true could silently defeat a user's explicit mcp enable . The guard is now symmetric via a mergeMCPServer canReenable flag: once a higher-trust scope has explicitly set the disabled flag, a lower-trust layer may not override that decision in either direction. Only the CLI override scope (canReenable=true) may lift a sticky user decision. The prior disabledSet-before-check ordering bug is also fixed: the higher-trust scope's prior disabledSet/Disabled is captured before folding in next's flag, so a project layer can still disable a default-enabled server (e.g. firecrawl) rather than being silently ignored. Ports the fix onto main's refactored MCP merge (internal/config/mcp_merge.go and mergeProjectMCPConfig), which splits project-scope merging from the general mergeMCPConfig path. Fixes #512 --- internal/config/mcp_merge.go | 38 +++++++-- internal/config/resolver.go | 11 ++- internal/config/resolver_test.go | 127 ++++++++++++++++++++++++++++++- 3 files changed, 163 insertions(+), 13 deletions(-) diff --git a/internal/config/mcp_merge.go b/internal/config/mcp_merge.go index c763f8c28..816ccb54c 100644 --- a/internal/config/mcp_merge.go +++ b/internal/config/mcp_merge.go @@ -6,7 +6,7 @@ import ( "strings" ) -func mergeMCPConfig(dst *MCPConfig, src MCPConfig) { +func mergeMCPConfig(dst *MCPConfig, src MCPConfig, canReenable bool) { if len(src.Servers) == 0 { return } @@ -14,10 +14,14 @@ func mergeMCPConfig(dst *MCPConfig, src MCPConfig) { dst.Servers = map[string]MCPServerConfig{} } for name, server := range src.Servers { - dst.Servers[name] = mergeMCPServer(dst.Servers[name], server) + dst.Servers[name] = mergeMCPServer(dst.Servers[name], server, canReenable) } } +// mergeProjectMCPConfig merges a project-scoped (lower-trust) MCP config into +// dst. It never re-enables a server the user explicitly disabled or disabled a +// server the user explicitly enabled: it merges with canReenable=false so the +// trust-boundary guard in mergeMCPServer keeps the user's decision authoritative. func mergeProjectMCPConfig(dst *MCPConfig, src MCPConfig) error { if len(src.Servers) == 0 { return nil @@ -27,7 +31,7 @@ func mergeProjectMCPConfig(dst *MCPConfig, src MCPConfig) error { } for name, server := range src.Servers { base := dst.Servers[name] - candidate := mergeMCPServer(base, server) + candidate := mergeMCPServer(base, server, false) if projectMCPServerTargetChanges(base, server) && hasInheritedMCPCredentialMaterial(server, candidate) { return fmt.Errorf("project MCP server %q cannot override target while inheriting user credentials; set headers/env/oauth explicitly or use a new server name", name) } @@ -37,7 +41,11 @@ func mergeProjectMCPConfig(dst *MCPConfig, src MCPConfig) error { return nil } -func mergeMCPServer(base MCPServerConfig, next MCPServerConfig) MCPServerConfig { +// mergeMCPServer merges a later config layer's MCP server into the base. The +// canReenable flag marks the CLI-override scope, the only layer that is allowed +// to lift a sticky user-level disable/enable decision (see the disabled-handling +// block). +func mergeMCPServer(base MCPServerConfig, next MCPServerConfig, canReenable bool) MCPServerConfig { if strings.TrimSpace(next.Type) != "" { base.Type = next.Type } @@ -62,8 +70,26 @@ func mergeMCPServer(base MCPServerConfig, next MCPServerConfig) MCPServerConfig if next.OAuth != nil { base.OAuth = next.OAuth } + // Capture the higher-trust scope's prior decision before folding in + // next.disabledSet. The trust boundary must be evaluated against the state + // the higher-trust layer actually left behind, not against next's own flag. + baseDisabledSet := base.disabledSet + baseDisabled := base.Disabled + if next.disabledSet { + base.disabledSet = true + } if next.disabledSet || next.Disabled { - base.Disabled = next.Disabled + // Trust boundary: once a higher-trust scope has explicitly set the + // disabled flag, a lower-trust layer (project config) cannot override + // that decision in either direction — the user's choice (whether to + // enable or disable a server) wins over the repo. This blocks a repo + // from re-enabling a server the user disabled, and from disabling a + // server the user explicitly enabled. The only layer allowed to + // override an explicit higher-scope decision is the CLI override scope + // (canReenable=true). + if canReenable || !(baseDisabledSet && baseDisabled != next.Disabled) { + base.Disabled = next.Disabled + } } if next.configured { base.configured = true @@ -75,7 +101,7 @@ func mergeMCPServer(base MCPServerConfig, next MCPServerConfig) MCPServerConfig } func projectMCPServerTargetChanges(base MCPServerConfig, next MCPServerConfig) bool { - if strings.TrimSpace(next.Type) != "" && mcpServerTransportKind(base) != mcpServerTransportKind(mergeMCPServer(base, next)) { + if strings.TrimSpace(next.Type) != "" && mcpServerTransportKind(base) != mcpServerTransportKind(mergeMCPServer(base, next, true)) { return true } if strings.TrimSpace(next.URL) != "" && strings.TrimSpace(next.URL) != strings.TrimSpace(base.URL) { diff --git a/internal/config/resolver.go b/internal/config/resolver.go index 7982c0161..73bf4b3d3 100644 --- a/internal/config/resolver.go +++ b/internal/config/resolver.go @@ -168,7 +168,10 @@ func ResolveMCP(options ResolveOptions) (MCPConfig, error) { if err != nil { return MCPConfig{}, err } - mergeMCPConfig(&cfg.MCP, fileConfig.MCP) + // User config is higher-trust than project config: it may re-enable a + // server the user disabled (a user-level disable is sticky, but the user + // scope itself may lift it). + mergeMCPConfig(&cfg.MCP, fileConfig.MCP, true) } // Drop the project layer when the workspace is untrusted, so a cloned repo's // ./.zero/config.json cannot register (and spawn) MCP servers. Fail-closed: @@ -182,7 +185,7 @@ func ResolveMCP(options ResolveOptions) (MCPConfig, error) { return MCPConfig{}, err } } - mergeMCPConfig(&cfg.MCP, options.Overrides.MCP) + mergeMCPConfig(&cfg.MCP, options.Overrides.MCP, true) return cfg.MCP, nil } @@ -209,7 +212,7 @@ func mergeConfig(dst *FileConfig, src FileConfig) { for _, provider := range src.Providers { mergeProvider(dst, provider) } - mergeMCPConfig(&dst.MCP, src.MCP) + mergeMCPConfig(&dst.MCP, src.MCP, true) if network := strings.TrimSpace(src.Sandbox.Network); network != "" { dst.Sandbox.Network = network } @@ -707,7 +710,7 @@ func applyOverrides(cfg *FileConfig, overrides Overrides) { if hasProviderFields(overrides.Provider) { mergeProvider(cfg, overrides.Provider) } - mergeMCPConfig(&cfg.MCP, overrides.MCP) + mergeMCPConfig(&cfg.MCP, overrides.MCP, true) } func mergeLocalControlConfig(dst *LocalControlConfig, src LocalControlConfig) { diff --git a/internal/config/resolver_test.go b/internal/config/resolver_test.go index 6624eeb1e..9d865ab1e 100644 --- a/internal/config/resolver_test.go +++ b/internal/config/resolver_test.go @@ -554,7 +554,7 @@ func TestResolveReplacesMCPServerOverlayCollections(t *testing.T) { } } -func TestResolveMCPServerLayersCanClearAndReenable(t *testing.T) { +func TestResolveMCPServerLayersCannotReenableUserDisabled(t *testing.T) { userPath := writeConfig(t, `{ "mcp": { "servers": { @@ -588,9 +588,11 @@ func TestResolveMCPServerLayersCanClearAndReenable(t *testing.T) { } docs := resolved.MCP.Servers["docs"] - if docs.Disabled { - t.Fatalf("docs.Disabled = true, want project layer to re-enable") + // A user-level disable is sticky: project config must not re-enable it. + if !docs.Disabled { + t.Fatal("docs.Disabled = false, want user-level disable to remain sticky") } + // Non-disable fields from the project layer still merge normally. if len(docs.Args) != 0 { t.Fatalf("docs.Args = %#v, want project layer to clear inherited args", docs.Args) } @@ -599,6 +601,125 @@ func TestResolveMCPServerLayersCanClearAndReenable(t *testing.T) { } } +func TestMergeMCPStickyDisableReenable(t *testing.T) { + disabled := MCPServerConfig{Disabled: true, disabledSet: true} + + // A lower-trust (project) layer must not lift a sticky user disable. + project := MCPServerConfig{Disabled: false, disabledSet: true} + if got := mergeMCPServer(disabled, project, false); !got.Disabled { + t.Fatal("project layer re-enabled a sticky user disable") + } + + // The user scope (explicit mcp enable command / CLI override) may re-enable. + override := MCPServerConfig{Disabled: false, disabledSet: true} + if got := mergeMCPServer(disabled, override, true); got.Disabled { + t.Fatal("user-scope override must be able to re-enable") + } + + // A user-scope disable stacked on a project attempt still sticks. + if got := mergeMCPServer(disabled, project, false); !got.Disabled { + t.Fatal("sticky disable must survive a project re-enable attempt") + } + + // Baseline: a project layer may disable a default-enabled server when no + // higher-trust scope has explicitly set disabled (the trust boundary must + // not block legitimate project disables). Regression guard for the + // disabledSet-before-check ordering bug. + defaultEnabled := MCPServerConfig{Disabled: false, disabledSet: false} + projectDisable := MCPServerConfig{Disabled: true, disabledSet: true} + if got := mergeMCPServer(defaultEnabled, projectDisable, false); !got.Disabled { + t.Fatal("project layer should be able to disable a default-enabled server") + } + + // And an empty project layer must not flip an unconfigured server to disabled. + if got := mergeMCPServer(MCPServerConfig{}, MCPServerConfig{}, false); got.Disabled { + t.Fatal("an empty project layer must not disable an unconfigured server") + } +} + +// TestResolveMCPCannotReenableUserDisabled exercises the trust boundary at the +// actual ResolveMCP entry point: a project config (project scope) must not lift +// a disable the user set in their higher-trust user config. +func TestResolveMCPCannotReenableUserDisabled(t *testing.T) { + userPath := writeConfig(t, `{ + "mcp": { + "servers": { + "docs": { + "type": "stdio", + "command": "docs-mcp", + "disabled": true + } + } + } + }`) + // Project config tries to re-enable it. + projectPath := writeConfig(t, `{ + "mcpServers": { + "docs": { + "disabled": false + } + } + }`) + + resolved, err := ResolveMCP(ResolveOptions{ + UserConfigPath: userPath, + ProjectConfigPath: projectPath, + }) + if err != nil { + t.Fatalf("ResolveMCP() error = %v", err) + } + + docs, ok := resolved.Servers["docs"] + if !ok { + t.Fatal("docs server missing from resolved config") + } + if !docs.Disabled { + t.Fatalf("docs.Disabled = false, want user-level disable to remain sticky across project re-enable") + } +} + +// TestResolveMCPUserLiftsProjectDisabled asserts the reverse direction is open: +// a user config (higher-trust scope) re-enabling a server the project config +// disabled must be permitted, since user > project in the trust hierarchy. +func TestResolveMCPUserLiftsProjectDisabled(t *testing.T) { + // Project config disables the server. + projectPath := writeConfig(t, `{ + "mcpServers": { + "docs": { + "type": "stdio", + "command": "docs-mcp", + "disabled": true + } + } + }`) + // User config re-enables it. + userPath := writeConfig(t, `{ + "mcp": { + "servers": { + "docs": { + "disabled": false + } + } + } + }`) + + resolved, err := ResolveMCP(ResolveOptions{ + UserConfigPath: userPath, + ProjectConfigPath: projectPath, + }) + if err != nil { + t.Fatalf("ResolveMCP() error = %v", err) + } + + docs, ok := resolved.Servers["docs"] + if !ok { + t.Fatal("docs server missing from resolved config") + } + if docs.Disabled { + t.Fatalf("docs.Disabled = true, want user scope to lift the project-level disable") + } +} + func TestResolveRejectsDuplicateMCPRootAliases(t *testing.T) { path := writeConfig(t, `{ "mcpServers": {