Merge branch...

Merge branch '10014-ee-spec-models-ci-build_spec-rb-ee-spec-requests-api-runner_spec-rb-and-ee-spec-services-ci-process_pipeline_service_spec-rb-are-all-failing-ce' into 'master'

Revert "Merge branch 'revert-8baf9e5f' into 'master'"

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

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"
 

app/models/ci/variable.rb

@@ -5,6 +5,7 @@ module Ci
     extend Gitlab::Ci::Model
     include HasVariable
     include Presentable
+    include Maskable
 
     belongs_to :project
 

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

app/models/concerns/maskable.rb

+# 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

changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml

+---
+title: Add support for masking CI variables.
+merge_request: 25293
+author:
+type: added

db/migrate/20190218134158_add_masked_to_ci_variables.rb

+# 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

db/migrate/20190218134209_add_masked_to_ci_group_variables.rb

+# 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

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
 

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
 

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

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

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

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

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

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
 

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

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

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
 

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

spec/models/concerns/maskable_spec.rb

+# 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

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
 

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) }

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
 

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