Skip to content

Permission rule parser: malformed rule with unbalanced parens silently becomes tool-wide catch-all #3459

@Banandana

Description

@Banandana

Disclosure: This issue was drafted with the help of an LLM (Claude). All file paths, line numbers, code snippets, and the existing-test observation below were verified against the current main of this repo via gh api. I also applied the proposed patch to my locally-installed cli.js bundle (0.14.5) and confirmed the malformed rule no longer denies unrelated commands. Maintainer judgment still wanted on the exact shape of the fix (sentinel toolName vs. explicit invalid flag vs. throwing at parse time).

Summary

A malformed permission rule such as Bash(rm -rf /)* — an opening ( with no matching closing ) at the end — is silently parsed into a rule with a valid canonical toolName and specifier: undefined. Downstream, matchesRule treats a missing specifier as "match any invocation of this tool", so the malformed rule becomes a catch-all deny for every shell command. The error message still echoes the original malformed string, making this look like a matcher bug rather than a parser bug:

Qwen Code requires permission to use "run_shell_command", but that permission was declined.
Matching deny rule: "Bash(rm -rf /)*".

…for a command like git status && git pull --rebase.

Reproduction

Settings:

{ "permissions": { "deny": ["Bash(rm -rf /)*"] } }

Then ask Qwen Code to run any shell command (e.g. git status). It is denied, with the malformed rule cited as the match — even though the command contains no rm.

Expected: the malformed rule should be inert (or rejected at parse time); it must not deny unrelated commands.

Root cause

packages/core/src/permissions/rule-parser.ts, lines 250–252 inside parseRule (function body: lines 231–263):

const specifier = normalized.endsWith(')')
  ? normalized.substring(openParen + 1, normalized.length - 1)
  : undefined;   // ← BUG

If the input contains ( but does not end with ), specifier is set to undefined. The returned rule has a real toolName (Bashrun_shell_command) with no specifier — structurally identical to a bare Bash tool-level rule.

Then in the same file, matchesRule short-circuits:

if (!toolMatchesRuleToolName(rule.toolName, canonicalCtxToolName)) {
  return false;
}
if (!rule.specifier) {
  return true;   // ← catch-all for the tool
}

So the malformed Bash(rm -rf /)* rule matches every shell invocation. Because rule.raw preserves the original string, findMatchingDenyRule reports it as the matching rule and the user sees a confusing error.

Existing test codifies the buggy behavior

packages/core/src/permissions/permission-manager.test.ts, lines 186–189:

it('handles malformed pattern (no closing paren)', async () => {
  const r = parseRule('Bash(git status');
  expect(r.specifier).toBeUndefined();
});

This test only asserts the shape of the return value — it does not verify the rule's matching behavior. So the current suite passes despite the parser producing a tool-wide catch-all. Any fix needs to update this test to also assert that the malformed rule does not match arbitrary commands.

Suggested fix (preferred: explicit invalid flag)

1. Add an optional flag to PermissionRule in packages/core/src/permissions/types.ts (around lines 45–62):

export interface PermissionRule {
  raw: string;
  toolName: string;
  specifier?: string;
  specifierKind?: SpecifierKind;
  /** True if the raw rule was malformed and should never match anything. */
  invalid?: boolean;
}

2. Set it in parseRule (rule-parser.ts, replacing lines 249–262):

const toolPart = normalized.substring(0, openParen).trim();
if (!normalized.endsWith(')')) {
  // Malformed: unbalanced parentheses. Keep `raw` so UIs can surface the
  // original string, but mark the rule as invalid so it can never match.
  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;
return { raw: trimmed, toolName: canonicalName, specifier, specifierKind };

3. Short-circuit in matchesRule (top of the function, before any other check):

export function matchesRule(rule: PermissionRule, /* ...args */): boolean {
  if (rule.invalid) return false;
  // ...existing logic
}

4. (Recommended) Emit a console.warn in parseRules so users notice the typo instead of the rule being silently dropped:

export function parseRules(raws: string[]): PermissionRule[] {
  return raws
    .filter((r) => r && r.trim())
    .map(parseRule)
    .map((r) => {
      if (r.invalid) {
        console.warn(`[permissions] Ignoring malformed rule: ${r.raw}`);
      }
      return r;
    });
}

Why this shape

  • Explicit: invalid: true is self-documenting and easy to grep for. No magic tool-name string.
  • Safe by construction: one top-of-function check in matchesRule guarantees no invalid rule can ever match, regardless of future changes to the tool-name/meta-category logic.
  • Preserves raw for the warning message and any future diagnostic UI.
  • Tiny surface: four small edits, no public-API changes.

Alternative A — sentinel toolName (no type change)

If you'd rather avoid touching types.ts:

if (!normalized.endsWith(')')) {
  return { raw: trimmed, toolName: '__malformed_rule_unbalanced_parens__' };
}

toolMatchesRuleToolName only returns true for exact canonical matches or the read_file / edit meta-categories, so the sentinel can't match any real tool. Works, but leans on a magic string and is slightly more fragile if the matcher's tool-name logic ever changes.

Alternative B — throw at parse time

Cleanest semantically (fail fast on bad config), but changes the contract of parseRules (currently never throws) and risks breaking users who have one typo among many valid rules. Probably a follow-up rather than the initial fix.

Alternative explicitly rejected — tolerant parse

Trying to "recover" a specifier by e.g. stripping trailing junk after the last ) would silently reinterpret user input — which is exactly the class of behavior that produced this bug. Fail loudly (or inertly), don't guess.

Test updates

Update the existing test in permission-manager.test.ts:186–189 to assert matching behavior, not just shape:

describe('parseRule malformed input', () => {
  it('does not match anything when the closing paren is missing', () => {
    const rule = parseRule('Bash(rm -rf /)*');
    expect(matchesRule(rule, 'run_shell_command', 'git status')).toBe(false);
    expect(matchesRule(rule, 'run_shell_command', 'rm -rf /')).toBe(false);
  });

  it('flags the rule as invalid', () => {
    expect(parseRule('Bash(git status').invalid).toBe(true);
    expect(parseRule('Bash(').invalid).toBe(true);
  });

  it('still parses well-formed rules correctly', () => {
    const rule = parseRule('Bash(rm -rf /)');
    expect(rule.invalid).toBeUndefined();
    expect(matchesRule(rule, 'run_shell_command', 'rm -rf /')).toBe(true);
    expect(matchesRule(rule, 'run_shell_command', 'git status')).toBe(false);
  });
});

(If the sentinel-name alternative is chosen instead, drop the invalid assertion and replace with a check that the sentinel tool name doesn't resolve to any canonical tool.)

Severity

I'd rate this security-relevant but low-risk in practice: the failure mode is "deny too much", not "allow too much", so a malformed deny rule merely bricks the shell rather than letting through something it shouldn't. However, it's trivially user-reachable via a typo and produces a misleading error that points away from the actual cause. An equivalent bug in an allow rule parser would be more serious — worth checking whether any similar silent-widening exists in allow/ask paths while this is being fixed.

Environment

  • @qwen-code/qwen-code 0.14.5 (latest on npm as of 2026-04-19)
  • Bug verified present in current main at packages/core/src/permissions/rule-parser.ts:250–252
  • Existing test that only checks shape (not behavior) at packages/core/src/permissions/permission-manager.test.ts:186–189
  • PermissionRule interface at packages/core/src/permissions/types.ts:45–62
  • Local cli.js patched with the sentinel-name variant and manually verified: malformed deny rule no longer denies unrelated commands; well-formed rules continue to work. The upstream test suite was not run against the patch.

Metadata

Metadata

Labels

type/bugSomething isn't working as expected

Type

No fields configured for Bug.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions