diff options
author | Stan Hu <stanhu@gmail.com> | 2019-06-07 13:15:27 +0000 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2019-06-07 13:15:27 +0000 |
commit | 25ef3a9687494bea8927d7109f6e684beba2eb95 (patch) | |
tree | df93c1ce4c1f2ea96918ff6a5f01568de3590bab | |
parent | 2320e4451fa0c0c60326a7d29fa53599559b6484 (diff) | |
parent | 6a90249b6a9c14391bf1ada9bb4726e8bf1e4ffd (diff) | |
download | gitlab-ce-25ef3a9687494bea8927d7109f6e684beba2eb95.tar.gz |
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', () => { |