Merge branch '5919_follow_up_on_5151_dismiss_vulnerability' into 'master'

Resolve Follow up on dismiss vulnerability or create issue

See merge request gitlab-org/gitlab-ee!5756

ee/app/models/vulnerability_feedback.rb

@@ -12,6 +12,7 @@ class VulnerabilityFeedback < ActiveRecord::Base
   validates :project, presence: true
   validates :author, presence: true
   validates :issue, presence: true, if: :issue?
+  validates :vulnerability_data, presence: true, if: :issue?
   validates :feedback_type, presence: true
   validates :category, presence: true
   validates :project_fingerprint, presence: true, uniqueness: { scope: [:project_id, :category, :feedback_type] }

ee/app/services/issues/create_from_vulnerability_data_service.rb

 module Issues
   class CreateFromVulnerabilityDataService < ::BaseService
     def execute
+      vulnerability = case @params[:category]
+                      when 'sast', 'dependency_scanning'
+                        Gitlab::Vulnerabilities::StandardVulnerability.new(params)
+                      when 'container_scanning'
+                        Gitlab::Vulnerabilities::ContainerScanningVulnerability.new(params)
+                      when 'dast'
+                        Gitlab::Vulnerabilities::DastVulnerability.new(params)
+                      end
+
       issue_params = {
-        title: issue_title(@params),
-        description: issue_content(@params)
+        title: "Investigate vulnerability: #{vulnerability.title}",
+        description: render_description(vulnerability)
       }
 
       issue = Issues::CreateService.new(@project, @current_user, issue_params).execute
@@ -21,133 +30,9 @@ module Issues
       super().merge(issue: issue)
     end
 
-    def issue_title(params)
-      title = case params[:category]
-              when 'sast', 'dependency_scanning', 'dast'
-                params[:name]
-              when 'container_scanning'
-                "#{params[:name]} in #{params[:namespace]}"
-              end
-
-      "Investigate vulnerability: #{title}"
-    end
-
-    def issue_content(params)
-      data = case params[:category]
-             when 'sast', 'dependency_scanning'
-               sast_data(params)
-             when 'container_scanning'
-               container_scanning_data(params)
-             when 'dast'
-               dast_data(params)
-             end
-
-      render_content data
-    end
-
-    def sast_data(params)
-      data = { identifiers: [] }
-      data[:severity] = params[:severity]
-      data[:confidence] = params[:confidence]
-      data[:description] = params[:description].presence ||
-        params[:name]
-      data[:solution] = params[:solution]
-      if params[:identifiers].present?
-        params[:identifiers].each do |identifier|
-          # Only show known identifiers
-          case identifier[:name]
-          when 'CVE'
-            data[:identifiers] << {
-              value: identifier[:value],
-              link: cve_link(identifier[:value])
-            }
-          when 'CWE'
-            data[:identifiers] << {
-              value: "CWE-#{identifier[:value]}",
-              link: cwe_link(identifier[:value])
-            }
-          end
-        end
-      end
-
-      data
-    end
-
-    def container_scanning_data(params)
-      data = { identifiers: [] }
-      data[:severity] = params[:severity]
-      data[:description] = params[:description].presence ||
-        "**#{params[:namespace]}** is affected by #{params[:name]}"
-
-      if params[:fixedby].present? &&
-          params[:featurename].present? &&
-          params[:featureversion].present?
-
-        data[:solution] = "Upgrade **#{params[:featurename]}** from `#{params[:featureversion]}` to `#{params[:fixedby]}`"
-      end
-
-      if params[:name].present?
-        data[:identifiers] << {
-          value: params[:name],
-          link: cve_link(params[:name])
-        }
-      end
-
-      data
-    end
-
-    def dast_data(params)
-      data = { identifiers: [] }
-      data[:severity] = params[:severity]
-      data[:confidence] = params[:confidence]
-      data[:description] = params[:desc]
-      data[:solution] = params[:solution]
-
-      if params[:cweid].present?
-        data[:identifiers] << {
-          value: "CWE-#{params[:cweid]}",
-          link: cwe_link(params[:cweid])
-        }
-      end
-
-      if params[:wascid].present?
-        data[:identifiers] << {
-          value: "WASC-#{params[:wascid]}"
-        }
-      end
-
-      data
-    end
-
-    def render_content(data)
-      content = "### Description:\n#{data[:description]}\n\n"
-      content << "* Severity: #{data[:severity]}\n" if data[:severity].present?
-      content << "* Confidence: #{data[:confidence]}\n" if data[:confidence].present?
-
-      content << "\n### Solution:\n#{data[:solution]}\n" if data[:solution].present?
-
-      if data[:identifiers].present?
-        content << "\n### Identifiers:\n\n"
-        data[:identifiers].each do |identifier|
-          content << if identifier[:link].present?
-                       "* [#{identifier[:value]}](#{identifier[:link]})\n"
-                     else
-                       "* #{identifier[:value]}\n"
-                     end
-        end
-      end
-
-      content
-    end
-
-    # cve_id must be 'CVE-YYYY-XXXX' (prefix + year + digits)
-    def cve_link(cve_id)
-      "https://cve.mitre.org/cgi-bin/cvename.cgi?name=#{cve_id}"
-    end
-
-    # cve_id must be a number only (no 'CWE-' prefix)
-    def cwe_link(cwe_id)
-      "https://cwe.mitre.org/data/definitions/#{cwe_id}.html"
+    def render_description(vulnerability)
+      view = ActionView::Base.new(ActionController::Base.view_paths, {})
+      view.render(file: 'vulnerabilities/issue_description.md.erb', locals: { vulnerability: vulnerability })
     end
   end
 end

ee/app/services/vulnerability_feedback_module/create_service.rb

@@ -4,24 +4,31 @@ module VulnerabilityFeedbackModule
       vulnerability_feedback = @project.vulnerability_feedback.new(@params)
       vulnerability_feedback.author = @current_user
 
-      if vulnerability_feedback.issue? # (feedback_type == 'issue')
-        return error('vulnerability_data is missing or empty') if vulnerability_feedback.vulnerability_data.blank?
+      # Wrap Feedback and Issue creation in the same transaction
+      ActiveRecord::Base.transaction do
+        if vulnerability_feedback.issue? && # (feedback_type == 'issue')
+            !vulnerability_feedback.vulnerability_data.blank?
 
           result = Issues::CreateFromVulnerabilityDataService
             .new(@project, @current_user, vulnerability_feedback.vulnerability_data)
             .execute
 
-        return result if result[:status] == :error
+          if result[:status] == :error
+            vulnerability_feedback.errors[:issue] << result[:message]
+            raise ActiveRecord::Rollback
+          end
 
           issue = result[:issue]
           vulnerability_feedback.issue = issue
         end
 
-      if vulnerability_feedback.save
+        # Ensure created issue is rolled back if feedback can't be saved
+        raise ActiveRecord::Rollback unless vulnerability_feedback.save
+      end
+
+      if vulnerability_feedback.persisted?
         success(vulnerability_feedback)
       else
-        # Rollback created issue
-        issue.destroy if issue
         error(vulnerability_feedback.errors)
       end
 

ee/app/views/vulnerabilities/issue_description.md.erb

+### Description:
+
+<%= vulnerability.description %>
+
+<% if vulnerability.severity.present? %>
+* Severity: <%= vulnerability.severity %>
+<% end %>
+<% if vulnerability.confidence.present? %>
+* Confidence: <%= vulnerability.confidence %>
+<% end %>
+
+<% if vulnerability.solution.present? %>
+### Solution:
+
+<%= vulnerability.solution %>
+<% end %>
+
+<% if vulnerability.identifiers.present? %>
+### Identifiers:
+
+<% vulnerability.identifiers.each do |identifier| %>
+<% if identifier[:link].present? %>
+* [<%= identifier[:value] %>](<%= identifier[:link] %>)
+<% else %>
+* <%= identifier[:value] %>
+<% end %>
+<% end %>
+
+<% end %>

ee/lib/gitlab/vulnerabilities/base_vulnerability.rb

+module Gitlab
+  module Vulnerabilities
+    class BaseVulnerability
+      attr_reader :data
+
+      def initialize(data)
+        @data = data
+      end
+
+      # Ensure mandatory properties are defined
+      %i[
+        title
+        description
+        severity
+        confidence
+        solution
+        identifiers
+      ].each do |method_name|
+        define_method(method_name) do
+          raise NotImplementedError
+        end
+      end
+
+      protected
+
+      # cve_id must be 'CVE-YYYY-XXXX' (prefix + year + digits)
+      def cve_link(cve_id)
+        "https://cve.mitre.org/cgi-bin/cvename.cgi?name=#{cve_id}"
+      end
+
+      # cve_id must be a number only (no 'CWE-' prefix)
+      def cwe_link(cwe_id)
+        "https://cwe.mitre.org/data/definitions/#{cwe_id}.html"
+      end
+    end
+  end
+end

ee/lib/gitlab/vulnerabilities/container_scanning_vulnerability.rb

+module Gitlab
+  module Vulnerabilities
+    class ContainerScanningVulnerability < BaseVulnerability
+      def title
+        "#{@data[:name]} in #{@data[:namespace]}"
+      end
+
+      # Passthrough properties
+      %i[
+        severity
+      ].each do |method_name|
+        define_method(method_name) do
+          @data[method_name]
+        end
+      end
+
+      def confidence
+      end
+
+      def description
+        @data[:description].presence ||
+          "**#{@data[:namespace]}** is affected by #{@data[:name]}"
+      end
+
+      def solution
+        if @data[:fixedby].present? &&
+            @data[:featurename].present? &&
+            @data[:featureversion].present?
+
+          "Upgrade **#{@data[:featurename]}** from `#{@data[:featureversion]}` to `#{@data[:fixedby]}`"
+        end
+      end
+
+      def identifiers
+        [{
+          value: @data[:name],
+          link: cve_link(@data[:name])
+        }]
+      end
+    end
+  end
+end

ee/lib/gitlab/vulnerabilities/dast_vulnerability.rb

+module Gitlab
+  module Vulnerabilities
+    class DastVulnerability < BaseVulnerability
+      def title
+        @data[:name]
+      end
+
+      # Passthrough properties
+      %i[
+        severity
+        confidence
+        solution
+      ].each do |method_name|
+        define_method(method_name) do
+          @data[method_name]
+        end
+      end
+
+      def description
+        @data[:desc]
+      end
+
+      def identifiers
+        ids = []
+
+        if @data[:cweid].present?
+          ids << {
+            value: "CWE-#{@data[:cweid]}",
+            link: cwe_link(@data[:cweid])
+          }
+        end
+
+        if @data[:wascid].present?
+          ids << {
+            value: "WASC-#{@data[:wascid]}"
+          }
+        end
+
+        ids
+      end
+    end
+  end
+end

ee/lib/gitlab/vulnerabilities/standard_vulnerability.rb

+module Gitlab
+  module Vulnerabilities
+    class StandardVulnerability < BaseVulnerability
+      def title
+        @data[:name]
+      end
+
+      # Passthrough properties
+      %i[
+        severity
+        confidence
+        solution
+      ].each do |method_name|
+        define_method(method_name) do
+          @data[method_name]
+        end
+      end
+
+      def description
+        @data[:description].presence || @data[:name]
+      end
+
+      def identifiers
+        return [] unless @data[:identifiers].present?
+
+        ids = []
+        @data[:identifiers].each do |identifier|
+          # Only show known identifiers
+          case identifier[:name]
+          when 'CVE'
+            ids << {
+              value: identifier[:value],
+              link: cve_link(identifier[:value])
+            }
+          when 'CWE'
+            ids << {
+              value: "CWE-#{identifier[:value]}",
+              link: cwe_link(identifier[:value])
+            }
+          end
+        end
+
+        ids
+      end
+    end
+  end
+end

ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb

@@ -16,10 +16,10 @@ describe Projects::VulnerabilityFeedbackController do
 
     let(:issue) { create(:issue, project: project) }
 
-    let!(:vuln_feedback_1) { create(:vulnerability_feedback, project: project, author: user, pipeline: pipeline_1, feedback_type: 'dismissal', category: 'sast', project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa1') }
-    let!(:vuln_feedback_2) { create(:vulnerability_feedback, project: project, author: user, pipeline: pipeline_1, feedback_type: 'issue', category: 'sast', project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa2', issue: issue) }
-    let!(:vuln_feedback_3) { create(:vulnerability_feedback, project: project, author: user, pipeline: pipeline_2, feedback_type: 'dismissal', category: 'sast', project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa3') }
-    let!(:vuln_feedback_4) { create(:vulnerability_feedback, project: project, author: user, pipeline: pipeline_2, feedback_type: 'dismissal', category: 'dependency_scanning', project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa4') }
+    let!(:vuln_feedback_1) { create(:vulnerability_feedback, :dismissal, :sast, project: project, author: user, pipeline: pipeline_1) }
+    let!(:vuln_feedback_2) { create(:vulnerability_feedback, :issue, :sast, project: project, author: user, pipeline: pipeline_1, issue: issue) }
+    let!(:vuln_feedback_3) { create(:vulnerability_feedback, :dismissal, :sast, project: project, author: user, pipeline: pipeline_2) }
+    let!(:vuln_feedback_4) { create(:vulnerability_feedback, :dismissal, :dependency_scanning, project: project, author: user, pipeline: pipeline_2) }
 
     context '@vulnerability_feedback' do
       it 'returns a successful 200 response' do
@@ -148,7 +148,7 @@ describe Projects::VulnerabilityFeedbackController do
 
   describe 'DELETE #destroy' do
     let(:pipeline) { create(:ci_pipeline, project: project) }
-    let!(:vuln_feedback) { create(:vulnerability_feedback, project: project, author: user, pipeline: pipeline, feedback_type: 'dismissal', category: 'sast', project_fingerprint: 'abc123') }
+    let!(:vuln_feedback) { create(:vulnerability_feedback, :dismissal, :sast, project: project, author: user, pipeline: pipeline) }
 
     context 'with valid params' do
       it 'returns a successful 204 response' do

ee/spec/factories/vulnerability_feedback.rb

+require 'digest'
+
 FactoryBot.define do
+  sequence :project_fingerprint do |n|
+    Digest::SHA1.hexdigest n.to_s
+  end
+
   factory :vulnerability_feedback do
     project
     author
@@ -6,6 +12,31 @@ FactoryBot.define do
     association :pipeline, factory: :ci_pipeline
     feedback_type 'dismissal'
     category 'sast'
-    project_fingerprint '418291a26024a1445b23fe64de9380cdcdfd1fa8'
+    project_fingerprint { generate(:project_fingerprint) }
+    vulnerability_data { { category: 'sast' } }
+
+    trait :dismissal do
+      feedback_type 'dismissal'
+    end
+
+    trait :issue do
+      feedback_type 'issue'
+    end
+
+    trait :sast do
+      category 'sast'
+    end
+
+    trait :dependency_scanning do
+      category 'dependency_scanning'
+    end
+
+    trait :container_scanning do
+      category 'container_scanning'
+    end
+
+    trait :dast do
+      category 'dast'
+    end
   end
 end

ee/spec/services/ee/issues/create_from_vulnerability_data_service_spec.rb

@@ -49,12 +49,14 @@ describe Issues::CreateFromVulnerabilityDataService, '#execute' do
         let(:expected_description) do
           <<~DESC.chomp
             ### Description:
+
             Description of Predictable pseudorandom number generator
 
             * Severity: Low
             * Confidence: High
 
             ### Solution:
+
             Please do something!
 
             ### Identifiers:
@@ -87,12 +89,14 @@ describe Issues::CreateFromVulnerabilityDataService, '#execute' do
         let(:expected_description) do
           <<~DESC.chomp
             ### Description:
+
             Predictable pseudorandom number generator
 
             * Severity: Low
             * Confidence: High
 
             ### Solution:
+
             Please do something!
 
             ### Identifiers:
@@ -129,12 +133,14 @@ describe Issues::CreateFromVulnerabilityDataService, '#execute' do
         let(:expected_description) do
           <<~DESC.chomp
             ### Description:
+
             Description of Predictable pseudorandom number generator
 
             * Severity: Low
             * Confidence: High
 
             ### Solution:
+
             Please do something!
 
             ### Identifiers:
@@ -167,12 +173,14 @@ describe Issues::CreateFromVulnerabilityDataService, '#execute' do
         let(:expected_description) do
           <<~DESC.chomp
             ### Description:
+
             Predictable pseudorandom number generator
 
             * Severity: Low
             * Confidence: High
 
             ### Solution:
+
             Please do something!
 
             ### Identifiers:
@@ -206,11 +214,13 @@ describe Issues::CreateFromVulnerabilityDataService, '#execute' do
         let(:expected_description) do
           <<~DESC.chomp
             ### Description:
+
             This is a description for CVE-2017-15650.
 
             * Severity: Low
 
             ### Solution:
+
             Upgrade **musl** from `1.1.14-r15` to `1.1.14-r16`
 
             ### Identifiers:
@@ -241,11 +251,13 @@ describe Issues::CreateFromVulnerabilityDataService, '#execute' do
         let(:expected_description) do
           <<~DESC.chomp
             ### Description:
+
             **alpine:v3.4** is affected by CVE-2017-15650
 
             * Severity: Low
 
             ### Solution:
+
             Upgrade **musl** from `1.1.14-r15` to `1.1.14-r16`
 
             ### Identifiers:
@@ -275,11 +287,13 @@ describe Issues::CreateFromVulnerabilityDataService, '#execute' do
       let(:expected_description) do
         <<~DESC.chomp
           ### Description:
+
           The Anti-MIME-Sniffing header X-Content-Type-Options was not set to nosniff.
 
           * Severity: Low
 
           ### Solution:
+
           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.
 
           ### Identifiers:

ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb

@@ -92,7 +92,7 @@ describe VulnerabilityFeedbackModule::CreateService, '#execute' do
 
       it 'returns error with correct message' do
         expect(result[:status]).to eq(:error)
-        expect(result[:message]).to eq('vulnerability_data is missing or empty')
+        expect(result[:message][:vulnerability_data]).to eq(["can't be blank"])
       end
     end