Merge branch 'fix_parsers_design_flaw' into 'master'

Fix parsers design flaw

See merge request gitlab-org/gitlab!50223

ee/app/models/ee/ci/build.rb

@@ -164,7 +164,7 @@ module EE
 
       def parse_security_artifact_blob(security_report, blob)
         report_clone = security_report.clone_as_blank
-        ::Gitlab::Ci::Parsers.fabricate!(security_report.type).parse!(blob, report_clone)
+        ::Gitlab::Ci::Parsers.fabricate!(security_report.type, blob, report_clone).parse!
         security_report.merge!(report_clone)
       end
 

ee/app/models/ee/ci/job_artifact.rb

@@ -120,7 +120,7 @@ module EE
 
         report = ::Gitlab::Ci::Reports::Security::Report.new(file_type, job.pipeline, nil).tap do |report|
           each_blob do |blob|
-            ::Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, report)
+            ::Gitlab::Ci::Parsers.fabricate!(file_type, blob, report).parse!
           end
         end
 

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

@@ -7,16 +7,21 @@ module Gitlab
         class Common
           SecurityReportParserError = Class.new(Gitlab::Ci::Parsers::ParserError)
 
-          def parse!(json_data, report)
-            report_data = parse_report(json_data)
-            raise SecurityReportParserError, "Invalid report format" unless report_data.is_a?(Hash)
+          def self.parse!(json_data, report)
+            new(json_data, report).parse!
+          end
 
-            create_scanner(report, report_data.dig('scan', 'scanner'))
-            create_scan(report, report_data.dig('scan'))
+          def initialize(json_data, report)
+            @json_data = json_data
+            @report = report
+          end
 
-            collate_remediations(report_data).each do |vulnerability|
-              create_vulnerability(report, vulnerability, report_data["version"])
-            end
+          def parse!
+            raise SecurityReportParserError, "Invalid report format" unless report_data.is_a?(Hash)
+
+            create_scanner
+            create_scan
+            collate_remediations.each { |vulnerability| create_vulnerability(vulnerability) }
 
             report_data
           rescue JSON::ParserError
@@ -26,25 +31,38 @@ module Gitlab
             raise SecurityReportParserError, "#{report.type} security report parsing failed"
           end
 
-          protected
+          private
+
+          attr_reader :json_data, :report
+
+          def report_data
+            @report_data ||= Gitlab::Json.parse!(json_data)
+          end
+
+          def report_version
+            @report_version ||= report_data['version']
+          end
+
+          def top_level_scanner
+            @top_level_scanner ||= report_data.dig('scan', 'scanner')
+          end
 
-          def parse_report(json_data)
-            Gitlab::Json.parse!(json_data)
+          def scan_data
+            @scan_data ||= report_data.dig('scan')
           end
 
           # map remediations to relevant vulnerabilities
-          def collate_remediations(report_data)
+          def collate_remediations
             return report_data["vulnerabilities"] || [] unless report_data["remediations"]
 
-            fixes = fixes_from(report_data)
             report_data["vulnerabilities"].map do |vulnerability|
               remediation = fixes[vulnerability['id']] || fixes[vulnerability['cve']]
               vulnerability.merge("remediations" => [remediation])
             end
           end
 
-          def fixes_from(report_data)
-            report_data['remediations'].each_with_object({}) do |item, memo|
+          def fixes
+            @fixes ||= report_data['remediations'].each_with_object({}) do |item, memo|
               item['fixes'].each do |fix|
                 id = fix['id'] || fix['cve']
                 memo[id] = item if id
@@ -53,56 +71,54 @@ module Gitlab
             end
           end
 
-          def create_vulnerability(report, data, version)
-            identifiers = create_identifiers(report, data['identifiers'])
-            links = create_links(report, data['links'])
+          def create_vulnerability(data)
+            identifiers = create_identifiers(data['identifiers'])
+            links = create_links(data['links'])
             location = create_location(data['location'] || {})
             remediations = create_remediations(data['remediations'])
 
             report.add_finding(
               ::Gitlab::Ci::Reports::Security::Finding.new(
-                uuid: calculate_uuid_v5(report, location, identifiers.first),
+                uuid: calculate_uuid_v5(identifiers.first, location),
                 report_type: report.type,
                 name: finding_name(data, identifiers, location),
                 compare_key: data['cve'] || '',
                 location: location,
-                severity: parse_severity_level(data['severity']&.downcase),
-                confidence: parse_confidence_level(data['confidence']&.downcase),
-                scanner: create_scanner(report, data['scanner']),
+                severity: parse_severity_level(data['severity']),
+                confidence: parse_confidence_level(data['confidence']),
+                scanner: create_scanner(data['scanner']),
                 scan: report&.scan,
                 identifiers: identifiers,
                 links: links,
                 remediations: remediations,
                 raw_metadata: data.to_json,
-                metadata_version: version,
+                metadata_version: report_version,
                 details: data['details'] || {}))
           end
 
-          def create_scan(report, scan_data)
+          def create_scan
             return unless scan_data.is_a?(Hash)
 
             report.scan = ::Gitlab::Ci::Reports::Security::Scan.new(scan_data)
           end
 
-          def create_scanner(report, scanner)
-            return unless scanner.is_a?(Hash)
+          def create_scanner(scanner_data = top_level_scanner)
+            return unless scanner_data.is_a?(Hash)
 
             report.add_scanner(
               ::Gitlab::Ci::Reports::Security::Scanner.new(
-                external_id: scanner['id'],
-                name: scanner['name'],
-                vendor: scanner.dig('vendor', 'name')))
+                external_id: scanner_data['id'],
+                name: scanner_data['name'],
+                vendor: scanner_data.dig('vendor', 'name')))
           end
 
-          def create_identifiers(report, identifiers)
+          def create_identifiers(identifiers)
             return [] unless identifiers.is_a?(Array)
 
-            identifiers.map do |identifier|
-              create_identifier(report, identifier)
-            end.compact
+            identifiers.map { |identifier| create_identifier(identifier) }.compact
           end
 
-          def create_identifier(report, identifier)
+          def create_identifier(identifier)
             return unless identifier.is_a?(Hash)
 
             report.add_identifier(
@@ -113,20 +129,16 @@ module Gitlab
                 url: identifier['url']))
           end
 
-          def create_links(report, links)
+          def create_links(links)
             return [] unless links.is_a?(Array)
 
-            links
-              .map { |link| create_link(report, link) }
-              .compact
+            links.map { |link| create_link(link) }.compact
           end
 
-          def create_link(report, link)
+          def create_link(link)
             return unless link.is_a?(Hash)
 
-            ::Gitlab::Ci::Reports::Security::Link.new(
-              name: link['name'],
-              url: link['url'])
+            ::Gitlab::Ci::Reports::Security::Link.new(name: link['name'], url: link['url'])
           end
 
           def create_remediations(remediations_data)
@@ -136,23 +148,17 @@ module Gitlab
           end
 
           def parse_severity_level(input)
-            return input if ::Enums::Vulnerability.severity_levels.key?(input)
-
-            'unknown'
+            input&.downcase.then { |value| ::Enums::Vulnerability.severity_levels.key?(value) ? value : 'unknown' }
           end
 
           def parse_confidence_level(input)
-            return input if ::Enums::Vulnerability.confidence_levels.key?(input)
-
-            'unknown'
+            input&.downcase.then { |value| ::Enums::Vulnerability.confidence_levels.key?(value) ? value : 'unknown' }
           end
 
           def create_location(location_data)
             raise NotImplementedError
           end
 
-          private
-
           def finding_name(data, identifiers, location)
             return data['message'] if data['message'].present?
             return data['name'] if data['name'].present?
@@ -161,7 +167,7 @@ module Gitlab
             "#{identifier.name} in #{location&.fingerprint_path}"
           end
 
-          def calculate_uuid_v5(report, location, primary_identifier)
+          def calculate_uuid_v5(primary_identifier, location)
             uuid_v5_name_components = {
               report_type: report.type,
               primary_identifier_fingerprint: primary_identifier&.fingerprint,

ee/lib/gitlab/ci/parsers/security/concerns/deprecated_syntax.rb

@@ -14,17 +14,19 @@ module Gitlab
               override :parse_report
             end
 
-            def parse_report(json_data)
-              report = super
+            def report_data
+              @report_data ||= begin
+                data = super
 
-              if report.is_a?(Array)
-                report = {
-                  "version" => self.class::DEPRECATED_REPORT_VERSION,
-                  "vulnerabilities" => report
-                }
-              end
+                if data.is_a?(Array)
+                  data = {
+                    "version" => self.class::DEPRECATED_REPORT_VERSION,
+                    "vulnerabilities" => data
+                  }
+                end
 
-              report
+                data
+              end
             end
           end
         end

ee/lib/gitlab/ci/parsers/security/dast.rb

@@ -5,20 +5,20 @@ module Gitlab
     module Parsers
       module Security
         class Dast < Common
-          def parse!(json_data, report)
-            report_data = super
+          def parse!
+            super
 
             report.scanned_resources = create_scanned_resources(report_data.dig('scan', 'scanned_resources'))
           end
 
           private
 
-          def parse_report(json_data)
-            report = super
-
-            return Formatters::Dast.new(report).format if Formatters::Dast.satisfies?(report)
-
-            report
+          def report_data
+            @report_data ||= begin
+              super.then do |data|
+                Formatters::Dast.satisfies?(data) ? Formatters::Dast.new(data).format : data
+              end
+            end
           end
 
           def create_scanned_resources(scanned_resources)

ee/spec/finders/security/findings_finder_spec.rb

@@ -56,10 +56,10 @@ RSpec.describe Security::FindingsFinder do
 
         before(:all) do
           ds_content = File.read(artifact_ds.file.path)
-          Gitlab::Ci::Parsers::Security::DependencyScanning.new.parse!(ds_content, report_ds)
+          Gitlab::Ci::Parsers::Security::DependencyScanning.parse!(ds_content, report_ds)
           report_ds.merge!(report_ds)
           sast_content = File.read(artifact_sast.file.path)
-          Gitlab::Ci::Parsers::Security::Sast.new.parse!(sast_content, report_sast)
+          Gitlab::Ci::Parsers::Security::Sast.parse!(sast_content, report_sast)
           report_sast.merge!(report_sast)
 
           { artifact_ds => report_ds, artifact_sast => report_sast }.each do |artifact, report|

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

@@ -8,13 +8,14 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
 
     let(:artifact) { build(:ee_ci_job_artifact, :common_security_report) }
     let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
-    let(:parser) { described_class.new }
     let(:location) { ::Gitlab::Ci::Reports::Security::Locations::DependencyScanning.new(file_path: 'yarn/yarn.lock', package_version: 'v2', package_name: 'saml2') }
 
     before do
-      allow(parser).to receive(:create_location).and_return(location)
+      allow_next_instance_of(described_class) do |parser|
+        allow(parser).to receive(:create_location).and_return(location)
+      end
 
-      artifact.each_blob { |blob| parser.parse!(blob, report) }
+      artifact.each_blob { |blob| described_class.parse!(blob, report) }
     end
 
     describe 'parsing finding.name' do
@@ -142,9 +143,8 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
       end
 
       it 'returns nil when scan is not a hash' do
-        parser =  described_class.new
         empty_report = Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago)
-        parser.parse!({}.to_json, empty_report)
+        described_class.parse!({}.to_json, empty_report)
 
         expect(empty_report.scan).to be(nil)
       end

ee/spec/lib/gitlab/ci/parsers/security/container_scanning_spec.rb

@@ -3,15 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Parsers::Security::ContainerScanning do
-  let(:parser) { described_class.new }
   let(:project) { artifact.project }
   let(:pipeline) { artifact.job.pipeline }
   let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
 
   before do
-    artifact.each_blob do |blob|
-      parser.parse!(blob, report)
-    end
+    artifact.each_blob { |blob| described_class.parse!(blob, report) }
   end
 
   describe '#parse!' do

ee/spec/lib/gitlab/ci/parsers/security/coverage_fuzzing_spec.rb

@@ -6,14 +6,11 @@ RSpec.describe Gitlab::Ci::Parsers::Security::CoverageFuzzing do
   let(:project) { artifact.project }
   let(:pipeline) { artifact.job.pipeline }
   let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
-  let(:parser) { described_class.new }
   let(:artifact) { create(:ee_ci_job_artifact, :coverage_fuzzing) }
 
   describe '#parse!' do
     before do
-      artifact.each_blob do |blob|
-        parser.parse!(blob, report)
-      end
+      artifact.each_blob { |blob| described_class.parse!(blob, report) }
     end
 
     it 'parses all identifiers and findings' do

ee/spec/lib/gitlab/ci/parsers/security/dast_spec.rb

@@ -10,7 +10,6 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Dast do
     let(:pipeline) { artifact.job.pipeline }
     let(:artifact) { create(:ee_ci_job_artifact, :dast) }
     let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
-    let(:parser) { described_class.new }
 
     where(:report_format,
           :occurrence_count,
@@ -32,9 +31,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Dast do
       let(:artifact) { create(:ee_ci_job_artifact, report_format) }
 
       before do
-        artifact.each_blob do |blob|
-          parser.parse!(blob, report)
-        end
+        artifact.each_blob { |blob| described_class.parse!(blob, report) }
       end
 
       it 'parses all identifiers, findings and scanned resources' do
@@ -75,9 +72,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Dast do
       let(:artifact) { create(:ee_ci_job_artifact, 'dast') }
 
       before do
-        artifact.each_blob do |blob|
-          parser.parse!(blob, report)
-        end
+        artifact.each_blob { |blob| described_class.parse!(blob, report) }
       end
 
       let(:raw_json) do
@@ -98,7 +93,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Dast do
       end
 
       it 'skips invalid URLs' do
-        parser.parse!(raw_json.to_json, report)
+        described_class.parse!(raw_json.to_json, report)
         expect(report.scanned_resources).to be_empty
       end
 

ee/spec/lib/gitlab/ci/parsers/security/dependency_scanning_spec.rb

@@ -10,7 +10,6 @@ RSpec.describe Gitlab::Ci::Parsers::Security::DependencyScanning do
     let(:pipeline) { artifact.job.pipeline }
     let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning) }
     let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
-    let(:parser) { described_class.new }
 
     where(:report_format, :occurrence_count, :identifier_count, :scanner_count, :file_path, :package_name, :package_version, :version) do
       :dependency_scanning             | 4 | 7 | 3 | 'app/pom.xml' | 'io.netty/netty' | '3.9.1.Final' | '1.3'
@@ -22,9 +21,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::DependencyScanning do
       let(:artifact) { create(:ee_ci_job_artifact, report_format) }
 
       before do
-        artifact.each_blob do |blob|
-          parser.parse!(blob, report)
-        end
+        artifact.each_blob { |blob| described_class.parse!(blob, report) }
       end
 
       it "parses all identifiers and findings" do
@@ -56,7 +53,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::DependencyScanning do
         report_hash[:vulnerabilities][0][:location] = nil
       end
 
-      it { expect { parser.parse!(report_hash.to_json, report) }.not_to raise_error }
+      it { expect { described_class.parse!(report_hash.to_json, report) }.not_to raise_error }
     end
 
     context "when parsing a vulnerability with a missing cve" do
@@ -66,16 +63,14 @@ RSpec.describe Gitlab::Ci::Parsers::Security::DependencyScanning do
         report_hash[:vulnerabilities][0][:cve] = nil
       end
 
-      it { expect { parser.parse!(report_hash.to_json, report) }.not_to raise_error }
+      it { expect { described_class.parse!(report_hash.to_json, report) }.not_to raise_error }
     end
 
     context "when vulnerabilities have remediations" do
       let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning_remediation) }
 
       before do
-        artifact.each_blob do |blob|
-          parser.parse!(blob, report)
-        end
+        artifact.each_blob { |blob| described_class.parse!(blob, report) }
       end
 
       it "generates occurrence with expected remediation" do

ee/spec/lib/gitlab/ci/parsers/security/sast_spec.rb

@@ -10,8 +10,6 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Sast do
 
     let(:created_at) { 2.weeks.ago }
 
-    subject(:parser) { described_class.new }
-
     context "when parsing valid reports" do
       where(:report_format, :scanner_length) do
         :sast               | 4
@@ -23,9 +21,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Sast do
         let(:artifact) { create(:ee_ci_job_artifact, report_format) }
 
         before do
-          artifact.each_blob do |blob|
-            parser.parse!(blob, report)
-          end
+          artifact.each_blob { |blob| described_class.parse!(blob, report) }
         end
 
         it "parses all identifiers and findings" do
@@ -57,7 +53,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Sast do
       let(:report) { Gitlab::Ci::Reports::Security::Report.new('sast', pipeline, created_at) }
       let(:blob) { Gitlab::Json.generate({}) }
 
-      it { expect(parser.parse!(blob, report)).to be_empty }
+      it { expect(described_class.parse!(blob, report)).to be_empty }
     end
   end
 end

ee/spec/lib/gitlab/ci/parsers/security/secret_detection_spec.rb

@@ -8,8 +8,6 @@ RSpec.describe Gitlab::Ci::Parsers::Security::SecretDetection do
 
     let(:created_at) { 2.weeks.ago }
 
-    subject(:parser) { described_class.new }
-
     context "when parsing valid reports" do
       where(report_format: %i(secret_detection))
 
@@ -18,9 +16,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::SecretDetection do
         let(:artifact) { create(:ee_ci_job_artifact, report_format) }
 
         before do
-          artifact.each_blob do |blob|
-            parser.parse!(blob, report)
-          end
+          artifact.each_blob { |blob| described_class.parse!(blob, report) }
         end
 
         it "parses all identifiers and findings" do
@@ -52,7 +48,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::SecretDetection do
       let(:report) { Gitlab::Ci::Reports::Security::Report.new('secret_detection', pipeline, created_at) }
       let(:blob) { Gitlab::Json.generate({}) }
 
-      it { expect(parser.parse!(blob, report)).to be_empty }
+      it { expect(described_class.parse!(blob, report)).to be_empty }
     end
   end
 end

lib/gitlab/ci/parsers.rb

@@ -15,8 +15,8 @@ module Gitlab
         }
       end
 
-      def self.fabricate!(file_type)
-        parsers.fetch(file_type.to_sym).new
+      def self.fabricate!(file_type, *args)
+        parsers.fetch(file_type.to_sym).new(*args)
       rescue KeyError
         raise ParserNotFoundError, "Cannot find any parser matching file type '#{file_type}'"
       end