From ce3dfab18c8c39d669299bc9f963ef78526a5ef3 Mon Sep 17 00:00:00 2001 From: LaZzyMan Date: Fri, 3 Apr 2026 10:39:39 +0800 Subject: [PATCH] fix(permissions): allow non-core tools to bypass coreTools allowlist The coreTools configuration was incorrectly restricting all tools including MCP, Skill, Agent, and other dynamically discovered tools. These tools should not be subject to the coreTools whitelist as they are either: - Dynamically discovered from user configuration (MCP tools) - Essential for system operation (skill, agent, exit_plan_mode, ask_user_question) This fix introduces a CORE_TOOLS set that explicitly lists built-in tools subject to coreTools allowlist. Tools not in this set bypass the check. Fixes #2782 Co-authored-by: Qwen-Coder --- .../permissions/permission-manager.test.ts | 51 +++++++++++++++++++ .../src/permissions/permission-manager.ts | 45 ++++++++++++++-- 2 files changed, 93 insertions(+), 3 deletions(-) diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 5b1048d5c..599c4c4d0 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -1254,6 +1254,57 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('read_file')).toBe(true); expect(await pm.isToolEnabled('run_shell_command')).toBe(true); // not denied, just unreviewed }); + + // Non-core tools bypass coreTools allowlist + it('MCP tools bypass coreTools allowlist check', async () => { + pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] })); + pm.initialize(); + // MCP tools should be enabled even if not in coreTools + expect( + await pm.isToolEnabled('mcp__markitdown__convert_to_markdown'), + ).toBe(true); + expect(await pm.isToolEnabled('mcp__puppeteer__navigate')).toBe(true); + }); + + it('Skill tool bypasses coreTools allowlist check', async () => { + pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] })); + pm.initialize(); + expect(await pm.isToolEnabled('skill')).toBe(true); + }); + + it('Agent tool bypasses coreTools allowlist check', async () => { + pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] })); + pm.initialize(); + expect(await pm.isToolEnabled('agent')).toBe(true); + }); + + it('exit_plan_mode tool bypasses coreTools allowlist check', async () => { + pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] })); + pm.initialize(); + expect(await pm.isToolEnabled('exit_plan_mode')).toBe(true); + }); + + it('ask_user_question tool bypasses coreTools allowlist check', async () => { + pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] })); + pm.initialize(); + expect(await pm.isToolEnabled('ask_user_question')).toBe(true); + }); + + it('Non-core tools still respect deny rules', async () => { + pm = new PermissionManager( + makeConfig({ + coreTools: ['read_file'], + permissionsDeny: ['mcp__markitdown'], + }), + ); + pm.initialize(); + // MCP tool should be disabled due to deny rule, even though it bypasses coreTools + expect( + await pm.isToolEnabled('mcp__markitdown__convert_to_markdown'), + ).toBe(false); + // Other MCP tools without deny rule should still be enabled + expect(await pm.isToolEnabled('mcp__puppeteer__navigate')).toBe(true); + }); }); describe('session rules', () => { diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index e17fac881..3f38347f9 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -396,6 +396,35 @@ export class PermissionManager { // Registry-level helper // --------------------------------------------------------------------------- + /** + * Core tools that are subject to the coreTools allowlist check. + * Tools not in this set (MCP, Skill, Agent, etc.) bypass the check. + */ + private static readonly CORE_TOOLS = new Set([ + 'read_file', + 'write_file', + 'edit', + 'glob', + 'grep_search', + 'run_shell_command', + 'list_directory', + 'web_fetch', + 'web_search', + 'todo_write', + 'save_memory', + 'lsp', + 'cron_create', + 'cron_list', + 'cron_delete', + ]); + + /** + * Check if a tool is a core tool subject to the coreTools allowlist check. + */ + private isCoreTool(toolName: string): boolean { + return PermissionManager.CORE_TOOLS.has(toolName); + } + /** * Determine whether a tool should be present in the tool registry. * @@ -403,13 +432,23 @@ export class PermissionManager { * (i.e. a whole-tool deny) matches. Specifier-based deny rules such as * `"Bash(rm -rf *)"` do NOT remove the tool from the registry – they only * deny specific invocations at runtime. + * + * Non-core tools (MCP, Skill, Agent, etc.) skip the coreTools allowlist + * check because they are dynamically discovered or essential for system + * operation. */ async isToolEnabled(toolName: string): Promise { const canonicalName = resolveToolName(toolName); - // If a coreTools allowlist is active, only explicitly listed tools are - // registered. This mirrors the legacy `tools.core` whitelist semantic: - // any tool NOT in the allowlist is excluded from the registry entirely. + // Non-core tools bypass coreTools allowlist check + if (!this.isCoreTool(canonicalName)) { + const decision = await this.evaluate({ toolName: canonicalName }); + return decision !== 'deny'; + } + + // Core tools: if a coreTools allowlist is active, only explicitly listed + // tools are registered. This mirrors the legacy `tools.core` whitelist + // semantic: any tool NOT in the allowlist is excluded from the registry. if (this.coreToolsAllowList !== null && this.coreToolsAllowList.size > 0) { if (!this.coreToolsAllowList.has(canonicalName)) { return false;