Automatic merge of gitlab-org/gitlab-ce master

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 {
       }
     });
 
-    // Always make sure there is an empty last row
-    this.$container.on('input trigger-change', inputSelector, () => {
+    this.$container.on('input trigger-change', inputSelector, e => {
+      // Always make sure there is an empty last row
       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,7 +38,7 @@ 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

@@ -8,4 +8,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

@@ -2106,6 +2106,9 @@ msgstr ""
 msgid "CiVariables|Remove variable row"
 msgstr ""
 
+msgid "CiVariables|This variable will not be masked"
+msgstr ""
+
 msgid "CiVariable|* (All environments)"
 msgstr ""
 
@@ -3868,10 +3871,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"

qa/README.md

@@ -55,16 +55,19 @@ You can also supply specific tests to run as another parameter. For example, to
 run the repository-related specs, you can execute:
 
 ```
-bin/qa Test::Instance::All http://localhost qa/specs/features/repository/
+bin/qa Test::Instance::All http://localhost -- qa/specs/features/browser_ui/3_create/repository
 ```
 
 Since the arguments would be passed to `rspec`, you could use all `rspec`
 options there. For example, passing `--backtrace` and also line number:
 
 ```
-bin/qa Test::Instance::All http://localhost qa/specs/features/project/create_spec.rb:3 --backtrace
+bin/qa Test::Instance::All http://localhost -- qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb:6 --backtrace
 ```
 
+Note that the separator `--` is required; all subsequent options will be
+ignored by the QA framework and passed to `rspec`.
+
 ### Overriding the authenticated user
 
 Unless told otherwise, the QA tests will run as the default `root` user seeded
@@ -117,7 +120,7 @@ tests that are expected to fail while a fix is in progress (similar to how
  can be used).
 
 ```
-bin/qa Test::Instance::All http://localhost --tag quarantine
+bin/qa Test::Instance::All http://localhost -- --tag quarantine
 ```
 
 If `quarantine` is used with other tags, tests will only be run if they have at
@@ -128,3 +131,25 @@ For example, suppose one test has `:smoke` and `:quarantine` metadata, and
 another test has `:ldap` and `:quarantine` metadata. If the tests are run with
 `--tag smoke --tag quarantine`, only the first test will run. The test with
 `:ldap` will not run even though it also has `:quarantine`.
+
+### Running tests with a feature flag enabled
+
+Tests can be run with with a feature flag enabled by using the command-line
+option `--enable-feature FEATURE_FLAG`. For example, to enable the feature flag
+that enforces Gitaly request limits, you would use the command:
+
+```
+bin/qa Test::Instance::All http://localhost --enable-feature gitaly_enforce_requests_limits
+```
+
+This will instruct the QA framework to enable the `gitaly_enforce_requests_limits`
+feature flag ([via the API](https://docs.gitlab.com/ee/api/features.html)), run
+all the tests in the `Test::Instance::All` scenario, and then disable the
+feature flag again.
+
+Note: the QA framework doesn't currently allow you to easily toggle a feature
+flag during a single test, [as you can in unit tests](https://docs.gitlab.com/ee/development/feature_flags.html#specs),
+but [that capability is planned](https://gitlab.com/gitlab-org/quality/team-tasks/issues/77).
+
+Note also that the `--` separator isn't used because `--enable-feature` is a QA
+framework option, not an `rspec` option.
\ No newline at end of file

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/services/verify_pages_domain_service_spec.rb

@@ -9,88 +9,130 @@ describe VerifyPagesDomainService do
   subject(:service) { described_class.new(domain) }
 
   describe '#execute' do
-    context 'verification code recognition (verified domain)' do
-      where(:domain_sym, :code_sym) do
-        :domain | :verification_code
-        :domain | :keyed_verification_code
+    where(:domain_sym, :code_sym) do
+      :domain | :verification_code
+      :domain | :keyed_verification_code
 
-        :verification_domain | :verification_code
-        :verification_domain | :keyed_verification_code
-      end
-
-      with_them do
-        set(:domain) { create(:pages_domain) }
+      :verification_domain | :verification_code
+      :verification_domain | :keyed_verification_code
+    end
 
-        let(:domain_name) { domain.send(domain_sym) }
-        let(:verification_code) { domain.send(code_sym) }
+    with_them do
+      let(:domain_name) { domain.send(domain_sym) }
+      let(:verification_code) { domain.send(code_sym) }
 
+      shared_examples 'verifies and enables the domain' do
         it 'verifies and enables the domain' do
-          stub_resolver(domain_name => ['something else', verification_code])
-
           expect(service.execute).to eq(status: :success)
+
           expect(domain).to be_verified
           expect(domain).to be_enabled
         end
+      end
 
-        it 'verifies and enables when the code is contained partway through a TXT record' do
-          stub_resolver(domain_name => "something #{verification_code} else")
+      shared_examples 'successful enablement and verification' do
+        context 'when txt record contains verification code' do
+          before do
+            stub_resolver(domain_name => ['something else', verification_code])
+          end
 
-          expect(service.execute).to eq(status: :success)
-          expect(domain).to be_verified
-          expect(domain).to be_enabled
+          include_examples 'verifies and enables the domain'
         end
 
-        it 'does not verify when the code is not present' do
-          stub_resolver(domain_name => 'something else')
-
-          expect(service.execute).to eq(error_status)
+        context 'when txt record contains verification code with other text' do
+          before do
+            stub_resolver(domain_name => "something #{verification_code} else")
+          end
 
-          expect(domain).not_to be_verified
-          expect(domain).to be_enabled
+          include_examples 'verifies and enables the domain'
         end
       end
 
-      context 'verified domain' do
-        set(:domain) { create(:pages_domain) }
+      context 'when domain is disabled(or new)' do
+        let(:domain) { create(:pages_domain, :disabled) }
 
-        it 'unverifies (but does not disable) when the right code is not present' do
-          stub_resolver(domain.domain => 'something else')
+        include_examples 'successful enablement and verification'
 
-          expect(service.execute).to eq(error_status)
-          expect(domain).not_to be_verified
-          expect(domain).to be_enabled
+        shared_examples 'unverifies and disables domain' do
+          it 'unverifies and disables domain' do
+            expect(service.execute).to eq(error_status)
+
+            expect(domain).not_to be_verified
+            expect(domain).not_to be_enabled
+          end
         end
 
-        it 'unverifies (but does not disable) when no records are present' do
-          stub_resolver
+        context 'when txt record does not contain verification code' do
+          before do
+            stub_resolver(domain_name => 'something else')
+          end
 
-          expect(service.execute).to eq(error_status)
-          expect(domain).not_to be_verified
-          expect(domain).to be_enabled
+          include_examples 'unverifies and disables domain'
+        end
+
+        context 'when no txt records are present' do
+          before do
+            stub_resolver
+          end
+
+          include_examples 'unverifies and disables domain'
         end
       end
 
-      context 'expired domain' do
-        set(:domain) { create(:pages_domain, :expired) }
+      context 'when domain is verified' do
+        let(:domain) { create(:pages_domain) }
 
-        it 'verifies and enables when the right code is present' do
-          stub_resolver(domain.domain => domain.keyed_verification_code)
+        include_examples 'successful enablement and verification'
 
-          expect(service.execute).to eq(status: :success)
+        context 'when txt record does not contain verification code' do
+          before do
+            stub_resolver(domain_name => 'something else')
+          end
 
-          expect(domain).to be_verified
-          expect(domain).to be_enabled
+          it 'unverifies but does not disable domain' do
+            expect(service.execute).to eq(error_status)
+            expect(domain).not_to be_verified
+            expect(domain).to be_enabled
+          end
         end
 
-        it 'disables when the right code is not present' do
-          error_status[:message] += '. It is now disabled.'
+        context 'when no txt records are present' do
+          before do
+            stub_resolver
+          end
 
-          stub_resolver
+          it 'unverifies but does not disable domain' do
+            expect(service.execute).to eq(error_status)
+            expect(domain).not_to be_verified
+            expect(domain).to be_enabled
+          end
+        end
+      end
 
-          expect(service.execute).to eq(error_status)
+      context 'when domain is expired' do
+        let(:domain) { create(:pages_domain, :expired) }
 
-          expect(domain).not_to be_verified
-          expect(domain).not_to be_enabled
+        context 'when the right code is present' do
+          before do
+            stub_resolver(domain_name => domain.keyed_verification_code)
+          end
+
+          include_examples 'verifies and enables the domain'
+        end
+
+        context 'when the right code is not present' do
+          before do
+            stub_resolver
+          end
+
+          it 'disables domain' do
+            error_status[:message] += '. It is now disabled.'
+
+            expect(service.execute).to eq(error_status)
+
+            expect(domain).not_to be_verified
+            expect(domain).not_to be_enabled
+          end
         end
       end
 

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