Merge branch '353126-surface-validation-errors-as-warnings' into 'master'

Surface validation errors as warnings

See merge request gitlab-org/gitlab!80930

app/services/security/merge_reports_service.rb

@@ -21,7 +21,10 @@ module Security
         source_reports.first.type,
         source_reports.first.pipeline,
         source_reports.first.created_at
-      ).tap { |report| report.errors = source_reports.flat_map(&:errors) }
+      ).tap do |report|
+        report.errors = source_reports.flat_map(&:errors)
+        report.warnings = source_reports.flat_map(&:warnings)
+      end
     end
 
     def copy_resources_to_target_report

config/feature_flags/development/enforce_security_report_validation.yml → config/feature_flags/development/show_report_validation_warnings.yml

 ---
-name: enforce_security_report_validation
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79798
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/351000
-milestone: '14.8'
+name: show_report_validation_warnings
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80930
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353125
+milestone: '14.9'
 type: development
 group: group::threat insights
 default_enabled: false

doc/api/graphql/reference/index.md

@@ -15255,6 +15255,7 @@ Represents the security scan information.
 | ---- | ---- | ----------- |
 | <a id="scanerrors"></a>`errors` | [`[String!]!`](#string) | List of errors. |
 | <a id="scanname"></a>`name` | [`String!`](#string) | Name of the scan. |
+| <a id="scanwarnings"></a>`warnings` | [`[String!]!`](#string) | List of warnings. |
 
 ### `ScanExecutionPolicy`
 

ee/app/graphql/types/scan_type.rb

@@ -12,5 +12,6 @@ module Types
 
     field :errors, [GraphQL::Types::String], null: false, description: 'List of errors.'
     field :name, GraphQL::Types::String, null: false, description: 'Name of the scan.'
+    field :warnings, [GraphQL::Types::String], null: false, description: 'List of warnings.'
   end
 end

ee/app/models/security/scan.rb

@@ -55,12 +55,24 @@ module Security
       scan_types.keys & Array(given_types).map(&:to_s)
     end
 
+    def has_warnings?
+      processing_warnings.present?
+    end
+
+    def processing_warnings
+      info.fetch('warnings', [])
+    end
+
+    def processing_warnings=(warnings)
+      info['warnings'] = warnings
+    end
+
     def has_errors?
       processing_errors.present?
     end
 
     def processing_errors
-      info&.fetch('errors', [])
+      info.fetch('errors', [])
     end
 
     def processing_errors=(errors)

ee/app/presenters/security/scan_presenter.rb

@@ -2,13 +2,18 @@
 
 module Security
   class ScanPresenter < Gitlab::View::Presenter::Delegated
-    ERROR_MESSAGE_FORMAT = '[%<type>s] %<message>s'
+    MESSAGE_FORMAT = '[%<type>s] %<message>s'
 
     presents ::Security::Scan, as: :scan
 
     delegator_override :errors
     def errors
-      processing_errors.to_a.map { |error| format(ERROR_MESSAGE_FORMAT, error.symbolize_keys) }
+      processing_errors.to_a.map { |error| format(MESSAGE_FORMAT, error.symbolize_keys) }
+    end
+
+    delegator_override :warnings
+    def warnings
+      processing_warnings.to_a.map { |warning| format(MESSAGE_FORMAT, warning.symbolize_keys) }
     end
   end
 end

ee/app/services/security/store_scan_service.rb

@@ -46,6 +46,7 @@ module Security
     def security_scan
       @security_scan ||= Security::Scan.safe_find_or_create_by!(build: job, scan_type: artifact.file_type) do |scan|
         scan.processing_errors = security_report.errors.map(&:stringify_keys) if security_report.errored?
+        scan.processing_warnings = security_report.warnings.map(&:stringify_keys)
         scan.status = job.success? ? :succeeded : :failed
       end
     end

ee/app/validators/json_schemas/security_scan_info.json

@@ -23,6 +23,25 @@
           "message"
         ]
       }
+    },
+    "warnings": {
+      "type": "array",
+      "items": {
+        "type": "object",
+        "additionalProperties": false,
+        "properties": {
+          "type": {
+            "type": "string"
+          },
+          "message": {
+            "type": "string"
+          }
+        },
+        "required": [
+          "type",
+          "message"
+        ]
+      }
     }
   }
 }

ee/spec/graphql/types/scan_type_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe GitlabSchema.types['Scan'] do
   include GraphqlHelpers
 
-  let(:fields) { %i(name errors) }
+  let(:fields) { %i(name errors warnings) }
 
   it { expect(described_class).to have_graphql_fields(fields) }
   it { expect(described_class).to require_graphql_authorizations(:read_scan) }
@@ -36,7 +36,17 @@ RSpec.describe GitlabSchema.types['Scan'] do
         security_scan.update!(info: { 'errors' => [{ 'type' => 'foo', 'message' => 'bar' }] })
       end
 
-      it { is_expected.to eq(['[foo] bar']) }
+      it { is_expected.to match_array(['[foo] bar']) }
+    end
+
+    describe 'warnings' do
+      let(:field_name) { :warnings }
+
+      before do
+        security_scan.update!(info: { 'warnings' => [{ 'type' => 'foo', 'message' => 'bar' }] })
+      end
+
+      it { is_expected.to match_array(['[foo] bar']) }
     end
   end
 end

ee/spec/models/security/scan_spec.rb

@@ -46,6 +46,94 @@ RSpec.describe Security::Scan do
     it { is_expected.to delegate_method(:name).to(:build) }
   end
 
+  describe '#has_warnings?' do
+    let(:scan) { build(:security_scan, info: info) }
+
+    subject { scan.has_warnings? }
+
+    context 'when the info attribute is nil' do
+      let(:info) { nil }
+
+      it 'is not valid' do
+        expect(scan.valid?).to eq(false)
+      end
+    end
+
+    context 'when the info attribute is present' do
+      let(:info) { { warnings: warnings } }
+
+      context 'when there is no warnings' do
+        let(:warnings) { [] }
+
+        it { is_expected.to eq(false) }
+      end
+
+      context 'when there are warnings' do
+        let(:warnings) { [{ type: 'Foo', message: 'Bar' }] }
+
+        it { is_expected.to eq(true) }
+      end
+    end
+  end
+
+  describe '#processing_warnings' do
+    let(:scan) { build(:security_scan, info: info) }
+    let(:info) { { warnings: validator_warnings } }
+
+    subject(:warnings) { scan.processing_warnings }
+
+    context 'when there are warnings' do
+      let(:validator_warnings) { [{ type: 'Foo', message: 'Bar' }] }
+
+      it 'returns all warnings' do
+        expect(warnings).to match_array([
+          { "message" => "Bar", "type" => "Foo" }
+        ])
+      end
+    end
+
+    context 'when there are no warnings' do
+      let(:validator_warnings) { [] }
+
+      it 'returns []' do
+        expect(warnings).to match_array(validator_warnings)
+      end
+    end
+  end
+
+  describe '#processing_warnings=' do
+    let(:scan) { create(:security_scan) }
+
+    subject(:set_warnings) { scan.processing_warnings = [:foo] }
+
+    it 'sets the warnings' do
+      expect { set_warnings }.to change { scan.info['warnings'] }.from(nil).to([:foo])
+    end
+  end
+
+  describe '#has_warnings?' do
+    let(:scan) { build(:security_scan, info: info) }
+    let(:info) { { warnings: validator_warnings } }
+
+    subject(:has_warnings?) { scan.has_warnings? }
+
+    context 'when there are warnings' do
+      let(:validator_warnings) { [{ type: 'Foo', message: 'Bar' }] }
+
+      it 'returns true' do
+        expect(has_warnings?).to eq(true)
+      end
+    end
+
+    context 'when there are no warnings' do
+      let(:validator_warnings) { [] }
+
+      it 'returns false' do
+        expect(has_warnings?).to eq(false)
+      end
+    end
+  end
+
   describe '#has_errors?' do
     let(:scan) { build(:security_scan, info: info) }
 
@@ -54,7 +142,9 @@ RSpec.describe Security::Scan do
     context 'when the info attribute is nil' do
       let(:info) { nil }
 
-      it { is_expected.to be_falsey }
+      it 'is not valid' do
+        expect(scan.valid?).to eq(false)
+      end
     end
 
     context 'when the info attribute presents' do
@@ -63,13 +153,13 @@ RSpec.describe Security::Scan do
       context 'when there is no error' do
         let(:errors) { [] }
 
-        it { is_expected.to be_falsey }
+        it { is_expected.to eq(false) }
       end
 
       context 'when there are errors' do
         let(:errors) { [{ type: 'Foo', message: 'Bar' }] }
 
-        it { is_expected.to be_truthy }
+        it { is_expected.to eq(true) }
       end
     end
   end

ee/spec/presenters/security/scan_presenter_spec.rb

@@ -4,11 +4,17 @@ require 'spec_helper'
 
 RSpec.describe Security::ScanPresenter do
   let(:presenter) { described_class.new(security_scan) }
-  let(:security_scan) { build_stubbed(:security_scan, info: { 'errors' => [{ 'type' => 'foo', 'message' => 'bar' }] }) }
+  let(:security_scan) { build_stubbed(:security_scan, info: { 'errors' => [{ 'type' => 'foo', 'message' => 'bar' }], 'warnings' => [{ 'type' => 'foo', 'message' => 'bar' }] }) }
 
   describe '#errors' do
     subject { presenter.errors }
 
-    it { is_expected.to eq(['[foo] bar']) }
+    it { is_expected.to match_array(['[foo] bar']) }
+  end
+
+  describe '#warnings' do
+    subject { presenter.warnings }
+
+    it { is_expected.to match_array(['[foo] bar']) }
   end
 end

ee/spec/services/security/store_scan_service_spec.rb

@@ -139,6 +139,28 @@ RSpec.describe Security::StoreScanService do
         expect(Security::StoreFindingsMetadataService).to have_received(:execute)
       end
 
+      context 'when the report has some warnings' do
+        before do
+          artifact.security_report.warnings << { 'type' => 'foo', 'message' => 'bar' }
+        end
+
+        let(:security_scan) { Security::Scan.last }
+
+        it 'calls the `Security::StoreFindingsMetadataService` to store findings' do
+          expect(store_scan).to be(true)
+
+          expect(Security::StoreFindingsMetadataService).to have_received(:execute)
+        end
+
+        it 'stores the warnings' do
+          store_scan
+
+          expect(security_scan.processing_warnings).to include(
+            { 'type' => 'foo', 'message' => 'bar' }
+          )
+        end
+      end
+
       context 'when the security scan already exists for the artifact' do
         let_it_be(:security_scan) { create(:security_scan, build: artifact.job, scan_type: :sast, status: :succeeded) }
         let_it_be(:unique_security_finding) do

lib/gitlab/ci/parsers/security/common.rb

@@ -42,14 +42,19 @@ module Gitlab
           attr_reader :json_data, :report, :validate
 
           def valid?
-            if Feature.enabled?(:enforce_security_report_validation)
-              if !validate || schema_validator.valid?
-                report.schema_validation_status = :valid_schema
-                true
+            if Feature.enabled?(:show_report_validation_warnings)
+              # We want validation to happen regardless of VALIDATE_SCHEMA CI variable
+              schema_validation_passed = schema_validator.valid?
+
+              if validate
+                schema_validator.errors.each { |error| report.add_error('Schema', error) } unless schema_validation_passed
+
+                schema_validation_passed
               else
-                report.schema_validation_status = :invalid_schema
-                schema_validator.errors.each { |error| report.add_error('Schema', error) }
-                false
+                # We treat all schema validation errors as warnings
+                schema_validator.errors.each { |error| report.add_warning('Schema', error) }
+
+                true
               end
             else
               return true if !validate || schema_validator.valid?

lib/gitlab/ci/reports/security/report.rb

@@ -6,7 +6,7 @@ module Gitlab
       module Security
         class Report
           attr_reader :created_at, :type, :pipeline, :findings, :scanners, :identifiers
-          attr_accessor :scan, :scanned_resources, :errors, :analyzer, :version, :schema_validation_status
+          attr_accessor :scan, :scanned_resources, :errors, :analyzer, :version, :schema_validation_status, :warnings
 
           delegate :project_id, to: :pipeline
 
@@ -19,6 +19,7 @@ module Gitlab
             @identifiers = {}
             @scanned_resources = []
             @errors = []
+            @warnings = []
           end
 
           def commit_sha
@@ -29,6 +30,10 @@ module Gitlab
             errors << { type: type, message: message }
           end
 
+          def add_warning(type, message)
+            warnings << { type: type, message: message }
+          end
+
           def errored?
             errors.present?
           end

spec/lib/gitlab/ci/parsers/security/common_spec.rb

@@ -26,8 +26,6 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
           allow(parser).to receive(:tracking_data).and_return(tracking_data)
           allow(parser).to receive(:create_flags).and_return(vulnerability_flags_data)
         end
-
-        artifact.each_blob { |blob| described_class.parse!(blob, report, vulnerability_finding_signatures_enabled) }
       end
 
       describe 'schema validation' do
@@ -40,40 +38,50 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
           allow(validator_class).to receive(:new).and_call_original
         end
 
-        context 'when enforce_security_report_validation is enabled' do
+        context 'when show_report_validation_warnings is enabled' do
           before do
-            stub_feature_flags(enforce_security_report_validation: true)
+            stub_feature_flags(show_report_validation_warnings: true)
           end
 
-          context 'when the validate flag is set as `true`' do
-            let(:validate) { true }
-
-            it 'instantiates the validator with correct params' do
-              parse_report
-
-              expect(validator_class).to have_received(:new).with(report.type, {})
-            end
-
-            context 'when the report data is valid according to the schema' do
-              let(:valid?) { true }
+          context 'when the validate flag is set to `false`' do
+            let(:validate) { false }
+            let(:valid?) { false }
+            let(:errors) { ['foo'] }
 
             before do
               allow_next_instance_of(validator_class) do |instance|
                 allow(instance).to receive(:valid?).and_return(valid?)
-                  allow(instance).to receive(:errors).and_return([])
+                allow(instance).to receive(:errors).and_return(errors)
               end
 
               allow(parser).to receive_messages(create_scanner: true, create_scan: true)
             end
 
-              it 'does not add errors to the report' do
-                expect { parse_report }.not_to change { report.errors }.from([])
+            it 'instantiates the validator with correct params' do
+              parse_report
+
+              expect(validator_class).to have_received(:new).with(report.type, {})
             end
 
-              it 'adds the schema validation status to the report' do
+            context 'when the report data is not valid according to the schema' do
+              it 'adds warnings to the report' do
+                expect { parse_report }.to change { report.warnings }.from([]).to([{ message: 'foo', type: 'Schema' }])
+              end
+
+              it 'keeps the execution flow as normal' do
                 parse_report
 
-                expect(report.schema_validation_status).to eq(:valid_schema)
+                expect(parser).to have_received(:create_scanner)
+                expect(parser).to have_received(:create_scan)
+              end
+            end
+
+            context 'when the report data is valid according to the schema' do
+              let(:valid?) { true }
+              let(:errors) { [] }
+
+              it 'does not add warnings to the report' do
+                expect { parse_report }.not_to change { report.errors }
               end
 
               it 'keeps the execution flow as normal' do
@@ -83,42 +91,62 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
                 expect(parser).to have_received(:create_scan)
               end
             end
+          end
 
-            context 'when the report data is not valid according to the schema' do
+          context 'when the validate flag is set to `true`' do
+            let(:validate) { true }
             let(:valid?) { false }
+            let(:errors) { ['foo'] }
 
             before do
               allow_next_instance_of(validator_class) do |instance|
                 allow(instance).to receive(:valid?).and_return(valid?)
-                  allow(instance).to receive(:errors).and_return(['foo'])
+                allow(instance).to receive(:errors).and_return(errors)
               end
 
               allow(parser).to receive_messages(create_scanner: true, create_scan: true)
             end
 
+            it 'instantiates the validator with correct params' do
+              parse_report
+
+              expect(validator_class).to have_received(:new).with(report.type, {})
+            end
+
+            context 'when the report data is not valid according to the schema' do
               it 'adds errors to the report' do
                 expect { parse_report }.to change { report.errors }.from([]).to([{ message: 'foo', type: 'Schema' }])
               end
 
-              it 'adds the schema validation status to the report' do
+              it 'does not try to create report entities' do
                 parse_report
 
-                expect(report.schema_validation_status).to eq(:invalid_schema)
+                expect(parser).not_to have_received(:create_scanner)
+                expect(parser).not_to have_received(:create_scan)
+              end
             end
 
-              it 'does not try to create report entities' do
+            context 'when the report data is valid according to the schema' do
+              let(:valid?) { true }
+              let(:errors) { [] }
+
+              it 'does not add errors to the report' do
+                expect { parse_report }.not_to change { report.errors }.from([])
+              end
+
+              it 'keeps the execution flow as normal' do
                 parse_report
 
-                expect(parser).not_to have_received(:create_scanner)
-                expect(parser).not_to have_received(:create_scan)
+                expect(parser).to have_received(:create_scanner)
+                expect(parser).to have_received(:create_scan)
               end
             end
           end
         end
 
-        context 'when enforce_security_report_validation is disabled' do
+        context 'when show_report_validation_warnings is disabled' do
           before do
-            stub_feature_flags(enforce_security_report_validation: false)
+            stub_feature_flags(show_report_validation_warnings: false)
           end
 
           context 'when the validate flag is set as `false`' do
@@ -181,6 +209,11 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
         end
       end
 
+      context 'report parsing' do
+        before do
+          artifact.each_blob { |blob| described_class.parse!(blob, report, vulnerability_finding_signatures_enabled) }
+        end
+
         describe 'parsing finding.name' do
           let(:artifact) { build(:ci_job_artifact, :common_security_report_with_blank_names) }
 
@@ -457,4 +490,5 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
         end
       end
     end
+  end
 end

spec/lib/gitlab/ci/reports/security/report_spec.rb

@@ -158,6 +158,16 @@ RSpec.describe Gitlab::Ci::Reports::Security::Report do
     end
   end
 
+  describe '#add_warning' do
+    context 'when the message is given' do
+      it 'adds a new warning to report' do
+        expect { report.add_warning('foo', 'bar') }.to change { report.warnings }
+                                                 .from([])
+                                                 .to([{ type: 'foo', message: 'bar' }])
+      end
+    end
+  end
+
   describe 'errored?' do
     subject { report.errored? }
 

spec/services/security/merge_reports_service_spec.rb

@@ -153,7 +153,18 @@ RSpec.describe Security::MergeReportsService, '#execute' do
       report_2.add_error('zoo', 'baz')
     end
 
-    it { is_expected.to eq([{ type: 'foo', message: 'bar' }, { type: 'zoo', message: 'baz' }]) }
+    it { is_expected.to match_array([{ type: 'foo', message: 'bar' }, { type: 'zoo', message: 'baz' }]) }
+  end
+
+  describe 'warnings on target report' do
+    subject { merged_report.warnings }
+
+    before do
+      report_1.add_warning('foo', 'bar')
+      report_2.add_warning('zoo', 'baz')
+    end
+
+    it { is_expected.to match_array([{ type: 'foo', message: 'bar' }, { type: 'zoo', message: 'baz' }]) }
   end
 
   it 'copies scanners into target report and eliminates duplicates' do