fix(core): prevent malformed permission rules from becoming tool-wide catch-alls (#3467)

* fix(core): prevent malformed permission rules from becoming tool-wide catch-alls

A permission rule with unbalanced parentheses (e.g. `Bash(rm -rf /)*`)
was silently parsed with `specifier: undefined`, causing `matchesRule`
to treat it as a catch-all that matches every invocation of the tool.
For deny rules this blocked all commands; for allow rules a typo could
silently auto-approve everything.

Add an `invalid` flag to `PermissionRule`. `parseRule` now marks rules
with unbalanced parens as invalid, `matchesRule` short-circuits them to
never match, and all entry points (`addSession*Rule`, `addPersistentRule`,
`parseRules`) warn on malformed input. `listRules` filters out invalid
rules so they don't appear in the /permissions UI.

* fix(cli): show error in /permissions dialog when adding malformed rule

When a user enters a rule with unbalanced parentheses via the "Add Rule"
input in the /permissions dialog, show an inline error message instead of
silently accepting and then hiding the invalid rule.

Closes #3459

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

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<DialogView>('rule-list');
   const [newRuleInput, setNewRuleInput] = useState('');
+  const [ruleInputError, setRuleInputError] = useState('');
   const [pendingRuleText, setPendingRuleText] = useState('');
   const [deleteTarget, setDeleteTarget] = useState<RuleWithSource | null>(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}
             />
           </Box>
+          {ruleInputError && (
+            <>
+              <Box height={1} />
+              <Text color={theme.status.error}>{ruleInputError}</Text>
+            </>
+          )}
         </Box>
         <Box marginTop={1} marginLeft={1}>
           <Text color={theme.text.secondary}>

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', () => {

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 });
+        }
       }
     };
 

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__') ||

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. */