Enforce Deny tool rules for external MCP clients

Raounak Sharmarequested to merge
623329-enforce-deny-external-mcp into master

What does this MR do and why?

Split out of !252615 (closed), which was closed as too large. This is part 2 of 2 and carries the actual behaviour change. Part 1 is !253145 (merged), which this MR targets.

Tool rules set to Always Deny were enforced for Duo Agent Platform workflows but not for external MCP clients. POST /api/v4/mcp authenticated and then executed, with nothing consulting ai_tool_rules in between — so an administrator who denied a tool in the AI Governance UI still saw Claude Code or Cursor run it, including manage_pipeline, the one governable tool that can delete a resource.

The check now happens on the server, where it holds regardless of how a client is configured. Behind duo_mcp_external_governance, off by default.

How the verdict is decided

ResolutionService#permission_for answers for one tool by its bare name. execute already answered a related question, but emitted prefixed spellings (gitlab_get_work_item_notes) that a caller holding get_work_item_notes cannot match.

Ai::ToolRules::McpGovernance takes the containers resolved from the call's arguments (part 1), asks each for its verdict, and returns the most restrictive. A call spanning two projects is denied if either denies it, so ordering the arguments cannot skip a Deny.

Three decisions worth flagging to a reviewer:

  • The flag is checked per resolved container, not once up front. Feature.enabled? with no actor returns false for an actor-gated flag, so an early return would switch governance off for exactly the groups it was enabled for.
  • The verdict is looked up under the tool's canonical name. Aliases are not settable in the UI, so without resolving them a rule could be routed around by spelling the tool differently — get_job_log instead of get_job.
  • A call that resolves no container is allowed. There is no namespace, so there are no rules to apply. Refusing every unscoped call would break tools that legitimately take no project.

The error class and the enforcement seam live in CE because Grape freezes namespace handlers before prepend_mod runs, and CE cannot name an ee/ constant. CE's hook is a no-op.

A tool is served under one name and governed under another

Nine of the served tools carry their rule under a different name. The server serves add_commit; the rule an administrator sets in the governance UI is create_commit. GovernedMcpTools already computed that mapping — by intersecting each tool's declared tool_aliases with the Duo catalog in Registry.all_tool_names — but nothing consulted it when resolving a verdict.

This was caught in manual testing, not by the specs. A Deny set in the UI simply did not apply, with nothing to indicate it: permission_for read the served name, found no rule, and fell back to the tool's annotation-derived default. Measured on a live instance before the fix, a deny on get_work_item_notes left both spellings returning HTTP 200.

Served as Rule is stored under
add_commit (destroy) create_commit
add_branch create_branch
save_merge_request create_merge_request, update_merge_request
save_work_item create_work_item, update_work_item
save_note create_merge_request_note, create_work_item_note
get_workitem_notes get_work_item_notes
get_merge_request_notes list_all_merge_request_notes
get_merge_request_diffs list_merge_request_diffs
list_merge_requests gitlab_merge_request_search

That is most of the write surface, so Deny was unenforced exactly where it matters most.

Resolution now consults every name a tool answers to and takes the most restrictive verdict, so either spelling reaches the rule. Alias resolution alone could not have fixed this: resolve_alias maps an alias to the served name, while the rule lives under the alias — it points away from the rule.

governed_tool_identity_spec walks all 12 mappings and asserts a Deny on the governing name denies both spellings, so a tool added later cannot reopen the hole silently.

What is governed

41 tools are served by the MCP server. 22 carry a rule under their own name — the rest are either unlisted, in Registry::UNGOVERNED_TOOLS, or resolve to a Duo Agent Platform catalog name and are governed under that name instead.

By action category, from each tool's own annotations:

Category Count Examples
Read 15 get_pipeline, search_labels
Write 4 attach_scan_profile, fork_repository, link_work_items, save_merge_request_review
Delete 3 manage_pipeline, accept_merge_request, save_pipeline

How to reproduce the issue

Verified on GDK.

Setup

  1. Create a PAT with the mcp scope (api alone is not sufficient).

  2. Enable the flags:

    Feature.enable(:duo_mcp_tool_governance)       # registry
Feature.enable(:duo_mcp_external_governance)   # this MR

  3. The AI Governance page needs a top-level group with Duo features on. On self-managed the group Duo settings page is SaaS-gated, so set it directly:

    Group.find_by_full_path('your-group').namespace_settings.update!(duo_features_enabled: true)

  4. Open Group → Settings → GitLab Duo → Governance → Tool management and set get_work_item_types to Always Deny.

Reproduce on master

curl -sS -X POST http://gdk.test:3000/api/v4/mcp \
  -H "Authorization: Bearer $TOKEN" -H 'Content-Type: application/json' \
  -d '{"jsonrpc":"2.0","id":1,"method":"tools/call",
       "params":{"name":"get_work_item_types","arguments":{"group_id":"your-group"}}}'

Observed wrong result — the denied tool runs and returns its payload:

{"jsonrpc":"2.0","result":{"content":[{"type":"text","text":"{\"workItemTypes\":[…]}"}],
 "isError":false},"id":1}

How to test the fix

Same request on this branch. Pass a scope argument — a call naming no project or group resolves no namespace and is served ungoverned by design.

{"jsonrpc":"2.0","error":{"code":-32002,"message":"Tool denied",
 "data":{"tool":"get_work_item_types"}},"id":1}

with HTTP 403. Set the tool back to Allow and the same call returns its result.

Note the Flipper cache: the running Puma process can take up to a minute to see a flag change made from the console.

Cases worth checking, each of which was a bug found in review

Case How to check Expected
REST-shaped tool Deny manage_pipeline, call with {"id":"group/project","pipeline_id":"1","name":"delete"} -32002
Alias Deny get_job, call it as get_job_log -32002 — an alias is not settable in the UI, so it must resolve to the canonical rule
Catalog name Deny create_commit, call add_commit -32002 — the UI name and the wire name differ for 9 tools
Project overrides group Allow on the group, Deny on one project, call naming that project -32002
List spanning namespaces Deny on the second of two namespaces in project_ids -32002, regardless of order
Wrong-type Global ID Pass gid://gitlab/Group/1 as a project_id ungoverned, not resolved as project 1
Flag off Feature.disable(:duo_mcp_external_governance) call executes as before
Ask Set a tool to Always Ask executes — only Deny stops a call on this path

Run the tests

bundle exec rspec spec/services/mcp spec/lib/api/mcp \
  ee/spec/lib/ai/tool_rules ee/spec/services/ai/tool_rules \
  ee/spec/lib/ee/api/mcp ee/spec/requests/api/mcp

QA

No end-to-end QA test added. This is an API-level policy decision with no UI surface of its own — ee/spec/requests/api/mcp/base_spec.rb drives the full stack through POST /api/v4/mcp and asserts the JSON-RPC error contract and status code, which is the boundary an E2E test would cover. The governance UI that sets these rules is unchanged.

Known limits

  • Only Deny stops a call. Ask has no prompt to raise on this path, so it executes. Documented.
  • A call must name a project or group. get_mcp_server_version takes no arguments and cannot be governed; it is declared ungovernable explicitly.
  • tools/list is not filtered. That is !252995 (merged).
  • A call naming several containers answers to all of them. Most restrictive wins, so a Deny cannot be skipped by argument ordering.
  • Namespace resolution is not user-scoped. A caller can name a project they cannot access; governance resolves that namespace and may answer 403 where another error would otherwise surface. Not a bypass — the tool still enforces its own authorization — but it is a weak signal that a rule exists there.

Open questions for review

  1. fork_repository is governed by the source project (id), not the fork target (namespace_id). For a write, the target may matter more.

Follow-up: tool_call_namespace now resolves less than governance does

ee/lib/ee/api/mcp/handlers/call_tool.rb still has tool_call_namespace, which reads only project_id / group_id and returns a single root ancestor. It feeds analytics attribution and expanded logging.

Governance deliberately stopped using it, because the two want different things:

tool_call_namespace governance
Reads project_id / group_id whatever argument the tool declares
Returns root ancestor only the container, so project rules apply
Returns one all of them, for tools taking lists
Cost of a miss an analytics label the enforcement boundary

So logging and governance can now disagree about which namespace a call belongs to: for the nine tools spelling their scope argument something other than project_id, tool_call_namespace resolves nothing and the log line carries no namespace, while governance resolves it correctly. Rewriting it on top of GovernanceNamespaceResolver would fix attribution too, but it changes logging behaviour and is not needed for enforcement.

Deviation from the issue

The implementation plan (https://gitlab.com/gitlab-org/gitlab/-/issues/612377) says to reuse tool_call_namespace(params) "rather than duplicating namespace resolution". This MR does not, for the reason above: that helper misses nine of the twenty-one governable tools, and a miss there is a silently ungoverned call rather than a missing log field.

https://gitlab.com/gitlab-org/gitlab/-/issues/623329 specifies check_tool_rule!(tool_name, current_user). It ended up as check_tool_rule!(tool, tool_name, params): the EE override needs the tool in order to ask it for its scope arguments, and nothing uses current_user.

References

Edited by Raounak Sharma

Merge request reports