Merge branch '13034-update-dast-report-fixture' into 'master'

Update DAST report fixture, Fix deduplication logic

See merge request gitlab-org/gitlab!17778

ee/app/services/security/merge_reports_service.rb

@@ -77,9 +77,10 @@ module Security
         occurrence.identifiers.each do |identifier|
           # TODO: remove .downcase here after the DAST parser is harmonized to the common library identifiers' keys format
           # See https://gitlab.com/gitlab-org/gitlab/issues/11976#note_191257912
-          next if identifier.external_type.casecmp("cwe").zero? # ignored because it describes a class of vulnerabilities
+          next if %w[cwe wasc].include?(identifier.external_type.downcase) # ignored because these describe a class of vulnerabilities
 
           seen = check_or_mark_seen_identifier!(identifier, occurrence.location.fingerprint, seen_identifiers)
+
           break if seen
         end
 

ee/changelogs/unreleased/13034-update-dast-report-fixture.yml

+---
+title: Fix deduplication of WASC vulnerabilities in the Security dashboard
+merge_request: 17778
+author:
+type: fixed

ee/spec/factories/ci/builds.rb

@@ -95,5 +95,11 @@ FactoryBot.define do
         build.job_artifacts << create(:ee_ci_job_artifact, :corrupted_license_management_report, job: build)
       end
     end
+
+    trait :low_severity_dast_report do
+      after(:build) do |build|
+        build.job_artifacts << create(:ee_ci_job_artifact, :low_severity_dast_report, job: build)
+      end
+    end
   end
 end

ee/spec/factories/ci/job_artifacts.rb

@@ -182,6 +182,16 @@ FactoryBot.define do
       end
     end
 
+    trait :low_severity_dast_report 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-low-severity.json'), 'text/plain')
+      end
+    end
+
     trait :metrics do
       file_format { :gzip }
       file_type { :metrics }

ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb

@@ -3,6 +3,22 @@
 require 'spec_helper'
 
 describe Security::PipelineVulnerabilitiesFinder do
+  class NoDeduplicationMergeReportsService
+    def initialize(*source_reports)
+      @source_reports = source_reports
+    end
+
+    def execute
+      @source_reports.last
+    end
+  end
+
+  def disable_deduplication
+    allow(::Security::MergeReportsService).to receive(:new) do |*args|
+      NoDeduplicationMergeReportsService.new(*args)
+    end
+  end
+
   describe '#execute' do
     set(:project) { create(:project, :repository) }
     set(:pipeline) { create(:ci_pipeline, :success, project: project) }
@@ -18,12 +34,20 @@ describe Security::PipelineVulnerabilitiesFinder do
     set(:artifact_sast) { create(:ee_ci_job_artifact, :sast, job: build_sast, project: project) }
 
     let(:cs_count) { read_fixture(artifact_cs)['unapproved'].count }
-    let(:dast_count) { read_fixture(artifact_dast)['site'].first['alerts'].first['instances'].count }
     let(:ds_count) { read_fixture(artifact_ds)['vulnerabilities'].count }
     let(:sast_count) { read_fixture(artifact_sast)['vulnerabilities'].count }
+    let(:dast_count) do
+      read_fixture(artifact_dast)['site'].sum do |site|
+        site['alerts'].sum do |alert|
+          alert['instances'].size
+        end
+      end
+    end
 
     before do
       stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true, dast: true)
+      # Stub out deduplication, if not done the expectations will vary based on the fixtures (which may/may not have duplicates)
+      disable_deduplication
     end
 
     subject { described_class.new(pipeline: pipeline, params: params).execute }
@@ -121,7 +145,7 @@ describe Security::PipelineVulnerabilitiesFinder do
         subject { described_class.new(pipeline: pipeline).execute }
 
         it 'returns all vulnerability severity levels' do
-          expect(subject.map(&:severity).uniq).to match_array %w[undefined unknown low medium high critical]
+          expect(subject.map(&:severity).uniq).to match_array %w[undefined unknown low medium high critical info]
         end
       end
 
@@ -159,7 +183,7 @@ describe Security::PipelineVulnerabilitiesFinder do
         it 'filters by all params' do
           expect(subject.count).to eq cs_count + dast_count + ds_count + sast_count
           expect(subject.map(&:confidence).uniq).to match_array %w[undefined unknown low medium high]
-          expect(subject.map(&:severity).uniq).to match_array %w[undefined unknown low medium high critical]
+          expect(subject.map(&:severity).uniq).to match_array %w[undefined unknown low medium high critical info]
         end
       end
 

ee/spec/fixtures/security_reports/feature-branch/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

@@ -18,8 +18,8 @@ describe Gitlab::Ci::Parsers::Security::Dast do
     end
 
     it 'parses all identifiers and occurrences' do
-      expect(report.occurrences.length).to eq(2)
-      expect(report.identifiers.length).to eq(3)
+      expect(report.occurrences.length).to eq(24)
+      expect(report.identifiers.length).to eq(15)
       expect(report.scanners.length).to eq(1)
     end
 
@@ -28,10 +28,9 @@ describe Gitlab::Ci::Parsers::Security::Dast do
 
       expect(location).to be_a(::Gitlab::Ci::Reports::Security::Locations::Dast)
       expect(location).to have_attributes(
-        hostname: 'http://bikebilly-spring-auto-devops-review-feature-br-3y2gpb.35.192.176.43.xip.io',
+        hostname: 'http://goat:8080',
         method_name: 'GET',
-        param: 'X-Content-Type-Options',
-        path: ''
+        path: '/WebGoat/login?error'
       )
     end
 
@@ -40,8 +39,8 @@ describe Gitlab::Ci::Parsers::Security::Dast do
 
       where(:attribute, :value) do
         :report_type | 'dast'
-        :severity | 'low'
-        :confidence | 'medium'
+        :severity | 'info'
+        :confidence | 'low'
       end
 
       with_them do

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

@@ -16,7 +16,7 @@ describe Gitlab::Ci::Parsers::Security::Formatters::Dast do
 
   describe '#format_vulnerability' do
     let(:instance) { file_vulnerability['instances'][1] }
-    let(:hostname) { 'http://bikebilly-spring-auto-devops-review-feature-br-3y2gpb.35.192.176.43.xip.io' }
+    let(:hostname) { 'http://goat:8080' }
     let(:sanitized_desc) { file_vulnerability['desc'].gsub('<p>', '').gsub('</p>', '') }
     let(:sanitized_solution) { file_vulnerability['solution'].gsub('<p>', '').gsub('</p>', '') }
     let(:version) { parsed_report['@version'] }
@@ -25,38 +25,38 @@ describe Gitlab::Ci::Parsers::Security::Formatters::Dast do
       data = formatter.format(instance, hostname)
 
       expect(data['category']).to eq('dast')
-      expect(data['message']).to eq('X-Content-Type-Options Header Missing')
+      expect(data['message']).to eq('Anti CSRF Tokens Scanner')
       expect(data['description']).to eq(sanitized_desc)
-      expect(data['cve']).to eq('10021')
-      expect(data['severity']).to eq('low')
+      expect(data['cve']).to eq('20012')
+      expect(data['severity']).to eq('high')
       expect(data['confidence']).to eq('medium')
       expect(data['solution']).to eq(sanitized_solution)
       expect(data['scanner']).to eq({ 'id' => 'zaproxy', 'name' => 'ZAProxy' })
-      expect(data['links']).to eq([{ 'url' => 'http://msdn.microsoft.com/en-us/library/ie/gg622941%28v=vs.85%29.aspx' },
-                                   { 'url' => 'https://www.owasp.org/index.php/List_of_useful_HTTP_headers' }])
+      expect(data['links']).to eq([{ 'url' => 'http://projects.webappsec.org/Cross-Site-Request-Forgery' },
+                                   { 'url' => 'http://cwe.mitre.org/data/definitions/352.html' }])
       expect(data['identifiers'][0]).to eq({
                                              'type'  => 'ZAProxy_PluginId',
-                                             'name'  => 'X-Content-Type-Options Header Missing',
-                                             'value' => '10021',
+                                             'name'  => 'Anti CSRF Tokens Scanner',
+                                             'value' => '20012',
                                              'url'   => "https://github.com/zaproxy/zaproxy/blob/w2019-01-14/docs/scanners.md"
                                            })
       expect(data['identifiers'][1]).to eq({
                                              'type'  => 'CWE',
-                                             'name'  => "CWE-16",
-                                             'value' => '16',
-                                             'url'   => "https://cwe.mitre.org/data/definitions/16.html"
+                                             'name'  => "CWE-352",
+                                             'value' => '352',
+                                             'url'   => "https://cwe.mitre.org/data/definitions/352.html"
                                            })
       expect(data['identifiers'][2]).to eq({
                                              'type'  => 'WASC',
-                                             'name'  => "WASC-15",
-                                             'value' => '15',
+                                             'name'  => "WASC-9",
+                                             'value' => '9',
                                              'url'   => "http://projects.webappsec.org/w/page/13246974/Threat%20Classification%20Reference%20Grid"
                                            })
       expect(data['location']).to eq({
-                                       'param' => 'X-Content-Type-Options',
+                                       'param' => nil,
                                        'method' => 'GET',
                                        'hostname' => hostname,
-                                       'path' => '/'
+                                       'path' => '/WebGoat/login'
                                      })
     end
   end

ee/spec/models/ci/build_spec.rb

@@ -139,7 +139,7 @@ describe Ci::Build do
           expect(security_reports.get_report('sast').occurrences.size).to eq(33)
           expect(security_reports.get_report('dependency_scanning').occurrences.size).to eq(4)
           expect(security_reports.get_report('container_scanning').occurrences.size).to eq(8)
-          expect(security_reports.get_report('dast').occurrences.size).to eq(2)
+          expect(security_reports.get_report('dast').occurrences.size).to eq(20)
         end
       end
 

ee/spec/services/security/merge_reports_service_spec.rb

@@ -12,6 +12,7 @@ describe Security::MergeReportsService, '#execute' do
   let(:identifier_2_primary) { build(:ci_reports_security_identifier, external_id: 'VULN-2', external_type: 'scanner-2') }
   let(:identifier_2_cve) { build(:ci_reports_security_identifier, external_id: 'CVE-2019-456', external_type: 'cve') }
   let(:identifier_cwe) { build(:ci_reports_security_identifier, external_id: '789', external_type: 'cwe') }
+  let(:identifier_wasc) { build(:ci_reports_security_identifier, external_id: '13', external_type: 'wasc') }
 
   let(:occurrence_id_1) do
     build(:ci_reports_security_occurrence,
@@ -63,7 +64,23 @@ describe Security::MergeReportsService, '#execute' do
          )
   end
 
-  let(:report_1_occurrences) { [occurrence_id_1, occurrence_id_2_loc_1, occurrence_cwe_2] }
+  let(:occurrence_wasc_1) do
+    build(:ci_reports_security_occurrence,
+          identifiers: [identifier_wasc],
+          scanner: scanner_1,
+          severity: :medium
+         )
+  end
+
+  let(:occurrence_wasc_2) do
+    build(:ci_reports_security_occurrence,
+          identifiers: [identifier_wasc],
+          scanner: scanner_2,
+          severity: :critical
+         )
+  end
+
+  let(:report_1_occurrences) { [occurrence_id_1, occurrence_id_2_loc_1, occurrence_cwe_2, occurrence_wasc_1] }
 
   let(:report_1) do
     build(
@@ -74,11 +91,13 @@ describe Security::MergeReportsService, '#execute' do
     )
   end
 
+  let(:report_2_occurrences) { [occurrence_id_2_loc_2, occurrence_wasc_2] }
+
   let(:report_2) do
     build(
       :ci_reports_security_report,
       scanners: [scanner_2],
-      occurrences: [occurrence_id_2_loc_2],
+      occurrences: report_2_occurrences,
       identifiers: occurrence_id_2_loc_2.identifiers
     )
   end
@@ -109,14 +128,23 @@ describe Security::MergeReportsService, '#execute' do
         identifier_1_cve,
         identifier_2_primary,
         identifier_2_cve,
-        identifier_cwe
+        identifier_cwe,
+        identifier_wasc
       )
     )
   end
 
-  it 'deduplicates and sorts the vulnerabilities by severity (desc) then by compare key' do
+  it 'deduplicates (except cwe and wasc) and sorts the vulnerabilities by severity (desc) then by compare key' do
     expect(subject.occurrences).to(
-      eq([occurrence_cwe_2, occurrence_cwe_1, occurrence_id_2_loc_2, occurrence_id_2_loc_1, occurrence_id_1])
+      eq([
+          occurrence_cwe_2,
+          occurrence_wasc_2,
+          occurrence_cwe_1,
+          occurrence_id_2_loc_2,
+          occurrence_id_2_loc_1,
+          occurrence_wasc_1,
+          occurrence_id_1
+      ])
     )
   end
 end

ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb

@@ -35,7 +35,7 @@ describe Security::SyncReportsToApprovalRulesService, '#execute' do
 
       context 'when only low-severity vulnerabilities are present' do
         before do
-          create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project)
+          create(:ee_ci_build, :success, :low_severity_dast_report, name: 'dast_job', pipeline: pipeline, project: project)
         end
 
         it 'lowers approvals_required count to zero' do
@@ -56,10 +56,6 @@ describe Security::SyncReportsToApprovalRulesService, '#execute' do
         end
 
         it "won't change approvals_required count" do
-          expect(
-            pipeline.security_reports.reports.values.all?(&:unsafe_severity?)
-          ).to be false
-
           expect { subject }
             .not_to change { report_approver_rule.reload.approvals_required }
         end
@@ -119,7 +115,7 @@ describe Security::SyncReportsToApprovalRulesService, '#execute' do
 
       context 'when only low-severity vulnerabilities are present' do
         before do
-          create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project)
+          create(:ee_ci_build, :success, :low_severity_dast_report, name: 'dast_job', pipeline: pipeline, project: project)
         end
 
         it 'lowers approvals_required count to zero' do