From 316889cb4789e8a4a43bf0c79a4269643a97c336 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Wed, 27 Feb 2019 01:22:51 +0100 Subject: Revert "Merge branch 'revert-8baf9e5f' into 'master'" This reverts commit f5201a816f2eff9393e16f362403451e5d86ee6c, reversing changes made to 48e6db0dad6f256e8423e0bd6c9b254803f50ccf. --- app/models/ci/group_variable.rb | 1 + app/models/ci/variable.rb | 1 + app/models/concerns/has_variable.rb | 6 +- app/models/concerns/maskable.rb | 22 +++ ...mple-masking-of-protected-variables-in-logs.yml | 5 + .../20190218134158_add_masked_to_ci_variables.rb | 21 +++ ...90218134209_add_masked_to_ci_group_variables.rb | 21 +++ db/schema.rb | 2 + lib/gitlab/ci/variables/collection/item.rb | 10 +- spec/factories/ci/group_variables.rb | 1 + spec/factories/ci/variables.rb | 1 + spec/features/group_variables_spec.rb | 2 +- spec/features/project_variables_spec.rb | 2 +- .../gitlab/ci/variables/collection/item_spec.rb | 20 ++- spec/lib/gitlab/ci/variables/collection_spec.rb | 4 +- spec/models/ci/build_spec.rb | 176 ++++++++++----------- spec/models/ci/group_variable_spec.rb | 1 + spec/models/ci/variable_spec.rb | 1 + spec/models/concerns/has_variable_spec.rb | 2 +- spec/models/concerns/maskable_spec.rb | 76 +++++++++ spec/requests/api/group_variables_spec.rb | 4 +- spec/requests/api/runner_spec.rb | 18 +-- spec/requests/api/variables_spec.rb | 4 +- .../features/variable_list_shared_examples.rb | 8 +- 24 files changed, 290 insertions(+), 119 deletions(-) create mode 100644 app/models/concerns/maskable.rb create mode 100644 changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml create mode 100644 db/migrate/20190218134158_add_masked_to_ci_variables.rb create mode 100644 db/migrate/20190218134209_add_masked_to_ci_group_variables.rb create mode 100644 spec/models/concerns/maskable_spec.rb diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb index 492d1d0329e..323ff560564 100644 --- a/app/models/ci/group_variable.rb +++ b/app/models/ci/group_variable.rb @@ -5,6 +5,7 @@ module Ci extend Gitlab::Ci::Model include HasVariable include Presentable + include Maskable belongs_to :group, class_name: "::Group" diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 524d79014f8..64836ea4fa4 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -5,6 +5,7 @@ module Ci extend Gitlab::Ci::Model include HasVariable include Presentable + include Maskable belongs_to :project diff --git a/app/models/concerns/has_variable.rb b/app/models/concerns/has_variable.rb index dfbe413a878..2ec42a1029b 100644 --- a/app/models/concerns/has_variable.rb +++ b/app/models/concerns/has_variable.rb @@ -21,9 +21,9 @@ module HasVariable def key=(new_key) super(new_key.to_s.strip) end + end - def to_runner_variable - { key: key, value: value, public: false } - end + def to_runner_variable + { key: key, value: value, public: false } end end diff --git a/app/models/concerns/maskable.rb b/app/models/concerns/maskable.rb new file mode 100644 index 00000000000..8793f0ec965 --- /dev/null +++ b/app/models/concerns/maskable.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Maskable + extend ActiveSupport::Concern + + # * Single line + # * No escape characters + # * No variables + # * No spaces + # * Minimal length of 8 characters + # * Absolutely no fun is allowed + REGEX = /\A\w{8,}\z/ + + included do + validates :masked, inclusion: { in: [true, false] } + validates :value, format: { with: REGEX }, if: :masked? + end + + def to_runner_variable + super.merge(masked: masked?) + end +end diff --git a/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml b/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml new file mode 100644 index 00000000000..5c3b6833235 --- /dev/null +++ b/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml @@ -0,0 +1,5 @@ +--- +title: Add support for masking CI variables. +merge_request: 25293 +author: +type: added diff --git a/db/migrate/20190218134158_add_masked_to_ci_variables.rb b/db/migrate/20190218134158_add_masked_to_ci_variables.rb new file mode 100644 index 00000000000..b4999d5b4a9 --- /dev/null +++ b/db/migrate/20190218134158_add_masked_to_ci_variables.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddMaskedToCiVariables < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :ci_variables, :masked, :boolean, default: false, allow_null: false + end + + def down + remove_column :ci_variables, :masked + end +end diff --git a/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb b/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb new file mode 100644 index 00000000000..8633875b341 --- /dev/null +++ b/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddMaskedToCiGroupVariables < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :ci_group_variables, :masked, :boolean, default: false, allow_null: false + end + + def down + remove_column :ci_group_variables, :masked + end +end diff --git a/db/schema.rb b/db/schema.rb index d835e48938d..86cb896f9a6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -405,6 +405,7 @@ ActiveRecord::Schema.define(version: 20190220150130) do t.boolean "protected", default: false, null: false t.datetime_with_timezone "created_at", null: false t.datetime_with_timezone "updated_at", null: false + t.boolean "masked", default: false, null: false t.index ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree end @@ -602,6 +603,7 @@ ActiveRecord::Schema.define(version: 20190220150130) do t.integer "project_id", null: false t.boolean "protected", default: false, null: false t.string "environment_scope", default: "*", null: false + t.boolean "masked", default: false, null: false t.index ["project_id", "key", "environment_scope"], name: "index_ci_variables_on_project_id_and_key_and_environment_scope", unique: true, using: :btree end diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb index e3e4e62cc02..833aa75adb5 100644 --- a/lib/gitlab/ci/variables/collection/item.rb +++ b/lib/gitlab/ci/variables/collection/item.rb @@ -5,12 +5,12 @@ module Gitlab module Variables class Collection class Item - def initialize(key:, value:, public: true, file: false) + def initialize(key:, value:, public: true, file: false, masked: false) raise ArgumentError, "`#{key}` must be of type String or nil value, while it was: #{value.class}" unless value.is_a?(String) || value.nil? @variable = { - key: key, value: value, public: public, file: file + key: key, value: value, public: public, file: file, masked: masked } end @@ -27,9 +27,13 @@ module Gitlab # don't expose `file` attribute at all (stems from what the runner # expects). # + # If the `variable_masking` feature is enabled we expose the `masked` + # attribute, otherwise it's not exposed. + # def to_runner_variable @variable.reject do |hash_key, hash_value| - hash_key == :file && hash_value == false + (hash_key == :file && hash_value == false) || + (hash_key == :masked && !Feature.enabled?(:variable_masking)) end end diff --git a/spec/factories/ci/group_variables.rb b/spec/factories/ci/group_variables.rb index 64716842b12..9bf520a2c0a 100644 --- a/spec/factories/ci/group_variables.rb +++ b/spec/factories/ci/group_variables.rb @@ -2,6 +2,7 @@ FactoryBot.define do factory :ci_group_variable, class: Ci::GroupVariable do sequence(:key) { |n| "VARIABLE_#{n}" } value 'VARIABLE_VALUE' + masked false trait(:protected) do protected true diff --git a/spec/factories/ci/variables.rb b/spec/factories/ci/variables.rb index 3d014b9b54f..97a7c9ba252 100644 --- a/spec/factories/ci/variables.rb +++ b/spec/factories/ci/variables.rb @@ -2,6 +2,7 @@ FactoryBot.define do factory :ci_variable, class: Ci::Variable do sequence(:key) { |n| "VARIABLE_#{n}" } value 'VARIABLE_VALUE' + masked false trait(:protected) do protected true diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb index 57e3ddfb39c..1a53e7c9512 100644 --- a/spec/features/group_variables_spec.rb +++ b/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', group: group) } let(:page_path) { group_settings_ci_cd_path(group) } before do diff --git a/spec/features/project_variables_spec.rb b/spec/features/project_variables_spec.rb index a93df3696d2..6bdf5df1036 100644 --- a/spec/features/project_variables_spec.rb +++ b/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') } let(:page_path) { project_settings_ci_cd_path(project) } before do diff --git a/spec/lib/gitlab/ci/variables/collection/item_spec.rb b/spec/lib/gitlab/ci/variables/collection/item_spec.rb index 8bf44acb228..3ff2fe18c15 100644 --- a/spec/lib/gitlab/ci/variables/collection/item_spec.rb +++ b/spec/lib/gitlab/ci/variables/collection/item_spec.rb @@ -6,7 +6,7 @@ describe Gitlab::Ci::Variables::Collection::Item do let(:expected_value) { variable_value } let(:variable) do - { key: variable_key, value: variable_value, public: true } + { key: variable_key, value: variable_value, public: true, masked: false } end describe '.new' do @@ -88,7 +88,7 @@ describe Gitlab::Ci::Variables::Collection::Item do resource = described_class.fabricate(variable) expect(resource).to be_a(described_class) - expect(resource).to eq(key: 'CI_VAR', value: '123', public: false) + expect(resource).to eq(key: 'CI_VAR', value: '123', public: false, masked: false) end it 'supports using another collection item' do @@ -134,7 +134,21 @@ describe Gitlab::Ci::Variables::Collection::Item do .to_runner_variable expect(runner_variable) - .to eq(key: 'VAR', value: 'value', public: true, file: true) + .to eq(key: 'VAR', value: 'value', public: true, file: true, masked: false) + end + end + + context 'when variable masking is disabled' do + before do + stub_feature_flags(variable_masking: false) + end + + it 'does not expose the masked field to the runner' do + runner_variable = described_class + .new(key: 'VAR', value: 'value', masked: true) + .to_runner_variable + + expect(runner_variable).to eq(key: 'VAR', value: 'value', public: true) end end end diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb index 5c91816a586..edb209c0cf4 100644 --- a/spec/lib/gitlab/ci/variables/collection_spec.rb +++ b/spec/lib/gitlab/ci/variables/collection_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::Ci::Variables::Collection do describe '.new' do it 'can be initialized with an array' do - variable = { key: 'VAR', value: 'value', public: true } + variable = { key: 'VAR', value: 'value', public: true, masked: false } collection = described_class.new([variable]) @@ -93,7 +93,7 @@ describe Gitlab::Ci::Variables::Collection do collection = described_class.new([{ key: 'TEST', value: '1' }]) expect(collection.to_runner_variables) - .to eq [{ key: 'TEST', value: '1', public: true }] + .to eq [{ key: 'TEST', value: '1', public: true, masked: false }] end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 17540443688..81ff727b458 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2114,55 +2114,55 @@ describe Ci::Build do context 'returns variables' do let(:predefined_variables) do [ - { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true }, - { key: 'CI_PIPELINE_URL', value: project.web_url + "/pipelines/#{pipeline.id}", public: true }, - { key: 'CI_JOB_ID', value: build.id.to_s, public: true }, - { key: 'CI_JOB_URL', value: project.web_url + "/-/jobs/#{build.id}", public: true }, - { key: 'CI_JOB_TOKEN', value: 'my-token', public: false }, - { key: 'CI_BUILD_ID', value: build.id.to_s, public: true }, - { key: 'CI_BUILD_TOKEN', value: 'my-token', public: false }, - { key: 'CI_REGISTRY_USER', value: 'gitlab-ci-token', public: true }, - { key: 'CI_REGISTRY_PASSWORD', value: 'my-token', public: false }, - { key: 'CI_REPOSITORY_URL', value: build.repo_url, public: false }, - { key: 'CI', value: 'true', public: true }, - { key: 'GITLAB_CI', value: 'true', public: true }, - { key: 'GITLAB_FEATURES', value: project.licensed_features.join(','), public: true }, - { key: 'CI_SERVER_NAME', value: 'GitLab', public: true }, - { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true }, - { key: 'CI_SERVER_VERSION_MAJOR', value: Gitlab.version_info.major.to_s, public: true }, - { key: 'CI_SERVER_VERSION_MINOR', value: Gitlab.version_info.minor.to_s, public: true }, - { key: 'CI_SERVER_VERSION_PATCH', value: Gitlab.version_info.patch.to_s, public: true }, - { key: 'CI_SERVER_REVISION', value: Gitlab.revision, public: true }, - { key: 'CI_JOB_NAME', value: 'test', public: true }, - { key: 'CI_JOB_STAGE', value: 'test', public: true }, - { key: 'CI_COMMIT_SHA', value: build.sha, public: true }, - { key: 'CI_COMMIT_SHORT_SHA', value: build.short_sha, public: true }, - { key: 'CI_COMMIT_BEFORE_SHA', value: build.before_sha, public: true }, - { key: 'CI_COMMIT_REF_NAME', value: build.ref, public: true }, - { key: 'CI_COMMIT_REF_SLUG', value: build.ref_slug, public: true }, - { key: 'CI_NODE_TOTAL', value: '1', public: true }, - { key: 'CI_BUILD_REF', value: build.sha, public: true }, - { key: 'CI_BUILD_BEFORE_SHA', value: build.before_sha, public: true }, - { key: 'CI_BUILD_REF_NAME', value: build.ref, public: true }, - { key: 'CI_BUILD_REF_SLUG', value: build.ref_slug, public: true }, - { key: 'CI_BUILD_NAME', value: 'test', public: true }, - { key: 'CI_BUILD_STAGE', value: 'test', public: true }, - { key: 'CI_PROJECT_ID', value: project.id.to_s, public: true }, - { key: 'CI_PROJECT_NAME', value: project.path, public: true }, - { key: 'CI_PROJECT_PATH', value: project.full_path, public: true }, - { key: 'CI_PROJECT_PATH_SLUG', value: project.full_path_slug, public: true }, - { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true }, - { key: 'CI_PROJECT_URL', value: project.web_url, public: true }, - { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true }, - { key: 'CI_PAGES_DOMAIN', value: Gitlab.config.pages.host, public: true }, - { key: 'CI_PAGES_URL', value: project.pages_url, public: true }, - { key: 'CI_API_V4_URL', value: 'http://localhost/api/v4', public: true }, - { key: 'CI_PIPELINE_IID', value: pipeline.iid.to_s, public: true }, - { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true }, - { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true }, - { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true }, - { key: 'CI_COMMIT_TITLE', value: pipeline.git_commit_title, public: true }, - { key: 'CI_COMMIT_DESCRIPTION', value: pipeline.git_commit_description, public: true } + { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true, masked: false }, + { key: 'CI_PIPELINE_URL', value: project.web_url + "/pipelines/#{pipeline.id}", public: true, masked: false }, + { key: 'CI_JOB_ID', value: build.id.to_s, public: true, masked: false }, + { key: 'CI_JOB_URL', value: project.web_url + "/-/jobs/#{build.id}", public: true, masked: false }, + { key: 'CI_JOB_TOKEN', value: 'my-token', public: false, masked: false }, + { key: 'CI_BUILD_ID', value: build.id.to_s, public: true, masked: false }, + { key: 'CI_BUILD_TOKEN', value: 'my-token', public: false, masked: false }, + { key: 'CI_REGISTRY_USER', value: 'gitlab-ci-token', public: true, masked: false }, + { key: 'CI_REGISTRY_PASSWORD', value: 'my-token', public: false, masked: false }, + { key: 'CI_REPOSITORY_URL', value: build.repo_url, public: false, masked: false }, + { key: 'CI', value: 'true', public: true, masked: false }, + { key: 'GITLAB_CI', value: 'true', public: true, masked: false }, + { key: 'GITLAB_FEATURES', value: project.licensed_features.join(','), public: true, masked: false }, + { key: 'CI_SERVER_NAME', value: 'GitLab', public: true, masked: false }, + { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true, masked: false }, + { key: 'CI_SERVER_VERSION_MAJOR', value: Gitlab.version_info.major.to_s, public: true, masked: false }, + { key: 'CI_SERVER_VERSION_MINOR', value: Gitlab.version_info.minor.to_s, public: true, masked: false }, + { key: 'CI_SERVER_VERSION_PATCH', value: Gitlab.version_info.patch.to_s, public: true, masked: false }, + { key: 'CI_SERVER_REVISION', value: Gitlab.revision, public: true, masked: false }, + { key: 'CI_JOB_NAME', value: 'test', public: true, masked: false }, + { key: 'CI_JOB_STAGE', value: 'test', public: true, masked: false }, + { key: 'CI_COMMIT_SHA', value: build.sha, public: true, masked: false }, + { key: 'CI_COMMIT_SHORT_SHA', value: build.short_sha, public: true, masked: false }, + { key: 'CI_COMMIT_BEFORE_SHA', value: build.before_sha, public: true, masked: false }, + { key: 'CI_COMMIT_REF_NAME', value: build.ref, public: true, masked: false }, + { key: 'CI_COMMIT_REF_SLUG', value: build.ref_slug, public: true, masked: false }, + { key: 'CI_NODE_TOTAL', value: '1', public: true, masked: false }, + { key: 'CI_BUILD_REF', value: build.sha, public: true, masked: false }, + { key: 'CI_BUILD_BEFORE_SHA', value: build.before_sha, public: true, masked: false }, + { key: 'CI_BUILD_REF_NAME', value: build.ref, public: true, masked: false }, + { key: 'CI_BUILD_REF_SLUG', value: build.ref_slug, public: true, masked: false }, + { key: 'CI_BUILD_NAME', value: 'test', public: true, masked: false }, + { key: 'CI_BUILD_STAGE', value: 'test', public: true, masked: false }, + { key: 'CI_PROJECT_ID', value: project.id.to_s, public: true, masked: false }, + { key: 'CI_PROJECT_NAME', value: project.path, public: true, masked: false }, + { key: 'CI_PROJECT_PATH', value: project.full_path, public: true, masked: false }, + { key: 'CI_PROJECT_PATH_SLUG', value: project.full_path_slug, public: true, masked: false }, + { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true, masked: false }, + { key: 'CI_PROJECT_URL', value: project.web_url, public: true, masked: false }, + { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true, masked: false }, + { key: 'CI_PAGES_DOMAIN', value: Gitlab.config.pages.host, public: true, masked: false }, + { key: 'CI_PAGES_URL', value: project.pages_url, public: true, masked: false }, + { key: 'CI_API_V4_URL', value: 'http://localhost/api/v4', public: true, masked: false }, + { key: 'CI_PIPELINE_IID', value: pipeline.iid.to_s, public: true, masked: false }, + { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true, masked: false }, + { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true, masked: false }, + { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true, masked: false }, + { key: 'CI_COMMIT_TITLE', value: pipeline.git_commit_title, public: true, masked: false }, + { key: 'CI_COMMIT_DESCRIPTION', value: pipeline.git_commit_description, public: true, masked: false } ] end @@ -2175,10 +2175,10 @@ describe Ci::Build do describe 'variables ordering' do context 'when variables hierarchy is stubbed' do - let(:build_pre_var) { { key: 'build', value: 'value', public: true } } - let(:project_pre_var) { { key: 'project', value: 'value', public: true } } - let(:pipeline_pre_var) { { key: 'pipeline', value: 'value', public: true } } - let(:build_yaml_var) { { key: 'yaml', value: 'value', public: true } } + let(:build_pre_var) { { key: 'build', value: 'value', public: true, masked: false } } + let(:project_pre_var) { { key: 'project', value: 'value', public: true, masked: false } } + let(:pipeline_pre_var) { { key: 'pipeline', value: 'value', public: true, masked: false } } + let(:build_yaml_var) { { key: 'yaml', value: 'value', public: true, masked: false } } before do allow(build).to receive(:predefined_variables) { [build_pre_var] } @@ -2200,7 +2200,7 @@ describe Ci::Build do project_pre_var, pipeline_pre_var, build_yaml_var, - { key: 'secret', value: 'value', public: false }]) + { key: 'secret', value: 'value', public: false, masked: false }]) end end @@ -2233,10 +2233,10 @@ describe Ci::Build do context 'when build has user' do let(:user_variables) do [ - { key: 'GITLAB_USER_ID', value: user.id.to_s, public: true }, - { key: 'GITLAB_USER_EMAIL', value: user.email, public: true }, - { key: 'GITLAB_USER_LOGIN', value: user.username, public: true }, - { key: 'GITLAB_USER_NAME', value: user.name, public: true } + { key: 'GITLAB_USER_ID', value: user.id.to_s, public: true, masked: false }, + { key: 'GITLAB_USER_EMAIL', value: user.email, public: true, masked: false }, + { key: 'GITLAB_USER_LOGIN', value: user.username, public: true, masked: false }, + { key: 'GITLAB_USER_NAME', value: user.name, public: true, masked: false } ] end @@ -2250,8 +2250,8 @@ describe Ci::Build do context 'when build has an environment' do let(:environment_variables) do [ - { key: 'CI_ENVIRONMENT_NAME', value: 'production', public: true }, - { key: 'CI_ENVIRONMENT_SLUG', value: 'prod-slug', public: true } + { key: 'CI_ENVIRONMENT_NAME', value: 'production', public: true, masked: false }, + { key: 'CI_ENVIRONMENT_SLUG', value: 'prod-slug', public: true, masked: false } ] end @@ -2286,7 +2286,7 @@ describe Ci::Build do before do environment_variables << - { key: 'CI_ENVIRONMENT_URL', value: url, public: true } + { key: 'CI_ENVIRONMENT_URL', value: url, public: true, masked: false } end context 'when the URL was set from the job' do @@ -2323,7 +2323,7 @@ describe Ci::Build do end let(:manual_variable) do - { key: 'CI_JOB_MANUAL', value: 'true', public: true } + { key: 'CI_JOB_MANUAL', value: 'true', public: true, masked: false } end it { is_expected.to include(manual_variable) } @@ -2331,7 +2331,7 @@ describe Ci::Build do context 'when build is for tag' do let(:tag_variable) do - { key: 'CI_COMMIT_TAG', value: 'master', public: true } + { key: 'CI_COMMIT_TAG', value: 'master', public: true, masked: false } end before do @@ -2343,7 +2343,7 @@ describe Ci::Build do context 'when CI variable is defined' do let(:ci_variable) do - { key: 'SECRET_KEY', value: 'secret_value', public: false } + { key: 'SECRET_KEY', value: 'secret_value', public: false, masked: false } end before do @@ -2358,7 +2358,7 @@ describe Ci::Build do let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } let(:protected_variable) do - { key: 'PROTECTED_KEY', value: 'protected_value', public: false } + { key: 'PROTECTED_KEY', value: 'protected_value', public: false, masked: false } end before do @@ -2390,7 +2390,7 @@ describe Ci::Build do context 'when group CI variable is defined' do let(:ci_variable) do - { key: 'SECRET_KEY', value: 'secret_value', public: false } + { key: 'SECRET_KEY', value: 'secret_value', public: false, masked: false } end before do @@ -2405,7 +2405,7 @@ describe Ci::Build do let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } let(:protected_variable) do - { key: 'PROTECTED_KEY', value: 'protected_value', public: false } + { key: 'PROTECTED_KEY', value: 'protected_value', public: false, masked: false } end before do @@ -2444,11 +2444,11 @@ describe Ci::Build do let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } let(:user_trigger_variable) do - { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false } + { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false, masked: false } end let(:predefined_trigger_variable) do - { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true } + { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true, masked: false } end before do @@ -2480,7 +2480,7 @@ describe Ci::Build do context 'when pipeline has a variable' do let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) } - it { is_expected.to include(pipeline_variable.to_runner_variable) } + it { is_expected.to include(key: pipeline_variable.key, value: pipeline_variable.value, public: false, masked: false) } end context 'when a job was triggered by a pipeline schedule' do @@ -2497,16 +2497,16 @@ describe Ci::Build do pipeline_schedule.reload end - it { is_expected.to include(pipeline_schedule_variable.to_runner_variable) } + it { is_expected.to include(key: pipeline_schedule_variable.key, value: pipeline_schedule_variable.value, public: false, masked: false) } end context 'when container registry is enabled' do let(:container_registry_enabled) { true } let(:ci_registry) do - { key: 'CI_REGISTRY', value: 'registry.example.com', public: true } + { key: 'CI_REGISTRY', value: 'registry.example.com', public: true, masked: false } end let(:ci_registry_image) do - { key: 'CI_REGISTRY_IMAGE', value: project.container_registry_url, public: true } + { key: 'CI_REGISTRY_IMAGE', value: project.container_registry_url, public: true, masked: false } end context 'and is disabled for project' do @@ -2535,13 +2535,13 @@ describe Ci::Build do build.update(runner: runner) end - it { is_expected.to include({ key: 'CI_RUNNER_ID', value: runner.id.to_s, public: true }) } - it { is_expected.to include({ key: 'CI_RUNNER_DESCRIPTION', value: 'description', public: true }) } - it { is_expected.to include({ key: 'CI_RUNNER_TAGS', value: 'docker, linux', public: true }) } + it { is_expected.to include({ key: 'CI_RUNNER_ID', value: runner.id.to_s, public: true, masked: false }) } + it { is_expected.to include({ key: 'CI_RUNNER_DESCRIPTION', value: 'description', public: true, masked: false }) } + it { is_expected.to include({ key: 'CI_RUNNER_TAGS', value: 'docker, linux', public: true, masked: false }) } end context 'when build is for a deployment' do - let(:deployment_variable) { { key: 'KUBERNETES_TOKEN', value: 'TOKEN', public: false } } + let(:deployment_variable) { { key: 'KUBERNETES_TOKEN', value: 'TOKEN', public: false, masked: false } } before do build.environment = 'production' @@ -2555,7 +2555,7 @@ describe Ci::Build do end context 'when project has custom CI config path' do - let(:ci_config_path) { { key: 'CI_CONFIG_PATH', value: 'custom', public: true } } + let(:ci_config_path) { { key: 'CI_CONFIG_PATH', value: 'custom', public: true, masked: false } } before do project.update(ci_config_path: 'custom') @@ -2572,7 +2572,7 @@ describe Ci::Build do it "includes AUTO_DEVOPS_DOMAIN" do is_expected.to include( - { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true }) + { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true, masked: false }) end end @@ -2583,7 +2583,7 @@ describe Ci::Build do it "includes AUTO_DEVOPS_DOMAIN" do is_expected.not_to include( - { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true }) + { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true, masked: false }) end end end @@ -2598,9 +2598,9 @@ describe Ci::Build do variables = subject.reverse.uniq { |variable| variable[:key] }.reverse expect(variables) - .not_to include(key: 'MYVAR', value: 'myvar', public: true) + .not_to include(key: 'MYVAR', value: 'myvar', public: true, masked: false) expect(variables) - .to include(key: 'MYVAR', value: 'pipeline value', public: false) + .to include(key: 'MYVAR', value: 'pipeline value', public: false, masked: false) end end @@ -2616,13 +2616,13 @@ describe Ci::Build do it 'includes CI_NODE_INDEX' do is_expected.to include( - { key: 'CI_NODE_INDEX', value: index.to_s, public: true } + { key: 'CI_NODE_INDEX', value: index.to_s, public: true, masked: false } ) end it 'includes correct CI_NODE_TOTAL' do is_expected.to include( - { key: 'CI_NODE_TOTAL', value: total.to_s, public: true } + { key: 'CI_NODE_TOTAL', value: total.to_s, public: true, masked: false } ) end end @@ -2641,7 +2641,7 @@ describe Ci::Build do it 'returns static predefined variables' do expect(build.variables.size).to be >= 28 expect(build.variables) - .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true) + .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true, masked: false) expect(build).not_to be_persisted end end @@ -2651,8 +2651,8 @@ describe Ci::Build do let(:deploy_token_variables) do [ - { key: 'CI_DEPLOY_USER', value: deploy_token.username, public: true }, - { key: 'CI_DEPLOY_PASSWORD', value: deploy_token.token, public: false } + { key: 'CI_DEPLOY_USER', value: deploy_token.username, public: true, masked: false }, + { key: 'CI_DEPLOY_PASSWORD', value: deploy_token.token, public: false, masked: false } ] end @@ -2711,7 +2711,7 @@ describe Ci::Build do end expect(variables) - .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true) + .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true, masked: false) end it 'does not return prohibited variables' do diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb index 1b10501701c..21d96bf3454 100644 --- a/spec/models/ci/group_variable_spec.rb +++ b/spec/models/ci/group_variable_spec.rb @@ -5,6 +5,7 @@ describe Ci::GroupVariable do it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(Presentable) } + it { is_expected.to include_module(Maskable) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id).with_message(/\(\w+\) has already been taken/) } describe '.unprotected' do diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 875e8b2b682..02c07a2bd83 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -6,6 +6,7 @@ describe Ci::Variable do describe 'validations' do it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(Presentable) } + it { is_expected.to include_module(Maskable) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) } end diff --git a/spec/models/concerns/has_variable_spec.rb b/spec/models/concerns/has_variable_spec.rb index 3fbe86c5b56..bff96e12ffa 100644 --- a/spec/models/concerns/has_variable_spec.rb +++ b/spec/models/concerns/has_variable_spec.rb @@ -57,7 +57,7 @@ describe HasVariable do describe '#to_runner_variable' do it 'returns a hash for the runner' do expect(subject.to_runner_variable) - .to eq(key: subject.key, value: subject.value, public: false) + .to include(key: subject.key, value: subject.value, public: false) end end end diff --git a/spec/models/concerns/maskable_spec.rb b/spec/models/concerns/maskable_spec.rb new file mode 100644 index 00000000000..aeba7ad862f --- /dev/null +++ b/spec/models/concerns/maskable_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Maskable do + let(:variable) { build(:ci_variable) } + + describe 'masked value validations' do + subject { variable } + + context 'when variable is masked' do + before do + subject.masked = true + end + + it { is_expected.not_to allow_value('hello').for(:value) } + it { is_expected.not_to allow_value('hello world').for(:value) } + it { is_expected.not_to allow_value('hello$VARIABLEworld').for(:value) } + it { is_expected.not_to allow_value('hello\rworld').for(:value) } + it { is_expected.to allow_value('helloworld').for(:value) } + end + + context 'when variable is not masked' do + before do + subject.masked = false + end + + it { is_expected.to allow_value('hello').for(:value) } + it { is_expected.to allow_value('hello world').for(:value) } + it { is_expected.to allow_value('hello$VARIABLEworld').for(:value) } + it { is_expected.to allow_value('hello\rworld').for(:value) } + it { is_expected.to allow_value('helloworld').for(:value) } + end + end + + describe 'REGEX' do + subject { Maskable::REGEX } + + it 'does not match strings shorter than 8 letters' do + expect(subject.match?('hello')).to eq(false) + end + + it 'does not match strings with spaces' do + expect(subject.match?('hello world')).to eq(false) + end + + it 'does not match strings with shell variables' do + expect(subject.match?('hello$VARIABLEworld')).to eq(false) + end + + it 'does not match strings with escape characters' do + expect(subject.match?('hello\rworld')).to eq(false) + end + + it 'does not match strings that span more than one line' do + string = <<~EOS + hello + world + EOS + + expect(subject.match?(string)).to eq(false) + end + + it 'matches valid strings' do + expect(subject.match?('helloworld')).to eq(true) + end + end + + describe '#to_runner_variable' do + subject { variable.to_runner_variable } + + it 'exposes the masked attribute' do + expect(subject).to include(:masked) + end + end +end diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb index e52f4c70407..66b9aae4b58 100644 --- a/spec/requests/api/group_variables_spec.rb +++ b/spec/requests/api/group_variables_spec.rb @@ -87,12 +87,12 @@ describe API::GroupVariables do it 'creates variable' do expect do - post api("/groups/#{group.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true } + post api("/groups/#{group.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'PROTECTED_VALUE_2', protected: true } end.to change {group.variables.count}.by(1) expect(response).to have_gitlab_http_status(201) expect(json_response['key']).to eq('TEST_VARIABLE_2') - expect(json_response['value']).to eq('VALUE_2') + expect(json_response['value']).to eq('PROTECTED_VALUE_2') expect(json_response['protected']).to be_truthy end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index e6c235ca26e..43c06f7c973 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -436,9 +436,9 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end let(:expected_variables) do - [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true }, - { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true }, - { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }] + [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true, 'masked' => false }, + { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true, 'masked' => false }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true, 'masked' => false }] end let(:expected_artifacts) do @@ -740,12 +740,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when triggered job is available' do let(:expected_variables) do - [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true }, - { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true }, - { 'key' => 'CI_PIPELINE_TRIGGERED', 'value' => 'true', 'public' => true }, - { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }, - { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false }, - { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }] + [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true, 'masked' => false }, + { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true, 'masked' => false }, + { 'key' => 'CI_PIPELINE_TRIGGERED', 'value' => 'true', 'public' => true, 'masked' => false }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true, 'masked' => false }, + { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false, 'masked' => false }, + { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false, 'masked' => false }] end let(:trigger) { create(:ci_trigger, project: project) } diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index cdac5b2f400..5df6baf0ddf 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -73,12 +73,12 @@ describe API::Variables do context 'authorized user with proper permissions' do it 'creates variable' do expect do - post api("/projects/#{project.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true } + post api("/projects/#{project.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'PROTECTED_VALUE_2', protected: true } end.to change {project.variables.count}.by(1) expect(response).to have_gitlab_http_status(201) expect(json_response['key']).to eq('TEST_VARIABLE_2') - expect(json_response['value']).to eq('VALUE_2') + expect(json_response['value']).to eq('PROTECTED_VALUE_2') expect(json_response['protected']).to be_truthy end diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 0a464d77cb7..73156d18c1b 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -8,7 +8,7 @@ shared_examples 'variable list' do it 'adds new CI 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('key value') + find('.js-ci-variable-input-value').set('key_value') end click_button('Save variables') @@ -19,7 +19,7 @@ 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('key value') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value') end end @@ -44,7 +44,7 @@ shared_examples 'variable list' do it 'adds 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('key value') + 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') @@ -58,7 +58,7 @@ 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('key value') + 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 -- cgit v1.2.3