Merge branch '43316-controller-parameters-handling-sensitive-information-should-use-a-more-specific-name' into 'master'

Resolve "Controller parameters handling sensitive information should use a more specific name"

Closes #43316

See merge request gitlab-org/gitlab-ce!17796

13 files changed, 35 insertions, 22 deletions

diff --git a/app/assets/javascripts/ci_variable_list/ci_variable_list.js b/app/assets/javascripts/ci_variable_list/ci_variable_list.js
index 745f3404295..e177a3bfdc7 100644
--- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js
+++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js
@@ -33,7 +33,7 @@ export default class VariableList {
         selector: '.js-ci-variable-input-key',
         default: '',
       },
-      value: {
+      secret_value: {
         selector: '.js-ci-variable-input-value',
         default: '',
       },
@@ -105,7 +105,7 @@ export default class VariableList {
     setupToggleButtons($row[0]);
 
     // Reset the resizable textarea
-    $row.find(this.inputMap.value.selector).css('height', '');
+    $row.find(this.inputMap.secret_value.selector).css('height', '');
 
     const $environmentSelect = $row.find('.js-variable-environment-toggle');
     if ($environmentSelect.length) {
diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb
index cb8771bc97e..6142e75b4c1 100644
--- a/app/controllers/groups/variables_controller.rb
+++ b/app/controllers/groups/variables_controller.rb
@@ -39,7 +39,7 @@ module Groups
     end
 
     def variable_params_attributes
-      %i[id key value protected _destroy]
+      %i[id key secret_value protected _destroy]
     end
 
     def authorize_admin_build!
diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index b478e7b5e05..fa258f3d9af 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/app/controllers/projects/pipeline_schedules_controller.rb
@@ -92,7 +92,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   def schedule_params
     params.require(:schedule)
       .permit(:description, :cron, :cron_timezone, :ref, :active,
-        variables_attributes: [:id, :key, :value, :_destroy] )
+        variables_attributes: [:id, :key, :secret_value, :_destroy] )
   end
 
   def authorize_play_pipeline_schedule!
diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb
index 7eb509e2e64..517d0b026c2 100644
--- a/app/controllers/projects/variables_controller.rb
+++ b/app/controllers/projects/variables_controller.rb
@@ -36,6 +36,6 @@ class Projects::VariablesController < Projects::ApplicationController
   end
 
   def variable_params_attributes
-    %i[id key value protected _destroy]
+    %i[id key secret_value protected _destroy]
   end
 end
diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb
index 1dd0e050ba9..62d768cc6cf 100644
--- a/app/models/ci/group_variable.rb
+++ b/app/models/ci/group_variable.rb
@@ -6,6 +6,8 @@ module Ci
 
     belongs_to :group
 
+    alias_attribute :secret_value, :value
+
     validates :key, uniqueness: {
       scope: :group_id,
       message: "(%{value}) has already been taken"
diff --git a/app/models/ci/pipeline_schedule_variable.rb b/app/models/ci/pipeline_schedule_variable.rb
index af989fb14b4..03df4e3e638 100644
--- a/app/models/ci/pipeline_schedule_variable.rb
+++ b/app/models/ci/pipeline_schedule_variable.rb
@@ -5,6 +5,8 @@ module Ci
 
     belongs_to :pipeline_schedule
 
+    alias_attribute :secret_value, :value
+
     validates :key, uniqueness: { scope: :pipeline_schedule_id }
   end
 end
diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb
index 7c71291de84..452cb910bca 100644
--- a/app/models/ci/variable.rb
+++ b/app/models/ci/variable.rb
@@ -6,6 +6,8 @@ module Ci
 
     belongs_to :project
 
+    alias_attribute :secret_value, :value
+
     validates :key, uniqueness: {
       scope: [:project_id, :environment_scope],
       message: "(%{value}) has already been taken"
diff --git a/app/views/ci/variables/_variable_row.html.haml b/app/views/ci/variables/_variable_row.html.haml
index 15201780451..5d4229c80af 100644
--- a/app/views/ci/variables/_variable_row.html.haml
+++ b/app/views/ci/variables/_variable_row.html.haml
@@ -10,7 +10,7 @@
 - id_input_name = "#{form_field}[variables_attributes][][id]"
 - destroy_input_name = "#{form_field}[variables_attributes][][_destroy]"
 - key_input_name = "#{form_field}[variables_attributes][][key]"
-- value_input_name = "#{form_field}[variables_attributes][][value]"
+- value_input_name = "#{form_field}[variables_attributes][][secret_value]"
 - protected_input_name = "#{form_field}[variables_attributes][][protected]"
 
 %li.js-row.ci-variable-row{ data: { is_persisted: "#{!id.nil?}" } }
diff --git a/changelogs/unreleased/43316-controller-parameters-handling-sensitive-information-should-use-a-more-specific-name.yml b/changelogs/unreleased/43316-controller-parameters-handling-sensitive-information-should-use-a-more-specific-name.yml
new file mode 100644
index 00000000000..de1cee6e436
--- /dev/null
+++ b/changelogs/unreleased/43316-controller-parameters-handling-sensitive-information-should-use-a-more-specific-name.yml
@@ -0,0 +1,5 @@
+---
+title: Use specific names for filtered CI variable controller parameters
+merge_request: 17796
+author:
+type: other
diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
index 966ffdf6996..3506305f755 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -80,7 +80,7 @@ describe Projects::PipelineSchedulesController do
       context 'when variables_attributes has one variable' do
         let(:schedule) do
           basic_param.merge({
-            variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
+            variables_attributes: [{ key: 'AAA', secret_value: 'AAA123' }]
           })
         end
 
@@ -101,7 +101,8 @@ describe Projects::PipelineSchedulesController do
       context 'when variables_attributes has two variables and duplicated' do
         let(:schedule) do
           basic_param.merge({
-            variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
+            variables_attributes: [{ key: 'AAA', secret_value: 'AAA123' },
+                                   { key: 'AAA', secret_value: 'BBB123' }]
           })
         end
 
@@ -152,7 +153,7 @@ describe Projects::PipelineSchedulesController do
         context 'when params include one variable' do
           let(:schedule) do
             basic_param.merge({
-              variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
+              variables_attributes: [{ key: 'AAA', secret_value: 'AAA123' }]
             })
           end
 
@@ -169,7 +170,8 @@ describe Projects::PipelineSchedulesController do
         context 'when params include two duplicated variables' do
           let(:schedule) do
             basic_param.merge({
-              variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
+              variables_attributes: [{ key: 'AAA', secret_value: 'AAA123' },
+                                     { key: 'AAA', secret_value: 'BBB123' }]
             })
           end
 
@@ -194,7 +196,7 @@ describe Projects::PipelineSchedulesController do
         context 'when adds a new variable' do
           let(:schedule) do
             basic_param.merge({
-              variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
+              variables_attributes: [{ key: 'AAA', secret_value: 'AAA123' }]
             })
           end
 
@@ -209,7 +211,7 @@ describe Projects::PipelineSchedulesController do
         context 'when adds a new duplicated variable' do
           let(:schedule) do
             basic_param.merge({
-              variables_attributes: [{ key: 'CCC', value: 'AAA123' }]
+              variables_attributes: [{ key: 'CCC', secret_value: 'AAA123' }]
             })
           end
 
@@ -224,7 +226,7 @@ describe Projects::PipelineSchedulesController do
         context 'when updates a variable' do
           let(:schedule) do
             basic_param.merge({
-              variables_attributes: [{ id: pipeline_schedule_variable.id, value: 'new_value' }]
+              variables_attributes: [{ id: pipeline_schedule_variable.id, secret_value: 'new_value' }]
             })
           end
 
@@ -252,7 +254,7 @@ describe Projects::PipelineSchedulesController do
           let(:schedule) do
             basic_param.merge({
               variables_attributes: [{ id: pipeline_schedule_variable.id, _destroy: true },
-                                     { key: 'CCC', value: 'CCC123' }]
+                                     { key: 'CCC', secret_value: 'CCC123' }]
             })
           end
 
diff --git a/spec/features/projects/pipeline_schedules_spec.rb b/spec/features/projects/pipeline_schedules_spec.rb
index 65e24862d43..065d00d51d4 100644
--- a/spec/features/projects/pipeline_schedules_spec.rb
+++ b/spec/features/projects/pipeline_schedules_spec.rb
@@ -160,9 +160,9 @@ feature 'Pipeline Schedules', :js do
         click_link 'New schedule'
         fill_in_schedule_form
         all('[name="schedule[variables_attributes][][key]"]')[0].set('AAA')
-        all('[name="schedule[variables_attributes][][value]"]')[0].set('AAA123')
+        all('[name="schedule[variables_attributes][][secret_value]"]')[0].set('AAA123')
         all('[name="schedule[variables_attributes][][key]"]')[1].set('BBB')
-        all('[name="schedule[variables_attributes][][value]"]')[1].set('BBB123')
+        all('[name="schedule[variables_attributes][][secret_value]"]')[1].set('BBB123')
         save_pipeline_schedule
       end
 
diff --git a/spec/javascripts/ci_variable_list/native_form_variable_list_spec.js b/spec/javascripts/ci_variable_list/native_form_variable_list_spec.js
index 1ea8d86cb7e..94a0c999d66 100644
--- a/spec/javascripts/ci_variable_list/native_form_variable_list_spec.js
+++ b/spec/javascripts/ci_variable_list/native_form_variable_list_spec.js
@@ -20,7 +20,7 @@ describe('NativeFormVariableList', () => {
     it('should clear out the `name` attribute on the inputs for the last empty row on form submission (avoid BE validation)', () => {
       const $row = $wrapper.find('.js-row');
       expect($row.find('.js-ci-variable-input-key').attr('name')).toBe('schedule[variables_attributes][][key]');
-      expect($row.find('.js-ci-variable-input-value').attr('name')).toBe('schedule[variables_attributes][][value]');
+      expect($row.find('.js-ci-variable-input-value').attr('name')).toBe('schedule[variables_attributes][][secret_value]');
 
       $wrapper.closest('form').trigger('trigger-submit');
 
diff --git a/spec/support/shared_examples/controllers/variables_shared_examples.rb b/spec/support/shared_examples/controllers/variables_shared_examples.rb
index d7acf8c0032..b615a8f54cf 100644
--- a/spec/support/shared_examples/controllers/variables_shared_examples.rb
+++ b/spec/support/shared_examples/controllers/variables_shared_examples.rb
@@ -16,19 +16,19 @@ shared_examples 'PATCH #update updates variables' do
   let(:variable_attributes) do
     { id: variable.id,
       key: variable.key,
-      value: variable.value,
+      secret_value: variable.value,
       protected: variable.protected?.to_s }
   end
   let(:new_variable_attributes) do
     { key: 'new_key',
-      value: 'dummy_value',
+      secret_value: 'dummy_value',
       protected: 'false' }
   end
 
   context 'with invalid new variable parameters' do
     let(:variables_attributes) do
       [
-        variable_attributes.merge(value: 'other_value'),
+        variable_attributes.merge(secret_value: 'other_value'),
         new_variable_attributes.merge(key: '...?')
       ]
     end
@@ -52,7 +52,7 @@ shared_examples 'PATCH #update updates variables' do
     let(:variables_attributes) do
       [
         new_variable_attributes,
-        new_variable_attributes.merge(value: 'other_value')
+        new_variable_attributes.merge(secret_value: 'other_value')
       ]
     end
 
@@ -74,7 +74,7 @@ shared_examples 'PATCH #update updates variables' do
   context 'with valid new variable parameters' do
     let(:variables_attributes) do
       [
-        variable_attributes.merge(value: 'other_value'),
+        variable_attributes.merge(secret_value: 'other_value'),
         new_variable_attributes
       ]
     end