Merge branch '342043_dont_parse_raw_metadata' into 'master'

Don't parse raw_metadata multiple times

See merge request gitlab-org/gitlab!74287

ee/app/services/security/ingestion/finding_map.rb

@@ -8,9 +8,7 @@ module Security
     # You can think this as the Message object in the pipeline design pattern
     # which is passed between tasks.
     class FindingMap
-      FINDING_ATTRIBUTES = %i[confidence metadata_version name raw_metadata report_type severity details].freeze
-      RAW_METADATA_ATTRIBUTES = %w[description message solution cve location].freeze
-      RAW_METADATA_PLACEHOLDER = { description: nil, message: nil, solution: nil, cve: nil, location: nil }.freeze
+      FINDING_ATTRIBUTES = %i[confidence metadata_version name raw_metadata report_type severity details description message cve solution].freeze
 
       attr_reader :security_finding, :report_finding
       attr_accessor :finding_id, :vulnerability_id, :new_record, :identifier_ids
@@ -44,16 +42,16 @@ module Security
       end
 
       def to_hash
-        # This was already an existing problem so we've used it here as well.
-        # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/342043
-        parsed_from_raw_metadata = Gitlab::Json.parse(report_finding.raw_metadata).slice(*RAW_METADATA_ATTRIBUTES).symbolize_keys
-
         report_finding.to_hash
                       .slice(*FINDING_ATTRIBUTES)
-                      .merge(RAW_METADATA_PLACEHOLDER)
-                      .merge(parsed_from_raw_metadata)
-                      .merge(primary_identifier_id: identifier_ids.first, location_fingerprint: report_finding.location.fingerprint, project_fingerprint: project_fingerprint)
-                      .merge(uuid: uuid, scanner_id: scanner_id)
+                      .merge!(
+                        uuid: uuid,
+                        scanner_id: scanner_id,
+                        project_fingerprint: project_fingerprint,
+                        primary_identifier_id: identifier_ids.first,
+                        location: report_finding.location_data,
+                        location_fingerprint: report_finding.location.fingerprint
+                      )
       end
     end
   end

ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb

@@ -29,7 +29,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
         location: location,
         metadata_version: 'sast:1.0',
         name: 'Cipher with no integrity',
-        raw_metadata: 'I am a stringified json object',
+        original_data: {},
         report_type: :sast,
         scanner: scanner,
         scan: nil,
@@ -71,7 +71,8 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
           location: location,
           metadata_version: 'sast:1.0',
           name: 'Cipher with no integrity',
-          raw_metadata: 'I am a stringified json object',
+          raw_metadata: '{}',
+          original_data: {},
           report_type: :sast,
           scanner: scanner,
           severity: :high,
@@ -98,7 +99,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
       end
     end
 
-    %i[compare_key identifiers location metadata_version name raw_metadata report_type scanner uuid].each do |attribute|
+    %i[compare_key identifiers location metadata_version name original_data report_type scanner uuid].each do |attribute|
       context "when attribute #{attribute} is missing" do
         before do
           params.delete(attribute)
@@ -144,6 +145,10 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
         severity: occurrence.severity,
         uuid: occurrence.uuid,
         details: occurrence.details,
+        cve: occurrence.compare_key,
+        description: occurrence.description,
+        message: occurrence.message,
+        solution: occurrence.solution,
         signatures: []
       })
     end

ee/spec/services/security/ingestion/finding_map_spec.rb

@@ -61,7 +61,7 @@ RSpec.describe Security::Ingestion::FindingMap do
         description: 'The cipher does not provide data integrity update 1',
         solution: 'GCM mode introduces an HMAC into the resulting encrypted data, providing integrity of the result.',
         message: nil,
-        cve: nil,
+        cve: report_finding.cve,
         location: {
           "class" => "com.gitlab.security_products.tests.App",
           "end_line" => 29,

lib/gitlab/ci/parsers/security/common.rb

@@ -114,7 +114,7 @@ module Gitlab
                 flags: flags,
                 links: links,
                 remediations: remediations,
-                raw_metadata: data.to_json,
+                original_data: data,
                 metadata_version: report_version,
                 details: data['details'] || {},
                 signatures: signatures,

lib/gitlab/ci/reports/security/finding.rb

@@ -17,7 +17,6 @@ module Gitlab
           attr_reader :name
           attr_reader :old_location
           attr_reader :project_fingerprint
-          attr_reader :raw_metadata
           attr_reader :report_type
           attr_reader :scanner
           attr_reader :scan
@@ -28,10 +27,13 @@ module Gitlab
           attr_reader :details
           attr_reader :signatures
           attr_reader :project_id
+          attr_reader :original_data
 
           delegate :file_path, :start_line, :end_line, to: :location
 
-          def initialize(compare_key:, identifiers:, flags: [], links: [], remediations: [], location:, metadata_version:, name:, raw_metadata:, report_type:, scanner:, scan:, uuid:, confidence: nil, severity: nil, details: {}, signatures: [], project_id: nil, vulnerability_finding_signatures_enabled: false) # rubocop:disable Metrics/ParameterLists
+          alias_method :cve, :compare_key
+
+          def initialize(compare_key:, identifiers:, flags: [], links: [], remediations: [], location:, metadata_version:, name:, original_data:, report_type:, scanner:, scan:, uuid:, confidence: nil, severity: nil, details: {}, signatures: [], project_id: nil, vulnerability_finding_signatures_enabled: false) # rubocop:disable Metrics/ParameterLists
             @compare_key = compare_key
             @confidence = confidence
             @identifiers = identifiers
@@ -40,7 +42,7 @@ module Gitlab
             @location = location
             @metadata_version = metadata_version
             @name = name
-            @raw_metadata = raw_metadata
+            @original_data = original_data
             @report_type = report_type
             @scanner = scanner
             @scan = scan
@@ -74,6 +76,10 @@ module Gitlab
               uuid
               details
               signatures
+              description
+              message
+              cve
+              solution
             ].each_with_object({}) do |key, hash|
               hash[key] = public_send(key) # rubocop:disable GitlabSecurity/PublicSend
             end
@@ -145,6 +151,26 @@ module Gitlab
             signatures.present?
           end
 
+          def raw_metadata
+            @raw_metadata ||= original_data.to_json
+          end
+
+          def description
+            original_data['description']
+          end
+
+          def message
+            original_data['message']
+          end
+
+          def solution
+            original_data['solution']
+          end
+
+          def location_data
+            original_data['location']
+          end
+
           private
 
           def generate_project_fingerprint

spec/factories/ci/reports/security/findings.rb

@@ -9,7 +9,7 @@ FactoryBot.define do
     metadata_version { 'sast:1.0' }
     name { 'Cipher with no integrity' }
     report_type { :sast }
-    raw_metadata do
+    original_data do
       {
         description: "The cipher does not provide data integrity update 1",
         solution: "GCM mode introduces an HMAC into the resulting encrypted data, providing integrity of the result.",
@@ -26,7 +26,7 @@ FactoryBot.define do
             url: "https://crypto.stackexchange.com/questions/31428/pbewithmd5anddes-cipher-does-not-check-for-integrity-first"
           }
         ]
-      }.to_json
+      }.deep_stringify_keys
     end
     scanner factory: :ci_reports_security_scanner
     severity { :high }