diff --git a/packages/cli/src/ui/components/PermissionsDialog.tsx b/packages/cli/src/ui/components/PermissionsDialog.tsx index 1ebb18d65..853f4fafa 100644 --- a/packages/cli/src/ui/components/PermissionsDialog.tsx +++ b/packages/cli/src/ui/components/PermissionsDialog.tsx @@ -24,7 +24,7 @@ import type { RuleWithSource, RuleType, } from '@qwen-code/qwen-code-core'; -import { isPathWithinRoot } from '@qwen-code/qwen-code-core'; +import { isPathWithinRoot, parseRule } from '@qwen-code/qwen-code-core'; // --------------------------------------------------------------------------- // Types @@ -164,6 +164,7 @@ export function PermissionsDialog({ // --- Dialog view state machine --- const [view, setView] = useState('rule-list'); const [newRuleInput, setNewRuleInput] = useState(''); + const [ruleInputError, setRuleInputError] = useState(''); const [pendingRuleText, setPendingRuleText] = useState(''); const [deleteTarget, setDeleteTarget] = useState(null); @@ -455,6 +456,7 @@ export function PermissionsDialog({ (value: string) => { if (value === '__add__') { setNewRuleInput(''); + setRuleInputError(''); setView('add-rule-input'); return; } @@ -471,6 +473,16 @@ export function PermissionsDialog({ const handleAddRuleSubmit = useCallback(() => { const trimmed = newRuleInput.trim(); if (!trimmed) return; + const rule = parseRule(trimmed); + if (rule.invalid) { + setRuleInputError( + t( + 'Malformed rule: unbalanced parentheses. Use the format ToolName(specifier).', + ), + ); + return; + } + setRuleInputError(''); setPendingRuleText(trimmed); setView('add-rule-scope'); }, [newRuleInput]); @@ -812,6 +824,12 @@ export function PermissionsDialog({ isActive={true} /> + {ruleInputError && ( + <> + + {ruleInputError} + + )} diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 0f3d06912..52203308c 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -186,7 +186,32 @@ describe('parseRule', () => { it('handles malformed pattern (no closing paren)', async () => { const r = parseRule('Bash(git status'); + expect(r.invalid).toBe(true); + expect(r.toolName).toBe('run_shell_command'); expect(r.specifier).toBeUndefined(); + // Must not match any command + expect(matchesRule(r, 'run_shell_command', 'git status')).toBe(false); + expect(matchesRule(r, 'run_shell_command', 'rm -rf /')).toBe(false); + }); + + it('handles malformed pattern with trailing junk after paren', async () => { + const r = parseRule('Bash(rm -rf /)*'); + expect(r.invalid).toBe(true); + expect(matchesRule(r, 'run_shell_command', 'git status')).toBe(false); + expect(matchesRule(r, 'run_shell_command', 'rm -rf /')).toBe(false); + }); + + it('handles malformed pattern with only open paren', async () => { + const r = parseRule('Bash('); + expect(r.invalid).toBe(true); + expect(matchesRule(r, 'run_shell_command', 'ls')).toBe(false); + }); + + it('still parses well-formed rules correctly', async () => { + const r = parseRule('Bash(rm -rf /)'); + expect(r.invalid).toBeUndefined(); + expect(matchesRule(r, 'run_shell_command', 'rm -rf /')).toBe(true); + expect(matchesRule(r, 'run_shell_command', 'git status')).toBe(false); }); }); @@ -1353,6 +1378,29 @@ describe('PermissionManager', () => { pm.addSessionDenyRule('run_shell_command'); expect(await pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); }); + + it('malformed session allow rule is silently ignored', async () => { + pm.addSessionAllowRule('Bash(git commit'); + // 'git commit' is not readonly, so default is 'ask'. + // The malformed rule must not act as catch-all allow. + expect( + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git commit', + }), + ).toBe('ask'); + }); + + it('malformed session deny rule is silently ignored', async () => { + pm.addSessionDenyRule('Bash(rm -rf /)*'); + // Should NOT deny — the malformed rule must not act as catch-all + expect( + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git status', + }), + ).not.toBe('deny'); + }); }); describe('allowedTools via permissionsAllow', () => { @@ -1389,6 +1437,21 @@ describe('PermissionManager', () => { ); expect(sessionAllow?.rule.toolName).toBe('run_shell_command'); }); + + it('excludes malformed rules from listing', async () => { + pm = new PermissionManager( + makeConfig({ + permissionsAllow: ['ReadFileTool'], + permissionsDeny: ['Bash(rm -rf /)*'], + }), + ); + pm.initialize(); + + const rules = pm.listRules(); + // The malformed deny rule should be filtered out + expect(rules.length).toBe(1); + expect(rules[0]!.rule.toolName).toBe('read_file'); + }); }); describe('hasMatchingAskRule', () => { diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index 3f38347f9..a2db685e9 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -16,6 +16,7 @@ import { extractShellOperations } from './shell-semantics.js'; import type { ShellOperation } from './shell-semantics.js'; import { isShellCommandReadOnlyAST } from '../utils/shellAstParser.js'; import { detectCommandSubstitution } from '../utils/shell-utils.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; import type { PermissionCheckContext, PermissionDecision, @@ -26,6 +27,8 @@ import type { RuleScope, } from './types.js'; +const debugLogger = createDebugLogger('PERMISSIONS'); + /** * Numeric priority for each PermissionDecision. * Higher number = more restrictive. Used to combine decisions by taking @@ -682,7 +685,14 @@ export class PermissionManager { */ addSessionAllowRule(raw: string): void { if (raw && raw.trim()) { - this.sessionRules.allow.push(parseRule(raw)); + const rule = parseRule(raw); + if (rule.invalid) { + debugLogger.warn( + `Ignoring malformed allow rule (unbalanced parentheses): ${rule.raw}`, + ); + return; + } + this.sessionRules.allow.push(rule); } } @@ -691,7 +701,14 @@ export class PermissionManager { */ addSessionDenyRule(raw: string): void { if (raw && raw.trim()) { - this.sessionRules.deny.push(parseRule(raw)); + const rule = parseRule(raw); + if (rule.invalid) { + debugLogger.warn( + `Ignoring malformed deny rule (unbalanced parentheses): ${rule.raw}`, + ); + return; + } + this.sessionRules.deny.push(rule); } } @@ -700,7 +717,14 @@ export class PermissionManager { */ addSessionAskRule(raw: string): void { if (raw && raw.trim()) { - this.sessionRules.ask.push(parseRule(raw)); + const rule = parseRule(raw); + if (rule.invalid) { + debugLogger.warn( + `Ignoring malformed ask rule (unbalanced parentheses): ${rule.raw}`, + ); + return; + } + this.sessionRules.ask.push(rule); } } @@ -719,6 +743,12 @@ export class PermissionManager { */ addPersistentRule(raw: string, type: RuleType): PermissionRule { const rule = parseRule(raw); + if (rule.invalid) { + debugLogger.warn( + `Ignoring malformed ${type} rule (unbalanced parentheses): ${rule.raw}`, + ); + return rule; + } // Deduplicate: skip if a rule with the same raw string already exists const exists = this.persistentRules[type].some((r) => r.raw === rule.raw); if (!exists) { @@ -789,7 +819,9 @@ export class PermissionManager { scope: RuleScope, ) => { for (const rule of rules) { - result.push({ rule, type, scope }); + if (!rule.invalid) { + result.push({ rule, type, scope }); + } } }; diff --git a/packages/core/src/permissions/rule-parser.ts b/packages/core/src/permissions/rule-parser.ts index 688fdb172..2f4613533 100644 --- a/packages/core/src/permissions/rule-parser.ts +++ b/packages/core/src/permissions/rule-parser.ts @@ -8,6 +8,9 @@ import path from 'node:path'; import os from 'node:os'; import picomatch from 'picomatch'; import { parse } from 'shell-quote'; +import { createDebugLogger } from '../utils/debugLogger.js'; + +const debugLogger = createDebugLogger('PERMISSIONS'); /** * Normalize a filesystem path to use POSIX-style forward slashes. @@ -247,10 +250,13 @@ export function parseRule(raw: string): PermissionRule { } const toolPart = normalized.substring(0, openParen).trim(); - const specifier = normalized.endsWith(')') - ? normalized.substring(openParen + 1, normalized.length - 1) - : undefined; + if (!normalized.endsWith(')')) { + // Malformed: unbalanced parentheses — mark as invalid so it never matches. + return { raw: trimmed, toolName: resolveToolName(toolPart), invalid: true }; + } + + const specifier = normalized.substring(openParen + 1, normalized.length - 1); const canonicalName = resolveToolName(toolPart); const specifierKind = specifier ? getSpecifierKind(canonicalName) : undefined; @@ -267,7 +273,17 @@ export function parseRule(raw: string): PermissionRule { * silently skipping any empty entries. */ export function parseRules(raws: string[]): PermissionRule[] { - return raws.filter((r) => r && r.trim()).map(parseRule); + return raws + .filter((r) => r && r.trim()) + .map(parseRule) + .map((r) => { + if (r.invalid) { + debugLogger.warn( + `Ignoring malformed rule (unbalanced parentheses): ${r.raw}`, + ); + } + return r; + }); } // ───────────────────────────────────────────────────────────────────────────── @@ -939,6 +955,11 @@ export function matchesRule( ): boolean { const canonicalCtxToolName = resolveToolName(toolName); + // ── Invalid (malformed) rules never match anything ────────────────── + if (rule.invalid) { + return false; + } + // ── MCP tool matching ──────────────────────────────────────────────── if ( rule.toolName.startsWith('mcp__') || diff --git a/packages/core/src/permissions/types.ts b/packages/core/src/permissions/types.ts index 01d919cba..d7c2ff296 100644 --- a/packages/core/src/permissions/types.ts +++ b/packages/core/src/permissions/types.ts @@ -59,6 +59,8 @@ export interface PermissionRule { * Set automatically during parsing based on the tool name/category. */ specifierKind?: SpecifierKind; + /** True if the raw rule was malformed (e.g. unbalanced parens) and should never match. */ + invalid?: boolean; } /** A complete set of permission rules organized by type. */