fix(config): enforce MCP trust boundary so project config cannot override user disable#609
Conversation
…ride 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. Previously the merge guard only blocked re-enabling a user-disabled server, so a repo that set disabled:true could silently override a user's explicit mcp enable <server>. The guard is now symmetric: 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 it. Fixes Gitlawb#512
There was a problem hiding this comment.
Pull request overview
This PR tightens the MCP configuration trust boundary so a lower-trust project config (.zero/config.json) cannot override a higher-trust user choice for an MCP server’s disabled state, addressing #512.
Changes:
- Introduces a
canReenableflag into MCP merge helpers to encode which config layers may overridedisabled. - Updates the
disabledmerge guard to be symmetric (project cannot re-enable a user-disabled server, nor disable a user-enabled server). - Updates/extends resolver tests to cover sticky user disable and user-over-project behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/config/resolver.go | Adds canReenable plumbing to MCP merge and updates the disabled merge logic to enforce the trust boundary. |
| internal/config/resolver_test.go | Renames an existing test and adds new cases for sticky disable behavior across merge layers and entry points. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if next.disabledSet { | ||
| base.disabledSet = true | ||
| } | ||
| if next.disabledSet || next.Disabled { |
| 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") | ||
| } | ||
| } |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughMCP server config merging now preserves explicit disables across trust boundaries unless the current layer is allowed to re-enable. The resolver wiring passes this rule through user config, project config, and CLI overrides, and the tests now cover the updated precedence behavior. ChangesMCP sticky disable
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/config/resolver_test.go (1)
604-623: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for project disabling an unconfigured/default server.
The current suite only exercises stickiness when the base already has an explicit user decision, which is why the
mergeMCPServerordering bug (seeresolver.goLines 873-887) slips through. A case wherebasehasdisabledSet=falseand a project layer (canReenable=false) setsdisabled:true— expecting the server to end up disabled — would catch it.💚 Suggested test
func TestMergeMCPProjectCanDisableUnconfigured(t *testing.T) { base := MCPServerConfig{} // no prior explicit decision project := MCPServerConfig{Disabled: true, disabledSet: true} if got := mergeMCPServer(base, project, false); !got.Disabled { t.Fatal("project layer must be able to disable an unconfigured server") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/resolver_test.go` around lines 604 - 623, Add a test covering the case where mergeMCPServer starts from an unconfigured base (disabledSet=false) and a project-layer MCPServerConfig disables it with canReenable=false; this should end up Disabled=true. Update the resolver_test.go coverage alongside TestMergeMCPStickyDisableReenable by adding a focused case for the project layer disabling a default server so the mergeMCPServer ordering bug in resolver.go is caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/resolver.go`:
- Around line 873-887: The merge logic in resolver.go is using base.disabledSet
after it has already been updated from next, which makes a project-layer
disabled: true look like an inherited higher-scope decision and prevents
disabling default servers. Snapshot the original base.disabledSet before the
merge in the disabled-set handling around the disabled flag merge, and use that
snapshot in the trust-boundary check inside the resolver merge path so project
config can still disable defaults like firecrawl. Add a regression test covering
a default server being disabled from project config.
---
Nitpick comments:
In `@internal/config/resolver_test.go`:
- Around line 604-623: Add a test covering the case where mergeMCPServer starts
from an unconfigured base (disabledSet=false) and a project-layer
MCPServerConfig disables it with canReenable=false; this should end up
Disabled=true. Update the resolver_test.go coverage alongside
TestMergeMCPStickyDisableReenable by adding a focused case for the project layer
disabling a default server so the mergeMCPServer ordering bug in resolver.go is
caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baf62d15-ecea-45eb-8862-73a8fc213264
📒 Files selected for processing (2)
internal/config/resolver.gointernal/config/resolver_test.go
Copilot review: the disabled trust-boundary guard read base.disabledSet after it had already been OR-ed with next.disabledSet, so a project layer could never disable a default-enabled server (the guard fired on the project's own flag instead of the higher-trust scope's decision). Capture base.disabledSet/base.Disabled before folding in next, and evaluate the boundary against those prior values. Adds regression coverage: a project layer may disable a default-enabled server when no higher-trust scope set disabled, and an empty project layer must not flip an unconfigured server to disabled. Co-Authored-By: Copilot <copilot@github.com>
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
LGTM.
User-level MCP disable (and explicit enable) should be sticky against project config. Making the guard symmetric with canReenable on the project layer is the right trust boundary: repo cannot re-enable what the user disabled, and cannot flip an explicit user enable either. CLI overrides still can.
Tests cover sticky disable, project re-enable blocked, user enable sticky vs project disable, and project still able to disable a default-enabled server. All CI green.
One merge note: #597 also touches MCP project merge. If both land, the second one may need a small rebase so sticky-disable and the credential/target-change guard both apply. Not blocking this PR on its own.
Summary
Fixes #512 — a user-level MCP server
disableis now sticky. The project config (.zero/config.jsonin a cloned repo) can no longer override the user's explicit choice:The old merge guard was asymmetric: it only blocked re-enabling a user-disabled server, so a repo that set
disabled: truecould silently defeat a user'smcp enable <server>. The guard is now symmetric — once a higher-trust scope has explicitly set thedisabledflag, a lower-trust layer may not override that decision in either direction. Only the CLI override scope (canReenable = true) may lift a sticky user disable.Changes
internal/config/resolver.go:mergeMCPConfig/mergeMCPServertake acanReenable bool.ResolveMCPpassescanReenable = path != options.ProjectConfigPath(user → true, project → false). The disabled-handling guard is now symmetric.internal/config/resolver_test.go:TestResolveMCPServerLayersCanClearAndReenable→TestResolveMCPServerLayersCannotReenableUserDisabled.TestMergeMCPStickyDisableReenable,TestResolveMCPCannotReenableUserDisabled, andTestResolveMCPUserLiftsProjectDisabled.Test plan
go test ./internal/config/...passesgo vet ./internal/config/...cleanSummary by CodeRabbit