Merge branch 'thomas-nilsson-irfu/gitlab-ce-thomas-nilsson-irfu-master-patch-13137' into 'master'

Allow masking if 8 or more characters in base64

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

11 files changed, 95 insertions, 8 deletions

diff --git a/app/assets/javascripts/ci_variable_list/ajax_variable_list.js b/app/assets/javascripts/ci_variable_list/ajax_variable_list.js
index 592e1fd1c31..0bba2a2e160 100644
--- a/app/assets/javascripts/ci_variable_list/ajax_variable_list.js
+++ b/app/assets/javascripts/ci_variable_list/ajax_variable_list.js
@@ -27,15 +27,24 @@ function generateErrorBoxContent(errors) {
 
 // Used for the variable list on CI/CD projects/groups settings page
 export default class AjaxVariableList {
-  constructor({ container, saveButton, errorBox, formField = 'variables', saveEndpoint }) {
+  constructor({
+    container,
+    saveButton,
+    errorBox,
+    formField = 'variables',
+    saveEndpoint,
+    maskableRegex,
+  }) {
     this.container = container;
     this.saveButton = saveButton;
     this.errorBox = errorBox;
     this.saveEndpoint = saveEndpoint;
+    this.maskableRegex = maskableRegex;
 
     this.variableList = new VariableList({
       container: this.container,
       formField,
+      maskableRegex,
     });
 
     this.bindEvents();
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 0390a3bf96a..0303e4e51dd 100644
--- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js
+++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js
@@ -16,9 +16,10 @@ function createEnvironmentItem(value) {
 }
 
 export default class VariableList {
-  constructor({ container, formField }) {
+  constructor({ container, formField, maskableRegex }) {
     this.$container = $(container);
     this.formField = formField;
+    this.maskableRegex = new RegExp(maskableRegex);
     this.environmentDropdownMap = new WeakMap();
 
     this.inputMap = {
@@ -196,9 +197,8 @@ export default class VariableList {
   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 isValueMaskable = this.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
diff --git a/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js b/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js
index ae0a8c74964..8a5300c9266 100644
--- a/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js
+++ b/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js
@@ -12,5 +12,6 @@ document.addEventListener('DOMContentLoaded', () => {
     saveButton: variableListEl.querySelector('.js-ci-variables-save-button'),
     errorBox: variableListEl.querySelector('.js-ci-variable-error-box'),
     saveEndpoint: variableListEl.dataset.saveEndpoint,
+    maskableRegex: variableListEl.dataset.maskableRegex,
   });
 });
diff --git a/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js b/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js
index 15c6fb550c1..885247335a4 100644
--- a/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js
+++ b/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js
@@ -21,6 +21,7 @@ document.addEventListener('DOMContentLoaded', () => {
     saveButton: variableListEl.querySelector('.js-ci-variables-save-button'),
     errorBox: variableListEl.querySelector('.js-ci-variable-error-box'),
     saveEndpoint: variableListEl.dataset.saveEndpoint,
+    maskableRegex: variableListEl.dataset.maskableRegex,
   });
 
   // hide extra auto devops settings based checkbox state
diff --git a/app/helpers/ci_variables_helper.rb b/app/helpers/ci_variables_helper.rb
index e313015c937..fc51f00d052 100644
--- a/app/helpers/ci_variables_helper.rb
+++ b/app/helpers/ci_variables_helper.rb
@@ -27,4 +27,8 @@ module CiVariablesHelper
       %w(File file)
     ]
   end
+
+  def ci_variable_maskable_regex
+    Maskable::REGEX.inspect.sub('\\A', '^').sub('\\z', '$').sub(/^\//, '').sub(/\/[a-z]*$/, '').gsub('\/', '/')
+  end
 end
diff --git a/app/models/concerns/maskable.rb b/app/models/concerns/maskable.rb
index 2943872ffab..e0f2c41b836 100644
--- a/app/models/concerns/maskable.rb
+++ b/app/models/concerns/maskable.rb
@@ -7,9 +7,9 @@ module Maskable
   # * No escape characters
   # * No variables
   # * No spaces
-  # * Minimal length of 8 characters
+  # * Minimal length of 8 characters from the Base64 alphabets (RFC4648)
   # * Absolutely no fun is allowed
-  REGEX = /\A\w{8,}\z/.freeze
+  REGEX = /\A[a-zA-Z0-9_+=\/-]{8,}\z/.freeze
 
   included do
     validates :masked, inclusion: { in: [true, false] }
diff --git a/app/views/ci/variables/_index.html.haml b/app/views/ci/variables/_index.html.haml
index 464b9faf282..94102b4dcd0 100644
--- a/app/views/ci/variables/_index.html.haml
+++ b/app/views/ci/variables/_index.html.haml
@@ -6,7 +6,7 @@
     = s_('Environment variables are configured by your administrator to be %{link_start}protected%{link_end} by default').html_safe % { link_start: link_start, link_end: '</a>'.html_safe }
 
 .row
-  .col-lg-12.js-ci-variable-list-section{ data: { save_endpoint: save_endpoint } }
+  .col-lg-12.js-ci-variable-list-section{ data: { save_endpoint: save_endpoint, maskable_regex: ci_variable_maskable_regex } }
     .hide.alert.alert-danger.js-ci-variable-error-box
 
     %ul.ci-variable-list
diff --git a/changelogs/unreleased/thomas-nilsson-irfu-gitlab-ce-thomas-nilsson-irfu-master-patch-13137.yml b/changelogs/unreleased/thomas-nilsson-irfu-gitlab-ce-thomas-nilsson-irfu-master-patch-13137.yml
new file mode 100644
index 00000000000..3391fcc9537
--- /dev/null
+++ b/changelogs/unreleased/thomas-nilsson-irfu-gitlab-ce-thomas-nilsson-irfu-master-patch-13137.yml
@@ -0,0 +1,5 @@
+---
+title: Allow masking if 8 or more characters in base64.
+merge_request: 29143
+author: thomas-nilsson-irfu
+type: changed
diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md
index fe64f5ab2e0..df455857dee 100644
--- a/doc/ci/variables/README.md
+++ b/doc/ci/variables/README.md
@@ -92,7 +92,7 @@ This means that the value of the variable will be hidden in job logs,
 though it must match certain requirements to do so:
 
 - The value must be in a single line.
-- The value must contain only letters, numbers, or underscores.
+- The value must only consist of characters from the Base64 alphabet, defined in [RFC4648](https://tools.ietf.org/html/rfc4648).
 - The value must be at least 8 characters long.
 - The value must not use variables.
 
diff --git a/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js b/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js
index 2839922fbd3..e6a969bd855 100644
--- a/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js
+++ b/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js
@@ -32,6 +32,7 @@ describe('AjaxFormVariableList', () => {
       saveButton,
       errorBox,
       saveEndpoint: container.dataset.saveEndpoint,
+      maskableRegex: container.dataset.maskableRegex,
     });
 
     spyOn(ajaxVariableList, 'updateRowsWithPersistedVariables').and.callThrough();
@@ -220,4 +221,11 @@ describe('AjaxFormVariableList', () => {
       expect(row.dataset.isPersisted).toEqual('true');
     });
   });
+
+  describe('maskableRegex', () => {
+    it('takes in the regex provided by the data attribute', () => {
+      expect(container.dataset.maskableRegex).toBe('^[a-zA-Z0-9_+=/-]{8,}$');
+      expect(ajaxVariableList.maskableRegex).toBe(container.dataset.maskableRegex);
+    });
+  });
 });
diff --git a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js
index 394e60fc22c..064113e879a 100644
--- a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js
+++ b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js
@@ -150,6 +150,65 @@ describe('VariableList', () => {
         .then(done)
         .catch(done.fail);
     });
+
+    describe('validateMaskability', () => {
+      let $row;
+
+      const maskingErrorElement = '.js-row:last-child .masking-validation-error';
+
+      beforeEach(() => {
+        $row = $wrapper.find('.js-row:last-child');
+        $row.find('.ci-variable-masked-item .js-project-feature-toggle').click();
+      });
+
+      it('has a regex provided via a data attribute', () => {
+        expect($wrapper.attr('data-maskable-regex')).toBe('^[a-zA-Z0-9_+=/-]{8,}$');
+      });
+
+      it('allows values that are 8 characters long', done => {
+        $row.find('.js-ci-variable-input-value').val('looooong');
+
+        getSetTimeoutPromise()
+          .then(() => {
+            expect($wrapper.find(maskingErrorElement)).toHaveClass('hide');
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+
+      it('rejects values that are shorter than 8 characters', done => {
+        $row.find('.js-ci-variable-input-value').val('short');
+
+        getSetTimeoutPromise()
+          .then(() => {
+            expect($wrapper.find(maskingErrorElement)).toBeVisible();
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+
+      it('allows values with base 64 characters', done => {
+        $row.find('.js-ci-variable-input-value').val('abcABC123_+=/-');
+
+        getSetTimeoutPromise()
+          .then(() => {
+            expect($wrapper.find(maskingErrorElement)).toHaveClass('hide');
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+
+      it('rejects values with other special characters', done => {
+        $row.find('.js-ci-variable-input-value').val('1234567$');
+
+        getSetTimeoutPromise()
+          .then(() => {
+            expect($wrapper.find(maskingErrorElement)).toBeVisible();
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
   });
 
   describe('toggleEnableRow method', () => {