Skip to content

Commit 6e7f587

Browse files
lets-order-some-frieslets-order-some-fries
committed
fix(cli): distinguish an unreadable MCP enablement config from a missing one
readConfig() returned {} for a SyntaxError exactly as it did for ENOENT. Downstream isFileEnabled() reads `state?.enabled ?? true`, so an empty config means everything is enabled - a trust boundary failing in the permissive direction. Every server the user disabled gets connected and its tools exposed. disable() then round-trips that same {} back to disk with one key added, erasing the rest of the file. readConfig() now returns a result discriminated on ok/missing/unreadable. Only missing yields an empty config. isFileEnabled() fails closed on unreadable; enable() and disable() refuse to write and throw McpServerEnablementConfigError, preserving the file for the user to repair. Valid JSON of the wrong shape fails open by the same route - {"x": "off"} parses fine and leaves state.enabled undefined - so it is treated as unreadable too, while entries with unknown extra fields are still accepted. The error is reported once per stretch of failures rather than on every read, and names the file path and the remedy. Fixes #28786
1 parent d5b3e3a commit 6e7f587

5 files changed

Lines changed: 263 additions & 25 deletions

File tree

‎packages/cli/src/commands/mcp/enableDisable.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import type { CommandModule } from 'yargs';
8-
import { debugLogger } from '@google/gemini-cli-core';
8+
import { debugLogger, getErrorMessage } from '@google/gemini-cli-core';
99
import {
1010
McpServerEnablementManager,
1111
canLoadServer,
@@ -60,7 +60,12 @@ async function handleEnable(args: Args): Promise<void> {
6060
manager.clearSessionDisable(name);
6161
debugLogger.log(`${GREEN}✓${RESET} Session disable cleared for '${name}'.`);
6262
} else {
63-
await manager.enable(name);
63+
try {
64+
await manager.enable(name);
65+
} catch (error) {
66+
debugLogger.log(`${RED}Error:${RESET} ${getErrorMessage(error)}`);
67+
return;
68+
}
6469
debugLogger.log(`${GREEN}✓${RESET} MCP server '${name}' enabled.`);
6570
}
6671

@@ -91,7 +96,12 @@ async function handleDisable(args: Args): Promise<void> {
9196
`${GREEN}✓${RESET} MCP server '${name}' disabled for this session.`,
9297
);
9398
} else {
94-
await manager.disable(name);
99+
try {
100+
await manager.disable(name);
101+
} catch (error) {
102+
debugLogger.log(`${RED}Error:${RESET} ${getErrorMessage(error)}`);
103+
return;
104+
}
95105
debugLogger.log(`${GREEN}✓${RESET} MCP server '${name}' disabled.`);
96106
}
97107
}

‎packages/cli/src/config/mcp/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
export {
88
McpServerEnablementManager,
9+
McpServerEnablementConfigError,
910
canLoadServer,
1011
normalizeServerId,
1112
isInSettingsList,

‎packages/cli/src/config/mcp/mcpServerEnablement.test.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,15 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => {
2222

2323
import {
2424
McpServerEnablementManager,
25+
McpServerEnablementConfigError,
2526
canLoadServer,
2627
normalizeServerId,
2728
isInSettingsList,
2829
type EnablementCallbacks,
2930
} from './mcpServerEnablement.js';
3031

32+
const CONFIG_PATH = '/virtual-home/.gemini/mcp-server-enablement.json';
33+
3134
let inMemoryFs: Record<string, string> = {};
3235

3336
function createMockEnablement(
@@ -121,6 +124,111 @@ describe('McpServerEnablementManager', () => {
121124
});
122125
});
123126

127+
describe('McpServerEnablementManager with an unreadable config file', () => {
128+
let manager: McpServerEnablementManager;
129+
130+
beforeEach(() => {
131+
inMemoryFs = {};
132+
setupFsMocks();
133+
McpServerEnablementManager.resetInstance();
134+
manager = McpServerEnablementManager.getInstance();
135+
});
136+
137+
afterEach(() => {
138+
vi.restoreAllMocks();
139+
McpServerEnablementManager.resetInstance();
140+
});
141+
142+
it.each([
143+
['malformed JSON', '{"playwright": {"enabled": false'],
144+
['a JSON array', '["playwright"]'],
145+
['a JSON scalar', '"playwright"'],
146+
['entries that are not enablement states', '{"playwright": "off"}'],
147+
[
148+
'an entry with a non-boolean enabled',
149+
'{"playwright": {"enabled": "no"}}',
150+
],
151+
])(
152+
'fails closed on %s rather than reporting every server enabled',
153+
async (_label, content) => {
154+
inMemoryFs[CONFIG_PATH] = content;
155+
156+
expect(await manager.isFileEnabled('playwright')).toBe(false);
157+
expect(await manager.isFileEnabled('never-configured')).toBe(false);
158+
},
159+
);
160+
161+
it('reports servers as persistently disabled in the display state', async () => {
162+
inMemoryFs[CONFIG_PATH] = '{ truncated';
163+
164+
expect(await manager.getDisplayState('playwright')).toEqual({
165+
enabled: false,
166+
isSessionDisabled: false,
167+
isPersistentDisabled: true,
168+
});
169+
});
170+
171+
it('refuses to disable a server and leaves the file untouched', async () => {
172+
const original = '{"playwright": {"enabled": false}, "github": ';
173+
inMemoryFs[CONFIG_PATH] = original;
174+
175+
await expect(manager.disable('other')).rejects.toBeInstanceOf(
176+
McpServerEnablementConfigError,
177+
);
178+
expect(inMemoryFs[CONFIG_PATH]).toBe(original);
179+
});
180+
181+
it('refuses to enable a server and leaves the file untouched', async () => {
182+
const original = '{"playwright": {"enabled": false}, "github": ';
183+
inMemoryFs[CONFIG_PATH] = original;
184+
185+
await expect(manager.enable('playwright')).rejects.toBeInstanceOf(
186+
McpServerEnablementConfigError,
187+
);
188+
expect(inMemoryFs[CONFIG_PATH]).toBe(original);
189+
});
190+
191+
it('names the config file so the user can repair it', async () => {
192+
inMemoryFs[CONFIG_PATH] = '{ truncated';
193+
194+
await expect(manager.disable('playwright')).rejects.toThrow(CONFIG_PATH);
195+
});
196+
197+
it('auto-enable re-enables nothing and does not throw', async () => {
198+
inMemoryFs[CONFIG_PATH] = '{ truncated';
199+
200+
await expect(manager.autoEnableServers(['playwright'])).resolves.toEqual(
201+
[],
202+
);
203+
expect(inMemoryFs[CONFIG_PATH]).toBe('{ truncated');
204+
});
205+
206+
it('recovers once the file is repaired', async () => {
207+
inMemoryFs[CONFIG_PATH] = '{ truncated';
208+
expect(await manager.isFileEnabled('playwright')).toBe(false);
209+
210+
inMemoryFs[CONFIG_PATH] = '{"playwright": {"enabled": false}}';
211+
expect(await manager.isFileEnabled('playwright')).toBe(false);
212+
expect(await manager.isFileEnabled('github')).toBe(true);
213+
214+
await manager.enable('playwright');
215+
expect(await manager.isFileEnabled('playwright')).toBe(true);
216+
});
217+
218+
it('still treats a missing file as an empty config', async () => {
219+
expect(await manager.isFileEnabled('playwright')).toBe(true);
220+
await expect(manager.enable('playwright')).resolves.toBeUndefined();
221+
});
222+
223+
it('accepts entries carrying unknown extra fields', async () => {
224+
inMemoryFs[CONFIG_PATH] =
225+
'{"playwright": {"enabled": false, "disabledAt": "2026-01-01"}}';
226+
227+
expect(await manager.isFileEnabled('playwright')).toBe(false);
228+
expect(await manager.isFileEnabled('github')).toBe(true);
229+
});
230+
});
231+
124232
describe('canLoadServer', () => {
125233
it('blocks when admin has disabled MCP', async () => {
126234
const result = await canLoadServer('s', { adminMcpEnabled: false });

‎packages/cli/src/config/mcp/mcpServerEnablement.ts‎

Lines changed: 123 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,55 @@ export interface McpServerEnablementConfig {
2222
[serverId: string]: McpServerEnablementState;
2323
}
2424

25+
/**
26+
* Outcome of reading the enablement file.
27+
*
28+
* `missing` and `unreadable` must stay distinct. A file that exists but cannot
29+
* be read or parsed says nothing about which servers the user disabled, so it
30+
* can neither be treated as an empty config nor be overwritten.
31+
*/
32+
type ReadConfigResult =
33+
| { status: 'ok' | 'missing'; config: McpServerEnablementConfig }
34+
| { status: 'unreadable' };
35+
36+
/**
37+
* Thrown when a write is refused because the config on disk is unreadable.
38+
* Writing would drop every entry the file still holds.
39+
*/
40+
export class McpServerEnablementConfigError extends Error {
41+
constructor(configFilePath: string) {
42+
super(
43+
`Cannot update MCP server enablement: ${configFilePath} exists but could not be read. ` +
44+
`Repair or delete that file and retry. Until then every MCP server is treated as disabled.`,
45+
);
46+
this.name = 'McpServerEnablementConfigError';
47+
}
48+
}
49+
50+
/**
51+
* Validate the parsed file shape. Valid JSON of the wrong shape fails open the
52+
* same way a parse error does: `{"playwright": "off"}` leaves `state.enabled`
53+
* undefined, which `isFileEnabled` would read as enabled.
54+
*
55+
* Unknown extra fields on an entry are accepted so older clients keep working
56+
* against configs written by newer ones.
57+
*/
58+
function isEnablementConfig(
59+
value: unknown,
60+
): value is McpServerEnablementConfig {
61+
if (typeof value !== 'object' || value === null || Array.isArray(value)) {
62+
return false;
63+
}
64+
return Object.values(value).every(
65+
(state) =>
66+
typeof state === 'object' &&
67+
state !== null &&
68+
!Array.isArray(state) &&
69+
'enabled' in state &&
70+
typeof state.enabled === 'boolean',
71+
);
72+
}
73+
2574
/**
2675
* For UI display - combines file and session state.
2776
*/
@@ -196,6 +245,7 @@ export class McpServerEnablementManager {
196245
private readonly configFilePath: string;
197246
private readonly configDir: string;
198247
private readonly sessionDisabled = new Set<string>();
248+
private hasReportedUnreadable = false;
199249

200250
/**
201251
* Get the singleton instance.
@@ -224,8 +274,13 @@ export class McpServerEnablementManager {
224274
* Does NOT include session state.
225275
*/
226276
async isFileEnabled(serverName: string): Promise<boolean> {
227-
const config = await this.readConfig();
228-
const state = config[normalizeServerId(serverName)];
277+
const result = await this.readConfig();
278+
if (result.status === 'unreadable') {
279+
// Fail closed. The file may disable this server and we cannot tell, so
280+
// reporting it enabled would reconnect a server the user switched off.
281+
return false;
282+
}
283+
const state = result.config[normalizeServerId(serverName)];
229284
return state?.enabled ?? true;
230285
}
231286

@@ -253,11 +308,14 @@ export class McpServerEnablementManager {
253308
*/
254309
async enable(serverName: string): Promise<void> {
255310
const normalizedName = normalizeServerId(serverName);
256-
const config = await this.readConfig();
311+
const result = await this.readConfig();
312+
if (result.status === 'unreadable') {
313+
throw new McpServerEnablementConfigError(this.configFilePath);
314+
}
257315

258-
if (normalizedName in config) {
259-
delete config[normalizedName];
260-
await this.writeConfig(config);
316+
if (normalizedName in result.config) {
317+
delete result.config[normalizedName];
318+
await this.writeConfig(result.config);
261319
}
262320
}
263321

@@ -266,9 +324,12 @@ export class McpServerEnablementManager {
266324
* Adds server to config file with enabled: false.
267325
*/
268326
async disable(serverName: string): Promise<void> {
269-
const config = await this.readConfig();
270-
config[normalizeServerId(serverName)] = { enabled: false };
271-
await this.writeConfig(config);
327+
const result = await this.readConfig();
328+
if (result.status === 'unreadable') {
329+
throw new McpServerEnablementConfigError(this.configFilePath);
330+
}
331+
result.config[normalizeServerId(serverName)] = { enabled: false };
332+
await this.writeConfig(result.config);
272333
}
273334

274335
/**
@@ -336,7 +397,17 @@ export class McpServerEnablementManager {
336397

337398
let wasDisabled = false;
338399
if (state.isPersistentDisabled) {
339-
await this.enable(normalizedName);
400+
try {
401+
await this.enable(normalizedName);
402+
} catch (error) {
403+
if (error instanceof McpServerEnablementConfigError) {
404+
// Nothing can be re-enabled while the file is unreadable, and
405+
// readConfig has already reported why. Enabling an extension is
406+
// not worth failing over this, so stop and report what was done.
407+
return enabledServers;
408+
}
409+
throw error;
410+
}
340411
wasDisabled = true;
341412
}
342413
if (state.isSessionDisabled) {
@@ -355,26 +426,58 @@ export class McpServerEnablementManager {
355426
/**
356427
* Read config from file asynchronously.
357428
*/
358-
private async readConfig(): Promise<McpServerEnablementConfig> {
429+
private async readConfig(): Promise<ReadConfigResult> {
430+
let content: string;
359431
try {
360-
const content = await fs.readFile(this.configFilePath, 'utf-8');
361-
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
362-
return JSON.parse(content) as McpServerEnablementConfig;
432+
content = await fs.readFile(this.configFilePath, 'utf-8');
363433
} catch (error) {
364434
if (
365435
error instanceof Error &&
366436
'code' in error &&
367437
error.code === 'ENOENT'
368438
) {
369-
return {};
439+
this.hasReportedUnreadable = false;
440+
return { status: 'missing', config: {} };
370441
}
371-
coreEvents.emitFeedback(
372-
'error',
373-
'Failed to read MCP server enablement config.',
374-
error,
442+
this.reportUnreadable(error);
443+
return { status: 'unreadable' };
444+
}
445+
446+
let parsed: unknown;
447+
try {
448+
parsed = JSON.parse(content);
449+
} catch (error) {
450+
this.reportUnreadable(error);
451+
return { status: 'unreadable' };
452+
}
453+
454+
if (!isEnablementConfig(parsed)) {
455+
this.reportUnreadable(
456+
new Error('Expected a map of server ID to { enabled: boolean }.'),
375457
);
376-
return {};
458+
return { status: 'unreadable' };
459+
}
460+
461+
this.hasReportedUnreadable = false;
462+
return { status: 'ok', config: parsed };
463+
}
464+
465+
/**
466+
* Report an unreadable config once per stretch of failures. `isFileEnabled`
467+
* runs for every server on every connection attempt, so emitting on each
468+
* read would bury the message in copies of itself.
469+
*/
470+
private reportUnreadable(error: unknown): void {
471+
if (this.hasReportedUnreadable) {
472+
return;
377473
}
474+
this.hasReportedUnreadable = true;
475+
coreEvents.emitFeedback(
476+
'error',
477+
`Failed to read MCP server enablement config at ${this.configFilePath}. ` +
478+
`Every MCP server is treated as disabled until the file is repaired or deleted.`,
479+
error,
480+
);
378481
}
379482

380483
/**

0 commit comments

Comments
 (0)