Detect duplicate variable keys from git push options

What does this MR do and why?

Duplicate CI variable keys passed through git push options were silently accepted, so git push -o ci.variable="FOO=1" -o ci.variable="FOO=2" created a pipeline holding two FOO variables instead of being rejected.

Gitlab::Ci::Pipeline::Chain::Build::Associations#validate_uniqueness reads var[:key], but pipelines created from a push reach it through Sidekiq, which serialises variables_attributes to string keys, so var[:key] was always nil and the check never fired. The top-level symbolize_keys in the worker does not reach the nested entries.

Ci::CreatePipelineService now symbolizes each variables_attributes entry before the chain runs, so every caller — the worker, the API, GraphQL, triggers — is read the same way. With that in place the string-key fallback observe_pipeline_variables carried for push options is unreachable and is removed.

Ci::PipelineCreation::PushOptions#variables is left emitting string keys on purpose: its output becomes Sidekiq job arguments, which must stay JSON-native.

References

Differences

Before: git push -o ci.variable="FOO=1" -o ci.variable="FOO=2" created a pipeline containing two variables named FOO, with no error.

After: the push is rejected with Duplicate variable name: FOO, matching the REST API, GraphQL, run pipeline UI, trigger token and ChatOps paths.

How to set up and validate locally

  1. Use a project that has a .gitlab-ci.yml.

  2. Push the same variable key twice:

    git push -o ci.variable="FOO=1" -o ci.variable="FOO=2"
  3. The pipeline is now rejected with Duplicate variable name: FOO.

  4. Push two distinct keys and confirm the pipeline is still created normally.

Run the specs:

bundle exec rspec spec/lib/ci/pipeline_creation/push_options_spec.rb spec/lib/gitlab/ci/pipeline/chain/build/associations_spec.rb

The new associations_spec.rb context builds variables_attributes through the real producer, so it fails on master and passes with this change. Both existing specs passed either way, which is why the bug survived.

Edited by Jeston Singh

Merge request reports

Loading
Loading