Skip to content

Commit 3c4b380

Browse files
ompatel-aimlgaldawave
authored and
shaurya-cd
committed
fix(core): prevent blacklist bypass in mcp list (google-gemini#27377)
Co-authored-by: Gal Zahavi <[email protected]>
1 parent bfc1ccf commit 3c4b380

7 files changed

Lines changed: 399 additions & 6 deletions

File tree

‎packages/cli/src/commands/mcp/list.test.ts‎

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,4 +475,258 @@ describe('mcp list command', () => {
475475
);
476476
expect(mockedCreateTransport).not.toHaveBeenCalled();
477477
});
478+
479+
it('should block servers excluded by user settings even if workspace settings override/clear the excluded list', async () => {
480+
const mockSettings = createMockSettings({
481+
user: {
482+
path: '/user/settings.json',
483+
settings: {
484+
mcp: {
485+
excluded: ['blocked-server'],
486+
},
487+
},
488+
originalSettings: {
489+
mcp: {
490+
excluded: ['blocked-server'],
491+
},
492+
},
493+
},
494+
workspace: {
495+
path: '/workspace/settings.json',
496+
settings: {
497+
mcp: {
498+
excluded: [],
499+
},
500+
},
501+
originalSettings: {
502+
mcp: {
503+
excluded: [],
504+
},
505+
},
506+
},
507+
mcpServers: {
508+
'blocked-server': { command: '/test/server' },
509+
},
510+
isTrusted: true,
511+
merged: {
512+
mcp: {
513+
excluded: [], // workspace has overridden user settings!
514+
},
515+
mcpServers: {
516+
'blocked-server': { command: '/test/server' },
517+
},
518+
},
519+
});
520+
521+
mockedLoadSettings.mockReturnValue(mockSettings);
522+
523+
await listMcpServers();
524+
525+
expect(debugLogger.log).toHaveBeenCalledWith(
526+
expect.stringContaining(
527+
'blocked-server: /test/server (stdio) - Blocked',
528+
),
529+
);
530+
expect(mockedCreateTransport).not.toHaveBeenCalled();
531+
});
532+
533+
it('should block servers case-insensitively when excluded', async () => {
534+
const mockSettings = createMockSettings({
535+
user: {
536+
path: '/user/settings.json',
537+
settings: {
538+
mcp: {
539+
excluded: ['BLOCKED-server'],
540+
},
541+
},
542+
originalSettings: {
543+
mcp: {
544+
excluded: ['BLOCKED-server'],
545+
},
546+
},
547+
},
548+
mcpServers: {
549+
'blocked-server': { command: '/test/server' },
550+
},
551+
isTrusted: true,
552+
merged: {
553+
mcpServers: {
554+
'blocked-server': { command: '/test/server' },
555+
},
556+
},
557+
});
558+
559+
mockedLoadSettings.mockReturnValue(mockSettings);
560+
561+
await listMcpServers();
562+
563+
expect(debugLogger.log).toHaveBeenCalledWith(
564+
expect.stringContaining(
565+
'blocked-server: /test/server (stdio) - Blocked',
566+
),
567+
);
568+
expect(mockedCreateTransport).not.toHaveBeenCalled();
569+
});
570+
571+
it('should restrict allowed servers to the intersection of all defined allowlists', async () => {
572+
const mockSettings = createMockSettings({
573+
user: {
574+
path: '/user/settings.json',
575+
settings: {
576+
mcp: {
577+
allowed: ['allowed-server-1', 'allowed-server-2'],
578+
},
579+
},
580+
originalSettings: {
581+
mcp: {
582+
allowed: ['allowed-server-1', 'allowed-server-2'],
583+
},
584+
},
585+
},
586+
workspace: {
587+
path: '/workspace/settings.json',
588+
settings: {
589+
mcp: {
590+
allowed: ['allowed-server-1', 'malicious-server'],
591+
},
592+
},
593+
originalSettings: {
594+
mcp: {
595+
allowed: ['allowed-server-1', 'malicious-server'],
596+
},
597+
},
598+
},
599+
mcpServers: {
600+
'allowed-server-1': { command: '/allowed/1' },
601+
'allowed-server-2': { command: '/allowed/2' },
602+
'malicious-server': { command: '/malicious' },
603+
},
604+
isTrusted: true,
605+
merged: {
606+
mcp: {
607+
allowed: ['allowed-server-1', 'malicious-server'], // workspace overrode user settings!
608+
},
609+
mcpServers: {
610+
'allowed-server-1': { command: '/allowed/1' },
611+
'allowed-server-2': { command: '/allowed/2' },
612+
'malicious-server': { command: '/malicious' },
613+
},
614+
},
615+
});
616+
617+
mockedLoadSettings.mockReturnValue(mockSettings);
618+
mockClient.connect.mockResolvedValue(undefined);
619+
mockClient.ping.mockResolvedValue(undefined);
620+
621+
await listMcpServers();
622+
623+
// allowed-server-1 is in the intersection, so it should connect
624+
expect(debugLogger.log).toHaveBeenCalledWith(
625+
expect.stringContaining(
626+
'allowed-server-1: /allowed/1 (stdio) - Connected',
627+
),
628+
);
629+
// allowed-server-2 and malicious-server are not in the intersection, so they should be Blocked
630+
expect(debugLogger.log).toHaveBeenCalledWith(
631+
expect.stringContaining(
632+
'allowed-server-2: /allowed/2 (stdio) - Blocked',
633+
),
634+
);
635+
expect(debugLogger.log).toHaveBeenCalledWith(
636+
expect.stringContaining(
637+
'malicious-server: /malicious (stdio) - Blocked',
638+
),
639+
);
640+
641+
expect(mockedCreateTransport).toHaveBeenCalledTimes(1);
642+
expect(mockedCreateTransport).toHaveBeenCalledWith(
643+
'allowed-server-1',
644+
expect.any(Object),
645+
false,
646+
expect.any(Object),
647+
);
648+
});
649+
650+
it('should block all servers if the intersection of user and workspace allowlists is empty (disjoint allowlists)', async () => {
651+
const mockSettings = createMockSettings({
652+
user: {
653+
path: '/user/settings.json',
654+
settings: {
655+
mcp: {
656+
allowed: ['user-allowed-server'],
657+
},
658+
},
659+
originalSettings: {
660+
mcp: {
661+
allowed: ['user-allowed-server'],
662+
},
663+
},
664+
},
665+
workspace: {
666+
path: '/workspace/settings.json',
667+
settings: {
668+
mcp: {
669+
allowed: ['workspace-allowed-server'],
670+
},
671+
},
672+
originalSettings: {
673+
mcp: {
674+
allowed: ['workspace-allowed-server'],
675+
},
676+
},
677+
},
678+
mcpServers: {
679+
'user-allowed-server': { command: '/allowed/user' },
680+
'workspace-allowed-server': { command: '/allowed/workspace' },
681+
},
682+
isTrusted: true,
683+
merged: {
684+
mcp: {
685+
allowed: ['workspace-allowed-server'], // workspace override
686+
},
687+
mcpServers: {
688+
'user-allowed-server': { command: '/allowed/user' },
689+
'workspace-allowed-server': { command: '/allowed/workspace' },
690+
},
691+
},
692+
});
693+
694+
mockedLoadSettings.mockReturnValue(mockSettings);
695+
696+
await listMcpServers();
697+
698+
// Since the intersection is empty ([]), both servers should be Blocked!
699+
expect(debugLogger.log).toHaveBeenCalledWith(
700+
expect.stringContaining(
701+
'user-allowed-server: /allowed/user (stdio) - Blocked',
702+
),
703+
);
704+
expect(debugLogger.log).toHaveBeenCalledWith(
705+
expect.stringContaining(
706+
'workspace-allowed-server: /allowed/workspace (stdio) - Blocked',
707+
),
708+
);
709+
expect(mockedCreateTransport).not.toHaveBeenCalled();
710+
});
711+
712+
it('should block all servers if allowlist is configured as empty array []', async () => {
713+
const mockSettings = createMockSettings({
714+
mcp: {
715+
allowed: [], // empty allowlist configured!
716+
},
717+
mcpServers: {
718+
'test-server': { command: '/test/server' },
719+
},
720+
isTrusted: true,
721+
});
722+
723+
mockedLoadSettings.mockReturnValue(mockSettings);
724+
725+
await listMcpServers();
726+
727+
expect(debugLogger.log).toHaveBeenCalledWith(
728+
expect.stringContaining('test-server: /test/server (stdio) - Blocked'),
729+
);
730+
expect(mockedCreateTransport).not.toHaveBeenCalled();
731+
});
478732
});

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,12 +159,16 @@ async function getServerStatus(
159159
server: MCPServerConfig,
160160
isTrusted: boolean,
161161
activeSettings: MergedSettings,
162+
consolidatedExcluded: string[],
163+
consolidatedAllowed: string[] | undefined,
162164
): Promise<MCPServerStatus> {
163165
const mcpEnablementManager = McpServerEnablementManager.getInstance();
166+
164167
const loadResult = await canLoadServer(serverName, {
165168
adminMcpEnabled: activeSettings.admin?.mcp?.enabled ?? true,
166-
allowedList: activeSettings.mcp?.allowed,
167-
excludedList: activeSettings.mcp?.excluded,
169+
allowedList: consolidatedAllowed,
170+
excludedList:
171+
consolidatedExcluded.length > 0 ? consolidatedExcluded : undefined,
168172
enablement: mcpEnablementManager.getEnablementCallbacks(),
169173
});
170174

@@ -227,6 +231,10 @@ export async function listMcpServers(
227231
);
228232
}
229233

234+
const consolidatedExcluded =
235+
loadedSettings.getConsolidatedExcludedMcpServers();
236+
const consolidatedAllowed = loadedSettings.getConsolidatedAllowedMcpServers();
237+
230238
debugLogger.log('Configured MCP servers:\n');
231239

232240
for (const serverName of serverNames) {
@@ -237,6 +245,8 @@ export async function listMcpServers(
237245
server,
238246
loadedSettings.isTrusted,
239247
activeSettings,
248+
consolidatedExcluded,
249+
consolidatedAllowed,
240250
);
241251

242252
let statusIndicator = '';

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,7 @@ export interface LoadCliConfigOptions {
576576
};
577577
worktreeSettings?: WorktreeSettings;
578578
skipExtensions?: boolean;
579+
loadedSettings?: LoadedSettings;
579580
}
580581

581582
export async function loadCliConfig(
@@ -584,7 +585,12 @@ export async function loadCliConfig(
584585
argv: CliArgs,
585586
options: LoadCliConfigOptions = {},
586587
): Promise<Config> {
587-
const { cwd = process.cwd(), projectHooks, skipExtensions = false } = options;
588+
const {
589+
cwd = process.cwd(),
590+
projectHooks,
591+
skipExtensions = false,
592+
loadedSettings,
593+
} = options;
588594
const debugMode = isDebugMode(argv);
589595

590596
const worktreeSettings =
@@ -985,12 +991,17 @@ export async function loadCliConfig(
985991
agents: settings.agents,
986992
adminSkillsEnabled,
987993
allowedMcpServers: mcpEnabled
988-
? (argv.allowedMcpServerNames ?? settings.mcp?.allowed)
994+
? (argv.allowedMcpServerNames ??
995+
(loadedSettings
996+
? loadedSettings.getConsolidatedAllowedMcpServers()
997+
: settings.mcp?.allowed))
989998
: undefined,
990999
blockedMcpServers: mcpEnabled
9911000
? argv.allowedMcpServerNames
9921001
? undefined
993-
: settings.mcp?.excluded
1002+
: loadedSettings
1003+
? loadedSettings.getConsolidatedExcludedMcpServers()
1004+
: settings.mcp?.excluded
9941005
: undefined,
9951006
blockedEnvironmentVariables:
9961007
settings.security?.environmentVariableRedaction?.blocked,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ export async function canLoadServer(
119119
}
120120

121121
// 2. Allowlist check
122-
if (config.allowedList && config.allowedList.length > 0) {
122+
if (config.allowedList !== undefined) {
123123
const { found, deprecationWarning } = isInSettingsList(
124124
normalizedId,
125125
config.allowedList,

0 commit comments

Comments
 (0)