feat(mcp): project .mcp.json + workspace approval gating with aligned scope precedence (#4615)#4713
feat(mcp): project .mcp.json + workspace approval gating with aligned scope precedence (#4615)#4713qqqys wants to merge 8 commits into
Conversation
… scope precedence (QwenLM#4615) Adds untrusted-source approval gating for MCP servers and a coherent cross-source precedence model. Sources & precedence (low -> high): user/default settings < project .mcp.json < workspace/system settings < session(ACP/IDE) < --mcp-config - Load project servers from .mcp.json (pure read, never connects), tagged scope:'project'. - Tag workspace/system settings servers with provenance scope at merge time so the winning entry keeps its source; centralize assembly in assembleMcpServers. - Gate checked-in/shareable sources (project + workspace) behind a hash-bound approval store; .mcp.json edits revert approval to pending. system/user/CLI/ extension/session sources are never gated. - .mcp.json now overrides USER settings (Claude parity) but never enterprise 'system' settings. - Route ACP/IDE-injected servers through a top-tier sessionMcpServers param so a repo .mcp.json can't override or gate them. - Startup approval dialog + 'qwen mcp approve|reject' + 'qwen mcp list' cover both gated sources; non-interactive sessions auto-approve (lenient). Co-Authored-By: Qwen-Coder <qwen-coder@alibabacloud.com>
📋 Review SummaryThis PR implements approval gating for untrusted MCP server sources ( 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
| return value; | ||
| }); | ||
|
|
||
| return crypto.createHash('sha256').update(stable).digest('hex').slice(0, 16); |
There was a problem hiding this comment.
[Critical] SHA-256 truncated to 16 hex characters (64 bits). Since the attacker controls .mcp.json (both the "initial approved" config and the later "modified" config), they can use a birthday attack to find a collision pair in approximately 2^32 SHA-256 evaluations — under 30 seconds on modern hardware. The attacker varies innocuous fields (e.g. env values, extra headers like X-Request-Id) to generate collision candidates while keeping the displayed summary identical.
This completely undermines the security guarantee of hash-bound approval: an attacker crafts two configs that hash to the same 64-bit value — one benign (to get initial approval) and one malicious (pushed later to silently bypass the approval gate).
Note: The Claude Code reference uses truncated hashes for reload detection (where collisions are harmless), not for security-critical approval binding. The threat models differ.
| return crypto.createHash('sha256').update(stable).digest('hex').slice(0, 16); | |
| return crypto.createHash('sha256').update(stable).digest('hex'); |
— claude-opus-4-6 via Qwen Code /review
| (choice: McpApprovalChoice) => { | ||
| const approvals = loadMcpApprovals(); | ||
| const root = config.getWorkingDir(); | ||
| setQueue((q) => { |
There was a problem hiding this comment.
[Suggestion] Side effects (disk I/O via approvals.setState → fs.writeFileSync, and MCP reconnect via reconnect → discoverToolsForServer) are performed inside the setQueue((q) => {...}) state updater function. React's contract specifies updater functions must be pure — React may invoke them more than once (StrictMode, concurrent features). If double-invoked, this fires duplicate disk writes and duplicate server connection attempts.
Move side effects out of the updater. Read queue from the closure and perform I/O after the state transition:
const handleMcpApprovalSelect = useCallback(
(choice: McpApprovalChoice) => {
const approvals = loadMcpApprovals();
const root = config.getWorkingDir();
const current = queue[0];
if (!current) return;
if (choice === McpApprovalChoice.APPROVE_ALL) {
for (const server of queue) {
approvals.setState(root, server.name, server.config, 'approved');
reconnect(server.name);
}
setQueue([]);
} else if (choice === McpApprovalChoice.APPROVE) {
approvals.setState(root, current.name, current.config, 'approved');
reconnect(current.name);
setQueue((q) => q.slice(1));
} else {
approvals.setState(root, current.name, current.config, 'rejected');
setQueue((q) => q.slice(1));
}
},
[config, reconnect, queue],
);— claude-opus-4-6 via Qwen Code /review
|
|
||
| return { | ||
| ...belowProject, | ||
| ...loadProjectMcpServers(cwd).servers, |
There was a problem hiding this comment.
[Suggestion] loadProjectMcpServers(cwd).errors is discarded here. If .mcp.json has a JSON syntax error, missing mcpServers key, or non-object entries, zero project servers load with no user-visible feedback. The user sees no MCP tools and no error — a silent failure that is very hard to diagnose.
Surface errors to the user (e.g. via stderr or a startup warning):
| ...loadProjectMcpServers(cwd).servers, | |
| const projectResult = loadProjectMcpServers(cwd); | |
| for (const err of projectResult.errors) { | |
| // eslint-disable-next-line no-console | |
| console.error(`Warning: ${err}`); | |
| } | |
| return { | |
| ...belowProject, | |
| ...projectResult.servers, | |
| ...aboveProject, | |
| ...(cliMcpServers ?? {}), | |
| }; |
— claude-opus-4-6 via Qwen Code /review
| errors.push(`${filePath}: server "${name}" is not an object — skipped`); | ||
| continue; | ||
| } | ||
| servers[name] = { |
There was a problem hiding this comment.
[Suggestion] A .mcp.json server entry named "__proto__" triggers the Object.prototype.__proto__ setter when assigned to this plain object. The entry is silently dropped from all downstream operations (Object.keys/entries/spread won't see it), making it invisible to the approval pipeline.
Use Object.create(null) for the accumulator to avoid prototype-inherited setters:
| servers[name] = { | |
| servers[name] = { |
Or change the declaration at line 84 to:
const servers: Record<string, MCPServerConfig> = Object.create(null);— claude-opus-4-6 via Qwen Code /review
| (name: string) => { | ||
| config.approveMcpServerForSession(name); | ||
| const registry = config.getToolRegistry(); | ||
| void registry?.discoverToolsForServer?.(name)?.catch?.(() => {}); |
There was a problem hiding this comment.
[Suggestion] reconnect swallows ALL errors from discoverToolsForServer with .catch?.(() => {}). If the MCP server binary is missing, the transport times out, or the server crashes on startup, there is zero user-facing feedback. The dialog closes, the server appears "approved", but no tools materialize.
At minimum, log the error so it surfaces in debug mode:
| void registry?.discoverToolsForServer?.(name)?.catch?.(() => {}); | |
| void registry?.discoverToolsForServer?.(name)?.catch?.((err: unknown) => { | |
| if (process.env['DEBUG']) { | |
| // eslint-disable-next-line no-console | |
| console.error(`MCP reconnect failed for ${name}:`, err); | |
| } | |
| }); |
— claude-opus-4-6 via Qwen Code /review
| // torn down if a prior pass had connected it. | ||
| if ( | ||
| cliConfig.isMcpServerDisabled(name) || | ||
| cliConfig.isMcpServerPendingApproval?.(name) |
There was a problem hiding this comment.
[Suggestion] The pending-approval guard is tested only on the discoverAllMcpTools bulk path (line ~979). Three other guard sites lack dedicated tests:
discoverAllMcpToolsIncremental(here, ~1632): the teardown of a previously-connected server that becomes pending mid-sessionreadResourcelazy-spawn guard (~2012) and re-check (~2096)discoverMcpToolsForServerInternal(~1151): single-server rediscovery path
These are security-critical paths — a regression could leave an untrusted server connected after its config changed, or allow lazy-spawning a pending-approval server via resource read. Add parity tests mirroring the existing disabled-server tests for each path.
— claude-opus-4-6 via Qwen Code /review
| config: MCPServerConfig, | ||
| status: McpApprovalStatus, | ||
| ): void { | ||
| const root = normalizeProjectRoot(projectRoot); |
There was a problem hiding this comment.
[Suggestion] setState does no structural validation on the per-project record before mutating. If mcpApprovals.json is corrupted with a non-object value under a project key (e.g. { "/home/user/proj": "garbage" }), this line does const project = this.file.config[root] ?? {} which gets the string "garbage" (truthy, so ?? doesn't fire), then project[serverName] = {...} throws a TypeError in strict mode — crashing the approval dialog.
| const root = normalizeProjectRoot(projectRoot); | |
| const existing = this.file.config[root]; | |
| const project: Record<string, McpApprovalRecord> = | |
| existing && typeof existing === 'object' && !Array.isArray(existing) | |
| ? (existing as Record<string, McpApprovalRecord>) | |
| : {}; | |
| project[serverName] = { hash: hashMcpServerConfig(config), status }; | |
| this.file.config[root] = project; |
— claude-opus-4-6 via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical · typecheck] 21 TypeScript errors across 3 test files — build fails.
packages/cli/src/commands/mcp.test.ts: 5 errors —mcpCommand.builderpossibly undefined / not callable (lines 23, 38)packages/cli/src/commands/mcp/list.test.ts: 11 errors —Cannot find namespace 'vi'(lines 43–64). Likely missingimport type { Mock } from 'vitest'or vitest types not in tsconfig.packages/cli/src/config/config.test.ts: 5 errors —'mcpServers' is possibly 'undefined'(lines 2107, 2112),Object is possibly 'undefined'(lines 2125, 2139), andcontextPercentageThresholddoes not exist onChatCompressionSettings(line 2493).
— qwen3.7-max via Qwen Code /review
|
|
||
| export function getMcpApprovalsPath(): string { | ||
| if (process.env['QWEN_CODE_MCP_APPROVALS_PATH']) { | ||
| return process.env['QWEN_CODE_MCP_APPROVALS_PATH']; |
There was a problem hiding this comment.
[Critical] QWEN_CODE_MCP_APPROVALS_PATH is read from process.env here but is NOT in PROJECT_ENV_HARDCODED_EXCLUSIONS in settings.ts. A malicious repository can include a .env file that sets this variable to an attacker-controlled path containing pre-approved entries for every server in .mcp.json. Attack flow: (1) user clones repo, (2) trusts the folder, (3) loadEnvironment reads .env and sets the env var, (4) getMcpApprovalsPath() returns the attacker's path, (5) all malicious servers appear pre-approved and auto-connect without any approval dialog.
This completely bypasses the approval gate — the primary security control this PR introduces.
| return process.env['QWEN_CODE_MCP_APPROVALS_PATH']; | |
| // NB: QWEN_CODE_MCP_APPROVALS_PATH must be in PROJECT_ENV_HARDCODED_EXCLUSIONS | |
| // in settings.ts to prevent project .env from redirecting the approvals store. | |
| if (process.env['QWEN_CODE_MCP_APPROVALS_PATH']) { | |
| return process.env['QWEN_CODE_MCP_APPROVALS_PATH']; | |
| } |
Also add 'QWEN_CODE_MCP_APPROVALS_PATH' to PROJECT_ENV_HARDCODED_EXCLUSIONS in settings.ts.
— qwen3.7-max via Qwen Code /review
| errors.push({ message: getErrorMessage(error), path: filePath }); | ||
| } | ||
|
|
||
| loadedMcpApprovals = new LoadedMcpApprovals( |
There was a problem hiding this comment.
[Critical] loadMcpApprovals() captures parse/IO errors into errors, but getPendingProjectMcpServers() (and all other callers) never reads this field. When the approvals file is corrupted (truncated by crash, partial write, disk error), the function returns an empty config, getState() returns 'pending' for every gated server, and all previously-approved servers silently revert to pending — with no stderr warning, no log line, and no UI message.
This is the most likely production issue: "Every MCP server stopped working and is asking for approval again" with no diagnostic trail to explain why.
Suggested fix: Surface errors at load time:
loadedMcpApprovals = new LoadedMcpApprovals(
{ path: filePath, config },
errors,
);
for (const err of errors) {
writeStderrLine(`Warning: MCP approvals file error: ${err.message}`);
}
return loadedMcpApprovals;— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
| return pending; | ||
| } |
There was a problem hiding this comment.
[Critical] saveMcpApprovals uses raw fs.writeFileSync instead of atomicWriteFile from packages/core/src/utils/atomicFileWrite.ts (used in 68+ call sites across the codebase). If the process is killed mid-write (Ctrl-C during the approval dialog, SIGKILL, power loss), the approvals file can be left truncated or containing partial JSON. Combined with the silent error handling above, this creates a silent total-loss scenario: the next startup reads a corrupt file, loses ALL approval decisions, and tells nobody.
The "Approve All" path calls setState in a loop (N sequential writes for N servers), widening the crash window.
| } | |
| export async function saveMcpApprovals(file: { | |
| path: string; | |
| config: McpApprovalsConfig; | |
| }): Promise<void> { | |
| try { | |
| const dirPath = path.dirname(file.path); | |
| if (!fs.existsSync(dirPath)) { | |
| fs.mkdirSync(dirPath, { recursive: true }); | |
| } | |
| await atomicWriteFile(file.path, JSON.stringify(file.config, null, 2), { | |
| mode: 0o600, | |
| }); | |
| } catch (error) { | |
| writeStderrLine('Error saving MCP approvals file.'); | |
| writeStderrLine(error instanceof Error ? error.message : String(error)); | |
| } | |
| } |
Note: this makes saveMcpApprovals async, which cascades to setState and callers — but all callers are already in async contexts.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const mcpServers = (parsed as { mcpServers?: unknown })?.mcpServers; | ||
| if (!mcpServers || typeof mcpServers !== 'object') { |
There was a problem hiding this comment.
[Suggestion] The guard checks typeof mcpServers !== 'object' but doesn't check Array.isArray(mcpServers). Since typeof [] === 'object', a .mcp.json with "mcpServers": [...] (array instead of object) passes the guard. Object.entries on an array yields [['0', elem0], ['1', elem1], ...], producing phantom servers with numeric names like "0", "1".
The inner Array.isArray(value) check on individual entries catches array-typed elements but not the array-typed parent.
| if (!mcpServers || typeof mcpServers !== 'object') { | |
| if (!mcpServers || typeof mcpServers !== 'object' || Array.isArray(mcpServers)) { | |
| return { | |
| servers: {}, | |
| path: filePath, | |
| errors: [`${filePath} has no "mcpServers" object`], | |
| }; |
— qwen3.7-max via Qwen Code /review
| // are not approved are listed WITHOUT connecting — inspecting an untrusted | ||
| // config must stay side-effect-free (#4615). Only approved / non-gated | ||
| // servers get a live connection test. | ||
| if (isGatedMcpScope(server.scope)) { |
There was a problem hiding this comment.
[Suggestion] This gated-scope branch (showing "Pending approval" / "Rejected" without connecting) is the core user-visible behavior of qwen mcp list for untrusted servers, but list.test.ts has zero tests exercising it. All four existing test servers lack scope: 'project' or scope: 'workspace', so isGatedMcpScope() always returns false in the test suite. Additionally, loadMcpApprovals and assembleMcpServers are not mocked — existing tests silently call real implementations.
A regression could silently attempt to connect untrusted .mcp.json servers during listing, violating the side-effect-free guarantee.
Suggested fix: Add tests for: (1) a project-scoped pending server shows "Pending approval" without calling createTransport; (2) a rejected workspace server shows "Rejected". Mock assembleMcpServers and loadMcpApprovals to control test inputs.
— qwen3.7-max via Qwen Code /review
| const pendingMcpServers = | ||
| bareMode || !interactive | ||
| ? undefined | ||
| : getPendingProjectMcpServers(mcpServers, cwd); |
There was a problem hiding this comment.
[Suggestion] This is the most security-critical branch in the PR — it determines whether gated servers auto-connect or require approval — yet config.test.ts has zero tests for pendingMcpServers or the non-interactive bypass path (grep for both terms returns no matches).
Without a test, a refactor of the interactive or bareMode detection could silently change the security posture of headless sessions in either direction: auto-approve when it shouldn't, or block when it should auto-approve.
Suggested fix: Add two tests: (1) interactive session with a .mcp.json server verifying isMcpServerPendingApproval(name) returns true; (2) non-interactive session (stdin not TTY) verifying isMcpServerPendingApproval(name) returns false.
— qwen3.7-max via Qwen Code /review
| if (!current) { | ||
| return; | ||
| } | ||
| if (choice === McpApprovalChoice.APPROVE_ALL) { |
There was a problem hiding this comment.
[Suggestion] When the user selects "Approve all," the hook iterates the entire queue approving every server, but MCPServerApprovalDialog only displays the first server's name and summary. The user never sees the command, args, or httpUrl of subsequent servers before the blanket approval.
A malicious .mcp.json could declare one legitimate-looking server followed by several malicious ones. The user reviews the first, hits "Approve all," and the malicious servers connect without any scrutiny.
Suggested fix: List all pending server names + summaries in the dialog before the radio buttons, or add a confirmation step for "Approve all" that shows what will be approved:
Approving all will connect these servers:
slack node slack.js (stdio)
telemetry curl example.com (http)
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| function summarize(config: MCPServerConfig): string { |
There was a problem hiding this comment.
[Suggestion] summarize() duplicates the same httpUrl→(http) / url→(sse) / command args→(stdio) logic that already exists in formatServerCommand() at packages/cli/src/ui/components/mcp/utils.ts:103-116. The only differences are cosmetic (.replace(/\s+\(/, ' (') vs .trim()).
Three copies of the same formatting logic means a future transport type requires edits in three places, and the slight drift already produces marginally different output for edge cases.
Suggested fix: Extract a shared formatMcpServerSummary(config: MCPServerConfig): string into mcp/utils.ts and reuse from all three call sites.
— qwen3.7-max via Qwen Code /review
| * returned list is what the discovery layer skips | ||
| * (`Config.isMcpServerPendingApproval`). See issue #4615. | ||
| */ | ||
| export function getPendingProjectMcpServers( |
There was a problem hiding this comment.
[Suggestion] The function name says "Project" but it iterates ALL gated scopes via isGatedMcpScope(config.scope), which returns true for both 'project' AND 'workspace'. Workspace-scoped servers (from .qwen/settings.json) are also included in the returned list.
A caller reading getPendingProjectMcpServers might assume workspace-scoped servers are excluded and add a separate workspace check, creating redundant or incorrect logic.
| export function getPendingProjectMcpServers( | |
| export function getPendingGatedMcpServers( |
— qwen3.7-max via Qwen Code /review
| * stdio spawn / transport / health check, so inspecting an untrusted | ||
| * `.mcp.json` has no side effects. See issue #4615. | ||
| */ | ||
| isMcpServerPendingApproval(serverName: string): boolean { |
There was a problem hiding this comment.
[Critical] getFailedMcpServerNames() (line 1884) skips disabled servers but does NOT skip pending-approval servers. Since the discovery layer never connects pending servers (status stays DISCONNECTED), getMCPServerStatus(name) !== CONNECTED is true for them, and they get included in the "failed" list.
Callers (gemini.tsx, acpAgent.ts, nonInteractive/session.ts) then emit "Warning: MCP server(s) failed to start: slack" — misleading diagnostics for servers that are merely awaiting approval, not actually broken.
This also means the surfaceFailuresOnce path in AppContainer.tsx may fire during the approval dialog, before the user has even had a chance to decide.
| isMcpServerPendingApproval(serverName: string): boolean { | |
| isMcpServerPendingApproval(serverName: string): boolean { | |
| return this.pendingMcpServers?.includes(serverName) ?? false; | |
| } | |
| // In getFailedMcpServerNames() (line 1891), add after the disabled check: | |
| // if (this.isMcpServerPendingApproval(name)) continue; |
— qwen3.7-max via Qwen Code /review
| throw new Error('mcp command builder must be a function'); | ||
| } | ||
| const builtYargs = builder(yargsInstance); | ||
| const options = builtYargs.getOptions(); |
There was a problem hiding this comment.
[Critical] TypeScript error: Property 'getOptions' does not exist on type 'Argv<{}> | PromiseLike<Argv<{}>>'. The builder function (typed as CommandModule['builder']) can return Argv | PromiseLike<Argv>. The runtime typeof builder !== 'function' guard narrows it to a function, but the return type is still the union.
| const options = builtYargs.getOptions(); | |
| const builtYargs = await builder(yargsInstance); | |
| const options = builtYargs.getOptions(); |
(Make the test callback async and await the builder result to unwrap the PromiseLike.)
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Both R2 Critical issues are fixed: getFailedMcpServerNames() now filters pending-approval servers (config.ts:1902), and the mcp.test.ts tsc error is resolved (async builder). The "Approve all" dialog now lists all pending servers for transparency. tsc and eslint clean, 217 tests pass across 5 MCP-related suites. LGTM! ✅ — qwen3.7-max via Qwen Code /review
Verification Report — PR #4713Commit: Test Results
Typecheck NoteThe 3 errors ( Test File Breakdown
Execution Environment
VerdictAll tests pass — 670 total (275 core + 395 cli). ESLint clean across 15 source files. The typecheck errors are from a cross-branch dependency (#4485 @google/genai upgrade) merged into this branch but not yet on main — not introduced by this PR's changes. No regressions detected. |
3 participants
Summary
Adds approval gating for untrusted, checked-in MCP server sources and a coherent cross-source precedence model, aligning behavior with Claude Code's
.mcp.jsonhandling. Refs #4615.Two source layers are now treated as untrusted-until-approved:
.mcp.json(repo root).qwen/settings.jsonmcpServersBoth are checked into / shared via the repo, so a server they declare is held behind a per-server, hash-bound approval gate before the discovery layer will connect it.
Precedence
Effective MCP server map is assembled in one place (
assembleMcpServers), lowest → highest (later wins on a name clash):Key points:
.mcp.jsonnow overrides user-level settings (Claude parity: project > user) but never overrides enterprise-enforcedsystemsettings.Approval / trust model
<QWEN_HOME>/mcpApprovals.json, keyed by(projectRoot, serverName)and bound to a canonical config hash. Editing the server in its source file changes the hash and reverts it topending.project+workspacescopes are gated (isGatedMcpScope).system/ user / extension / CLI / session sources are trusted and connect as before.-p, piped) auto-approve (lenient), matching Claude Code.qwen mcp approve [name|--all]/qwen mcp reject [name|--all];qwen mcp listshowsPending approval/Rejectedfor gated servers without connecting them.How provenance is tracked
mergeSettingsstamps each settings scope's servers with their origin (workspace/system) before the shallow merge, so the winning entry keeps the scope it actually came from..mcp.jsonservers are taggedprojectat load time.assembleMcpServersthen partitions by scope to realize the precedence above.Testing
mcpServers(precedence),mcpApprovals(hash binding + gated-scope filter),mcpJson,approve(project + workspace),useMcpApproval, plusloadCliConfigsession-tier coverage andsettingsmerge tagging.typecheck+eslintclean; core MCP suites (mcp-client-manager,configHash) green.Known limitations / follow-ups
qwen mcp reconnect <name>builds a minimal config without the pending set, so an explicit reconnect can bypass the gate (pre-existing; explicit user action).Escpersists arejecteddecision (escape-to-deny parity with folder-trust); undo viaqwen mcp approve.