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
38 changes: 32 additions & 6 deletions internal/config/mcp_merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,22 @@ import (
"strings"
)

func mergeMCPConfig(dst *MCPConfig, src MCPConfig) {
func mergeMCPConfig(dst *MCPConfig, src MCPConfig, canReenable bool) {
if len(src.Servers) == 0 {
return
}
if dst.Servers == nil {
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
Expand All @@ -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)
}
Expand All @@ -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
}
Expand All @@ -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
Expand All @@ -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) {
Expand Down
11 changes: 7 additions & 4 deletions internal/config/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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) {
Expand Down
127 changes: 124 additions & 3 deletions internal/config/resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ func TestResolveReplacesMCPServerOverlayCollections(t *testing.T) {
}
}

func TestResolveMCPServerLayersCanClearAndReenable(t *testing.T) {
func TestResolveMCPServerLayersCannotReenableUserDisabled(t *testing.T) {
userPath := writeConfig(t, `{
"mcp": {
"servers": {
Expand Down Expand Up @@ -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)
}
Expand All @@ -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": {
Expand Down
Loading