Merge branch '239172-add-entity-columns-to-vulnerability-occurrences' into 'master'

Add entity-related columns to vulnerability_occurrences

See merge request gitlab-org/gitlab!51739

changelogs/unreleased/239172-add-entity-columns-to-vulnerability-occurrences.yml

+---
+title: Add entity columns to vulnerability occurrences
+merge_request: 51739
+author:
+type: changed

db/migrate/20210105153342_add_entity_columns_to_vulnerability_occurrences.rb

+# frozen_string_literal: true
+
+class AddEntityColumnsToVulnerabilityOccurrences < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  # rubocop:disable Migration/AddLimitToTextColumns
+  # limit is added in 20200501000002_add_text_limit_to_sprints_extended_title
+  def change
+    add_column :vulnerability_occurrences, :description, :text
+    add_column :vulnerability_occurrences, :message, :text
+    add_column :vulnerability_occurrences, :solution, :text
+    add_column :vulnerability_occurrences, :cve, :text
+    add_column :vulnerability_occurrences, :location, :jsonb
+  end
+  # rubocop:enable Migration/AddLimitToTextColumns
+end

db/migrate/20210105154321_add_text_limit_to_vulnerability_occurrences_entity_columns.rb

+# frozen_string_literal: true
+
+class AddTextLimitToVulnerabilityOccurrencesEntityColumns < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :vulnerability_occurrences, :description, 15000
+    add_text_limit :vulnerability_occurrences, :message, 3000
+    add_text_limit :vulnerability_occurrences, :solution, 7000
+    add_text_limit :vulnerability_occurrences, :cve, 48400
+  end
+
+  def down
+    remove_text_limit :vulnerability_occurrences, :description
+    remove_text_limit :vulnerability_occurrences, :message
+    remove_text_limit :vulnerability_occurrences, :solution
+    remove_text_limit :vulnerability_occurrences, :cve
+  end
+end

db/schema_migrations/20210105153342

+7a252c5d76c1e71421c3aa3e01584cdeeec6a5002ba6ef0824674c64f92e2764
\ No newline at end of file

db/schema_migrations/20210105154321

+9327676097c49bb1a221d79dd351ad8c57a434f19e32f49951c0d6d655c2fa4e
\ No newline at end of file

db/structure.sql

@@ -18009,7 +18009,16 @@ CREATE TABLE vulnerability_occurrences (
     metadata_version character varying NOT NULL,
     raw_metadata text NOT NULL,
     vulnerability_id bigint,
-    details jsonb DEFAULT '{}'::jsonb NOT NULL
+    details jsonb DEFAULT '{}'::jsonb NOT NULL,
+    description text,
+    message text,
+    solution text,
+    cve text,
+    location jsonb,
+    CONSTRAINT check_4a3a60f2ba CHECK ((char_length(solution) <= 7000)),
+    CONSTRAINT check_ade261da6b CHECK ((char_length(description) <= 15000)),
+    CONSTRAINT check_df6dd20219 CHECK ((char_length(message) <= 3000)),
+    CONSTRAINT check_f602da68dd CHECK ((char_length(cve) <= 48400))
 );
 
 CREATE SEQUENCE vulnerability_occurrences_id_seq

ee/app/models/vulnerabilities/finding.rb

@@ -62,6 +62,11 @@ module Vulnerabilities
     validates :raw_metadata, presence: true
     validates :details, json_schema: { filename: 'vulnerability_finding_details', draft: 7 }
 
+    validates :description, length: { maximum: 15000 }
+    validates :message, length: { maximum: 3000 }
+    validates :solution, length: { maximum: 7000 }
+    validates :cve, length: { maximum: 48400 }
+
     delegate :name, :external_id, to: :scanner, prefix: true, allow_nil: true
 
     scope :report_type, -> (type) { where(report_type: report_types[type]) }
@@ -217,15 +222,15 @@ module Vulnerabilities
     end
 
     def description
-      metadata.dig('description')
+      super.presence || metadata.dig('description')
     end
 
     def solution
-      metadata.dig('solution') || remediations&.first&.dig('summary')
+      super.presence || metadata.dig('solution') || remediations&.first&.dig('summary')
     end
 
     def location
-      metadata.fetch('location', {})
+      super.presence || metadata.fetch('location', {})
     end
 
     def file
@@ -309,11 +314,11 @@ module Vulnerabilities
     end
 
     def message
-      metadata.dig('message')
+      super.presence || metadata.dig('message')
     end
 
     def cve_value
-      identifiers.find(&:cve?)&.name
+      cve || identifiers.find(&:cve?)&.name
     end
 
     def cwe_value

ee/app/services/security/store_report_service.rb

@@ -50,7 +50,8 @@ module Security
       end
 
       vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan, :links)
-      vulnerability_finding = create_or_find_vulnerability_finding(finding, vulnerability_params)
+      entity_params = Gitlab::Json.parse(vulnerability_params&.dig(:raw_metadata)).slice('description', 'message', 'solution', 'cve', 'location')
+      vulnerability_finding = create_or_find_vulnerability_finding(finding, vulnerability_params.merge(entity_params))
 
       update_vulnerability_scanner(finding)
 

ee/spec/models/vulnerabilities/finding_spec.rb

@@ -37,6 +37,11 @@ RSpec.describe Vulnerabilities::Finding do
     it { is_expected.to validate_presence_of(:severity) }
     it { is_expected.to validate_presence_of(:confidence) }
 
+    it { is_expected.to validate_length_of(:description).is_at_most(15000) }
+    it { is_expected.to validate_length_of(:message).is_at_most(3000) }
+    it { is_expected.to validate_length_of(:solution).is_at_most(7000) }
+    it { is_expected.to validate_length_of(:cve).is_at_most(48400) }
+
     context 'when value for details field is valid' do
       it 'is valid' do
         finding.details = {}
@@ -453,26 +458,34 @@ RSpec.describe Vulnerabilities::Finding do
   end
 
   describe '#remediations' do
-    let(:raw_remediation) { { summary: 'foo', diff: 'bar' }.stringify_keys }
-    let(:raw_metadata) { { remediations: [raw_remediation] }.to_json }
-    let(:finding) { create(:vulnerabilities_finding, raw_metadata: raw_metadata) }
+    let_it_be(:project) { create_default(:project) }
+    let_it_be(:finding, refind: true) { create(:vulnerabilities_finding) }
 
     subject { finding.remediations }
 
     context 'when the finding has associated remediation records' do
-      let!(:persisted_remediation) { create(:vulnerabilities_remediation, findings: [finding]) }
-      let(:remediation_hash) { { 'summary' => persisted_remediation.summary, 'diff' => persisted_remediation.diff } }
+      let_it_be(:persisted_remediation) { create(:vulnerabilities_remediation, findings: [finding]) }
+      let_it_be(:remediation_hash) { { 'summary' => persisted_remediation.summary, 'diff' => persisted_remediation.diff } }
 
       it { is_expected.to eq([remediation_hash]) }
     end
 
     context 'when the finding does not have associated remediation records' do
       context 'when the finding has remediations in `raw_metadata`' do
+        let(:raw_remediation) { { summary: 'foo', diff: 'bar' }.stringify_keys }
+
+        before do
+          raw_metadata = { remediations: [raw_remediation] }.to_json
+          finding.update!(raw_metadata: raw_metadata)
+        end
+
         it { is_expected.to eq([raw_remediation]) }
       end
 
       context 'when the finding does not have remediations in `raw_metadata`' do
-        let(:raw_metadata) { {}.to_json }
+        before do
+          finding.update!(raw_metadata: {}.to_json)
+        end
 
         it { is_expected.to be_nil }
       end
@@ -689,6 +702,23 @@ RSpec.describe Vulnerabilities::Finding do
     it { is_expected.to eq(vulnerabilities_finding.scanner.name) }
   end
 
+  describe '#description' do
+    let(:finding) { build(:vulnerabilities_finding) }
+    let(:expected_description) { finding.metadata['description'] }
+
+    subject { finding.description }
+
+    context 'when description metadata key is present' do
+      it { is_expected.to eql(expected_description) }
+    end
+
+    context 'when description data is present' do
+      let(:finding) { build(:vulnerabilities_finding, description: 'Vulnerability description') }
+
+      it { is_expected.to eq('Vulnerability description') }
+    end
+  end
+
   describe '#solution' do
     subject { vulnerabilities_finding.solution }
 
@@ -698,13 +728,37 @@ RSpec.describe Vulnerabilities::Finding do
       it { is_expected.to eq(vulnerabilities_finding.metadata['solution']) }
     end
 
-    context 'when remediations key is present' do
+    context 'when remediations key is present in finding' do
       let(:vulnerabilities_finding) do
         build(:vulnerabilities_finding_with_remediation, summary: "Test remediation")
       end
 
       it { is_expected.to eq(vulnerabilities_finding.remediations.dig(0, 'summary')) }
     end
+
+    context 'when solution data is present' do
+      let(:vulnerabilities_finding) { build(:vulnerabilities_finding, solution: 'Vulnerability solution') }
+
+      it { is_expected.to eq('Vulnerability solution') }
+    end
+  end
+
+  describe '#location' do
+    let(:finding) { build(:vulnerabilities_finding) }
+    let(:expected_location) { finding.metadata['location'] }
+
+    subject { finding.location }
+
+    context 'when location metadata key is present' do
+      it { is_expected.to eql(expected_location) }
+    end
+
+    context 'when location data is present' do
+      let(:location) { { 'class' => 'class', 'end_line' => 3, 'file' => 'test_file.rb', 'start_line' => 1 } }
+      let(:finding) { build(:vulnerabilities_finding, location: location) }
+
+      it { is_expected.to eq(location) }
+    end
   end
 
   describe '#evidence' do
@@ -810,7 +864,15 @@ RSpec.describe Vulnerabilities::Finding do
 
     subject { finding.message }
 
-    it { is_expected.to eql(expected_message) }
+    context 'when message metadata key is present' do
+      it { is_expected.to eql(expected_message) }
+    end
+
+    context 'when message data is present' do
+      let(:finding) { build(:vulnerabilities_finding, message: 'Vulnerability message') }
+
+      it { is_expected.to eq('Vulnerability message') }
+    end
   end
 
   describe '#cve_value' do
@@ -823,7 +885,15 @@ RSpec.describe Vulnerabilities::Finding do
       finding.identifiers << build(:vulnerabilities_identifier, external_type: 'cve', name: expected_cve)
     end
 
-    it { is_expected.to eql(expected_cve) }
+    context 'when cve metadata key is present' do
+      it { is_expected.to eql(expected_cve) }
+    end
+
+    context 'when cve data is present' do
+      let(:finding) { build(:vulnerabilities_finding, cve: 'Vulnerability cve') }
+
+      it { is_expected.to eq('Vulnerability cve') }
+    end
   end
 
   describe '#cwe_value' do

ee/spec/services/security/store_report_service_spec.rb

@@ -67,6 +67,24 @@ RSpec.describe Security::StoreReportService, '#execute' do
       end
     end
 
+    context 'when report data includes all raw_metadata' do
+      let(:trait) { :dependency_scanning_remediation }
+
+      it 'inserts top level finding data', :aggregate_failures do
+        subject
+
+        finding = Vulnerabilities::Finding.last
+        finding.raw_metadata = nil
+
+        expect(finding.metadata).to be_blank
+        expect(finding.cve).not_to be_nil
+        expect(finding.description).not_to be_nil
+        expect(finding.location).not_to be_nil
+        expect(finding.message).not_to be_nil
+        expect(finding.solution).not_to be_nil
+      end
+    end
+
     context 'invalid data' do
       let(:artifact) { create(:ee_ci_job_artifact, :sast) }
       let(:finding_without_name) { build(:ci_reports_security_finding, name: nil) }