Merge branch '13784-validate-variables-for-masking' into 'master'

Add control for masking variable values in runner logs

Closes #13784

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

app/assets/javascripts/ci_variable_list/ci_variable_list.js

@@ -40,6 +40,12 @@ export default class VariableList {
         // converted. we need the value as a string.
         default: $('.js-ci-variable-input-protected').attr('data-default'),
       },
+      masked: {
+        selector: '.js-ci-variable-input-masked',
+        // use `attr` instead of `data` as we don't want the value to be
+        // converted. we need the value as a string.
+        default: $('.js-ci-variable-input-masked').attr('data-default'),
+      },
       environment_scope: {
         // We can't use a `.js-` class here because
         // gl_dropdown replaces the <input> and doesn't copy over the class
@@ -88,13 +94,16 @@ export default class VariableList {
       }
     });
 
+    this.$container.on('input trigger-change', inputSelector, e => {
       // Always make sure there is an empty last row
-    this.$container.on('input trigger-change', inputSelector, () => {
       const $lastRow = this.$container.find('.js-row').last();
 
       if (this.checkIfRowTouched($lastRow)) {
         this.insertRow($lastRow);
       }
+
+      // If masked, validate value against regex
+      this.validateMaskability($(e.currentTarget).closest('.js-row'));
     });
   }
 
@@ -171,12 +180,33 @@ export default class VariableList {
 
   checkIfRowTouched($row) {
     return Object.keys(this.inputMap).some(name => {
+      // Row should not qualify as touched if only switches have been touched
+      if (['protected', 'masked'].includes(name)) return false;
+
       const entry = this.inputMap[name];
       const $el = $row.find(entry.selector);
       return $el.length && $el.val() !== entry.default;
     });
   }
 
+  validateMaskability($row) {
+    const invalidInputClass = 'gl-field-error-outline';
+
+    const maskableRegex = /^\w{8,}$/; // Eight or more alphanumeric characters plus underscores
+    const variableValue = $row.find(this.inputMap.secret_value.selector).val();
+    const isValueMaskable = maskableRegex.test(variableValue) || variableValue === '';
+    const isMaskedChecked = $row.find(this.inputMap.masked.selector).val() === 'true';
+
+    // Show a validation error if the user wants to mask an unmaskable variable value
+    $row
+      .find(this.inputMap.secret_value.selector)
+      .toggleClass(invalidInputClass, isMaskedChecked && !isValueMaskable);
+    $row
+      .find('.js-secret-value-placeholder')
+      .toggleClass(invalidInputClass, isMaskedChecked && !isValueMaskable);
+    $row.find('.masking-validation-error').toggle(isMaskedChecked && !isValueMaskable);
+  }
+
   toggleEnableRow(isEnabled = true) {
     this.$container.find(this.inputMap.key.selector).attr('disabled', !isEnabled);
     this.$container.find('.js-row-remove-button').attr('disabled', !isEnabled);

app/assets/stylesheets/framework/ci_variable_list.scss

@@ -66,6 +66,7 @@
   }
 }
 
+.ci-variable-masked-item,
 .ci-variable-protected-item {
   flex: 0 1 auto;
   display: flex;

app/controllers/groups/variables_controller.rb

@@ -41,7 +41,7 @@ module Groups
     end
 
     def variable_params_attributes
-      %i[id key secret_value protected _destroy]
+      %i[id key secret_value protected masked _destroy]
     end
 
     def authorize_admin_build!

app/controllers/projects/variables_controller.rb

@@ -38,6 +38,6 @@ class Projects::VariablesController < Projects::ApplicationController
   end
 
   def variable_params_attributes
-    %i[id key secret_value protected _destroy]
+    %i[id key secret_value protected masked _destroy]
   end
 end

app/helpers/ci_variables_helper.rb

@@ -12,4 +12,12 @@ module CiVariablesHelper
       ci_variable_protected_by_default?
     end
   end
+
+  def ci_variable_masked?(variable, only_key_value)
+    if variable && !only_key_value
+      variable.masked
+    else
+      true
+    end
+  end
 end

app/serializers/group_variable_entity.rb

@@ -6,4 +6,5 @@ class GroupVariableEntity < Grape::Entity
   expose :value
 
   expose :protected?, as: :protected
+  expose :masked?, as: :masked
 end

app/serializers/variable_entity.rb

@@ -6,4 +6,5 @@ class VariableEntity < Grape::Entity
   expose :value
 
   expose :protected?, as: :protected
+  expose :masked?, as: :masked
 end

app/views/ci/variables/_content.html.haml

-= _('Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use environment variables for passwords, secret keys, or whatever you want.')
+= _('Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. Additionally, they will be masked by default so they are hidden in job logs, though they must match certain regexp requirements to do so. You can use environment variables for passwords, secret keys, or whatever you want.')
 = _('You may also add variables that are made available to the running application by prepending the variable key with <code>K8S_SECRET_</code>.').html_safe
 = link_to _('More information'), help_page_path('ci/variables/README', anchor: 'variables')

app/views/ci/variables/_header.html.haml

 - expanded = local_assigns.fetch(:expanded)
 
 %h4
-  = _('Environment variables')
+  = _('Variables')
   = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'variables'), target: '_blank', rel: 'noopener noreferrer'
 
 %button.btn.btn-default.js-settings-toggle{ type: 'button' }

app/views/ci/variables/_variable_row.html.haml

@@ -7,12 +7,15 @@
 - value = variable&.value
 - is_protected_default = ci_variable_protected_by_default?
 - is_protected = ci_variable_protected?(variable, only_key_value)
+- is_masked_default = true
+- is_masked = ci_variable_masked?(variable, only_key_value)
 
 - 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][][secret_value]"
 - protected_input_name = "#{form_field}[variables_attributes][][protected]"
+- masked_input_name = "#{form_field}[variables_attributes][][masked]"
 
 %li.js-row.ci-variable-row{ data: { is_persisted: "#{!id.nil?}" } }
   .ci-variable-row-body
@@ -22,7 +25,7 @@
       name: key_input_name,
       value: key,
       placeholder: s_('CiVariables|Input variable key') }
-    .ci-variable-body-item
+    .ci-variable-body-item.gl-show-field-errors
       .form-control.js-secret-value-placeholder.qa-ci-variable-input-value{ class: ('hide' unless id) }
         = '*' * 20
       %textarea.js-ci-variable-input-value.js-secret-value.qa-ci-variable-input-value.form-control{ class: ('hide' if id),
@@ -30,6 +33,7 @@
         name: value_input_name,
         placeholder: s_('CiVariables|Input variable value') }
         = value
+      %p.masking-validation-error.gl-field-error.hide= s_("CiVariables|This variable will not be masked")
     - unless only_key_value
       .ci-variable-body-item.ci-variable-protected-item
         .append-right-default
@@ -46,5 +50,21 @@
             = sprite_icon('status_success_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-checked')
             = sprite_icon('status_failed_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-unchecked')
       = render_if_exists 'ci/variables/environment_scope', form_field: form_field, variable: variable
+    - unless only_key_value
+      .ci-variable-body-item.ci-variable-masked-item
+        .append-right-default
+          = s_("CiVariable|Masked")
+        %button{ type: 'button',
+          class: "js-project-feature-toggle project-feature-toggle #{'is-checked' if is_masked}",
+          "aria-label": s_("CiVariable|Toggle masked") }
+          %input{ type: "hidden",
+            class: 'js-ci-variable-input-masked js-project-feature-toggle-input',
+            name: masked_input_name,
+            value: is_masked,
+            data: { default: is_masked_default.to_s } }
+          %span.toggle-icon
+            = sprite_icon('status_success_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-checked')
+            = sprite_icon('status_failed_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-unchecked')
+      = render_if_exists 'ci/variables/environment_scope', form_field: form_field, variable: variable
   %button.js-row-remove-button.ci-variable-row-remove-button{ type: 'button', 'aria-label': s_('CiVariables|Remove variable row') }
     = icon('minus-circle')

changelogs/unreleased/13784-validate-variables-for-masking.yml

+---
+title: Add control for masking variable values in runner logs
+merge_request: 25476
+author:
+type: added

locale/gitlab.pot

@@ -1620,6 +1620,9 @@ msgstr ""
 msgid "CiVariables|Remove variable row"
 msgstr ""
 
+msgid "CiVariables|This variable will not be masked"
+msgstr ""
+
 msgid "CiVariable|* (All environments)"
 msgstr ""
 
@@ -1629,9 +1632,15 @@ msgstr ""
 msgid "CiVariable|Error occurred while saving variables"
 msgstr ""
 
+msgid "CiVariable|Masked"
+msgstr ""
+
 msgid "CiVariable|Protected"
 msgstr ""
 
+msgid "CiVariable|Toggle masked"
+msgstr ""
+
 msgid "CiVariable|Toggle protected"
 msgstr ""
 
@@ -3181,10 +3190,7 @@ msgstr ""
 msgid "Enter the merge request title"
 msgstr ""
 
-msgid "Environment variables"
-msgstr ""
-
-msgid "Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use environment variables for passwords, secret keys, or whatever you want."
+msgid "Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. Additionally, they will be masked by default so they are hidden in job logs, though they must match certain regexp requirements to do so. You can use environment variables for passwords, secret keys, or whatever you want."
 msgstr ""
 
 msgid "Environment variables are configured by your administrator to be %{link_start}protected%{link_end} by default"
@@ -8948,6 +8954,9 @@ msgstr ""
 msgid "Value"
 msgstr ""
 
+msgid "Variables"
+msgstr ""
+
 msgid "Various container registry settings."
 msgstr ""
 

spec/features/group_variables_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe 'Group variables', :js do
   let(:user) { create(:user) }
   let(:group) { create(:group) }
-  let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test_value', group: group) }
+  let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test_value', masked: true, group: group) }
   let(:page_path) { group_settings_ci_cd_path(group) }
 
   before do

spec/features/project_variables_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe 'Project variables', :js do
   let(:user)     { create(:user) }
   let(:project)  { create(:project) }
-  let(:variable) { create(:ci_variable, key: 'test_key', value: 'test_value') }
+  let(:variable) { create(:ci_variable, key: 'test_key', value: 'test_value', masked: true) }
   let(:page_path) { project_settings_ci_cd_path(project) }
 
   before do

spec/fixtures/api/schemas/variable.json

@@ -4,12 +4,14 @@
     "id",
     "key",
     "value",
+    "masked",
     "protected"
   ],
   "properties": {
     "id": { "type": "integer" },
     "key": { "type": "string" },
     "value": { "type": "string" },
+    "masked": { "type": "boolean" },
     "protected": { "type": "boolean" },
     "environment_scope": { "type": "string", "optional": true }
   },

spec/javascripts/ci_variable_list/ci_variable_list_spec.js

@@ -127,20 +127,25 @@ describe('VariableList', () => {
       variableList.init();
     });
 
-    it('should add another row when editing the last rows protected checkbox', done => {
+    it('should not add another row when editing the last rows protected checkbox', done => {
       const $row = $wrapper.find('.js-row:last-child');
       $row.find('.ci-variable-protected-item .js-project-feature-toggle').click();
 
       getSetTimeoutPromise()
         .then(() => {
-          expect($wrapper.find('.js-row').length).toBe(2);
+          expect($wrapper.find('.js-row').length).toBe(1);
+        })
+        .then(done)
+        .catch(done.fail);
+    });
 
-          // Check for the correct default in the new row
-          const $protectedInput = $wrapper
-            .find('.js-row:last-child')
-            .find('.js-ci-variable-input-protected');
+    it('should not add another row when editing the last rows masked checkbox', done => {
+      const $row = $wrapper.find('.js-row:last-child');
+      $row.find('.ci-variable-masked-item .js-project-feature-toggle').click();
 
-          expect($protectedInput.val()).toBe('false');
+      getSetTimeoutPromise()
+        .then(() => {
+          expect($wrapper.find('.js-row').length).toBe(1);
         })
         .then(done)
         .catch(done.fail);

spec/support/features/variable_list_shared_examples.rb

@@ -23,10 +23,13 @@ shared_examples 'variable list' do
     end
   end
 
-  it 'adds empty variable' do
+  it 'adds a new protected variable' do
     page.within('.js-ci-variable-list-section .js-row:last-child') do
       find('.js-ci-variable-input-key').set('key')
-      find('.js-ci-variable-input-value').set('')
+      find('.js-ci-variable-input-value').set('key_value')
+      find('.ci-variable-protected-item .js-project-feature-toggle').click
+
+      expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
     end
 
     click_button('Save variables')
@@ -37,17 +40,17 @@ shared_examples 'variable list' do
     # We check the first row because it re-sorts to alphabetical order on refresh
     page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
       expect(find('.js-ci-variable-input-key').value).to eq('key')
-      expect(find('.js-ci-variable-input-value', visible: false).value).to eq('')
+      expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value')
+      expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
     end
   end
 
-  it 'adds new protected variable' do
+  it 'defaults to masked' do
     page.within('.js-ci-variable-list-section .js-row:last-child') do
       find('.js-ci-variable-input-key').set('key')
       find('.js-ci-variable-input-value').set('key_value')
-      find('.ci-variable-protected-item .js-project-feature-toggle').click
 
-      expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
     end
 
     click_button('Save variables')
@@ -59,7 +62,7 @@ shared_examples 'variable list' do
     page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
       expect(find('.js-ci-variable-input-key').value).to eq('key')
       expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value')
-      expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
     end
   end
 
@@ -163,27 +166,6 @@ shared_examples 'variable list' do
     end
   end
 
-  it 'edits variable with empty value' do
-    page.within('.js-ci-variable-list-section') do
-      click_button('Reveal value')
-
-      page.within('.js-row:nth-child(1)') do
-        find('.js-ci-variable-input-key').set('new_key')
-        find('.js-ci-variable-input-value').set('')
-      end
-
-      click_button('Save variables')
-      wait_for_requests
-
-      visit page_path
-
-      page.within('.js-row:nth-child(1)') do
-        expect(find('.js-ci-variable-input-key').value).to eq('new_key')
-        expect(find('.js-ci-variable-input-value', visible: false).value).to eq('')
-      end
-    end
-  end
-
   it 'edits variable to be protected' do
     # Create the unprotected variable
     page.within('.js-ci-variable-list-section .js-row:last-child') do
@@ -251,6 +233,57 @@ shared_examples 'variable list' do
     end
   end
 
+  it 'edits variable to be unmasked' do
+    page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
+
+      find('.ci-variable-masked-item .js-project-feature-toggle').click
+
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false')
+    end
+
+    click_button('Save variables')
+    wait_for_requests
+
+    visit page_path
+
+    page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false')
+    end
+  end
+
+  it 'edits variable to be masked' do
+    page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
+
+      find('.ci-variable-masked-item .js-project-feature-toggle').click
+
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false')
+    end
+
+    click_button('Save variables')
+    wait_for_requests
+
+    visit page_path
+
+    page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false')
+
+      find('.ci-variable-masked-item .js-project-feature-toggle').click
+
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
+    end
+
+    click_button('Save variables')
+    wait_for_requests
+
+    visit page_path
+
+    page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
+      expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
+    end
+  end
+
   it 'handles multiple edits and deletion in the middle' do
     page.within('.js-ci-variable-list-section') do
       # Create 2 variables
@@ -297,11 +330,11 @@ shared_examples 'variable list' do
   it 'shows validation error box about duplicate keys' do
     page.within('.js-ci-variable-list-section .js-row:last-child') do
       find('.js-ci-variable-input-key').set('samekey')
-      find('.js-ci-variable-input-value').set('value1')
+      find('.js-ci-variable-input-value').set('value123')
     end
     page.within('.js-ci-variable-list-section .js-row:last-child') do
       find('.js-ci-variable-input-key').set('samekey')
-      find('.js-ci-variable-input-value').set('value2')
+      find('.js-ci-variable-input-value').set('value456')
     end
 
     click_button('Save variables')
@@ -314,4 +347,34 @@ shared_examples 'variable list' do
       expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables have duplicate values \(.+\)/)
     end
   end
+
+  it 'shows validation error box about empty values' do
+    page.within('.js-ci-variable-list-section .js-row:last-child') do
+      find('.js-ci-variable-input-key').set('empty_value')
+      find('.js-ci-variable-input-value').set('')
+    end
+
+    click_button('Save variables')
+    wait_for_requests
+
+    page.within('.js-ci-variable-list-section') do
+      expect(all('.js-ci-variable-error-box ul li').count).to eq(1)
+      expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables value is invalid/)
+    end
+  end
+
+  it 'shows validation error box about unmaskable values' do
+    page.within('.js-ci-variable-list-section .js-row:last-child') do
+      find('.js-ci-variable-input-key').set('unmaskable_value')
+      find('.js-ci-variable-input-value').set('???')
+    end
+
+    click_button('Save variables')
+    wait_for_requests
+
+    page.within('.js-ci-variable-list-section') do
+      expect(all('.js-ci-variable-error-box ul li').count).to eq(1)
+      expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables value is invalid/)
+    end
+  end
 end