Merge branch '11930-handle-multiple-entries-dast-report' into 'master'

Handle multiple entries in the DAST report artifact,  Add specs for legacy DAST report

See merge request gitlab-org/gitlab!17779

changelogs/unreleased/11930-handle-multiple-entries-dast-report.yml

+---
+title: The Security Dashboard displays DAST vulnerabilities for all the scanned sites, not just the first
+merge_request: 17779
+author:
+type: added

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

@@ -19,24 +19,31 @@ module Gitlab
 
           def format_report(data)
             {
-              'vulnerabilities' => extract_vulnerabilities_from(data),
+              'vulnerabilities' => extract_vulnerabilities_from(Array.wrap(data['site'])),
               'version' => FORMAT_VERSION
             }
           end
 
-          def extract_vulnerabilities_from(data)
-            site = data['site'].first
-            results = []
+          # Log messages to be added here to track usage of legacy reports,
+          # parsing failures and any other scenarios: https://gitlab.com/gitlab-org/gitlab/issues/34668
+          def extract_vulnerabilities_from(sites = [])
+            return [] if sites.empty?
 
-            if site
-              host = site['@name']
+            vulnerabilities = []
 
-              site['alerts'].each do |vulnerability|
-                results += flatten_vulnerabilities(vulnerability, host)
+            sites.each do |site|
+              site_report = Hash(site)
+              next if site_report.blank?
+
+              # If host is blank for legacy reports
+              host = site_report['@name']
+
+              site_report['alerts'].each do |vulnerability|
+                vulnerabilities += flatten_vulnerabilities(vulnerability, host)
               end
             end
 
-            results
+            vulnerabilities
           end
 
           def flatten_vulnerabilities(vulnerability, host)

ee/spec/factories/ci/job_artifacts.rb

@@ -182,6 +182,26 @@ FactoryBot.define do
       end
     end
 
+    trait :dast_deprecated do
+      file_format { :raw }
+      file_type { :dast }
+
+      after(:build) do |artifact, _|
+        artifact.file = fixture_file_upload(
+          Rails.root.join('ee/spec/fixtures/security_reports/deprecated/gl-dast-report.json'), 'text/plain')
+      end
+    end
+
+    trait :dast_multiple_sites do
+      file_format { :raw }
+      file_type { :dast }
+
+      after(:build) do |artifact, _|
+        artifact.file = fixture_file_upload(
+          Rails.root.join('ee/spec/fixtures/security_reports/master/gl-dast-report-multiple-sites.json'), 'text/plain')
+      end
+    end
+
     trait :low_severity_dast_report do
       file_format { :raw }
       file_type { :dast }

ee/spec/fixtures/security_reports/deprecated/gl-dast-report.json

+{
+  "site": [
+    {
+      "alerts": [
+        {
+          "sourceid": "3",
+          "wascid": "15",
+          "cweid": "16",
+          "reference": "<p>http://msdn.microsoft.com/en-us/library/ie/gg622941%28v=vs.85%29.aspx</p><p>https://www.owasp.org/index.php/List_of_useful_HTTP_headers</p>",
+          "otherinfo": "<p>This issue still applies to error type pages (401, 403, 500, etc) as those pages are often still affected by injection issues, in which case there is still concern for browsers sniffing pages away from their actual content type.</p><p>At \"High\" threshold this scanner will not alert on client or server error responses.</p>",
+          "solution": "<p>Ensure that the application/web server sets the Content-Type header appropriately, and that it sets the X-Content-Type-Options header to 'nosniff' for all web pages.</p><p>If possible, ensure that the end user uses a standards-compliant and modern web browser that does not perform MIME-sniffing at all, or that can be directed by the web application/web server to not perform MIME-sniffing.</p>",
+          "count": "2",
+          "pluginid": "10021",
+          "alert": "X-Content-Type-Options Header Missing",
+          "name": "X-Content-Type-Options Header Missing",
+          "riskcode": "1",
+          "confidence": "2",
+          "riskdesc": "Low (Medium)",
+          "desc": "<p>The Anti-MIME-Sniffing header X-Content-Type-Options was not set to 'nosniff'. This allows older versions of Internet Explorer and Chrome to perform MIME-sniffing on the response body, potentially causing the response body to be interpreted and displayed as a content type other than the declared content type. Current (early 2014) and legacy versions of Firefox will use the declared content type (if one is set), rather than performing MIME-sniffing.</p>",
+          "instances": [
+            {
+              "param": "X-Content-Type-Options",
+              "method": "GET",
+              "uri": "http://bikebilly-spring-auto-devops-review-feature-br-3y2gpb.35.192.176.43.xip.io"
+            },
+            {
+              "param": "X-Content-Type-Options",
+              "method": "GET",
+              "uri": "http://bikebilly-spring-auto-devops-review-feature-br-3y2gpb.35.192.176.43.xip.io/"
+            }
+          ]
+        }
+      ],
+      "@ssl": "false",
+      "@port": "80",
+      "@host": "bikebilly-spring-auto-devops-review-feature-br-3y2gpb.35.192.176.43.xip.io",
+      "@name": "http://bikebilly-spring-auto-devops-review-feature-br-3y2gpb.35.192.176.43.xip.io"
+    }
+  ],
+  "@generated": "Fri, 13 Apr 2018 09:22:01",
+  "@version": "2.7.0"
+}

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

@@ -3,51 +3,68 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Parsers::Security::Dast do
-  let(:parser) { described_class.new }
+  using RSpec::Parameterized::TableSyntax
 
   describe '#parse!' do
     let(:project) { artifact.project }
     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.sha) }
+    let(:parser) { described_class.new }
 
-    before do
-      artifact.each_blob do |blob|
-        parser.parse!(blob, report)
-      end
+    where(:report_format,
+          :occurrence_count,
+          :identifier_count,
+          :scanner_count,
+          :last_occurrence_hostname,
+          :last_occurrence_method_name,
+          :last_occurrence_path,
+          :last_occurrence_severity,
+          :last_occurrence_confidence) do
+      :dast                 | 24 | 15 | 1 | 'http://goat:8080' | 'GET' | '/WebGoat/start.mvc' | 'info' | 'low'
+      :dast_multiple_sites  | 25 | 15 | 1 | 'https://goat:8080' | 'GET' | '/WebGoat/registration' | 'high' | 'medium'
+      :dast_deprecated      | 2 | 3 | 1 | 'http://bikebilly-spring-auto-devops-review-feature-br-3y2gpb.35.192.176.43.xip.io' | 'GET' | '/' | 'low' | 'medium'
     end
 
-    it 'parses all identifiers and occurrences' do
-      expect(report.occurrences.length).to eq(24)
-      expect(report.identifiers.length).to eq(15)
-      expect(report.scanners.length).to eq(1)
-    end
+    with_them do
+      let(:artifact) { create(:ee_ci_job_artifact, report_format) }
 
-    it 'generates expected location' do
-      location = report.occurrences.first.location
+      before do
+        artifact.each_blob do |blob|
+          parser.parse!(blob, report)
+        end
+      end
 
-      expect(location).to be_a(::Gitlab::Ci::Reports::Security::Locations::Dast)
-      expect(location).to have_attributes(
-        hostname: 'http://goat:8080',
-        method_name: 'GET',
-        path: '/WebGoat/login?error'
-      )
-    end
+      it 'parses all identifiers and occurrences' do
+        expect(report.occurrences.length).to eq(occurrence_count)
+        expect(report.identifiers.length).to eq(identifier_count)
+        expect(report.scanners.length).to eq(scanner_count)
+      end
 
-    describe 'occurrence properties' do
-      using RSpec::Parameterized::TableSyntax
+      it 'generates expected location' do
+        location = report.occurrences.last.location
 
-      where(:attribute, :value) do
-        :report_type | 'dast'
-        :severity | 'info'
-        :confidence | 'low'
+        expect(location).to be_a(::Gitlab::Ci::Reports::Security::Locations::Dast)
+        expect(location).to have_attributes(
+          hostname: last_occurrence_hostname,
+          method_name: last_occurrence_method_name,
+          path: last_occurrence_path
+        )
       end
 
-      with_them do
-        it 'saves properly occurrence' do
-          occurrence = report.occurrences.last
+      describe 'occurrence properties' do
+        where(:attribute, :value) do
+          :report_type | 'dast'
+          :severity | last_occurrence_severity
+          :confidence | last_occurrence_confidence
+        end
+
+        with_them do
+          it 'saves properly occurrence' do
+            occurrence = report.occurrences.last
 
-          expect(occurrence.public_send(attribute)).to eq(value)
+            expect(occurrence.public_send(attribute)).to eq(value)
+          end
         end
       end
     end