fix(permissions): prevent MCP permission identity collisions - #10202

@qwen-code /triage

/review

@qwen-code /review

@qwen-code /review

Follow-up after the previous review rounds:

• Current head: 1e5ff83cf3f59950fef6048f7565bd87c4cf0949
• Allow evaluation now uses the collision-filtered grant context only.
• Restrictive deny / ask evaluation preserves raw MCP identity plus the registered provider-safe spelling, so legacy/provider-sanitized restrictive rules remain fail-closed without re-broadening grants.
• Subagent disallowedTools matching now receives the authoritative raw MCP identity as well.
• The no-resolver fallback is pinned to raw identity only for grants.
• Temporary validation workflows have been removed from the PR diff.

Focused fork validation passed 498/498 tests plus tsc --noEmit -p packages/core/tsconfig.json.

A fresh @qwen-code /review has been requested against this head; the currently visible CHANGES_REQUESTED submissions are from older SHAs.

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

@qwen-code /review

🔬 Maintainer review + tmux E2E report — no Critical found at current head

Code review (head 1ad331195f9f)

The only change since the last completed review round (bot round 4 on abd822f8071a, Suggestions only) is a merge of main — verified via compare: the merge touches none of this PR's files, so the reviewed production code is byte-identical at the current head. Spot-checks of the former Criticals at this head:

• R2-3 (two-context restrictive loop broke on first ask, demoting a later-spelling deny): fixed — the loop now only breaks on deny, records the first ask, and applies it after the loop only if no spelling produced deny ("deny that matches a later spelling must still win globally").
• R2-2 (agent-core disallowedTools went raw-only, silently un-blocking sanitized-spelling entries): fixed — matching is now dual: matchesMcpPattern(pattern, name, rawMcpToolName) || matchesMcpPattern(pattern, name); the second (sanitized) pass restores the old spellings, which is the fail-closed direction for disallow lists.
• Grant path semantics verified by reading: grants use collision-filtered legacy aliases (filterUnambiguousMcpPermissionAliases) + the authoritative raw identity appended last; restrictive rules evaluate both spellings; wildcards/server-level rules match on the raw identity without lossy sanitization; matchesRule excludes the raw identity from the legacy-exact alias scan.

No new merge-blocking issues found. Remaining open threads are Suggestion-level (already reported).

E2E (interactive TUI under tmux, real built bundle)

Built the head from source and ran the interactive CLI (Ask permissions mode) against two minimal stdio MCP servers whose names collide under provider sanitization: srv.dot and srv_dot (both expose ping; the dotted server registers as mcp__srv_dot__ping_01lsbax, raw identity mcp__srv.dot__ping). User-scope settings: permissions.allow: ["mcp__srv_dot__*"] — a wildcard written in the sanitized spelling.

Under the pre-PR matching (sanitized-both-sides wildcard), mcp__srv_dot__* prefix-matches mcp__srv_dot__ping_01lsbax and would have auto-approved the second call — the dialog is the observable proof the collision hole is closed.

Methodology notes:

• Headless -p control runs (with and without the allow rule) auto-execute MCP tools either way — non-interactive mode does not enforce MCP confirmations — so the discrimination above was done in the interactive TUI, where ask surfaces a real dialog.
• Local full build initially failed at packages/sdk-typescript: the browser-bundle size guard (220220 > 215*1024). This is not caused by this PR: the merge touched no SDK files, the built browser daemon bundle contains none of this PR's code (verified by symbol scan), and the growth traces to main's recent daemon SDK change (fix(sdk): Surface daemon JSON-RPC error details #10571) landing without a limit bump. I relaxed the constant in my scratch build only, to produce the test bundle. Maintainers may want to raise MAX_DAEMON_BROWSER_BUNDLE_BYTES on main if/when this lane runs in CI.

Status

• No Critical at current head → no Request Changes from me.
• CI on this head has not run the test lanes yet (fork-PR authorization pending), and the standing CHANGES_REQUESTED state predates the fixes — so I'm not approving; a green full-CI pass plus maintainer sign-off is still needed.

— qqqys · code review + tmux E2E

@qwen-code /review

Review follow-ups are pushed on HEAD 98bb096dee92f62a3c0fc01d280d07279a096347.

Key follow-ups since the previous review:

• centralized restrictive L4 evaluation in permission-helpers.ts
• added live resolver KEEP coverage in permissionFlow
• added real ToolRegistry.getUnambiguousMcpPermissionAliases() coverage with actual MCP tools
• added fail-closed no-raw-identity regression coverage
• resolved outdated Critical threads against the current implementation
• updated the PR reviewer test plan with the expanded focused suite

Please review the current HEAD rather than superseded earlier commits.

🔬 Maintainer verification — real local environment, head 98bb096dee

I rebuilt this PR locally and ran it against real MCP servers and a real provider, comparing
the PR bundle with a pre-PR bundle produced from the same worktree. The reported vulnerability
reproduces on main and is closed by this PR; documented backwards compatibility holds.
No Critical found. The three red CI jobs are not caused by this PR — details in F1.

Scope note: an earlier maintainer report covered head 1ad331195f (merge-only delta at the time).
This is an independent run at the current head 98bb096dee, which adds the permission-helpers.ts
centralization and the ToolRegistry alias resolver, and it covers issue-variant 2 (legacy alias
collisions) and the subagent disallowedTools path, which the earlier report did not.

Rig

Bundle separation was verified by symbol scan: dist-pr contains evaluateRestrictivePermissionRules /
getUnambiguousMcpPermissionAliases, dist-base contains neither.

Registered identities the CLI actually produced (read off the wire, matching the issue's model exactly):

1. The vulnerability reproduces, and the PR closes it

--approval-mode default, headless. EXECUTED = auto-approved without confirmation (the bug);
ask = confirmation required.

Backwards compatibility that the PR promises — all still granted:

Screenshots (interactive TUI, Ask permissions mode, real bundles)

Same settings.json (allow: ["mcp__foo.bar__*"]), same prompt, only the bundle differs:

Before (main) — the grant for server foo.bar silently authorizes server foo_bar:

After (this PR) — the same call stops at the confirmation dialog:

After (this PR) — the intended server is still granted by the same rule, so the fix is not a blanket revocation:

Issue variant 2 — one ambiguous legacy alias mcp__srv__foo_bar owned by both foo/bar and foo:bar.
Before: auto-executes. After: confirmation required.

2. Restrictive rules stay fail-closed (no fail-open regression)

Run under --yolo, which bypasses ask but not deny, so only a genuine deny blocks
(control R0/R8 confirm the isolation). "blocked" = the call never reached the MCP server.

(S1/S2 are run in default mode instead, since ask is the observable there; they show that a
historical sanitized-spelling ask rule still overrides an explicit exact allow.)

R5/R6 are the important pair: the alias that lost its grant power is still fully effective for
deny — exactly the asymmetry the PR describes.

3. Subagent disallowedTools

A real subagent (disallowedTools in agent frontmatter) was launched and its own model request was
read off the wire to see which MCP tools it was given:

So the blocklist got more precise without losing the historical spelling.

4. Mutation testing — is each production hunk load-bearing?

Eight mutants applied to the PR's production code, each run against the PR's focused suite
(470 tests) and, where reachable, re-bundled and re-run through the live rig.

M8 survives but is equivalent, not a coverage gap: resolveToolName is an identity map for
mcp__* names, so the only alias that exclusion can drop is the raw identity itself — and
matchesMcpPattern already returns true for pattern === matchTarget before the legacy-exact
scan runs. The exclusion is defensive; removing it changes no reachable outcome.

Everything else is killed twice over — by tests and by the live rig — so no production hunk is dead weight.

5. Repo checks (local, on the merge ref)

• Focused suite from the PR description: 470 passed (6 files).
• Broad slice src/permissions src/core src/tools src/agents: 9269 passed, 6 skipped, 0 failed (221 files).
• tsc --noEmit on packages/core: clean.
• eslint --max-warnings 0 on all 11 changed files: clean.

Findings

F1 — the three red CI jobs are a stale main-side guard, not this PR (actionable: merge main).
Test (ubuntu), Integration Tests (no-AK) and web-shell E2E Smoke all die in the same
npm ci → prepare → build step with the identical error:
Error: Browser daemon SDK bundle is 220220 bytes; expected <= 220160.
That limit is MAX_DAEMON_BROWSER_BUNDLE_BYTES = 215 * 1024 in
packages/sdk-typescript/scripts/build.js — a file this PR does not touch. main already made
that budget advisory in #10630 (2268c9bc06, 2026-08-31 11:48Z), but this branch's last main
merge is 1ad331195f (2026-08-31 03:53Z), so the head tree CI checks out still carries the hard
limit. Locally the same bundle measures 221 474 bytes: it fails the old guard and only warns under
the current one. Merging current origin/main into the branch should clear all three jobs —
no code change needed here. (Please merge rather than rebase, per the force-push note above.)

F2 — Suggestion: an undocumented compatibility break for rules written in the sanitized spelling.
Two cases lose their grant, both fail-closed (extra prompt, never an unintended approval):

This follows directly from making the raw identity authoritative and I think it is the right call,
but the PR description only mentions ambiguous aliases and cross-identity matches losing power —
a unique wildcard written in the provider-safe spelling loses power too, silently. Worth noting
because the confirmation dialog persists and displays the registered name
(permissionRules: [this.registeredToolName], e.g. mcp__foo_bar__evil_1oxrpi0), so a user who
generalizes by hand from what the UI shows would naturally write mcp__foo_bar__* — a rule that
will now never match. A line in docs/users/configuration/settings.md (which currently only
documents "mcp__puppeteer") saying server/wildcard rules must use the raw server and tool
names would close this.

F3 — Nit: two files are not prettier-clean.
packages/core/src/core/permission-helpers.ts and permissionFlow.ts differ from
prettier output (an interface … extends wrap, a ternary wrap, and the fullAliases array).
This will not turn CI red — scripts/lint.js --prettier runs prettier --write . — but the
committed source drifts from repo formatting. npm run format fixes it.

F4 — Observation, no action: the cross-context deny > ask ordering is unreachable in product code.
Both callers of evaluatePermissionFlow (coreToolScheduler.ts:2205 and
Session.ts:7238) first run pm.isToolEnabled(canonicalName) with a bare context — no aliases,
no raw identity — and reject on deny before the flow is entered. Instrumented traces confirm it: a
deny in a sanitized/registered spelling never reaches evaluateRestrictivePermissionRules, which is
why M5 flips no live behavior while still failing two unit tests. The multi-context ordering is
therefore defense-in-depth for the flow's own contract rather than a live gate. The raw-spelling
deny path is live and does depend on restrictive context #1 (R2/R4/R5/R6 traces show
restrictiveMatched=true), so the mechanism as a whole is load-bearing.

Verdict

• Reported issue reproduces on main (both variants) and is closed by this PR, verified in a real
  bundle with real MCP servers, not just in unit tests.
• Documented backwards compatibility (unique legacy aliases, the 63-char truncated form, exact
  registered names, mcp__*, mcp__server, mcp__server__*) is preserved.
• Restrictive rules do not fail open in any spelling I could construct.
• No Critical from me. F1 is what actually blocks the merge and it is a branch-freshness issue, not a
  code issue: merge current main, let CI go green. F2/F3 are non-blocking.
• The standing CHANGES_REQUESTED predates these fixes and can only be cleared by the review bot
  itself or by a human approval/dismissal.

— local end-to-end verification: real MCP servers + real bundle A/B + 8 mutants + interactive TUI screenshots

🔬 Maintainer verification, round 2 — head 75e91d6287 (after main was merged)

Follow-up to my round-1 report
(head 98bb096dee). Since then the branch merged main twice, which moved the base by 54 commits
and touched two files directly on this PR's path (permission-manager.ts, coreToolScheduler.ts).
So I rebuilt the whole rig on the current merge ref and re-ran everything, then used the run to
adjudicate the three [probe] items the review bot deferred in round 7.

Result: the fix survives the merge unchanged, round-1's F1 (red CI) is resolved, and I found one
thing I would fix before merging — a fail-open widening that this PR introduces as a side effect.

Rig (rebuilt, not reused)

Bundle separation re-verified by symbol scan: evaluateRestrictivePermissionRules /
getUnambiguousMcpPermissionAliases appear in dist-pr, in neither file of dist-base.
All 8 registered identities came up byte-identical to round 1 (read off the wire).

1. The fix is intact after the main merge

Grant matrix, --approval-mode default, headless. Identical to round 1, case for case:

Restrictive matrix under --yolo (bypasses ask, not deny, so only a genuine deny blocks;
R0/R8 are the isolation controls): R0 EXECUTED, R1–R7 blocked, R8 EXECUTED — identical to round 1
in both arms. Blocked means the witness file stays empty: the call never reached the MCP server.

Repo checks on the merge ref: focused suite 473 passed (6 files, up from 470 — main added 3),
broad slice src/permissions src/core src/tools src/agents 9309 passed / 6 skipped / 0 failed
(221 files), tsc --noEmit on packages/core clean, eslint --max-warnings 0 on all 11 changed
files clean. Mutants M1–M8 re-run against the 473-test suite: same kill counts as round 1
(M1 3, M2 3, M3 2, M4 4, M5 2, M6 2, M7 2, M8 0 — M8 still the equivalent survivor I argued in round 1).

2. Round-1 F1 (three red CI jobs) is resolved

The merge picked up #10630, which made the daemon bundle budget advisory. Locally the same step now
prints Browser daemon SDK bundle is 221839 bytes / exceeds the 221184-byte warning threshold
as a warning and npm ci exits 0. On CI, Integration Tests (no-AK) — one of the three jobs that
died in that step — is now green (9m29s). Test (ubuntu-latest) is still running as I write this.
No code change was needed; F1 is closed.

3. Adjudicating the three deferred [probe] items

D7-1 — bare * rule: confirmed, and it is the one finding I would act on (N1 below)

D7-2 — disallowedTools ignores legacy aliases: real, but pre-existing, not a regression (N2 below)

D7-3 — "no test covers restrictive-ask-beats-grant-allow": not reproducible at this head

I tried to kill the !restrictiveMatchCtx guard two ways and the suite caught both:

permissionFlow.mcp-restrictive-collision.test.ts > preserves forced ask when no restrictive deny exists
is the test that pins it. And the behavior holds live on the real bundle: with
allow: [<exact registered name>] plus ask: ["mcp__foo_bar"] (historical sanitized spelling),
the call still stops at the prompt — pre-PR and PR, wildcard variant too. I would drop D7-3.

N1 — Suggestion → I'd fix before merge: a bare * rule silently becomes an all-MCP wildcard

matchesMcpPattern pre-PR sanitized the wildcard prefix, and sanitizeToolNameForProvider('')
returns "tool_", so a bare * matched no MCP tool. The PR drops the sanitization
(correctly — that is the whole point) and the prefix becomes the empty string, so
matchTarget.startsWith('') is true for every MCP tool.

Live A/B, same settings.json, only the bundle differs:

Before (main) — allow: ["*"] is inert, the MCP prompt still appears:

After (this PR) — the same rule auto-executes the MCP tool with no prompt, in Ask permissions mode:

Three things make me flag it rather than shrug:

1. It is asymmetric and undocumented. * still matches nothing for non-MCP tools —
   toolMatchesRuleToolName has no wildcard branch (matchesRule(parseRule('*'), 'read_file') →
   false, 'run_shell_command' → false, 'web_fetch' → false, MCP tool → true).
   settings.md documents no bare-* rule, and the PR description does not mention this change.

2. It is reachable from repository-controlled config, not just hand-edited settings.
   A skill's allowedTools entries are handed verbatim to PermissionManager.addSessionAllowRule
   (skill-utils.ts:318, no validation in parseAllowedToolsField), and allowedTools: ["*"] is a
   natural way to write "this skill may use any tool". Driving the real classes:

   applySkillAllowedTools(pm, ['*'])
   pm.evaluate({toolName:'mcp__foo_bar__evil'})  ->  allow
   pm.evaluate({toolName:'read_file'})           ->  default
   

   Before this PR that entry was harmless for MCP tools; after it, it auto-approves all of them
   for the rest of the session.

3. It is untested, and a one-line guard closes it with zero fallout. Adding
   && pattern.length > 1 to the wildcard branch:

   • kills 0 of 473 focused tests (nothing pins the new behavior),
   • restores pre-PR semantics in both directions (allow: ["*"] → ask, deny: ["*"] → inert),
   • leaves every case this PR fixes untouched, re-verified on a rebuilt bundle:
     A1 EXECUTED, A2 ask, B1 EXECUTED, B2 ask, C1 ask, D2 ask, E1 EXECUTED, mcp__srv__* EXECUTED,
     mcp__* EXECUTED.

   

If a bare * should mean "all MCP tools", that is a defensible design — but then it needs a doc
line and a test, and it should probably mean all tools, not only MCP ones. Either way it should not
land as an unremarked side effect of a collision fix.

N2 — Suggestion (pre-existing, not caused by this PR): disallowedTools does not honour legacy aliases

agent-core.ts now threads the raw MCP identity into the subagent blocklist, but it still never
consults permissionAliases. Real subagent launched, its own model request read off the wire
(the tool is mcp__srv2__uniq_name_1quugtb, legacy alias mcp__srv2__uniq_name, unique — so
the alias is a valid grant/deny spelling elsewhere):

The same alias in permissions.deny does block the call at runtime (verified under --yolo:
witness empty, both arms). So an operator can write one spelling that works in settings.json and
silently does nothing in agent frontmatter. Not a regression and not a merge blocker — but since this
PR edits exactly that line, wiring permissionAliases in (or documenting the restriction in
sub-agents.md, which currently only mentions "MCP server-level patterns") is the natural follow-up.

Still open from round 1

• F2 (Suggestion): the sanitized-spelling wildcard compatibility break is still undocumented —
  H1 (allow: ["mcp__foo_bar__*"] for server foo.bar) goes EXECUTED → ask, and the confirmation
  dialog persists the registered name, so a user generalizing from the UI writes exactly the rule
  that will never match. One line in settings.md closes it.
• F3 (Nit): permission-helpers.ts and permissionFlow.ts are still not prettier-clean
  (an interface … extends wrap, a pmForcedAsk: ternary wrap, the fullAliases array).
  CI will not turn red (scripts/lint.js --prettier runs --write), but npm run format fixes it.

Verdict

• The reported issue still reproduces on main and is still closed by this PR, on the merged tree,
  in a real bundle with real MCP servers.
• Backwards compatibility, restrictive fail-closed behavior, and every mutant verdict are unchanged
  by the main merge.
• Round-1 F1 is resolved; CI is green except the one job still running.
• Of the three deferred probes: D7-3 is stale, D7-2 is real but pre-existing, D7-1 is real and I'd
  fix it here — it is one line, it is untested either way, and it is the only place in this PR where
  the change moves in the fail-open direction.
• No Critical from me. With N1 applied I would merge this.

— round 2: rebuilt on the current merge ref · real MCP servers · bundle A/B + candidate-fix bundle · 11 mutants · TUI screenshots

CI follow-up: the previous Qwen Code CI failure was isolated to the unrelated timing-budget test src/utils/shellAstParser.test.ts > classifyShellCommandSafety > classifies adversarial rule inputs within the CPU budget (1361.812 ms vs < 1000 ms). Security Checks passed, and the latest Qwen review reports “no blockers”. I pushed an empty-tree commit (5ee305d) solely to retrigger CI without changing the PR diff. The new upstream Qwen Code CI and Security Checks runs are currently at the fork action_required approval gate, so a maintainer needs to approve the workflows; after approval this should give us a clean fresh signal.