Merge branch '258648-sort-vulnerabilities-in-dependency-list' into 'master'

Sort vulnerabilities in dependency list [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62983

ee/app/services/security/dependency_list_service.rb

@@ -55,7 +55,8 @@ module Security
       when 'packager'
         collection.sort_by! { |a| a[:packager] }
       when 'severity'
-        collection = sort_by_severity(collection)
+        sort_dependency_vulnerabilities_by_severity!(collection) if Feature.enabled?(:sort_dependency_vulnerabilities, @pipeline.project)
+        sort_dependencies_by_severity!(collection)
       else
         collection.sort_by! { |a| a[:name] }
       end
@@ -65,16 +66,25 @@ module Security
       collection
     end
 
-    # vulnerabilities are initially sorted by severity in report
-    # https://gitlab.com/gitlab-org/security-products/analyzers/common/blob/ee9d378f46d9cc4e7b7592786a2a558dcc72b0f5/issue/report.go#L15
-    # So we can assume that first vulnerability in vulnerabilities array
-    # will have highest severity
-    def sort_by_severity(collection)
-      collection.sort do |dep_i, dep_j|
+    def compare_severity_levels(level1, level2)
+      ::Enums::Vulnerability.severity_levels[level2] <=> ::Enums::Vulnerability.severity_levels[level1]
+    end
+
+    def sort_dependency_vulnerabilities_by_severity!(collection)
+      collection.each do |dependency|
+        dependency[:vulnerabilities].sort! do |vulnerability1, vulnerability2|
+          compare_severity_levels(vulnerability1[:severity], vulnerability2[:severity])
+        end
+      end
+    end
+
+    # vulnerabilities are already sorted by severity level so we can assume that first vulnerability in
+    # vulnerabilities array will have highest severity
+    def sort_dependencies_by_severity!(collection)
+      collection.sort! do |dep_i, dep_j|
         level_i = dep_i.dig(:vulnerabilities, 0, :severity) || :info
         level_j = dep_j.dig(:vulnerabilities, 0, :severity) || :info
-
-        ::Enums::Vulnerability.severity_levels[level_j] <=> ::Enums::Vulnerability.severity_levels[level_i]
+        compare_severity_levels(level_i, level_j)
       end
     end
   end

ee/config/feature_flags/development/sort_dependency_vulnerabilities.yml

+---
+name: sort_dependency_vulnerabilities
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/62983
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/332852
+milestone: '14.0'
+type: development
+group: group::composition analysis
+default_enabled: false

ee/spec/services/security/dependency_list_service_spec.rb

@@ -5,10 +5,16 @@ require 'spec_helper'
 RSpec.describe Security::DependencyListService do
   describe '#execute' do
     let_it_be(:pipeline) { create(:ee_ci_pipeline, :with_dependency_list_report) }
-    let_it_be(:nokogiri_finding) { create(:vulnerabilities_finding, :detected, :with_dependency_scanning_metadata, :with_pipeline) }
+    let_it_be(:nokogiri_finding) { create(:vulnerabilities_finding, :detected, :with_dependency_scanning_metadata, :with_pipeline, raw_severity: 'High') }
     let_it_be(:nokogiri_pipeline) { create(:vulnerabilities_finding_pipeline, finding: nokogiri_finding, pipeline: pipeline) }
-    let_it_be(:other_finding) { create(:vulnerabilities_finding, :detected, :with_dependency_scanning_metadata, package: 'saml2-js', file: 'yarn/yarn.lock', version: '1.5.0', raw_severity: 'Unknown') }
-    let_it_be(:other_pipeline) { create(:vulnerabilities_finding_pipeline, finding: other_finding, pipeline: pipeline) }
+
+    let_it_be(:unknown_severity_finding) { create(:vulnerabilities_finding, :detected, :with_dependency_scanning_metadata, package: 'saml2-js', file: 'yarn/yarn.lock', version: '1.5.0', raw_severity: 'Unknown') }
+    let_it_be(:medium_severity_finding) { create(:vulnerabilities_finding, :detected, :with_dependency_scanning_metadata, package: 'saml2-js', file: 'yarn/yarn.lock',  version: '1.5.0', raw_severity: 'Medium') }
+    let_it_be(:critical_severity_finding) { create(:vulnerabilities_finding, :detected, :with_dependency_scanning_metadata, package: 'saml2-js', file: 'yarn/yarn.lock', version: '1.5.0', raw_severity: 'Critical') }
+
+    let_it_be(:unknown_severity_pipeline) { create(:vulnerabilities_finding_pipeline, finding: unknown_severity_finding, pipeline: pipeline) }
+    let_it_be(:medium_severity_pipeline) { create(:vulnerabilities_finding_pipeline, finding: medium_severity_finding, pipeline: pipeline) }
+    let_it_be(:critical_severity_pipeline) { create(:vulnerabilities_finding_pipeline, finding: critical_severity_finding, pipeline: pipeline) }
 
     subject { described_class.new(pipeline: pipeline, params: params).execute }
 
@@ -91,7 +97,12 @@ RSpec.describe Security::DependencyListService do
         end
       end
 
-      context 'sorted by desc severity' do
+      # this test ensures the dependency list severity sort order is `info, unknown, low, medium, high, critical`
+      # which is asending severity order, however, the UI label for this sort order is currently `desc`.
+      # TODO: change the UI label to use `asc` for this sort order and use `desc` for the default sort order
+      # of `critical, high, medium, low, unknown, info`
+      # See https://gitlab.com/gitlab-org/gitlab/-/issues/332653
+      context 'sorted by asc severity' do
         let(:params) do
           {
             sort: 'desc',
@@ -99,12 +110,54 @@ RSpec.describe Security::DependencyListService do
           }
         end
 
-        it 'returns array of data properly sorted' do
-          nokogiri_index = subject.find_index { |dep| dep[:name] == 'nokogiri' }
-          saml2js_index = subject.find_index { |dep| dep[:name] == 'saml2-js' }
+        context('when the sort_dependency_vulnerabilities feature flag is true') do
+          it 'returns array of data sorted by package severity level in ascending order' do
+            dependencies = subject.last(2).map do |dependency|
+              {
+                name: dependency[:name],
+                vulnerabilities: dependency[:vulnerabilities].map do |vulnerability|
+                  vulnerability[:severity]
+                end
+              }
+            end
+
+            expect(dependencies).to eq([{ name: "nokogiri", vulnerabilities: ["high"] },
+                                        { name: "saml2-js", vulnerabilities: %w(critical medium unknown) }])
+          end
+
+          it 'returns array of data with package vulnerabilities sorted in descending order' do
+            saml2js_dependency = subject.find { |dep| dep[:name] == 'saml2-js' }
+            saml2js_severities = saml2js_dependency[:vulnerabilities].map {|v| v[:severity] }
+
+            expect(saml2js_severities).to eq(%w(critical medium unknown))
+          end
+        end
 
-          expect(nokogiri_index).to be > saml2js_index
-          expect(subject).to end_with(subject[nokogiri_index])
+        context('when the sort_dependency_vulnerabilities feature flag is false') do
+          before do
+            stub_feature_flags(sort_dependency_vulnerabilities: false)
+          end
+
+          it 'returns array of data sorted by package severity level in descending order' do
+            dependencies = subject.last(2).map do |dependency|
+              {
+                name: dependency[:name],
+                vulnerabilities: dependency[:vulnerabilities].map do |vulnerability|
+                  vulnerability[:severity]
+                end
+              }
+            end
+
+            expect(dependencies).to eq([{ name: "saml2-js", vulnerabilities: %w(unknown medium critical) },
+                                        { name: "nokogiri", vulnerabilities: ["high"] }])
+          end
+
+          it 'returns array of data with package vulnerabilities sorted in ascending order' do
+            saml2js_dependency = subject.find { |dep| dep[:name] == 'saml2-js' }
+            saml2js_severities = saml2js_dependency[:vulnerabilities].map {|v| v[:severity] }
+
+            expect(saml2js_severities).to eq(%w(unknown medium critical))
+          end
         end
       end
     end