mutually_exclusive in ee/lib/api/project_mirror.rb names a non-existent parameter, disabling the validation

Summary

In ee/lib/api/project_mirror.rb, the params block for PUT /projects/:id/mirror/pull ("Update project pull mirroring settings") contains a mutually_exclusive declaration that names a parameter which does not exist:

params do
  optional :enabled, type: Grape::API::Boolean, desc: 'Enables pull mirroring in a project'
  # ...
  optional :only_mirror_protected_branches, type: Grape::API::Boolean, desc: 'Only mirror protected branches'
  optional :mirror_overwrites_diverged_branches, type: Grape::API::Boolean, desc: 'Pull mirror overwrites diverged branches'
  optional :mirror_branch_regex, type: String, desc: 'Only mirror branches with names that match this regex'
  mutually_exclusive :only_protected_branches, :mirror_branch_regex
  at_least_one_of :enabled, :url, :auth_user, :auth_password, :mirror_overwrites_diverged_branches,
    :mirror_trigger_builds, :only_mirror_protected_branches, :mirror_branch_regex
end

The declared parameter is only_mirror_protected_branches, but the mutually_exclusive call references only_protected_branches — the mirror_ segment is missing. The at_least_one_of declaration on the very next line uses the correct name, which is good evidence this is a typo rather than an intentional reference to some other attribute.

The likely origin is a copy-paste. ee/lib/ee/api/helpers/remote_mirrors_helpers.rb:13 contains the byte-identical line:

mutually_exclusive :only_protected_branches, :mirror_branch_regex

There it is correct, because the push-mirror (remote mirrors) params really do declare only_protected_branches, in lib/api/helpers/remote_mirrors_helpers.rb:10. The pull-mirror endpoint names its equivalent parameter only_mirror_protected_branches instead, and does not use those helpers.

Grape's mutual-exclusion validators resolve their declared attribute names against the keys actually present in the request. Grape::Validations::Validators::MultipleParamsBase#keys_in_common intersects the validator's attribute list with resource_params.keys. Because only_protected_branches is never a key in any real request, that intersection can never reach two entries, so the validation never fires. The rule is dead code.

Verified, not inferred

Confirmed against Grape 2.4.0 with a self-contained script — two endpoints differing only in that one symbol, both sent both parameters:

as written (typo)  -> HTTP 200  {"accepted":true}
with the fix       -> HTTP 400  {"error":"only_mirror_protected_branches, mirror_branch_regex are mutually exclusive"}

The script, runnable from a GitLab checkout with bundle exec ruby <file>:

require 'grape'
require 'rack/test'

B = Grape::API::Boolean

class ProbeAPI < Grape::API
  format :json

  params do
    optional :only_mirror_protected_branches, type: B
    optional :mirror_branch_regex, type: String
    mutually_exclusive :only_protected_branches, :mirror_branch_regex   # as written today
  end
  put('/as_written') { { accepted: true } }

  params do
    optional :only_mirror_protected_branches, type: B
    optional :mirror_branch_regex, type: String
    mutually_exclusive :only_mirror_protected_branches, :mirror_branch_regex   # proposed fix
  end
  put('/fixed') { { accepted: true } }
end

include Rack::Test::Methods
def app = ProbeAPI

both = { only_mirror_protected_branches: true, mirror_branch_regex: 'main' }
put '/as_written', both
puts "as written (typo)  -> HTTP #{last_response.status}  #{last_response.body}"
put '/fixed', both
puts "with the fix       -> HTTP #{last_response.status}  #{last_response.body}"

Ruled out: nothing else declares this parameter

The name is not supplied by a helper, a shared params block, or any other include:

  • The params block above declares all eight parameters inline. Neither that block nor ee/lib/api/project_mirror.rb as a whole contains a single use : statement, so no shared params block is mixed in.

  • The only place only_protected_branches is declared as a parameter anywhere is lib/api/helpers/remote_mirrors_helpers.rb:10, which belongs to the push-mirror endpoints and is never referenced from ee/lib/api/project_mirror.rb.

  • Grape's own route introspection confirms it. Comparing the two entries in route.params for this endpoint:

    only_mirror_protected_branches  {:required=>false, :desc=>"Only mirror protected branches", :type=>"Grape::API::Boolean"}
only_protected_branches         {:required=>false}

    Every genuinely declared parameter carries desc and type. The bare {:required=>false} entry is what Grape records for a name referenced by a validator with no requires/optional declaration behind it. Since it is undeclared, declared_params(include_missing: false) in the endpoint body never returns it, so the value is discarded even if a client sends it.

Impact 1: the documented constraint is not enforced

doc/api/project_pull_mirroring.md documents this constraint twice — on the request attribute tables for both the update and the create/configure operations:

mirror_branch_regex — Contains a regular expression. Only branches with names matching the regex are mirrored. Requires only_mirror_protected_branches to be disabled.

Today a request that sets both only_mirror_protected_branches=true and mirror_branch_regex=<pattern> is accepted with no 400, contradicting that documentation, and both settings reach the service.

Worth the owning team confirming the intended behaviour before fixing: whether the combination should be rejected outright, or whether one setting should take precedence and the documentation should change instead.

Impact 2: a non-existent parameter leaks into the published OpenAPI spec

The gitlab-grape-openapi gem generates doc/api/openapi/openapi_v3.yaml from these Grape definitions. From version 0.5.0 the generator reads mutually_exclusive declarations and appends a prose note to each named parameter's description, since OpenAPI 3.0 has no native keyword for cross-parameter exclusion. The generator trusts the names in the declaration, so it emitted a request-body property for only_protected_branches, which this endpoint does not accept:

only_protected_branches:
  type: string
  nullable: true
  description: Mutually exclusive with `mirror_branch_regex`.

Two details worth noting:

  • The description consists solely of the generated note, because no desc: exists for it anywhere — nothing declares this parameter.
  • Its type is string, not the Grape::API::Boolean used by the real parameter. That follows from the same root cause: Grape records no type for a validator-only name, so the generator falls back to string.

So the published API reference now advertises a string parameter that the endpoint silently ignores. That has no behavioural effect by itself, but it is incorrect user-facing documentation.

The gem is working as designed here — it faithfully reports what the source declares. The bump only made a pre-existing typo visible.

Steps to reproduce

curl --request PUT \
  --header "PRIVATE-TOKEN: <your_access_token>" \
  --url "https://gitlab.example.com/api/v4/projects/<project_id>/mirror/pull" \
  --data "only_mirror_protected_branches=true" \
  --data "mirror_branch_regex=^release/.*"

Expected: 400 Bad Request, per the documented constraint. Actual: the request succeeds and both settings are persisted.

Proposed fix

-        mutually_exclusive :only_protected_branches, :mirror_branch_regex
+        mutually_exclusive :only_mirror_protected_branches, :mirror_branch_regex

A one-word change with a real behaviour consequence. It needs:

  • A request spec asserting that sending both parameters returns 400. No such coverage exists today, which is why this went unnoticed for eleven months. The nearest existing example, in ee/spec/requests/api/project_mirror_spec.rb, sets only_mirror_protected_branches without mirror_branch_regex, so it never exercises the constraint.
  • A Changelog: fixed trailer with EE: true, since it changes API behaviour.
  • Regeneration of doc/api/openapi/openapi_v3.yaml via bin/rake gitlab:openapi:v3:generate, which drops the phantom property.

Risk

This switches on a validation that has been inert for eleven months, so requests that currently succeed with both parameters set would begin returning 400. That is a user-visible change for any existing caller sending both. Mitigating factor: the endpoint carries route_setting :lifecycle, :experiment, so it sits outside GA stability guarantees. The team should decide whether it warrants a deprecation note first.

Scope and audit

An audit of the wider API found this is an isolated case, not a pattern. Two independent checks agreed:

  • Scanning all 1,847 operations in the generated v3 spec for a parameter whose description consists only of a generated exclusion note — the signature of a name nothing declares — returned exactly one hit: this one.
  • A source-level audit of all 115 mutually_exclusive, exactly_one_of, at_least_one_of, and all_or_none_of declarations across lib/api and ee/lib/api flagged six candidates. Five were confirmed benign:
    • Two in lib/api/helpers/snippets_helpers.rb, where the referenced names are declared in the endpoint block that does use :update_file_params.
    • Two in ee/lib/api/ldap_group_links.rb, where the parameters are declared with string names rather than symbols.
    • One in lib/api/pages_domains.rb, where the referenced names are the as: aliases of real parameters — confirmed working by the passing spec "fails to create pages domain without key".

Caveat: the generator skips mutually_exclusive constraints nested inside request-body object properties, so a typo in a nested block would not surface as a phantom property in the spec. The source-level audit does cover those nested blocks and found nothing further.

History

  • Introduced in !167901 (merged) ("REST API: add an endpoint to configure pull mirrors"), commit ce0aeec2ee7e, 2024-10-18.
  • That MR contributed to #494294 (closed).
  • Owning team per .gitlab/CODEOWNERS: [Source Code Backend] @gitlab-com/create-team/source-code/backend. The file has no explicit CODEOWNERS entry, but it declares feature_category :source_code_management, and the mirror services it calls sit in that section.