Masks variables in error messages

Merge branch 'security-lm-mask-variables-in-error-14-8' into '14-8-stable-ee'

See merge request gitlab-org/security/gitlab!2291

Changelog: security

lib/gitlab/ci/config/external/context.rb

@@ -70,6 +70,16 @@ module Gitlab
             }
           end
+          def mask_variables_from(location)
+            variables.reduce(location.dup) do |loc, variable|
+              if variable[:masked]
+                Gitlab::Ci::MaskSecret.mask!(loc, variable[:value])
+              else
+                loc
+              end
+            end
+          end
+
           protected
           attr_writer :expandset, :execution_deadline, :logger

lib/gitlab/ci/config/external/file/artifact.rb

@@ -37,7 +37,7 @@ module Gitlab
             def validate_content!
               return unless ensure_preconditions_satisfied!
-              errors.push("File `#{location}` is empty!") unless content.present?
+              errors.push("File `#{masked_location}` is empty!") unless content.present?
             end
             def ensure_preconditions_satisfied!

lib/gitlab/ci/config/external/file/base.rb

@@ -79,21 +79,21 @@ module Gitlab
             def validate_location!
               if invalid_location_type?
-                errors.push("Included file `#{location}` needs to be a string")
+                errors.push("Included file `#{masked_location}` needs to be a string")
               elsif invalid_extension?
-                errors.push("Included file `#{location}` does not have YAML extension!")
+                errors.push("Included file `#{masked_location}` does not have YAML extension!")
               end
             end
             def validate_content!
               if content.blank?
-                errors.push("Included file `#{location}` is empty or does not exist!")
+                errors.push("Included file `#{masked_location}` is empty or does not exist!")
               end
             end
             def validate_hash!
               if to_hash.blank?
-                errors.push("Included file `#{location}` does not have valid YAML syntax!")
+                errors.push("Included file `#{masked_location}` does not have valid YAML syntax!")
               end
             end
@@ -104,6 +104,12 @@ module Gitlab
             def expand_context_attrs
               {}
             end
+
+            def masked_location
+              strong_memoize(:masked_location) do
+                context.mask_variables_from(location)
+              end
+            end
           end
         end
       end

lib/gitlab/ci/config/external/file/local.rb

@@ -23,11 +23,11 @@ module Gitlab
             def validate_content!
               if context.project&.repository.nil?
-                errors.push("Local file `#{location}` does not have project!")
+                errors.push("Local file `#{masked_location}` does not have project!")
               elsif content.nil?
-                errors.push("Local file `#{location}` does not exist!")
+                errors.push("Local file `#{masked_location}` does not exist!")
               elsif content.blank?
-                errors.push("Local file `#{location}` is empty!")
+                errors.push("Local file `#{masked_location}` is empty!")
               end
             end

lib/gitlab/ci/config/external/file/project.rb

@@ -35,9 +35,9 @@ module Gitlab
               elsif sha.nil?
                 errors.push("Project `#{project_name}` reference `#{ref_name}` does not exist!")
               elsif content.nil?
-                errors.push("Project `#{project_name}` file `#{location}` does not exist!")
+                errors.push("Project `#{project_name}` file `#{masked_location}` does not exist!")
               elsif content.blank?
-                errors.push("Project `#{project_name}` file `#{location}` is empty!")
+                errors.push("Project `#{project_name}` file `#{masked_location}` is empty!")
               end
             end

lib/gitlab/ci/config/external/file/remote.rb

@@ -24,7 +24,7 @@ module Gitlab
               super
               unless ::Gitlab::UrlSanitizer.valid?(location)
-                errors.push("Remote file `#{location}` does not have a valid address!")
+                errors.push("Remote file `#{masked_location}` does not have a valid address!")
               end
             end
@@ -32,17 +32,17 @@ module Gitlab
               begin
                 response = Gitlab::HTTP.get(location)
               rescue SocketError
-                errors.push("Remote file `#{location}` could not be fetched because of a socket error!")
+                errors.push("Remote file `#{masked_location}` could not be fetched because of a socket error!")
               rescue Timeout::Error
-                errors.push("Remote file `#{location}` could not be fetched because of a timeout error!")
+                errors.push("Remote file `#{masked_location}` could not be fetched because of a timeout error!")
               rescue Gitlab::HTTP::Error
-                errors.push("Remote file `#{location}` could not be fetched because of HTTP error!")
+                errors.push("Remote file `#{masked_location}` could not be fetched because of HTTP error!")
               rescue Gitlab::HTTP::BlockedUrlError => e
                 errors.push("Remote file could not be fetched because #{e}!")
               end
               if response&.code.to_i >= 400
-                errors.push("Remote file `#{location}` could not be fetched because of HTTP code `#{response.code}` error!")
+                errors.push("Remote file `#{masked_location}` could not be fetched because of HTTP code `#{response.code}` error!")
               end
               response.body if errors.none?

lib/gitlab/ci/config/external/file/template.rb

@@ -26,7 +26,7 @@ module Gitlab
               super
               unless template_name_valid?
-                errors.push("Template file `#{location}` is not a valid location!")
+                errors.push("Template file `#{masked_location}` is not a valid location!")
               end
             end

lib/gitlab/ci/config/external/mapper.rb

@@ -142,7 +142,7 @@ module Gitlab
               file_class.new(location, context)
             end.select(&:matching?)
-            raise AmbigiousSpecificationError, "Include `#{location.to_json}` needs to match exactly one accessor!" unless matching.one?
+            raise AmbigiousSpecificationError, "Include `#{masked_location(location.to_json)}` needs to match exactly one accessor!" unless matching.one?
             matching.first
           end
@@ -177,6 +177,10 @@ module Gitlab
           def expand(data)
             ExpandVariables.expand(data, -> { context.variables_hash })
           end
+
+          def masked_location(location)
+            context.mask_variables_from(location)
+          end
         end
       end
     end

spec/lib/gitlab/ci/config/external/file/artifact_spec.rb

@@ -127,6 +127,12 @@ RSpec.describe Gitlab::Ci::Config::External::File::Artifact do
                 let!(:metadata) { create(:ci_job_artifact, :metadata, job: generator_job) }
                 context 'when file is empty' do
+                  let(:params) { { artifact: 'secret_stuff/generated.yml', job: 'generator' } }
+                  let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_stuff', 'masked' => true }]) }
+                  let(:context) do
+                    Gitlab::Ci::Config::External::Context.new(parent_pipeline: parent_pipeline, variables: variables)
+                  end
+
                   before do
                     allow_next_instance_of(Gitlab::Ci::ArtifactFileReader) do |reader|
                       allow(reader).to receive(:read).and_return('')
@@ -134,7 +140,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Artifact do
                   end
                   let(:expected_error) do
-                    'File `generated.yml` is empty!'
+                    'File `xxxxxxxxxxxx/generated.yml` is empty!'
                   end
                   it_behaves_like 'is invalid'

spec/lib/gitlab/ci/config/external/file/base_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 RSpec.describe Gitlab::Ci::Config::External::File::Base do
-  let(:context_params) { { sha: 'HEAD' } }
+  let(:variables) { }
+  let(:context_params) { { sha: 'HEAD', variables: variables } }
   let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
   let(:test_class) do
@@ -76,7 +77,8 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
     end
     context 'when there are YAML syntax errors' do
-      let(:location) { 'some/file/config.yml' }
+      let(:location) { 'some/file/secret_file_name.yml' }
+      let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_file_name', 'masked' => true }]) }
       before do
         allow_any_instance_of(test_class)
@@ -85,7 +87,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       it 'is not a valid file' do
         expect(subject).not_to be_valid
-        expect(subject.error_message).to match /does not have valid YAML syntax/
+        expect(subject.error_message).to eq('Included file `some/file/xxxxxxxxxxxxxxxx.yml` does not have valid YAML syntax!')
       end
     end
   end

spec/lib/gitlab/ci/config/external/file/local_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
   let_it_be(:user) { create(:user) }
   let(:sha) { '12345' }
+  let(:variables) { project.predefined_variables.to_runner_variables }
   let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
   let(:params) { { local: location } }
   let(:local_file) { described_class.new(params, context) }
@@ -18,7 +19,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
       sha: sha,
       user: user,
       parent_pipeline: parent_pipeline,
-      variables: project.predefined_variables.to_runner_variables
+      variables: variables
     }
   end
@@ -66,7 +67,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
       end
     end
-    context 'when is not a valid local path' do
+    context 'when it is not a valid local path' do
       let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
       it 'returns false' do
@@ -74,13 +75,23 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
       end
     end
-    context 'when is not a yaml file' do
+    context 'when it is not a yaml file' do
       let(:location) { '/config/application.rb' }
       it 'returns false' do
         expect(local_file.valid?).to be_falsy
       end
     end
+
+    context 'when it is an empty file' do
+      let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret', 'masked' => true }]) }
+      let(:location) { '/lib/gitlab/ci/templates/secret/existent-file.yml' }
+
+      it 'returns false and adds an error message about an empty file' do
+        allow_any_instance_of(described_class).to receive(:fetch_local_content).and_return("")
+        expect(local_file.errors).to include("Local file `/lib/gitlab/ci/templates/xxxxxx/existent-file.yml` is empty!")
+      end
+    end
   end
   describe '#content' do
@@ -116,10 +127,11 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
   end
   describe '#error_message' do
-    let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
+    let(:location) { '/lib/gitlab/ci/templates/secret_file.yml' }
+    let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_file', 'masked' => true }]) }
     it 'returns an error message' do
-      expect(local_file.error_message).to eq("Local file `#{location}` does not exist!")
+      expect(local_file.error_message).to eq("Local file `/lib/gitlab/ci/templates/xxxxxxxxxxx.yml` does not exist!")
     end
   end

spec/lib/gitlab/ci/config/external/file/project_spec.rb

@@ -11,6 +11,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
   let(:parent_pipeline) { double(:parent_pipeline) }
   let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
   let(:project_file) { described_class.new(params, context) }
+  let(:variables) { project.predefined_variables.to_runner_variables }
   let(:context_params) do
     {
@@ -18,7 +19,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       sha: '12345',
       user: context_user,
       parent_pipeline: parent_pipeline,
-      variables: project.predefined_variables.to_runner_variables
+      variables: variables
     }
   end
@@ -108,18 +109,19 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
     context 'when an empty file is used' do
       let(:params) do
-        { project: project.full_path, file: '/file.yml' }
+        { project: project.full_path, file: '/secret_file.yml' }
       end
+      let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_file', 'masked' => true }]) }
       let(:root_ref_sha) { project.repository.root_ref_sha }
       before do
-        stub_project_blob(root_ref_sha, '/file.yml') { '' }
+        stub_project_blob(root_ref_sha, '/secret_file.yml') { '' }
       end
       it 'returns false' do
         expect(project_file).not_to be_valid
-        expect(project_file.error_message).to include("Project `#{project.full_path}` file `/file.yml` is empty!")
+        expect(project_file.error_message).to include("Project `#{project.full_path}` file `/xxxxxxxxxxx.yml` is empty!")
       end
     end
@@ -135,13 +137,15 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
     end
     context 'when non-existing file is requested' do
+      let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret-invalid-file', 'masked' => true }]) }
+
       let(:params) do
-        { project: project.full_path, file: '/invalid-file.yml' }
+        { project: project.full_path, file: '/secret-invalid-file.yml' }
       end
       it 'returns false' do
         expect(project_file).not_to be_valid
-        expect(project_file.error_message).to include("Project `#{project.full_path}` file `/invalid-file.yml` does not exist!")
+        expect(project_file.error_message).to include("Project `#{project.full_path}` file `/xxxxxxxxxxxxxxxxxxx.yml` does not exist!")
       end
     end

spec/lib/gitlab/ci/config/external/file/remote_spec.rb

@@ -5,11 +5,12 @@ require 'spec_helper'
 RSpec.describe Gitlab::Ci::Config::External::File::Remote do
   include StubRequests
-  let(:context_params) { { sha: '12345' } }
+  let(:variables) {Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_file', 'masked' => true }]) }
+  let(:context_params) { { sha: '12345', variables: variables } }
   let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
   let(:params) { { remote: location } }
   let(:remote_file) { described_class.new(params, context) }
-  let(:location) { 'https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }
+  let(:location) { 'https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.secret_file.yml' }
   let(:remote_file_content) do
     <<~HEREDOC
       before_script:
@@ -144,10 +145,10 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
     subject { remote_file.error_message }
     context 'when remote file location is not valid' do
-      let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }
+      let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/?secret_file.yml' }
       it 'returns an error message describing invalid address' do
-        expect(subject).to match /does not have a valid address!/
+        expect(subject).to eq('Remote file `not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/?xxxxxxxxxxx.yml` does not have a valid address!')
       end
     end
@@ -157,7 +158,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
       end
       it 'returns error message about a timeout' do
-        expect(subject).to match /could not be fetched because of a timeout error!/
+        expect(subject).to eq('Remote file `https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.xxxxxxxxxxx.yml` could not be fetched because of a timeout error!')
       end
     end
@@ -167,7 +168,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
       end
       it 'returns error message about a HTTP error' do
-        expect(subject).to match /could not be fetched because of HTTP error!/
+        expect(subject).to eq('Remote file `https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.xxxxxxxxxxx.yml` could not be fetched because of HTTP error!')
       end
     end
@@ -177,7 +178,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
       end
       it 'returns error message about a timeout' do
-        expect(subject).to match /could not be fetched because of HTTP code `404` error!/
+        expect(subject).to eq('Remote file `https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.xxxxxxxxxxx.yml` could not be fetched because of HTTP code `404` error!')
       end
     end

spec/lib/gitlab/ci/config/external/file/template_spec.rb

@@ -54,11 +54,13 @@ RSpec.describe Gitlab::Ci::Config::External::File::Template do
     end
     context 'with invalid template name' do
-      let(:template) { 'Template.yml' }
+      let(:template) { 'SecretTemplate.yml' }
+      let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'SecretTemplate', 'masked' => true }]) }
+      let(:context_params) { { project: project, sha: '12345', user: user, variables: variables } }
       it 'returns false' do
         expect(template_file).not_to be_valid
-        expect(template_file.error_message).to include('Template file `Template.yml` is not a valid location!')
+        expect(template_file.error_message).to include('`xxxxxxxxxxxxxx.yml` is not a valid location!')
       end
     end

spec/lib/gitlab/ci/config/external/mapper_spec.rb

@@ -11,7 +11,8 @@ RSpec.describe Gitlab::Ci::Config::External::Mapper do
   let(:local_file) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
   let(:remote_url) { 'https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }
   let(:template_file) { 'Auto-DevOps.gitlab-ci.yml' }
-  let(:context_params) { { project: project, sha: '123456', user: user, variables: project.predefined_variables } }
+  let(:variables) { project.predefined_variables }
+  let(:context_params) { { project: project, sha: '123456', user: user, variables: variables } }
   let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
   let(:file_content) do
@@ -92,13 +93,16 @@ RSpec.describe Gitlab::Ci::Config::External::Mapper do
       end
       context 'when the key is a hash of file and remote' do
+        let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret-file', 'masked' => true }]) }
+        let(:local_file) { 'secret-file.yml' }
+        let(:remote_url) { 'https://gitlab.com/secret-file.yml' }
         let(:values) do
           { include: { 'local' => local_file, 'remote' => remote_url },
             image: 'ruby:2.7' }
         end
         it 'returns ambigious specification error' do
-          expect { subject }.to raise_error(described_class::AmbigiousSpecificationError)
+          expect { subject }.to raise_error(described_class::AmbigiousSpecificationError, 'Include `{"local":"xxxxxxxxxxx.yml","remote":"https://gitlab.com/xxxxxxxxxxx.yml"}` needs to match exactly one accessor!')
         end
       end

spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb

@@ -53,6 +53,24 @@ RSpec.describe Ci::CreatePipelineService do
     end
     context 'when failed to create the pipeline' do
+      context 'when errors are raised and masked variables are involved' do
+        let_it_be(:variable) { create(:ci_variable, project: project, key: 'GL_TOKEN', value: 'test_value', masked: true) }
+
+        let(:config) do
+          <<~YAML
+          include:
+            - local: $GL_TOKEN/gitlab-ci.txt
+          YAML
+        end
+
+        it 'contains errors and masks variables' do
+          error_message = "Included file `xxxxxxxxxx/gitlab-ci.txt` does not have YAML extension!"
+          expect(pipeline.yaml_errors).to eq(error_message)
+          expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message)
+          expect(pipeline.errors.full_messages).to contain_exactly(error_message)
+        end
+      end
+
       context 'when warnings are raised' do
         let(:config) do
           <<~YAML