Merge branch 'sh-sanitize-pipeline-params' into 'master'

Redact push options from error logs

Closes #202129

See merge request gitlab-org/gitlab!24540

app/services/git/base_hooks_service.rb

@@ -81,15 +81,17 @@ module Git
     end
 
     def pipeline_params
-      {
-        before: oldrev,
-        after: newrev,
-        ref: ref,
-        variables_attributes: generate_vars_from_push_options || [],
-        push_options: params[:push_options] || {},
-        checkout_sha: Gitlab::DataBuilder::Push.checkout_sha(
-          project.repository, newrev, ref)
-      }
+      strong_memoize(:pipeline_params) do
+        {
+          before: oldrev,
+          after: newrev,
+          ref: ref,
+          variables_attributes: generate_vars_from_push_options || [],
+          push_options: params[:push_options] || {},
+          checkout_sha: Gitlab::DataBuilder::Push.checkout_sha(
+            project.repository, newrev, ref)
+        }
+      end
     end
 
     def ci_variables_from_push_options
@@ -156,12 +158,16 @@ module Git
         project_path: project.full_path,
         message: "Error creating pipeline",
         errors: exception.to_s,
-        pipeline_params: pipeline_params
+        pipeline_params: sanitized_pipeline_params
       }
 
       logger.warn(data)
     end
 
+    def sanitized_pipeline_params
+      pipeline_params.except(:push_options)
+    end
+
     def logger
       if Gitlab::Runtime.sidekiq?
         Sidekiq.logger

changelogs/unreleased/sh-sanitize-pipeline-params.yml

+---
+title: Redact push options from error logs
+merge_request: 24540
+author:
+type: fixed

spec/services/git/branch_push_service_spec.rb

@@ -12,6 +12,7 @@ describe Git::BranchPushService, services: true do
   let(:newrev)   { sample_commit.id }
   let(:branch)   { 'master' }
   let(:ref)      { "refs/heads/#{branch}" }
+  let(:push_options) { nil }
 
   before do
     project.add_maintainer(user)
@@ -19,7 +20,7 @@ describe Git::BranchPushService, services: true do
 
   describe 'Push branches' do
     subject do
-      execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+      execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref, push_options: push_options)
     end
 
     context 'new branch' do
@@ -113,6 +114,20 @@ describe Git::BranchPushService, services: true do
 
         expect { subject }.not_to change { Ci::Pipeline.count }
       end
+
+      context 'with push options' do
+        let(:push_options) { ['mr.create'] }
+
+        it 'sanitizes push options' do
+          allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true)
+          expect(Sidekiq.logger).to receive(:warn) do |args|
+            pipeline_params = args[:pipeline_params]
+            expect(pipeline_params.keys).to match_array(%i(before after ref variables_attributes checkout_sha))
+          end
+
+          expect { subject }.not_to change { Ci::Pipeline.count }
+        end
+      end
     end
   end
 
@@ -637,8 +652,8 @@ describe Git::BranchPushService, services: true do
     end
   end
 
-  def execute_service(project, user, change)
-    service = described_class.new(project, user, change: change)
+  def execute_service(project, user, change, push_options = {})
+    service = described_class.new(project, user, change: change, push_options: push_options)
     service.execute
     service
   end