<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/engineering/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Close this issue](https://contributors.gitlab.com/manage-issue?action=close&projectId=278964&issueIid=628962) </details> <!--IssueSummary end--> ## Context The GitLab MCP server enforces **Always Deny** AI tool-governance rules for external MCP clients such as Claude Code and Cursor. Before a tool runs, governance works out which project or group the call acts on and checks that namespace's rules. This shipped in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/258209 (GitLab 19.6) behind the feature flags `duo_mcp_server_governance_rule_enforcement` and `duo_workflow_local_tool_governance`. Both are disabled by default. It replaced https://gitlab.com/gitlab-org/gitlab/-/merge_requests/253147, which was closed. During review of that MR, AppSec asked for further hardening, and this issue tracks it. Last checked against `master` at db8a3d7ec969. ## Item 1: guard governed-tool argument coverage Mostly done. One gap remains. ### What shipped A tool declares its container arguments with the `container_arguments` macro. The kinds are `project`, `group`, `project_or_group` and `record`. A `record` argument names a record whose project or group is looked up from it. A tool can also override `governed_containers`. [`governable_tools_container_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/ee/spec/lib/ai/tool_rules/governable_tools_container_spec.rb) runs in CI and checks every governable tool: - It declares at least one container argument that it accepts, including `url` when it takes one. - Every input property is declared, or is listed in the spec's `not_a_container` list. That list is the waiver list. - The `not_a_container` entries are not stale. - Every declared kind is one the resolver reads. - A `record:` argument without a lookup lambda comes with a `governed_containers` override. The two gaps that prompted this item, and the record arguments listed in the original description, are now declared: | Argument | Declared in | | --- | --- | | `target_project_id` (`save_merge_request`) | [`save_merge_request_service.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/app/services/mcp/tools/merge_requests/save_merge_request_service.rb#L141-142) | | `pipeline_id` (`save_pipeline`) | [`save_pipeline_service.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/app/services/mcp/tools/pipelines/save_pipeline_service.rb#L12) | | `parent_id` (`save_work_item`) | [`save_work_item_service.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/app/services/mcp/tools/work_items/save_work_item_service.rb#L9) | | `work_items_ids` (`link_work_items`) | [`link_work_items_service.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/app/services/mcp/tools/work_items/link_work_items_service.rb#L7-17) | | `epic_id` (EE `create_issue`) | [`ee/lib/ee/api/helpers/issues_helpers.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/ee/lib/ee/api/helpers/issues_helpers.rb#L66-69) | `EXTRA_CONTAINER_ARGUMENTS` is gone. REST tools declare their arguments through the `:mcp` route setting, which [`ApiTool#container_arguments`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/app/services/mcp/tools/base/api_tool.rb#L40-49) merges with the route's boundary type. ### What remains The guard checks that an override exists. It never calls it, so an override that returns no container for a valid input still passes CI. Three cases were found by hand: - `save_note` with only `note_id`, fixed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/264854 (open) - `save_note` `resolve_discussion` with only `discussion_id`: https://gitlab.com/gitlab-org/gitlab/-/work_items/637282 - `save_merge_request_review` `resolve_discussion`, the same pattern, noted in https://gitlab.com/gitlab-org/gitlab/-/work_items/637282 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/261045 (draft) gives every governable tool its own `governed_containers`, built on the lookup its execute path uses, and treats a failed lookup as no container. After it lands, what each override returns is the thing to check. The direction is under discussion in https://gitlab.com/gitlab-org/gitlab/-/work_items/628962#note_4151627456. One option is a CI spec that calls every override with representative inputs. The other is a runtime refusal, which is Item 2. ## Item 2: refuse a governed call that names no container Not started. A call whose containers all fail to resolve still runs ungoverned. [`check_tool_governance!`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/ee/lib/ee/api/mcp/handlers/call_tool.rb#L35-42) keeps the resolved containers that have the flags on and that the caller can read, and returns before any rule check when none are left. The handler spec `when an identifier resolves to no container` expects the tool to answer for itself. The [user documentation](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/doc/user/ai-governance/tool-governance.md) describes this: if the call names a project or group that does not exist or that you cannot access, its rules are not checked. AppSec asked that such calls be refused, with only tools explicitly marked as having no target skipping the check. Tools fall into three classes: - Always-scoped tools, for example `save_pipeline`, `accept_merge_request` and `add_commit`. Refusing when no container resolves is correct, because it means something went wrong. - Never-scoped tools. These are marked `ungovernable!`: `get_mcp_server_version`, `get_user` and `send_duo_session_input`. - Optionally-scoped tools. `list_projects`, `list_merge_requests` and `list_groups` have `required: []` and legitimately span everything the caller can see. Refusing them when no container is named would break working behavior. The work starts with classifying every governable tool into these classes. There is no marker for always-scoped or optionally-scoped tools in the code yet. Points to settle for the refusal: - Refuse on the unfiltered `governed_containers` result. The flag and readability filter runs afterwards, so refusing on the filtered list would also refuse calls into namespaces that don't have the flag. - With no container, there is no root namespace for [`McpGovernance.governs?`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/ee/lib/ai/tool_rules/mcp_governance.rb#L17-22) to check the flags against. The refusal needs its own gate. - A lookup that fails because the record does not exist, or because the caller cannot see it, returns a refusal instead of the tool's own error. The refusal must not reveal whether the record exists. For optionally-scoped tools, there are three options: 1. Govern by the caller's default GitLab Duo namespace. [`tools/list`](https://gitlab.com/gitlab-org/gitlab/-/blob/db8a3d7ec9690b92bc69825ef373eec0b6152afd/ee/lib/ee/api/mcp/handlers/list_tools.rb#L30-33) already resolves rules against the caller's `governing_namespace`, which falls back to their default GitLab Duo namespace. 1. Refuse the call and require the caller to name an explicit container. 1. Keep allowing the call unscoped, accepting that these tools stay outside enforcement. This is a product decision as much as a code one. The always-scoped refusal does not depend on it and can ship first. ## Item 3: two further AppSec points - The flag-keying concern no longer applies. `ContainerAccess#every_identifier_resolved?` was removed. Governance checks the flags against each resolved container's root namespace. A `url` that matches no project or group resolves to no container, which is the Item 2 case. - The lookups are bounded. A list argument can name at most `MAX_IDENTIFIERS_PER_ARGUMENT` (100) identifiers, and a call can make at most `MAX_LOOKUPS_PER_CALL` (20) path or URL lookups. The per-call cap is behind `mcp_container_lookup_cap` (https://gitlab.com/gitlab-org/gitlab/-/work_items/635194). The hourly MCP endpoint rate limits are rolling out in https://gitlab.com/gitlab-org/gitlab/-/work_items/635267. ## Blocks flag enablement Item 2, and the remaining Item 1 gap, should land before `duo_mcp_server_governance_rule_enforcement` is enabled by default, per the AppSec request.