diff --git a/app/models/concerns/vulnerability_finding_helpers.rb b/app/models/concerns/vulnerability_finding_helpers.rb new file mode 100644 index 0000000000000000000000000000000000000000..cf50305faaba6f6644af8c76638bcae6dea9283f --- /dev/null +++ b/app/models/concerns/vulnerability_finding_helpers.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module VulnerabilityFindingHelpers + extend ActiveSupport::Concern +end + +VulnerabilityFindingHelpers.prepend_if_ee('EE::VulnerabilityFindingHelpers') diff --git a/app/models/concerns/vulnerability_finding_signature_helpers.rb b/app/models/concerns/vulnerability_finding_signature_helpers.rb new file mode 100644 index 0000000000000000000000000000000000000000..f57e3cb0bfb0a4e9042b1a99e08e44457202b836 --- /dev/null +++ b/app/models/concerns/vulnerability_finding_signature_helpers.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module VulnerabilityFindingSignatureHelpers + extend ActiveSupport::Concern +end + +VulnerabilityFindingSignatureHelpers.prepend_if_ee('EE::VulnerabilityFindingSignatureHelpers') diff --git a/ee/app/finders/security/findings_finder.rb b/ee/app/finders/security/findings_finder.rb index a538aecaab23e6eb6eeec9bb57fed14e9372e6da..8c55b60a34c9a7aadeda9dbebba286b2927eb5d7 100644 --- a/ee/app/finders/security/findings_finder.rb +++ b/ee/app/finders/security/findings_finder.rb @@ -47,10 +47,13 @@ def build_vulnerability_finding(security_finding) report_finding = report_finding_for(security_finding) return Vulnerabilities::Finding.new unless report_finding - finding_data = report_finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :links) + finding_data = report_finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :links, :signatures) identifiers = report_finding.identifiers.map do |identifier| Vulnerabilities::Identifier.new(identifier.to_hash) end + signatures = report_finding.signatures.map do |signature| + Vulnerabilities::FindingSignature.new(signature.to_hash) + end Vulnerabilities::Finding.new(finding_data).tap do |finding| finding.location_fingerprint = report_finding.location.fingerprint @@ -59,6 +62,7 @@ def build_vulnerability_finding(security_finding) finding.sha = pipeline.sha finding.scanner = security_finding.scanner finding.identifiers = identifiers + finding.signatures = signatures end end diff --git a/ee/app/finders/security/pipeline_vulnerabilities_finder.rb b/ee/app/finders/security/pipeline_vulnerabilities_finder.rb index 708ebca7c17fd707214a2f3aaeba3b29c9a9a45b..17a5fee391a1a9d1d1c0214ba07d109b211e5c6c 100644 --- a/ee/app/finders/security/pipeline_vulnerabilities_finder.rb +++ b/ee/app/finders/security/pipeline_vulnerabilities_finder.rb @@ -75,7 +75,7 @@ def vulnerabilities_by_finding_fingerprint(report_type, report) def normalize_report_findings(report_findings, vulnerabilities) report_findings.map do |report_finding| finding_hash = report_finding.to_hash - .except(:compare_key, :identifiers, :location, :scanner, :links) + .except(:compare_key, :identifiers, :location, :scanner, :links, :signatures) finding = Vulnerabilities::Finding.new(finding_hash) # assigning Vulnerabilities to Findings to enable the computed state @@ -90,6 +90,9 @@ def normalize_report_findings(report_findings, vulnerabilities) finding.identifiers = report_finding.identifiers.map do |identifier| Vulnerabilities::Identifier.new(identifier.to_hash) end + finding.signatures = report_finding.signatures.map do |signature| + Vulnerabilities::FindingSignature.new(signature.to_hash) + end finding end @@ -111,18 +114,36 @@ def include_dismissed? end def dismissal_feedback?(finding) - dismissal_feedback_by_fingerprint[finding.project_fingerprint] + if ::Feature.enabled?(:vulnerability_finding_signatures, pipeline.project) && !finding.signatures.empty? + dismissal_feedback_by_finding_signatures(finding) + else + dismissal_feedback_by_project_fingerprint(finding) + end + end + + def all_dismissal_feedbacks + strong_memoize(:all_dismissal_feedbacks) do + pipeline.project + .vulnerability_feedback + .for_dismissal + end + end + + def dismissal_feedback_by_finding_signatures(finding) + potential_uuids = Set.new([*finding.signature_uuids, finding.uuid].compact) + all_dismissal_feedbacks.any? { |dismissal| potential_uuids.include?(dismissal.finding_uuid) } end def dismissal_feedback_by_fingerprint strong_memoize(:dismissal_feedback_by_fingerprint) do - pipeline.project - .vulnerability_feedback - .for_dismissal - .group_by(&:project_fingerprint) + all_dismissal_feedbacks.group_by(&:project_fingerprint) end end + def dismissal_feedback_by_project_fingerprint(finding) + dismissal_feedback_by_fingerprint[finding.project_fingerprint] + end + def confidence_levels Array(params.fetch(:confidence, Vulnerabilities::Finding.confidences.keys)) end diff --git a/ee/app/models/concerns/ee/vulnerability_finding_helpers.rb b/ee/app/models/concerns/ee/vulnerability_finding_helpers.rb new file mode 100644 index 0000000000000000000000000000000000000000..bcccfe0598cb89939aef7333c12f58b22bb16c7f --- /dev/null +++ b/ee/app/models/concerns/ee/vulnerability_finding_helpers.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module EE + module VulnerabilityFindingHelpers + extend ActiveSupport::Concern + + def matches_signatures(other_signatures, other_uuid) + other_signature_types = other_signatures.index_by(&:algorithm_type) + + # highest first + match_result = nil + signatures.sort_by(&:priority).reverse_each do |signature| + matching_other_signature = other_signature_types[signature.algorithm_type] + next if matching_other_signature.nil? + + match_result = matching_other_signature == signature + break + end + + if match_result.nil? + Set.new([uuid, *signature_uuids]).include?(other_uuid) + else + match_result + end + end + + def signature_uuids + signatures.map do |signature| + hex_sha = signature.signature_hex + ::Security::VulnerabilityUUID.generate( + report_type: report_type, + location_fingerprint: hex_sha, + primary_identifier_fingerprint: primary_identifier&.fingerprint, + project_id: project_id + ) + end + end + end +end diff --git a/ee/app/models/concerns/ee/vulnerability_finding_signature_helpers.rb b/ee/app/models/concerns/ee/vulnerability_finding_signature_helpers.rb new file mode 100644 index 0000000000000000000000000000000000000000..0b8ea6a59ac4251c47eb45b5b218392e412ffbaa --- /dev/null +++ b/ee/app/models/concerns/ee/vulnerability_finding_signature_helpers.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module EE + module VulnerabilityFindingSignatureHelpers + extend ActiveSupport::Concern + + # If the location object describes a physical location within a file + # (filename + line numbers), the 'location' algorithm_type should be used + # + # If the location object describes arbitrary data, then the 'hash' + # algorithm_type should be used. + PRIORITIES = { + 'scope_offset' => 3, + 'location' => 2, + 'hash' => 1 + }.freeze + + class_methods do + def priority(algorithm_type) + raise ArgumentError.new("No priority for #{algorithm_type.inspect}") unless PRIORITIES.key?(algorithm_type) + + PRIORITIES[algorithm_type] + end + end + + def priority + self.class.priority(algorithm_type) + end + end +end diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb index 4708eb78c32c3c5fe6914abab23f96c9662d2844..cad241d913f1768c84845f9ab2bd0b358d779abd 100644 --- a/ee/app/models/license.rb +++ b/ee/app/models/license.rb @@ -180,6 +180,7 @@ class License < ApplicationRecord subepics threat_monitoring vulnerability_auto_fix + vulnerability_finding_signatures ] EEU_FEATURES.freeze diff --git a/ee/app/models/vulnerabilities/finding.rb b/ee/app/models/vulnerabilities/finding.rb index 2cbccb3b2b8fde8a05402519b63b31407baeac7d..5e8c3f62886c723aa8c8406c3a14f734a2ad6d13 100644 --- a/ee/app/models/vulnerabilities/finding.rb +++ b/ee/app/models/vulnerabilities/finding.rb @@ -5,6 +5,7 @@ class Finding < ApplicationRecord include ShaAttribute include ::Gitlab::Utils::StrongMemoize include Presentable + include ::VulnerabilityFindingHelpers # https://gitlab.com/groups/gitlab-org/-/epics/3148 # https://gitlab.com/gitlab-org/gitlab/-/issues/214563#note_370782508 is why the table names are not renamed @@ -332,12 +333,16 @@ def assets end end - alias_method :==, :eql? # eql? is necessary in some cases like array intersection + alias_method :==, :eql? def eql?(other) - other.report_type == report_type && - other.location_fingerprint == location_fingerprint && - other.first_fingerprint == first_fingerprint + return false unless other.report_type == report_type && other.primary_identifier_fingerprint == primary_identifier_fingerprint + + if ::Feature.enabled?(:vulnerability_finding_signatures, project) + matches_signatures(other.signatures, other.uuid) + else + other.location_fingerprint == location_fingerprint + end end # Array.difference (-) method uses hash and eql? methods to do comparison @@ -348,7 +353,7 @@ def hash # when Finding is persisted and identifiers are not preloaded. return super if persisted? && !identifiers.loaded? - report_type.hash ^ location_fingerprint.hash ^ first_fingerprint.hash + report_type.hash ^ location_fingerprint.hash ^ primary_identifier_fingerprint.hash end def severity_value @@ -380,7 +385,7 @@ def pipeline_branch protected - def first_fingerprint + def primary_identifier_fingerprint identifiers.first&.fingerprint end diff --git a/ee/app/models/vulnerabilities/finding_signature.rb b/ee/app/models/vulnerabilities/finding_signature.rb index a7db39a50276940cb8bdcc019c468f2546e5acb2..4792e24267371ea2cde7f2e4089fc745e06d7cbe 100644 --- a/ee/app/models/vulnerabilities/finding_signature.rb +++ b/ee/app/models/vulnerabilities/finding_signature.rb @@ -5,11 +5,23 @@ class FindingSignature < ApplicationRecord self.table_name = 'vulnerability_finding_signatures' include BulkInsertSafe + include VulnerabilityFindingSignatureHelpers belongs_to :finding, foreign_key: 'finding_id', inverse_of: :signatures, class_name: 'Vulnerabilities::Finding' enum algorithm_type: { hash: 1, location: 2, scope_offset: 3 }, _prefix: :algorithm validates :finding, presence: true + + def signature_hex + signature_sha.unpack1("H*") + end + + def eql?(other) + other.algorithm_type == algorithm_type && + other.signature_sha == signature_sha + end + + alias_method :==, :eql? end end diff --git a/ee/app/services/security/store_report_service.rb b/ee/app/services/security/store_report_service.rb index 2ac1fa9885cfbac8bde4ed1a738d269717444da1..ae68ed2665e42f668dc34431f3a0f0a6e45f2798 100644 --- a/ee/app/services/security/store_report_service.rb +++ b/ee/app/services/security/store_report_service.rb @@ -61,17 +61,21 @@ def create_vulnerability_finding(vulnerability_findings_by_uuid, finding) return end - vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan, :links) + vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan, :links, :signatures) entity_params = Gitlab::Json.parse(vulnerability_params&.dig(:raw_metadata)).slice('description', 'message', 'solution', 'cve', 'location') # Vulnerabilities::Finding (`vulnerability_occurrences`) vulnerability_finding = vulnerability_findings_by_uuid[finding.uuid] || - create_new_vulnerability_finding(finding, vulnerability_params.merge(entity_params)) + find_or_create_vulnerability_finding(finding, vulnerability_params.merge(entity_params)) update_vulnerability_scanner(finding) unless Feature.enabled?(:optimize_sql_query_for_security_report, project) update_vulnerability_finding(vulnerability_finding, vulnerability_params) reset_remediations_for(vulnerability_finding, finding) - update_finding_signatures(finding, vulnerability_finding) + + if ::Feature.enabled?(:vulnerability_finding_signatures, project) + update_feedbacks(vulnerability_finding, vulnerability_params[:uuid]) + update_finding_signatures(finding, vulnerability_finding) + end # The maximum number of identifiers is not used in validation # we just want to ignore the rest if a finding has more than that. @@ -86,8 +90,16 @@ def create_vulnerability_finding(vulnerability_findings_by_uuid, finding) create_vulnerability(vulnerability_finding, pipeline) end + def find_or_create_vulnerability_finding(finding, create_params) + if ::Feature.enabled?(:vulnerability_finding_signatures, project) + find_or_create_vulnerability_finding_with_signatures(finding, create_params) + else + find_or_create_vulnerability_finding_with_location(finding, create_params) + end + end + # rubocop: disable CodeReuse/ActiveRecord - def create_new_vulnerability_finding(finding, create_params) + def find_or_create_vulnerability_finding_with_location(finding, create_params) find_params = { scanner: scanners_objects[finding.scanner.key], primary_identifier: identifiers_objects[finding.primary_identifier.key], @@ -120,6 +132,56 @@ def create_new_vulnerability_finding(finding, create_params) end end + def get_matched_findings(finding, normalized_signatures, find_params) + project.vulnerability_findings.where(**find_params).filter do |vf| + vf.matches_signatures(normalized_signatures, finding.uuid) + end + end + + def find_or_create_vulnerability_finding_with_signatures(finding, create_params) + find_params = { + # this isn't taking prioritization into account (happens in the filter + # block below), but it *does* limit the number of findings we have to sift through + location_fingerprint: [finding.location.fingerprint, *finding.signatures.map(&:signature_hex)], + scanner: scanners_objects[finding.scanner.key], + primary_identifier: identifiers_objects[finding.primary_identifier.key] + } + + normalized_signatures = finding.signatures.map do |signature| + ::Vulnerabilities::FindingSignature.new(signature.to_hash) + end + + matched_findings = get_matched_findings(finding, normalized_signatures, find_params) + + begin + vulnerability_finding = matched_findings.first + if vulnerability_finding.nil? + find_params[:uuid] = finding.uuid + vulnerability_finding = project + .vulnerability_findings + .create_with(create_params) + .find_or_initialize_by(find_params) + end + + vulnerability_finding.uuid = finding.uuid + + vulnerability_finding.location_fingerprint = if finding.signatures.empty? + finding.location.fingerprint + else + finding.signatures.max_by(&:priority).signature_hex + end + + vulnerability_finding.location = create_params.dig(:location) + vulnerability_finding.save! + + vulnerability_finding + rescue ActiveRecord::RecordNotUnique + get_matched_findings(finding, normalized_signatures, find_params).first + rescue ActiveRecord::RecordInvalid => e + Gitlab::ErrorTracking.track_and_raise_exception(e, create_params: create_params&.dig(:raw_metadata)) + end + end + def update_vulnerability_scanner(finding) scanner = scanners_objects[finding.scanner.key] scanner.update!(finding.scanner.to_hash) @@ -223,6 +285,14 @@ def create_vulnerability_pipeline_object(vulnerability_finding, pipeline) end # rubocop: enable CodeReuse/ActiveRecord + def update_feedbacks(vulnerability_finding, new_uuid) + vulnerability_finding.load_feedback.each do |feedback| + feedback.finding_uuid = new_uuid + feedback.vulnerability_data = vulnerability_finding.raw_metadata + feedback.save! + end + end + def update_finding_signatures(finding, vulnerability_finding) to_update = {} to_create = [] @@ -240,12 +310,12 @@ def update_finding_signatures(finding, vulnerability_finding) next if poro_signature.nil? poro_signatures.delete(signature.algorithm_type) - to_update[signature.id] = poro_signature.to_h + to_update[signature.id] = poro_signature.to_hash end # any remaining poro signatures left are new poro_signatures.values.each do |poro_signature| - attributes = poro_signature.to_h.merge(finding_id: vulnerability_finding.id) + attributes = poro_signature.to_hash.merge(finding_id: vulnerability_finding.id) to_create << ::Vulnerabilities::FindingSignature.new(attributes: attributes, created_at: Time.zone.now, updated_at: Time.zone.now) end diff --git a/ee/changelogs/unreleased/improve_vuln_tracking-backend_use_fingerprints.yml b/ee/changelogs/unreleased/improve_vuln_tracking-backend_use_fingerprints.yml new file mode 100644 index 0000000000000000000000000000000000000000..59214838ccc9f1904aac3405d1585b7cb3381418 --- /dev/null +++ b/ee/changelogs/unreleased/improve_vuln_tracking-backend_use_fingerprints.yml @@ -0,0 +1,5 @@ +--- +title: Improve vulnerability tracking using finding signatures +merge_request: 54608 +author: +type: added diff --git a/ee/config/feature_flags/development/vulnerability_finding_signatures.yml b/ee/config/feature_flags/development/vulnerability_finding_signatures.yml new file mode 100644 index 0000000000000000000000000000000000000000..df555253bace8bfb2bf87a41efb0643021fdee5d --- /dev/null +++ b/ee/config/feature_flags/development/vulnerability_finding_signatures.yml @@ -0,0 +1,8 @@ +--- +name: vulnerability_finding_signatures +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54608 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/322044 +milestone: '13.11' +type: development +group: group::vulnerability research +default_enabled: false diff --git a/ee/lib/ee/gitlab/background_migration/update_location_fingerprint_for_container_scanning_findings.rb b/ee/lib/ee/gitlab/background_migration/update_location_fingerprint_for_container_scanning_findings.rb index e8b8177b5e26686b10698d291c7175a627836ad3..48227dfb2762c4be4fa9cd93bbe0303e02ab1236 100644 --- a/ee/lib/ee/gitlab/background_migration/update_location_fingerprint_for_container_scanning_findings.rb +++ b/ee/lib/ee/gitlab/background_migration/update_location_fingerprint_for_container_scanning_findings.rb @@ -51,13 +51,14 @@ def version_semver_like?(version) override :perform def perform(start_id, stop_id) Finding.container_scanning - .select(:id, "raw_metadata::json->'location' AS loc") + .select(:id, "raw_metadata::json->'location' AS loc") .where(id: start_id..stop_id) .each do |finding| next if finding.loc.nil? package = finding.loc.dig('dependency', 'package', 'name') image = finding.loc.dig('image') + new_fingerprint = finding.calculate_new_fingerprint(image, package) next if new_fingerprint.blank? diff --git a/ee/lib/gitlab/ci/parsers/security/common.rb b/ee/lib/gitlab/ci/parsers/security/common.rb index 457af2dd7861d1321435caeb44033dd7e5457b96..3f80bed37d146aa912bb0e7e1f47a7142b081191 100644 --- a/ee/lib/gitlab/ci/parsers/security/common.rb +++ b/ee/lib/gitlab/ci/parsers/security/common.rb @@ -7,13 +7,14 @@ module Security class Common SecurityReportParserError = Class.new(Gitlab::Ci::Parsers::ParserError) - def self.parse!(json_data, report) - new(json_data, report).parse! + def self.parse!(json_data, report, vulnerability_finding_signatures_enabled = false) + new(json_data, report, vulnerability_finding_signatures_enabled).parse! end - def initialize(json_data, report) + def initialize(json_data, report, vulnerability_finding_signatures_enabled = false) @json_data = json_data @report = report + @vulnerability_finding_signatures_enabled = vulnerability_finding_signatures_enabled end def parse! @@ -80,11 +81,20 @@ def create_vulnerability(data) links = create_links(data['links']) location = create_location(data['location'] || {}) remediations = create_remediations(data['remediations']) - signatures = create_signatures(tracking_data(data)) + signatures = create_signatures(location, tracking_data(data)) + + if @vulnerability_finding_signatures_enabled && !signatures.empty? + # NOT the signature_sha - the compare key is hashed + # to create the project_fingerprint + highest_priority_signature = signatures.max_by(&:priority) + uuid = calculate_uuid_v5(identifiers.first, highest_priority_signature.signature_hex) + else + uuid = calculate_uuid_v5(identifiers.first, location&.fingerprint) + end report.add_finding( ::Gitlab::Ci::Reports::Security::Finding.new( - uuid: calculate_uuid_v5(identifiers.first, location), + uuid: uuid, report_type: report.type, name: finding_name(data, identifiers, location), compare_key: data['cve'] || '', @@ -99,14 +109,19 @@ def create_vulnerability(data) raw_metadata: data.to_json, metadata_version: report_version, details: data['details'] || {}, - signatures: signatures)) + signatures: signatures, + project_id: report.project_id, + vulnerability_finding_signatures_enabled: @vulnerability_finding_signatures_enabled)) end - def create_signatures(data) - return [] if data.nil? || data['items'].nil? + def create_signatures(location, tracking) + tracking ||= { 'type' => 'hash', 'items' => [] } + + create_hash_signatures!(tracking['items']) if tracking['type'] == 'hash' signature_algorithms = Hash.new { |hash, key| hash[key] = [] } - data['items'].each do |item| + + tracking['items'].each do |item| next unless item.key?('signatures') item['signatures'].each do |signature| @@ -117,19 +132,32 @@ def create_signatures(data) signature_algorithms.map do |algorithm, values| value = values.join('|') - begin - signature = ::Gitlab::Ci::Reports::Security::FindingSignature.new( - algorithm_type: algorithm, - signature_value: value - ) - signature.valid? ? signature : nil - rescue ArgumentError => e - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e) + signature = ::Gitlab::Ci::Reports::Security::FindingSignature.new( + algorithm_type: algorithm, + signature_value: value + ) + + if signature.valid? + signature + else + Gitlab::AppLogger.warn(message: "Vulnerability tracking signature is not valid", signature: signature.to_s) nil end end.compact end + def create_hash_signatures!(items) + return if items.nil? + + items.each do |item| + item_data = item['data'] + next if item_data.nil? + + item['signatures'] ||= [] + item['signatures'] << { 'algorithm' => 'hash', 'value' => Digest::SHA1.hexdigest(item_data) } + end + end + def create_scan return unless scan_data.is_a?(Hash) @@ -201,11 +229,11 @@ def finding_name(data, identifiers, location) "#{identifier.name} in #{location&.fingerprint_path}" end - def calculate_uuid_v5(primary_identifier, location) + def calculate_uuid_v5(primary_identifier, location_fingerprint) uuid_v5_name_components = { report_type: report.type, primary_identifier_fingerprint: primary_identifier&.fingerprint, - location_fingerprint: location&.fingerprint, + location_fingerprint: location_fingerprint, project_id: report.project_id } diff --git a/ee/lib/gitlab/ci/reports/security/finding.rb b/ee/lib/gitlab/ci/reports/security/finding.rb index 387834e655a4b09c818e784a53c72b8833e62715..9e6d24708344213089b5154a212095119bb18971 100644 --- a/ee/lib/gitlab/ci/reports/security/finding.rb +++ b/ee/lib/gitlab/ci/reports/security/finding.rb @@ -5,6 +5,8 @@ module Ci module Reports module Security class Finding + include ::VulnerabilityFindingHelpers + UNSAFE_SEVERITIES = %w[unknown high critical].freeze attr_reader :compare_key @@ -25,10 +27,11 @@ class Finding attr_reader :remediations attr_reader :details attr_reader :signatures + attr_reader :project_id delegate :file_path, :start_line, :end_line, to: :location - def initialize(compare_key:, identifiers:, links: [], remediations: [], location:, metadata_version:, name:, raw_metadata:, report_type:, scanner:, scan:, uuid:, confidence: nil, severity: nil, details: {}, signatures: []) # rubocop:disable Metrics/ParameterLists + def initialize(compare_key:, identifiers:, 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 @compare_key = compare_key @confidence = confidence @identifiers = identifiers @@ -45,6 +48,8 @@ def initialize(compare_key:, identifiers:, links: [], remediations: [], location @remediations = remediations @details = details @signatures = signatures + @project_id = project_id + @vulnerability_finding_signatures_enabled = vulnerability_finding_signatures_enabled @project_fingerprint = generate_project_fingerprint end @@ -66,6 +71,7 @@ def to_hash severity uuid details + signatures ].each_with_object({}) do |key, hash| hash[key] = public_send(key) # rubocop:disable GitlabSecurity/PublicSend end @@ -85,13 +91,22 @@ def unsafe? end def eql?(other) - report_type == other.report_type && - location.fingerprint == other.location.fingerprint && - primary_fingerprint == other.primary_fingerprint + return false unless report_type == other.report_type && primary_identifier_fingerprint == other.primary_identifier_fingerprint + + if @vulnerability_finding_signatures_enabled + matches_signatures(other.signatures, other.uuid) + else + location.fingerprint == other.location.fingerprint + end end def hash - report_type.hash ^ location.fingerprint.hash ^ primary_fingerprint.hash + if @vulnerability_finding_signatures_enabled && !signatures.empty? + highest_signature = signatures.max_by(&:priority) + report_type.hash ^ highest_signature.signature_hex.hash ^ primary_identifier_fingerprint.hash + else + report_type.hash ^ location.fingerprint.hash ^ primary_identifier_fingerprint.hash + end end def valid? @@ -104,7 +119,7 @@ def keys end end - def primary_fingerprint + def primary_identifier_fingerprint primary_identifier&.fingerprint end diff --git a/ee/lib/gitlab/ci/reports/security/finding_signature.rb b/ee/lib/gitlab/ci/reports/security/finding_signature.rb index bc4d04cd32e7dd3a0d3bc30bb1027786811c1ef1..db628c3b57176fe12c20dd27800405e518fbd555 100644 --- a/ee/lib/gitlab/ci/reports/security/finding_signature.rb +++ b/ee/lib/gitlab/ci/reports/security/finding_signature.rb @@ -5,6 +5,8 @@ module Ci module Reports module Security class FindingSignature + include VulnerabilityFindingSignatureHelpers + attr_accessor :algorithm_type, :signature_value def initialize(params = {}) @@ -12,11 +14,19 @@ def initialize(params = {}) @signature_value = params.dig(:signature_value) end + def priority + ::Vulnerabilities::FindingSignature.priority(algorithm_type) + end + def signature_sha Digest::SHA1.digest(signature_value) end - def to_h + def signature_hex + signature_sha.unpack1("H*") + end + + def to_hash { algorithm_type: algorithm_type, signature_sha: signature_sha @@ -26,6 +36,13 @@ def to_h def valid? ::Vulnerabilities::FindingSignature.algorithm_types.key?(algorithm_type) end + + def eql?(other) + other.algorithm_type == algorithm_type && + other.signature_sha == signature_sha + end + + alias_method :==, :eql? end end end diff --git a/ee/lib/gitlab/ci/reports/security/locations/container_scanning.rb b/ee/lib/gitlab/ci/reports/security/locations/container_scanning.rb index f335df42d4e5d1349860abd924c22bf9c1f8d51b..f93eddea2ea4b29c56ae14f1efb679ed0071dd25 100644 --- a/ee/lib/gitlab/ci/reports/security/locations/container_scanning.rb +++ b/ee/lib/gitlab/ci/reports/security/locations/container_scanning.rb @@ -18,12 +18,12 @@ def initialize(image:, operating_system:, package_name: nil, package_version: ni @package_version = package_version end - private - def fingerprint_data "#{docker_image_name_without_tag}:#{package_name}" end + private + def docker_image_name_without_tag base_name, version = image.split(':') diff --git a/ee/lib/gitlab/ci/reports/security/locations/coverage_fuzzing.rb b/ee/lib/gitlab/ci/reports/security/locations/coverage_fuzzing.rb index 7429004f53568f60fea59037a7ae8bb508ed3ea7..49705ead2ea60cbf325cb12c563895d0a2e0b9ca 100644 --- a/ee/lib/gitlab/ci/reports/security/locations/coverage_fuzzing.rb +++ b/ee/lib/gitlab/ci/reports/security/locations/coverage_fuzzing.rb @@ -16,8 +16,6 @@ def initialize(crash_address:, crash_type:, crash_state:) @crash_state = crash_state end - private - def fingerprint_data "#{crash_type}:#{crash_state}" end diff --git a/ee/lib/gitlab/ci/reports/security/locations/dast.rb b/ee/lib/gitlab/ci/reports/security/locations/dast.rb index 605e60e7490e8c314bab8a5ff585ffb9bde5bcda..da438b709104d837dc0ed1ccdc4048931182f0ab 100644 --- a/ee/lib/gitlab/ci/reports/security/locations/dast.rb +++ b/ee/lib/gitlab/ci/reports/security/locations/dast.rb @@ -20,8 +20,6 @@ def initialize(hostname:, method_name:, path:, param: nil) alias_method :fingerprint_path, :path - private - def fingerprint_data "#{path}:#{method_name}:#{param}" end diff --git a/ee/lib/gitlab/ci/reports/security/locations/dependency_scanning.rb b/ee/lib/gitlab/ci/reports/security/locations/dependency_scanning.rb index 8af5febc8a4cf2ecdd4857940e55ecfaefb994e4..05dc7a61ddf9c4736e95d4e120fcda2e3f6631f1 100644 --- a/ee/lib/gitlab/ci/reports/security/locations/dependency_scanning.rb +++ b/ee/lib/gitlab/ci/reports/security/locations/dependency_scanning.rb @@ -18,8 +18,6 @@ def initialize(file_path:, package_name:, package_version: nil) @package_version = package_version end - private - def fingerprint_data "#{file_path}:#{package_name}" end diff --git a/ee/lib/gitlab/ci/reports/security/locations/sast.rb b/ee/lib/gitlab/ci/reports/security/locations/sast.rb index effc48580c7da242bd5775da7681ab298e858c3f..23ffa91e72059b66dfaffd398ba24814029c6c70 100644 --- a/ee/lib/gitlab/ci/reports/security/locations/sast.rb +++ b/ee/lib/gitlab/ci/reports/security/locations/sast.rb @@ -22,8 +22,6 @@ def initialize(file_path:, start_line:, end_line: nil, class_name: nil, method_n @start_line = start_line end - private - def fingerprint_data "#{file_path}:#{start_line}:#{end_line}" end diff --git a/ee/lib/gitlab/ci/reports/security/locations/secret_detection.rb b/ee/lib/gitlab/ci/reports/security/locations/secret_detection.rb index 24aa979e4afa0491153be4c35c072d9ee3640bb8..0fd5cc5af113127c299d10aa0adfa1a9f62f743c 100644 --- a/ee/lib/gitlab/ci/reports/security/locations/secret_detection.rb +++ b/ee/lib/gitlab/ci/reports/security/locations/secret_detection.rb @@ -22,8 +22,6 @@ def initialize(file_path:, start_line:, end_line: nil, class_name: nil, method_n @start_line = start_line end - private - def fingerprint_data "#{file_path}:#{start_line}:#{end_line}" end diff --git a/ee/lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb b/ee/lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb index e2c8c50b1971c6323c10e8faf913840313cef746..699c161512d3e8a16ce1f6d7e08274479e0d00df 100644 --- a/ee/lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb +++ b/ee/lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb @@ -8,12 +8,17 @@ class VulnerabilityReportsComparer include Gitlab::Utils::StrongMemoize attr_reader :base_report, :head_report + attr_reader :added, :fixed ACCEPTABLE_REPORT_AGE = 1.week def initialize(base_report, head_report) @base_report = base_report @head_report = head_report + + @added = [] + @fixed = [] + calculate_changes end def base_report_created_at @@ -30,16 +35,34 @@ def base_report_out_of_date ACCEPTABLE_REPORT_AGE.ago > @base_report.created_at end - def added - strong_memoize(:added) do - head_report.findings - base_report.findings - end - end + def calculate_changes + @added = [] + @fixed = [] + @existing = [] + + base_findings = base_report.findings + head_findings = head_report.findings - def fixed - strong_memoize(:fixed) do - base_report.findings - head_report.findings + head_findings_hash = head_findings.index_by(&:object_id) + + # This is slow - O(N^2). If we didn't need to worry about one high + # priority fingerprint that doesn't match overruling a lower + # priority fingerprint that does match, we'd be able to do some + # set operations here + base_findings.each do |base_finding| + still_exists = false + head_findings.each do |head_finding| + next unless base_finding.eql?(head_finding) + + still_exists = true + head_findings_hash.delete(head_finding.object_id) + break + end + + @fixed << base_finding unless still_exists end + + @added = head_findings_hash.values end end end diff --git a/ee/spec/factories/ci/reports/security/findings.rb b/ee/spec/factories/ci/reports/security/findings.rb index 9ad1cedd1851746469c8b379d1620f1c085dd853..e3971bc48f33d94ac2ef612a503123c48ac3aa8b 100644 --- a/ee/spec/factories/ci/reports/security/findings.rb +++ b/ee/spec/factories/ci/reports/security/findings.rb @@ -39,6 +39,7 @@ project_id: n ) end + vulnerability_finding_signatures_enabled { false } skip_create diff --git a/ee/spec/factories/vulnerabilities/feedback.rb b/ee/spec/factories/vulnerabilities/feedback.rb index f8b4f031762b339e2157d0ed151c54831a986ea9..34b81bd469d99dd3b846e85f6b7fed0b1718727c 100644 --- a/ee/spec/factories/vulnerabilities/feedback.rb +++ b/ee/spec/factories/vulnerabilities/feedback.rb @@ -17,6 +17,7 @@ category { 'sast' } project_fingerprint { generate(:project_fingerprint) } vulnerability_data { { category: 'sast' } } + finding_uuid { Gitlab::UUID.v5(SecureRandom.hex) } trait :dismissal do feedback_type { 'dismissal' } diff --git a/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb b/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb index e687d3449171c45f77d62a7292413e792c44240b..5be86ae14e918a2e4a54f24af26b63e289c53f32 100644 --- a/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb +++ b/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb @@ -83,12 +83,12 @@ def disable_deduplication # use the same number of queries, regardless of the number of findings # contained in the pipeline report. - sast_findings = pipeline.security_reports.reports['sast'].findings + container_scanning_findings = pipeline.security_reports.reports['container_scanning'].findings dep_findings = pipeline.security_reports.reports['dependency_scanning'].findings - # this test is invalid if we don't have more sast findings than dep findings - expect(sast_findings.count).to be > dep_findings.count + # this test is invalid if we don't have more container_scanning findings than dep findings + expect(container_scanning_findings.count).to be > dep_findings.count - (sast_findings + dep_findings).each do |report_finding| + (container_scanning_findings + dep_findings).each do |report_finding| # create a finding and a vulnerability for each report finding # (the vulnerability is created with the :confirmed trait) create(:vulnerabilities_finding, @@ -103,7 +103,7 @@ def disable_deduplication # should be the same number of queries between different report types expect do - described_class.new(pipeline: pipeline, params: { report_type: %w[sast] }).execute + described_class.new(pipeline: pipeline, params: { report_type: %w[container_scanning] }).execute end.to issue_same_number_of_queries_as { described_class.new(pipeline: pipeline, params: { report_type: %w[dependency_scanning] }).execute } @@ -117,11 +117,11 @@ def disable_deduplication orig_security_reports = pipeline.security_reports new_finding = create(:ci_reports_security_finding) expect do - described_class.new(pipeline: pipeline, params: { report_type: %w[sast] }).execute + described_class.new(pipeline: pipeline, params: { report_type: %w[container_scanning] }).execute end.to issue_same_number_of_queries_as { - orig_security_reports.reports['sast'].add_finding(new_finding) + orig_security_reports.reports['container_scanning'].add_finding(new_finding) allow(pipeline).to receive(:security_reports).and_return(orig_security_reports) - described_class.new(pipeline: pipeline, params: { report_type: %w[sast] }).execute + described_class.new(pipeline: pipeline, params: { report_type: %w[container_scanning] }).execute } end end @@ -130,9 +130,11 @@ def disable_deduplication context 'when sast' do let(:params) { { report_type: %w[sast] } } let(:sast_report_fingerprints) {pipeline.security_reports.reports['sast'].findings.map(&:location).map(&:fingerprint) } + let(:sast_report_uuids) {pipeline.security_reports.reports['sast'].findings.map(&:uuid) } it 'includes only sast' do expect(subject.findings.map(&:location_fingerprint)).to match_array(sast_report_fingerprints) + expect(subject.findings.map(&:uuid)).to match_array(sast_report_uuids) expect(subject.findings.count).to eq(sast_count) end end @@ -172,52 +174,107 @@ def disable_deduplication let(:ds_finding) { pipeline.security_reports.reports["dependency_scanning"].findings.first } let(:sast_finding) { pipeline.security_reports.reports["sast"].findings.first } - let!(:feedback) do - [ - create( - :vulnerability_feedback, - :dismissal, - :dependency_scanning, - project: project, - pipeline: pipeline, - project_fingerprint: ds_finding.project_fingerprint, - vulnerability_data: ds_finding.raw_metadata - ), - create( - :vulnerability_feedback, - :dismissal, - :sast, - project: project, - pipeline: pipeline, - project_fingerprint: sast_finding.project_fingerprint, - vulnerability_data: sast_finding.raw_metadata - ) - ] - end + context 'when vulnerability_finding_signatures feature flag is disabled' do + let!(:feedback) do + [ + create( + :vulnerability_feedback, + :dismissal, + :dependency_scanning, + project: project, + pipeline: pipeline, + project_fingerprint: ds_finding.project_fingerprint, + vulnerability_data: ds_finding.raw_metadata, + finding_uuid: ds_finding.uuid + ), + create( + :vulnerability_feedback, + :dismissal, + :sast, + project: project, + pipeline: pipeline, + project_fingerprint: sast_finding.project_fingerprint, + vulnerability_data: sast_finding.raw_metadata, + finding_uuid: sast_finding.uuid + ) + ] + end - context 'when unscoped' do - subject { described_class.new(pipeline: pipeline).execute } + before do + stub_feature_flags(vulnerability_finding_signatures: false) + end + + context 'when unscoped' do + subject { described_class.new(pipeline: pipeline).execute } - it 'returns non-dismissed vulnerabilities' do - expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count - feedback.count) - expect(subject.findings.map(&:project_fingerprint)).not_to include(*feedback.map(&:project_fingerprint)) + it 'returns non-dismissed vulnerabilities' do + expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count - feedback.count) + expect(subject.findings.map(&:project_fingerprint)).not_to include(*feedback.map(&:project_fingerprint)) + end end - end - context 'when `dismissed`' do - subject { described_class.new(pipeline: pipeline, params: { report_type: %w[dependency_scanning], scope: 'dismissed' } ).execute } + context 'when `dismissed`' do + subject { described_class.new(pipeline: pipeline, params: { report_type: %w[dependency_scanning], scope: 'dismissed' } ).execute } - it 'returns non-dismissed vulnerabilities' do - expect(subject.findings.count).to eq(ds_count - 1) - expect(subject.findings.map(&:project_fingerprint)).not_to include(ds_finding.project_fingerprint) + it 'returns non-dismissed vulnerabilities' do + expect(subject.findings.count).to eq(ds_count - 1) + expect(subject.findings.map(&:project_fingerprint)).not_to include(ds_finding.project_fingerprint) + end + end + + context 'when `all`' do + let(:params) { { report_type: %w[sast dast container_scanning dependency_scanning], scope: 'all' } } + + it 'returns all vulnerabilities' do + expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count) + end end end - context 'when `all`' do - let(:params) { { report_type: %w[sast dast container_scanning dependency_scanning], scope: 'all' } } + context 'when vulnerability_finding_signatures feature flag is enabled' do + let!(:feedback) do + [ + create( + :vulnerability_feedback, + :dismissal, + :sast, + project: project, + pipeline: pipeline, + project_fingerprint: sast_finding.project_fingerprint, + vulnerability_data: sast_finding.raw_metadata, + finding_uuid: sast_finding.uuid + ) + ] + end - it 'returns all vulnerabilities' do - expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count) + before do + stub_feature_flags(vulnerability_finding_signatures: true) + end + + context 'when unscoped' do + subject { described_class.new(pipeline: pipeline).execute } + + it 'returns non-dismissed vulnerabilities' do + expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count - feedback.count) + expect(subject.findings.map(&:project_fingerprint)).not_to include(*feedback.map(&:project_fingerprint)) + end + end + + context 'when `dismissed`' do + subject { described_class.new(pipeline: pipeline, params: { report_type: %w[sast], scope: 'dismissed' } ).execute } + + it 'returns non-dismissed vulnerabilities' do + expect(subject.findings.count).to eq(sast_count - 1) + expect(subject.findings.map(&:project_fingerprint)).not_to include(sast_finding.project_fingerprint) + end + end + + context 'when `all`' do + let(:params) { { report_type: %w[sast dast container_scanning dependency_scanning], scope: 'all' } } + + it 'returns all vulnerabilities' do + expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count) + end end end end diff --git a/ee/spec/lib/gitlab/ci/parsers/security/common_spec.rb b/ee/spec/lib/gitlab/ci/parsers/security/common_spec.rb index fb39ee92becfaea99e3f4ef1110ccf0414abd370..00cadb21b852411e342e137cee683cead0889558 100644 --- a/ee/spec/lib/gitlab/ci/parsers/security/common_spec.rb +++ b/ee/spec/lib/gitlab/ci/parsers/security/common_spec.rb @@ -4,212 +4,195 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do describe '#parse!' do - let_it_be(:pipeline) { create(:ci_pipeline) } - - let(:artifact) { build(:ee_ci_job_artifact, :common_security_report) } - let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) } - let(:location) { ::Gitlab::Ci::Reports::Security::Locations::DependencyScanning.new(file_path: 'yarn/yarn.lock', package_version: 'v2', package_name: 'saml2') } - let(:tracking_data) do - { - 'type' => 'source', - 'items' => [ - 'signatures' => [ - { 'algorithm' => 'hash', 'value' => 'hash_value' }, - { 'algorithm' => 'location', 'value' => 'location_value' }, - { 'algorithm' => 'scope_offset', 'value' => 'scope_offset_value' } - ] - ] - } - end + where(vulnerability_finding_signatures_enabled: [true, false]) + with_them do + let_it_be(:pipeline) { create(:ci_pipeline) } + + let(:artifact) { build(:ee_ci_job_artifact, :common_security_report) } + let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) } + let(:location) { ::Gitlab::Ci::Reports::Security::Locations::DependencyScanning.new(file_path: 'yarn/yarn.lock', package_version: 'v2', package_name: 'saml2') } + let(:tracking_data) { nil } + + before do + allow_next_instance_of(described_class) do |parser| + allow(parser).to receive(:create_location).and_return(location) + allow(parser).to receive(:tracking_data).and_return(tracking_data) + end - before do - allow_next_instance_of(described_class) do |parser| - allow(parser).to receive(:create_location).and_return(location) - allow(parser).to receive(:tracking_data).and_return(tracking_data) + artifact.each_blob { |blob| described_class.parse!(blob, report, vulnerability_finding_signatures_enabled) } end - artifact.each_blob { |blob| described_class.parse!(blob, report) } - end - - describe 'parsing finding.name' do - let(:artifact) { build(:ee_ci_job_artifact, :common_security_report_with_blank_names) } - context 'when message is provided' do - it 'sets message from the report as a finding name' do - vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1020' } - expected_name = Gitlab::Json.parse(vulnerability.raw_metadata)['message'] + describe 'parsing finding.name' do + let(:artifact) { build(:ee_ci_job_artifact, :common_security_report_with_blank_names) } - expect(vulnerability.name).to eq(expected_name) - end - end - - context 'when message is not provided' do - context 'and name is provided' do - it 'sets name from the report as a name' do - vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1030' } - expected_name = Gitlab::Json.parse(vulnerability.raw_metadata)['name'] + context 'when message is provided' do + it 'sets message from the report as a finding name' do + vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1020' } + expected_name = Gitlab::Json.parse(vulnerability.raw_metadata)['message'] expect(vulnerability.name).to eq(expected_name) end end - context 'and name is not provided' do - context 'when CVE identifier exists' do - it 'combines identifier with location to create name' do - vulnerability = report.findings.find { |x| x.compare_key == 'CVE-2017-11429' } - expect(vulnerability.name).to eq("CVE-2017-11429 in yarn.lock") + context 'when message is not provided' do + context 'and name is provided' do + it 'sets name from the report as a name' do + vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1030' } + expected_name = Gitlab::Json.parse(vulnerability.raw_metadata)['name'] + + expect(vulnerability.name).to eq(expected_name) end end - context 'when CWE identifier exists' do - it 'combines identifier with location to create name' do - vulnerability = report.findings.find { |x| x.compare_key == 'CWE-2017-11429' } - expect(vulnerability.name).to eq("CWE-2017-11429 in yarn.lock") + context 'and name is not provided' do + context 'when CVE identifier exists' do + it 'combines identifier with location to create name' do + vulnerability = report.findings.find { |x| x.compare_key == 'CVE-2017-11429' } + expect(vulnerability.name).to eq("CVE-2017-11429 in yarn.lock") + end + end + + context 'when CWE identifier exists' do + it 'combines identifier with location to create name' do + vulnerability = report.findings.find { |x| x.compare_key == 'CWE-2017-11429' } + expect(vulnerability.name).to eq("CWE-2017-11429 in yarn.lock") + end end - end - context 'when neither CVE nor CWE identifier exist' do - it 'combines identifier with location to create name' do - vulnerability = report.findings.find { |x| x.compare_key == 'OTHER-2017-11429' } - expect(vulnerability.name).to eq("other-2017-11429 in yarn.lock") + context 'when neither CVE nor CWE identifier exist' do + it 'combines identifier with location to create name' do + vulnerability = report.findings.find { |x| x.compare_key == 'OTHER-2017-11429' } + expect(vulnerability.name).to eq("other-2017-11429 in yarn.lock") + end end end end end - end - describe 'parsing finding.details' do - context 'when details are provided' do - it 'sets details from the report' do - vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1020' } - expected_details = Gitlab::Json.parse(vulnerability.raw_metadata)['details'] + describe 'parsing finding.details' do + context 'when details are provided' do + it 'sets details from the report' do + vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1020' } + expected_details = Gitlab::Json.parse(vulnerability.raw_metadata)['details'] - expect(vulnerability.details).to eq(expected_details) + expect(vulnerability.details).to eq(expected_details) + end end - end - context 'when details are not provided' do - it 'sets empty hash' do - vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1030' } - expect(vulnerability.details).to eq({}) + context 'when details are not provided' do + it 'sets empty hash' do + vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1030' } + expect(vulnerability.details).to eq({}) + end end end - end - describe 'parsing remediations' do - let(:expected_remediation) { create(:ci_reports_security_remediation, diff: '') } + describe 'parsing remediations' do + let(:expected_remediation) { create(:ci_reports_security_remediation, diff: '') } - it 'finds remediation with same cve' do - vulnerability = report.findings.find { |x| x.compare_key == "CVE-1020" } - remediation = { 'fixes' => [{ 'cve' => 'CVE-1020' }], 'summary' => '', 'diff' => '' } + it 'finds remediation with same cve' do + vulnerability = report.findings.find { |x| x.compare_key == "CVE-1020" } + remediation = { 'fixes' => [{ 'cve' => 'CVE-1020' }], 'summary' => '', 'diff' => '' } - expect(Gitlab::Json.parse(vulnerability.raw_metadata).dig('remediations').first).to include remediation - expect(vulnerability.remediations.first.checksum).to eq(expected_remediation.checksum) - end + expect(Gitlab::Json.parse(vulnerability.raw_metadata).dig('remediations').first).to include remediation + expect(vulnerability.remediations.first.checksum).to eq(expected_remediation.checksum) + end - it 'finds remediation with same id' do - vulnerability = report.findings.find { |x| x.compare_key == "CVE-1030" } - remediation = { 'fixes' => [{ 'cve' => 'CVE', 'id' => 'bb2fbeb1b71ea360ce3f86f001d4e84823c3ffe1a1f7d41ba7466b14cfa953d3' }], 'summary' => '', 'diff' => '' } + it 'finds remediation with same id' do + vulnerability = report.findings.find { |x| x.compare_key == "CVE-1030" } + remediation = { 'fixes' => [{ 'cve' => 'CVE', 'id' => 'bb2fbeb1b71ea360ce3f86f001d4e84823c3ffe1a1f7d41ba7466b14cfa953d3' }], 'summary' => '', 'diff' => '' } - expect(Gitlab::Json.parse(vulnerability.raw_metadata).dig('remediations').first).to include remediation - expect(vulnerability.remediations.first.checksum).to eq(expected_remediation.checksum) - end + expect(Gitlab::Json.parse(vulnerability.raw_metadata).dig('remediations').first).to include remediation + expect(vulnerability.remediations.first.checksum).to eq(expected_remediation.checksum) + end - it 'does not find remediation with different id' do - fix_with_id = { - "fixes": [ - { - "id": "2134", - "cve": "CVE-1" - } - ], - "summary": "", - "diff": "" - } + it 'does not find remediation with different id' do + fix_with_id = { + "fixes": [ + { + "id": "2134", + "cve": "CVE-1" + } + ], + "summary": "", + "diff": "" + } - report.findings.map do |vulnerability| - expect(Gitlab::Json.parse(vulnerability.raw_metadata).dig('remediations')).not_to include(fix_with_id) + report.findings.map do |vulnerability| + expect(Gitlab::Json.parse(vulnerability.raw_metadata).dig('remediations')).not_to include(fix_with_id) + end end end - end - describe 'parsing scanners' do - subject(:scanner) { report.findings.first.scanner } + describe 'parsing scanners' do + subject(:scanner) { report.findings.first.scanner } - context 'when vendor is not missing in scanner' do - it 'returns scanner with parsed vendor value' do - expect(scanner.vendor).to eq('GitLab') + context 'when vendor is not missing in scanner' do + it 'returns scanner with parsed vendor value' do + expect(scanner.vendor).to eq('GitLab') + end end end - end - describe 'parsing scan' do - it 'returns scan object for each finding' do - scans = report.findings.map(&:scan) + describe 'parsing scan' do + it 'returns scan object for each finding' do + scans = report.findings.map(&:scan) - expect(scans.map(&:status).all?('success')).to be(true) - expect(scans.map(&:type).all?('dependency_scanning')).to be(true) - expect(scans.map(&:start_time).all?('placeholder-value')).to be(true) - expect(scans.map(&:end_time).all?('placeholder-value')).to be(true) - expect(scans.size).to eq(3) - expect(scans.first).to be_a(::Gitlab::Ci::Reports::Security::Scan) - end + expect(scans.map(&:status).all?('success')).to be(true) + expect(scans.map(&:type).all?('dependency_scanning')).to be(true) + expect(scans.map(&:start_time).all?('placeholder-value')).to be(true) + expect(scans.map(&:end_time).all?('placeholder-value')).to be(true) + expect(scans.size).to eq(3) + expect(scans.first).to be_a(::Gitlab::Ci::Reports::Security::Scan) + end - it 'returns nil when scan is not a hash' do - empty_report = Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) - described_class.parse!({}.to_json, empty_report) + it 'returns nil when scan is not a hash' do + empty_report = Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) + described_class.parse!({}.to_json, empty_report) - expect(empty_report.scan).to be(nil) + expect(empty_report.scan).to be(nil) + end end - end - describe 'parsing links' do - it 'returns links object for each finding', :aggregate_failures do - links = report.findings.flat_map(&:links) + describe 'parsing links' do + it 'returns links object for each finding', :aggregate_failures do + links = report.findings.flat_map(&:links) - expect(links.map(&:url)).to match_array(['https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1020', 'https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1030']) - expect(links.map(&:name)).to match_array([nil, 'CVE-1030']) - expect(links.size).to eq(2) - expect(links.first).to be_a(::Gitlab::Ci::Reports::Security::Link) - end - end - - describe 'setting the uuid' do - let(:finding_uuids) { report.findings.map(&:uuid) } - let(:uuid_1) do - Security::VulnerabilityUUID.generate( - report_type: "dependency_scanning", - primary_identifier_fingerprint: "4ff8184cd18485b6e85d5b101e341b12eacd1b3b", - location_fingerprint: "33dc9f32c77dde16d39c69d3f78f27ca3114a7c5", - project_id: pipeline.project_id - ) + expect(links.map(&:url)).to match_array(['https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1020', 'https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1030']) + expect(links.map(&:name)).to match_array([nil, 'CVE-1030']) + expect(links.size).to eq(2) + expect(links.first).to be_a(::Gitlab::Ci::Reports::Security::Link) + end end - let(:uuid_2) do - Security::VulnerabilityUUID.generate( - report_type: "dependency_scanning", - primary_identifier_fingerprint: "d55f9e66e79882ae63af9fd55cc822ab75307e31", - location_fingerprint: "33dc9f32c77dde16d39c69d3f78f27ca3114a7c5", - project_id: pipeline.project_id - ) - end + describe 'setting the uuid' do + let(:finding_uuids) { report.findings.map(&:uuid) } + let(:uuid_1) do + Security::VulnerabilityUUID.generate( + report_type: "dependency_scanning", + primary_identifier_fingerprint: "4ff8184cd18485b6e85d5b101e341b12eacd1b3b", + location_fingerprint: "33dc9f32c77dde16d39c69d3f78f27ca3114a7c5", + project_id: pipeline.project_id + ) + end - let(:expected_uuids) { [uuid_1, uuid_2, nil] } + let(:uuid_2) do + Security::VulnerabilityUUID.generate( + report_type: "dependency_scanning", + primary_identifier_fingerprint: "d55f9e66e79882ae63af9fd55cc822ab75307e31", + location_fingerprint: "33dc9f32c77dde16d39c69d3f78f27ca3114a7c5", + project_id: pipeline.project_id + ) + end - it 'sets the UUIDv5 for findings', :aggregate_failures do - expect(finding_uuids).to match_array(expected_uuids) - end - end + let(:expected_uuids) { [uuid_1, uuid_2, nil] } - describe 'parsing signature' do - context 'with valid signature information' do - it 'creates signatures for each algorithm' do - finding = report.findings.first - expect(finding.signatures.size).to eq(3) - expect(finding.signatures.map(&:algorithm_type).to_set).to eq(Set['hash', 'location', 'scope_offset']) + it 'sets the UUIDv5 for findings', :aggregate_failures do + expect(finding_uuids).to match_array(expected_uuids) end end - context 'with invalid signature information' do + describe 'parsing tracking' do let(:tracking_data) do { 'type' => 'source', @@ -217,16 +200,66 @@ 'signatures' => [ { 'algorithm' => 'hash', 'value' => 'hash_value' }, { 'algorithm' => 'location', 'value' => 'location_value' }, - { 'algorithm' => 'INVALID', 'value' => 'scope_offset_value' } + { 'algorithm' => 'scope_offset', 'value' => 'scope_offset_value' } ] ] } end - it 'ignores invalid algorithm types' do - finding = report.findings.first - expect(finding.signatures.size).to eq(2) - expect(finding.signatures.map(&:algorithm_type).to_set).to eq(Set['hash', 'location']) + context 'with valid tracking information' do + it 'creates signatures for each algorithm' do + finding = report.findings.first + expect(finding.signatures.size).to eq(3) + expect(finding.signatures.map(&:algorithm_type).to_set).to eq(Set['hash', 'location', 'scope_offset']) + end + end + + context 'with invalid tracking information' do + let(:tracking_data) do + { + 'type' => 'source', + 'items' => [ + 'signatures' => [ + { 'algorithm' => 'hash', 'value' => 'hash_value' }, + { 'algorithm' => 'location', 'value' => 'location_value' }, + { 'algorithm' => 'INVALID', 'value' => 'scope_offset_value' } + ] + ] + } + end + + it 'ignores invalid algorithm types' do + finding = report.findings.first + expect(finding.signatures.size).to eq(2) + expect(finding.signatures.map(&:algorithm_type).to_set).to eq(Set['hash', 'location']) + end + end + + context 'with valid tracking information' do + it 'creates signatures for each signature algorithm' do + finding = report.findings.first + expect(finding.signatures.size).to eq(3) + expect(finding.signatures.map(&:algorithm_type)).to eq(%w[hash location scope_offset]) + + signatures = finding.signatures.index_by(&:algorithm_type) + expected_values = tracking_data['items'][0]['signatures'].index_by { |x| x['algorithm'] } + expect(signatures['hash'].signature_value).to eq(expected_values['hash']['value']) + expect(signatures['location'].signature_value).to eq(expected_values['location']['value']) + expect(signatures['scope_offset'].signature_value).to eq(expected_values['scope_offset']['value']) + end + + it 'sets the uuid according to the higest priority signature' do + finding = report.findings.first + highest_signature = finding.signatures.max_by(&:priority) + + identifiers = if vulnerability_finding_signatures_enabled + "#{finding.report_type}-#{finding.primary_identifier.fingerprint}-#{highest_signature.signature_hex}-#{report.project_id}" + else + "#{finding.report_type}-#{finding.primary_identifier.fingerprint}-#{finding.location.fingerprint}-#{report.project_id}" + end + + expect(finding.uuid).to eq(Gitlab::UUID.v5(identifiers)) + end end end end diff --git a/ee/spec/lib/gitlab/ci/reports/security/finding_signature_spec.rb b/ee/spec/lib/gitlab/ci/reports/security/finding_signature_spec.rb index f0153b9b44ce8dfbca59d407915e0f06f39c10c4..23e6b40a039fdcef7494f37ae2ae2944f072f676 100644 --- a/ee/spec/lib/gitlab/ci/reports/security/finding_signature_spec.rb +++ b/ee/spec/lib/gitlab/ci/reports/security/finding_signature_spec.rb @@ -18,15 +18,12 @@ expect(subject.algorithm_type).to eq(params[:algorithm_type]) expect(subject.signature_value).to eq(params[:signature_value]) end - end - end - describe '#to_h' do - it 'returns a hash representation of the signature' do - expect(subject.to_h).to eq( - algorithm_type: params[:algorithm_type], - signature_sha: Digest::SHA1.digest(params[:signature_value]) - ) + describe '#valid?' do + it 'returns true' do + expect(subject.valid?).to eq(true) + end + end end end @@ -50,4 +47,13 @@ end end end + + describe '#to_hash' do + it 'returns a hash representation of the signature' do + expect(subject.to_hash).to eq( + algorithm_type: params[:algorithm_type], + signature_sha: Digest::SHA1.digest(params[:signature_value]) + ) + end + end end diff --git a/ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb b/ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb index 55262ae24de562b04e6c6646ae9e03314f2f21c4..25134aa9b42af87e371d43cd401fb9091e1fa38e 100644 --- a/ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb +++ b/ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb @@ -137,7 +137,8 @@ scan: occurrence.scan, severity: occurrence.severity, uuid: occurrence.uuid, - details: occurrence.details + details: occurrence.details, + signatures: [] }) end end @@ -197,87 +198,98 @@ end describe '#eql?' do - let(:identifier) { build(:ci_reports_security_identifier) } - let(:location) { build(:ci_reports_security_locations_sast) } - let(:finding) { build(:ci_reports_security_finding, severity: 'low', report_type: :sast, identifiers: [identifier], location: location) } - - let(:report_type) { :secret_detection } - let(:identifier_external_id) { 'foo' } - let(:location_start_line) { 0 } - let(:other_identifier) { build(:ci_reports_security_identifier, external_id: identifier_external_id) } - let(:other_location) { build(:ci_reports_security_locations_sast, start_line: location_start_line) } - let(:other_finding) do - build(:ci_reports_security_finding, - severity: 'low', - report_type: report_type, - identifiers: [other_identifier], - location: other_location) - end + where(vulnerability_finding_signatures_enabled: [true, false]) + with_them do + let(:identifier) { build(:ci_reports_security_identifier) } + let(:location) { build(:ci_reports_security_locations_sast) } + let(:finding) { build(:ci_reports_security_finding, severity: 'low', report_type: :sast, identifiers: [identifier], location: location, vulnerability_finding_signatures_enabled: vulnerability_finding_signatures_enabled) } + + let(:report_type) { :secret_detection } + let(:identifier_external_id) { 'foo' } + let(:location_start_line) { 0 } + let(:other_identifier) { build(:ci_reports_security_identifier, external_id: identifier_external_id) } + let(:other_location) { build(:ci_reports_security_locations_sast, start_line: location_start_line) } + let(:other_finding) do + build(:ci_reports_security_finding, + severity: 'low', + report_type: report_type, + identifiers: [other_identifier], + location: other_location, + vulnerability_finding_signatures_enabled: vulnerability_finding_signatures_enabled) + end - subject { finding.eql?(other_finding) } + let(:signature) { ::Gitlab::Ci::Reports::Security::FindingSignature.new(algorithm_type: 'location', signature_value: 'value1') } - context 'when the primary_identifier is nil' do - let(:identifier) { nil } + subject { finding.eql?(other_finding) } - it 'does not raise an exception' do - expect { subject }.not_to raise_error + context 'when the primary_identifier is nil' do + let(:identifier) { nil } + + it 'does not raise an exception' do + expect { subject }.not_to raise_error + end end - end - context 'when the other finding has same `report_type`' do - let(:report_type) { :sast } + context 'when the other finding has same `report_type`' do + let(:report_type) { :sast } - context 'when the other finding has same primary identifier fingerprint' do - let(:identifier_external_id) { identifier.external_id } + context 'when the other finding has same primary identifier fingerprint' do + let(:identifier_external_id) { identifier.external_id } - context 'when the other finding has same location fingerprint' do - let(:location_start_line) { location.start_line } + context 'when the other finding has same location signature' do + before do + finding.signatures << signature + other_finding.signatures << signature + end - it { is_expected.to be(true) } - end + let(:location_start_line) { location.start_line } - context 'when the other finding does not have same location fingerprint' do - it { is_expected.to be(false) } + it { is_expected.to be(true) } + end + + context 'when the other finding does not have same location signature' do + it { is_expected.to be(false) } + end end - end - context 'when the other finding does not have same primary identifier fingerprint' do - context 'when the other finding has same location fingerprint' do - let(:location_start_line) { location.start_line } + context 'when the other finding does not have same primary identifier fingerprint' do + context 'when the other finding has same location signature' do + let(:location_start_line) { location.start_line } - it { is_expected.to be(false) } - end + it { is_expected.to be(false) } + end - context 'when the other finding does not have same location fingerprint' do - it { is_expected.to be(false) } + context 'when the other finding does not have same location signature' do + it { is_expected.to be(false) } + end end end - end - context 'when the other finding does not have same `report_type`' do - context 'when the other finding has same primary identifier fingerprint' do - let(:identifier_external_id) { identifier.external_id } + context 'when the other finding does not have same `report_type`' do + context 'when the other finding has same primary identifier fingerprint' do + let(:identifier_external_id) { identifier.external_id } - context 'when the other finding has same location fingerprint' do - let(:location_start_line) { location.start_line } + context 'when the other finding has same location signature' do + let(:location_start_line) { location.start_line } - it { is_expected.to be(false) } - end + it { is_expected.to be(false) } + end - context 'when the other finding does not have same location fingerprint' do - it { is_expected.to be(false) } + context 'when the other finding does not have same location signature' do + it { is_expected.to be(false) } + end end - end - context 'when the other finding does not have same primary identifier fingerprint' do - context 'when the other finding has same location fingerprint' do - let(:location_start_line) { location.start_line } + context 'when the other finding does not have same primary identifier fingerprint' do + context 'when the other finding has same location signature' do + let(:location_start_line) { location.start_line } - it { is_expected.to be(false) } - end + it { is_expected.to be(false) } + end - context 'when the other finding does not have same location fingerprint' do - it { is_expected.to be(false) } + context 'when the other finding does not have same location signature' do + it { is_expected.to be(false) } + end end end end @@ -345,4 +357,55 @@ it { is_expected.to match_array(expected_keys) } end + + describe '#hash' do + let(:scanner) { build(:ci_reports_security_scanner) } + let(:identifiers) { [build(:ci_reports_security_identifier)] } + let(:location) { build(:ci_reports_security_locations_sast) } + let(:uuid) { SecureRandom.uuid } + + context 'with vulnerability_finding_signatures enabled' do + let(:finding) do + build(:ci_reports_security_finding, + scanner: scanner, + identifiers: identifiers, + location: location, + uuid: uuid, + compare_key: '', + vulnerability_finding_signatures_enabled: true) + end + + let(:low_priority_signature) { ::Gitlab::Ci::Reports::Security::FindingSignature.new(algorithm_type: 'location', signature_value: 'value1') } + let(:high_priority_signature) { ::Gitlab::Ci::Reports::Security::FindingSignature.new(algorithm_type: 'scope_offset', signature_value: 'value2') } + + it 'returns the expected hash with no signatures' do + expect(finding.signatures.length).to eq(0) + expect(finding.hash).to eq(finding.report_type.hash ^ finding.location.fingerprint.hash ^ finding.primary_identifier_fingerprint.hash) + end + + it 'returns the expected hash with signatures' do + finding.signatures << low_priority_signature + finding.signatures << high_priority_signature + + expect(finding.signatures.length).to eq(2) + expect(finding.hash).to eq(finding.report_type.hash ^ high_priority_signature.signature_hex.hash ^ finding.primary_identifier_fingerprint.hash) + end + end + + context 'without vulnerability_finding_signatures enabled' do + let(:finding) do + build(:ci_reports_security_finding, + scanner: scanner, + identifiers: identifiers, + location: location, + uuid: uuid, + compare_key: '', + vulnerability_finding_signatures_enabled: false) + end + + it 'returns the expected hash' do + expect(finding.hash).to eq(finding.report_type.hash ^ finding.location.fingerprint.hash ^ finding.primary_identifier_fingerprint.hash) + end + end + end end diff --git a/ee/spec/lib/gitlab/ci/reports/security/vulnerability_reports_comparer_spec.rb b/ee/spec/lib/gitlab/ci/reports/security/vulnerability_reports_comparer_spec.rb index 6f8961b63a6f65c8e6728a3c37dacb4e3073714d..6c69d1a253a13fbeaa680c711eb56b353c37b890 100644 --- a/ee/spec/lib/gitlab/ci/reports/security/vulnerability_reports_comparer_spec.rb +++ b/ee/spec/lib/gitlab/ci/reports/security/vulnerability_reports_comparer_spec.rb @@ -8,128 +8,133 @@ let(:base_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '123', confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: ::Enums::Vulnerability.severity_levels[:critical]) } let(:base_report) { build(:ci_reports_security_aggregated_reports, findings: [base_vulnerability])} - let(:head_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '123', confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: ::Enums::Vulnerability.severity_levels[:critical]) } + let(:head_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: base_vulnerability.location_fingerprint, confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: ::Enums::Vulnerability.severity_levels[:critical], uuid: base_vulnerability.uuid) } let(:head_report) { build(:ci_reports_security_aggregated_reports, findings: [head_vulnerability])} - before do - allow(base_vulnerability).to receive(:location).and_return({}) - allow(head_vulnerability).to receive(:location).and_return({}) - end - subject { described_class.new(base_report, head_report) } - describe '#base_report_out_of_date' do - context 'no base report' do - let(:base_report) { build(:ci_reports_security_aggregated_reports, reports: [], findings: [])} + where(vulnerability_finding_signatures_enabled: [true, false]) - it 'is not out of date' do - expect(subject.base_report_out_of_date).to be false - end + with_them do + before do + allow(base_vulnerability).to receive(:location).and_return({}) + allow(head_vulnerability).to receive(:location).and_return({}) + stub_feature_flags(vulnerability_finding_signatures: vulnerability_finding_signatures_enabled) end - context 'base report older than one week' do - let(:report) { build(:ci_reports_security_report, created_at: 1.week.ago - 60.seconds) } - let(:base_report) { build(:ci_reports_security_aggregated_reports, reports: [report])} + describe '#base_report_out_of_date' do + context 'no base report' do + let(:base_report) { build(:ci_reports_security_aggregated_reports, reports: [], findings: [])} - it 'is not out of date' do - expect(subject.base_report_out_of_date).to be true + it 'is not out of date' do + expect(subject.base_report_out_of_date).to be false + end end - end - context 'base report less than one week old' do - let(:report) { build(:ci_reports_security_report, created_at: 1.week.ago + 60.seconds) } - let(:base_report) { build(:ci_reports_security_aggregated_reports, reports: [report])} + context 'base report older than one week' do + let(:report) { build(:ci_reports_security_report, created_at: 1.week.ago - 60.seconds) } + let(:base_report) { build(:ci_reports_security_aggregated_reports, reports: [report])} + + it 'is not out of date' do + expect(subject.base_report_out_of_date).to be true + end + end + + context 'base report less than one week old' do + let(:report) { build(:ci_reports_security_report, created_at: 1.week.ago + 60.seconds) } + let(:base_report) { build(:ci_reports_security_aggregated_reports, reports: [report])} - it 'is not out of date' do - expect(subject.base_report_out_of_date).to be false + it 'is not out of date' do + expect(subject.base_report_out_of_date).to be false + end end end - end - describe '#added' do - let(:vuln) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '888', confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: Enums::Vulnerability.severity_levels[:critical]) } - let(:low_vuln) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '888', confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: Enums::Vulnerability.severity_levels[:low]) } + describe '#added' do + let(:vuln) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '888', confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: Enums::Vulnerability.severity_levels[:critical]) } + let(:low_vuln) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '888', confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: Enums::Vulnerability.severity_levels[:low]) } - context 'with new vulnerability' do - let(:head_report) { build(:ci_reports_security_aggregated_reports, findings: [head_vulnerability, vuln])} + context 'with new vulnerability' do + let(:head_report) { build(:ci_reports_security_aggregated_reports, findings: [head_vulnerability, vuln])} - it 'points to source tree' do - expect(subject.added).to eq([vuln]) + it 'points to source tree' do + expect(subject.added).to eq([vuln]) + end end - end - context 'when comparing reports with different fingerprints' do - let(:base_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: 'A') } - let(:head_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: 'B') } - let(:head_report) { build(:ci_reports_security_aggregated_reports, findings: [head_vulnerability, vuln])} + context 'when comparing reports with different fingerprints' do + let(:base_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: 'A') } + let(:head_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: 'B') } + let(:head_report) { build(:ci_reports_security_aggregated_reports, findings: [head_vulnerability, vuln])} - it 'does not find any overlap' do - expect(subject.added).to eq([head_vulnerability, vuln]) + it 'does not find any overlap' do + expect(subject.added).to eq([head_vulnerability, vuln]) + end end - end - context 'order' do - let(:head_report) { build(:ci_reports_security_aggregated_reports, findings: [head_vulnerability, vuln, low_vuln])} + context 'order' do + let(:head_report) { build(:ci_reports_security_aggregated_reports, findings: [head_vulnerability, vuln, low_vuln])} - it 'does not change' do - expect(subject.added).to eq([vuln, low_vuln]) + it 'does not change' do + expect(subject.added).to eq([vuln, low_vuln]) + end end end - end - describe '#fixed' do - let(:vuln) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '888') } - let(:medium_vuln) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '888', confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: Enums::Vulnerability.severity_levels[:medium]) } + describe '#fixed' do + let(:vuln) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '888') } + let(:medium_vuln) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: '888', confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: Enums::Vulnerability.severity_levels[:medium], uuid: vuln.uuid) } - context 'with fixed vulnerability' do - let(:base_report) { build(:ci_reports_security_aggregated_reports, findings: [base_vulnerability, vuln])} + context 'with fixed vulnerability' do + let(:base_report) { build(:ci_reports_security_aggregated_reports, findings: [base_vulnerability, vuln])} - it 'points to base tree' do - expect(subject.fixed).to eq([vuln]) + it 'points to base tree' do + expect(subject.fixed).to eq([vuln]) + end end - end - context 'when comparing reports with different fingerprints' do - let(:base_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: 'A') } - let(:head_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: 'B') } - let(:base_report) { build(:ci_reports_security_aggregated_reports, findings: [base_vulnerability, vuln])} + context 'when comparing reports with different fingerprints' do + let(:base_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: 'A') } + let(:head_vulnerability) { build(:vulnerabilities_finding, report_type: :sast, identifiers: [identifier], location_fingerprint: 'B') } + let(:base_report) { build(:ci_reports_security_aggregated_reports, findings: [base_vulnerability, vuln])} - it 'does not find any overlap' do - expect(subject.fixed).to eq([base_vulnerability, vuln]) + it 'does not find any overlap' do + expect(subject.fixed).to eq([base_vulnerability, vuln]) + end end - end - context 'order' do - let(:base_report) { build(:ci_reports_security_aggregated_reports, findings: [vuln, medium_vuln, base_vulnerability])} + context 'order' do + let(:base_report) { build(:ci_reports_security_aggregated_reports, findings: [vuln, medium_vuln, base_vulnerability])} - it 'does not change' do - expect(subject.fixed).to eq([vuln, medium_vuln]) + it 'does not change' do + expect(subject.fixed).to eq([vuln, medium_vuln]) + end end end - end - describe 'with empty vulnerabilities' do - let(:empty_report) { build(:ci_reports_security_aggregated_reports, reports: [], findings: [])} + describe 'with empty vulnerabilities' do + let(:empty_report) { build(:ci_reports_security_aggregated_reports, reports: [], findings: [])} - it 'returns empty array when reports are not present' do - comparer = described_class.new(empty_report, empty_report) + it 'returns empty array when reports are not present' do + comparer = described_class.new(empty_report, empty_report) - expect(comparer.fixed).to eq([]) - expect(comparer.added).to eq([]) - end + expect(comparer.fixed).to eq([]) + expect(comparer.added).to eq([]) + end - it 'returns added vulnerability when base is empty and head is not empty' do - comparer = described_class.new(empty_report, head_report) + it 'returns added vulnerability when base is empty and head is not empty' do + comparer = described_class.new(empty_report, head_report) - expect(comparer.fixed).to eq([]) - expect(comparer.added).to eq([head_vulnerability]) - end + expect(comparer.fixed).to eq([]) + expect(comparer.added).to eq([head_vulnerability]) + end - it 'returns fixed vulnerability when head is empty and base is not empty' do - comparer = described_class.new(base_report, empty_report) + it 'returns fixed vulnerability when head is empty and base is not empty' do + comparer = described_class.new(base_report, empty_report) - expect(comparer.fixed).to eq([base_vulnerability]) - expect(comparer.added).to eq([]) + expect(comparer.fixed).to eq([base_vulnerability]) + expect(comparer.added).to eq([]) + end end end end diff --git a/ee/spec/models/concerns/vulnerability_finding_signature_helpers_spec.rb b/ee/spec/models/concerns/vulnerability_finding_signature_helpers_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..0a71699971e7971d35111b51586f70efe46f4256 --- /dev/null +++ b/ee/spec/models/concerns/vulnerability_finding_signature_helpers_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe VulnerabilityFindingSignatureHelpers do + let(:cls) do + Class.new do + include VulnerabilityFindingSignatureHelpers + attr_accessor :algorithm_type + + def initialize(algorithm_type) + @algorithm_type = algorithm_type + end + end + end + + describe '#priority' do + it 'returns numeric values of the priority string' do + expect(cls.new('scope_offset').priority).to eq(3) + expect(cls.new('location').priority).to eq(2) + expect(cls.new('hash').priority).to eq(1) + end + end + + describe '#self.priority' do + it 'returns the numeric value of the provided string' do + expect(cls.priority('scope_offset')).to eq(3) + expect(cls.priority('location')).to eq(2) + expect(cls.priority('hash')).to eq(1) + end + end +end diff --git a/ee/spec/models/vulnerabilities/finding_spec.rb b/ee/spec/models/vulnerabilities/finding_spec.rb index 9c09f1e24cdbf5dd3f394e22716dcf2256c3fcc8..5e893c354181b8399b7a860dd9d387d1f8fbc42a 100644 --- a/ee/spec/models/vulnerabilities/finding_spec.rb +++ b/ee/spec/models/vulnerabilities/finding_spec.rb @@ -7,638 +7,619 @@ it { is_expected.to define_enum_for(:report_type) } it { is_expected.to define_enum_for(:severity) } - describe 'associations' do - it { is_expected.to belong_to(:project) } - it { is_expected.to belong_to(:primary_identifier).class_name('Vulnerabilities::Identifier') } - it { is_expected.to belong_to(:scanner).class_name('Vulnerabilities::Scanner') } - it { is_expected.to belong_to(:vulnerability).inverse_of(:findings) } - it { is_expected.to have_many(:pipelines).class_name('Ci::Pipeline') } - it { is_expected.to have_many(:finding_pipelines).class_name('Vulnerabilities::FindingPipeline').with_foreign_key('occurrence_id') } - it { is_expected.to have_many(:identifiers).class_name('Vulnerabilities::Identifier') } - it { is_expected.to have_many(:finding_identifiers).class_name('Vulnerabilities::FindingIdentifier').with_foreign_key('occurrence_id') } - it { is_expected.to have_many(:finding_links).class_name('Vulnerabilities::FindingLink').with_foreign_key('vulnerability_occurrence_id') } - it { is_expected.to have_many(:finding_remediations).class_name('Vulnerabilities::FindingRemediation').with_foreign_key('vulnerability_occurrence_id') } - it { is_expected.to have_many(:remediations).through(:finding_remediations) } - it { is_expected.to have_many(:finding_evidences).class_name('Vulnerabilities::FindingEvidence').with_foreign_key('vulnerability_occurrence_id') } - end - - describe 'validations' do - let(:finding) { build(:vulnerabilities_finding) } - - it { is_expected.to validate_presence_of(:scanner) } - it { is_expected.to validate_presence_of(:project) } - it { is_expected.to validate_presence_of(:uuid) } - it { is_expected.to validate_presence_of(:project_fingerprint) } - it { is_expected.to validate_presence_of(:primary_identifier) } - it { is_expected.to validate_presence_of(:location_fingerprint) } - it { is_expected.to validate_presence_of(:name) } - it { is_expected.to validate_presence_of(:report_type) } - it { is_expected.to validate_presence_of(:metadata_version) } - it { is_expected.to validate_presence_of(:raw_metadata) } - 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 = {} - - expect(finding).to be_valid + where(vulnerability_finding_signatures_enabled: [true, false]) + with_them do + before do + stub_feature_flags(vulnerability_finding_signatures: vulnerability_finding_signatures_enabled) + end + + describe 'associations' do + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:primary_identifier).class_name('Vulnerabilities::Identifier') } + it { is_expected.to belong_to(:scanner).class_name('Vulnerabilities::Scanner') } + it { is_expected.to belong_to(:vulnerability).inverse_of(:findings) } + it { is_expected.to have_many(:pipelines).class_name('Ci::Pipeline') } + it { is_expected.to have_many(:finding_pipelines).class_name('Vulnerabilities::FindingPipeline').with_foreign_key('occurrence_id') } + it { is_expected.to have_many(:identifiers).class_name('Vulnerabilities::Identifier') } + it { is_expected.to have_many(:finding_identifiers).class_name('Vulnerabilities::FindingIdentifier').with_foreign_key('occurrence_id') } + it { is_expected.to have_many(:finding_links).class_name('Vulnerabilities::FindingLink').with_foreign_key('vulnerability_occurrence_id') } + it { is_expected.to have_many(:finding_remediations).class_name('Vulnerabilities::FindingRemediation').with_foreign_key('vulnerability_occurrence_id') } + it { is_expected.to have_many(:remediations).through(:finding_remediations) } + it { is_expected.to have_many(:finding_evidences).class_name('Vulnerabilities::FindingEvidence').with_foreign_key('vulnerability_occurrence_id') } + end + + describe 'validations' do + let(:finding) { build(:vulnerabilities_finding) } + + it { is_expected.to validate_presence_of(:scanner) } + it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_presence_of(:uuid) } + it { is_expected.to validate_presence_of(:project_fingerprint) } + it { is_expected.to validate_presence_of(:primary_identifier) } + it { is_expected.to validate_presence_of(:location_fingerprint) } + it { is_expected.to validate_presence_of(:name) } + it { is_expected.to validate_presence_of(:report_type) } + it { is_expected.to validate_presence_of(:metadata_version) } + it { is_expected.to validate_presence_of(:raw_metadata) } + 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 = {} + + expect(finding).to be_valid + end end - end - context 'when value for details field is invalid' do - it 'returns errors' do - finding.details = { invalid: 'data' } + context 'when value for details field is invalid' do + it 'returns errors' do + finding.details = { invalid: 'data' } - expect(finding).to be_invalid - expect(finding.errors.full_messages).to eq(["Details must be a valid json schema"]) + expect(finding).to be_invalid + expect(finding.errors.full_messages).to eq(["Details must be a valid json schema"]) + end end end - end - context 'database uniqueness' do - let(:finding) { create(:vulnerabilities_finding) } - let(:new_finding) { finding.dup.tap { |o| o.uuid = SecureRandom.uuid } } + context 'database uniqueness' do + let(:finding) { create(:vulnerabilities_finding) } + let(:new_finding) { finding.dup.tap { |o| o.cve = SecureRandom.uuid } } - it "when all index attributes are identical" do - expect { new_finding.save! }.to raise_error(ActiveRecord::RecordNotUnique) - end + it "when all index attributes are identical" do + expect { new_finding.save! }.to raise_error(ActiveRecord::RecordNotUnique) + end - describe 'when some parameters are changed' do - using RSpec::Parameterized::TableSyntax + describe 'when some parameters are changed' do + using RSpec::Parameterized::TableSyntax - # we use block to delay object creations - where(:key, :factory_name) do - :primary_identifier | :vulnerabilities_identifier - :scanner | :vulnerabilities_scanner - :project | :project - end + # we use block to delay object creations + where(:key, :factory_name) do + :primary_identifier | :vulnerabilities_identifier + :scanner | :vulnerabilities_scanner + :project | :project + end - with_them do - it "is valid" do - expect { new_finding.update!({ key => create(factory_name) }) }.not_to raise_error + with_them do + it "is valid" do + expect { new_finding.update!({ key => create(factory_name), 'uuid' => SecureRandom.uuid }) }.not_to raise_error + end end end end - end - context 'order' do - let!(:finding1) { create(:vulnerabilities_finding, confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: ::Enums::Vulnerability.severity_levels[:high]) } - let!(:finding2) { create(:vulnerabilities_finding, confidence: ::Enums::Vulnerability.confidence_levels[:medium], severity: ::Enums::Vulnerability.severity_levels[:critical]) } - let!(:finding3) { create(:vulnerabilities_finding, confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: ::Enums::Vulnerability.severity_levels[:critical]) } + context 'order' do + let!(:finding1) { create(:vulnerabilities_finding, confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: ::Enums::Vulnerability.severity_levels[:high]) } + let!(:finding2) { create(:vulnerabilities_finding, confidence: ::Enums::Vulnerability.confidence_levels[:medium], severity: ::Enums::Vulnerability.severity_levels[:critical]) } + let!(:finding3) { create(:vulnerabilities_finding, confidence: ::Enums::Vulnerability.confidence_levels[:high], severity: ::Enums::Vulnerability.severity_levels[:critical]) } - it 'orders by severity and confidence' do - expect(described_class.all.ordered).to eq([finding3, finding2, finding1]) + it 'orders by severity and confidence' do + expect(described_class.all.ordered).to eq([finding3, finding2, finding1]) + end end - end - describe '.report_type' do - let(:report_type) { :sast } + describe '.report_type' do + let(:report_type) { :sast } - subject { described_class.report_type(report_type) } + subject { described_class.report_type(report_type) } - context 'when finding has the corresponding report type' do - let!(:finding) { create(:vulnerabilities_finding, report_type: report_type) } + context 'when finding has the corresponding report type' do + let!(:finding) { create(:vulnerabilities_finding, report_type: report_type) } - it 'selects the finding' do - is_expected.to eq([finding]) + it 'selects the finding' do + is_expected.to eq([finding]) + end end - end - context 'when finding does not have security reports' do - let!(:finding) { create(:vulnerabilities_finding, report_type: :dependency_scanning) } + context 'when finding does not have security reports' do + let!(:finding) { create(:vulnerabilities_finding, report_type: :dependency_scanning) } - it 'does not select the finding' do - is_expected.to be_empty + it 'does not select the finding' do + is_expected.to be_empty + end end end - end - describe '.for_pipelines_with_sha' do - let(:project) { create(:project) } - let(:pipeline) { create(:ci_pipeline, :success, project: project) } + describe '.for_pipelines_with_sha' do + let(:project) { create(:project) } + let(:pipeline) { create(:ci_pipeline, :success, project: project) } - before do - create(:vulnerabilities_finding, pipelines: [pipeline], project: project) - end + before do + create(:vulnerabilities_finding, pipelines: [pipeline], project: project) + end - subject(:findings) { described_class.for_pipelines_with_sha([pipeline]) } + subject(:findings) { described_class.for_pipelines_with_sha([pipeline]) } - it 'sets the sha' do - expect(findings.first.sha).to eq(pipeline.sha) + it 'sets the sha' do + expect(findings.first.sha).to eq(pipeline.sha) + end end - end - describe '.by_report_types' do - let!(:vulnerability_sast) { create(:vulnerabilities_finding, report_type: :sast) } - let!(:vulnerability_secret_detection) { create(:vulnerabilities_finding, report_type: :secret_detection) } - let!(:vulnerability_dast) { create(:vulnerabilities_finding, report_type: :dast) } - let!(:vulnerability_depscan) { create(:vulnerabilities_finding, report_type: :dependency_scanning) } - let!(:vulnerability_covfuzz) { create(:vulnerabilities_finding, report_type: :coverage_fuzzing) } - let!(:vulnerability_apifuzz) { create(:vulnerabilities_finding, report_type: :api_fuzzing) } + describe '.by_report_types' do + let!(:vulnerability_sast) { create(:vulnerabilities_finding, report_type: :sast) } + let!(:vulnerability_secret_detection) { create(:vulnerabilities_finding, report_type: :secret_detection) } + let!(:vulnerability_dast) { create(:vulnerabilities_finding, report_type: :dast) } + let!(:vulnerability_depscan) { create(:vulnerabilities_finding, report_type: :dependency_scanning) } + let!(:vulnerability_covfuzz) { create(:vulnerabilities_finding, report_type: :coverage_fuzzing) } + let!(:vulnerability_apifuzz) { create(:vulnerabilities_finding, report_type: :api_fuzzing) } - subject { described_class.by_report_types(param) } + subject { described_class.by_report_types(param) } - context 'with one param' do - let(:param) { Vulnerabilities::Finding.report_types['sast'] } + context 'with one param' do + let(:param) { Vulnerabilities::Finding.report_types['sast'] } - it 'returns found record' do - is_expected.to contain_exactly(vulnerability_sast) + it 'returns found record' do + is_expected.to contain_exactly(vulnerability_sast) + end end - end - context 'with array of params' do - let(:param) do - [ - Vulnerabilities::Finding.report_types['dependency_scanning'], - Vulnerabilities::Finding.report_types['dast'], - Vulnerabilities::Finding.report_types['secret_detection'], - Vulnerabilities::Finding.report_types['coverage_fuzzing'], - Vulnerabilities::Finding.report_types['api_fuzzing'] - ] - end + context 'with array of params' do + let(:param) do + [ + Vulnerabilities::Finding.report_types['dependency_scanning'], + Vulnerabilities::Finding.report_types['dast'], + Vulnerabilities::Finding.report_types['secret_detection'], + Vulnerabilities::Finding.report_types['coverage_fuzzing'], + Vulnerabilities::Finding.report_types['api_fuzzing'] + ] + end - it 'returns found records' do - is_expected.to contain_exactly( - vulnerability_dast, - vulnerability_depscan, - vulnerability_secret_detection, - vulnerability_covfuzz, - vulnerability_apifuzz) + it 'returns found records' do + is_expected.to contain_exactly( + vulnerability_dast, + vulnerability_depscan, + vulnerability_secret_detection, + vulnerability_covfuzz, + vulnerability_apifuzz) + end end - end - context 'without found record' do - let(:param) { ::Enums::Vulnerability.report_types['container_scanning']} + context 'without found record' do + let(:param) { ::Enums::Vulnerability.report_types['container_scanning']} - it 'returns empty collection' do - is_expected.to be_empty + it 'returns empty collection' do + is_expected.to be_empty + end end end - end - describe '.by_projects' do - let!(:vulnerability1) { create(:vulnerabilities_finding) } - let!(:vulnerability2) { create(:vulnerabilities_finding) } + describe '.by_projects' do + let!(:vulnerability1) { create(:vulnerabilities_finding) } + let!(:vulnerability2) { create(:vulnerabilities_finding) } - subject { described_class.by_projects(param) } + subject { described_class.by_projects(param) } - context 'with found record' do - let(:param) { vulnerability1.project_id } + context 'with found record' do + let(:param) { vulnerability1.project_id } - it 'returns found record' do - is_expected.to contain_exactly(vulnerability1) + it 'returns found record' do + is_expected.to contain_exactly(vulnerability1) + end end end - end - describe '.by_scanners' do - context 'with found record' do - it 'returns found record' do - vulnerability1 = create(:vulnerabilities_finding) - create(:vulnerabilities_finding) - param = vulnerability1.scanner_id + describe '.by_scanners' do + context 'with found record' do + it 'returns found record' do + vulnerability1 = create(:vulnerabilities_finding) + create(:vulnerabilities_finding) + param = vulnerability1.scanner_id - result = described_class.by_scanners(param) + result = described_class.by_scanners(param) - expect(result).to contain_exactly(vulnerability1) + expect(result).to contain_exactly(vulnerability1) + end end end - end - describe '.by_severities' do - let!(:vulnerability_high) { create(:vulnerabilities_finding, severity: :high) } - let!(:vulnerability_low) { create(:vulnerabilities_finding, severity: :low) } + describe '.by_severities' do + let!(:vulnerability_high) { create(:vulnerabilities_finding, severity: :high) } + let!(:vulnerability_low) { create(:vulnerabilities_finding, severity: :low) } - subject { described_class.by_severities(param) } + subject { described_class.by_severities(param) } - context 'with one param' do - let(:param) { described_class.severities[:low] } + context 'with one param' do + let(:param) { described_class.severities[:low] } - it 'returns found record' do - is_expected.to contain_exactly(vulnerability_low) + it 'returns found record' do + is_expected.to contain_exactly(vulnerability_low) + end end - end - context 'without found record' do - let(:param) { described_class.severities[:unknown] } + context 'without found record' do + let(:param) { described_class.severities[:unknown] } - it 'returns empty collection' do - is_expected.to be_empty + it 'returns empty collection' do + is_expected.to be_empty + end end end - end - describe '.by_confidences' do - let!(:vulnerability_high) { create(:vulnerabilities_finding, confidence: :high) } - let!(:vulnerability_low) { create(:vulnerabilities_finding, confidence: :low) } + describe '.by_confidences' do + let!(:vulnerability_high) { create(:vulnerabilities_finding, confidence: :high) } + let!(:vulnerability_low) { create(:vulnerabilities_finding, confidence: :low) } - subject { described_class.by_confidences(param) } + subject { described_class.by_confidences(param) } - context 'with matching param' do - let(:param) { described_class.confidences[:low] } + context 'with matching param' do + let(:param) { described_class.confidences[:low] } - it 'returns found record' do - is_expected.to contain_exactly(vulnerability_low) + it 'returns found record' do + is_expected.to contain_exactly(vulnerability_low) + end end - end - context 'with non-matching param' do - let(:param) { described_class.confidences[:unknown] } + context 'with non-matching param' do + let(:param) { described_class.confidences[:unknown] } - it 'returns empty collection' do - is_expected.to be_empty + it 'returns empty collection' do + is_expected.to be_empty + end end end - end - describe '.counted_by_severity' do - let!(:high_vulnerabilities) { create_list(:vulnerabilities_finding, 3, severity: :high) } - let!(:medium_vulnerabilities) { create_list(:vulnerabilities_finding, 2, severity: :medium) } - let!(:low_vulnerabilities) { create_list(:vulnerabilities_finding, 1, severity: :low) } + describe '.counted_by_severity' do + let!(:high_vulnerabilities) { create_list(:vulnerabilities_finding, 3, severity: :high) } + let!(:medium_vulnerabilities) { create_list(:vulnerabilities_finding, 2, severity: :medium) } + let!(:low_vulnerabilities) { create_list(:vulnerabilities_finding, 1, severity: :low) } - subject { described_class.counted_by_severity } + subject { described_class.counted_by_severity } - it 'returns counts' do - is_expected.to eq({ 4 => 1, 5 => 2, 6 => 3 }) + it 'returns counts' do + is_expected.to eq({ 4 => 1, 5 => 2, 6 => 3 }) + end end - end - describe '.undismissed' do - let_it_be(:project) { create(:project) } - let_it_be(:project2) { create(:project) } + describe '.undismissed' do + let_it_be(:project) { create(:project) } + let_it_be(:project2) { create(:project) } - let!(:finding1) { create(:vulnerabilities_finding, project: project) } - let!(:finding2) { create(:vulnerabilities_finding, project: project, report_type: :dast) } - let!(:finding3) { create(:vulnerabilities_finding, project: project2) } + let!(:finding1) { create(:vulnerabilities_finding, project: project) } + let!(:finding2) { create(:vulnerabilities_finding, project: project, report_type: :dast) } + let!(:finding3) { create(:vulnerabilities_finding, project: project2) } - before do - create( - :vulnerability_feedback, - :dismissal, - project: finding1.project, - project_fingerprint: finding1.project_fingerprint - ) - create( - :vulnerability_feedback, - :dismissal, - project_fingerprint: finding2.project_fingerprint, - project: project2 - ) - create( - :vulnerability_feedback, - :dismissal, - category: :sast, - project_fingerprint: finding2.project_fingerprint, - project: finding2.project - ) - end - - it 'returns all non-dismissed findings' do - expect(described_class.undismissed).to contain_exactly(finding2, finding3) - end - - it 'returns non-dismissed findings for project' do - expect(project2.vulnerability_findings.undismissed).to contain_exactly(finding3) - end - end - - describe '.dismissed' do - let_it_be(:project) { create(:project) } - let_it_be(:project2) { create(:project) } + before do + create( + :vulnerability_feedback, + :dismissal, + project: finding1.project, + project_fingerprint: finding1.project_fingerprint + ) + create( + :vulnerability_feedback, + :dismissal, + project_fingerprint: finding2.project_fingerprint, + project: project2 + ) + create( + :vulnerability_feedback, + :dismissal, + category: :sast, + project_fingerprint: finding2.project_fingerprint, + project: finding2.project + ) + end - let!(:finding1) { create(:vulnerabilities_finding, project: project) } - let!(:finding2) { create(:vulnerabilities_finding, project: project, report_type: :dast) } - let!(:finding3) { create(:vulnerabilities_finding, project: project2) } + it 'returns all non-dismissed findings' do + expect(described_class.undismissed).to contain_exactly(finding2, finding3) + end - before do - create( - :vulnerability_feedback, - :dismissal, - project: finding1.project, - project_fingerprint: finding1.project_fingerprint - ) - create( - :vulnerability_feedback, - :dismissal, - project_fingerprint: finding2.project_fingerprint, - project: project2 - ) - create( - :vulnerability_feedback, - :dismissal, - category: :sast, - project_fingerprint: finding2.project_fingerprint, - project: finding2.project - ) - end - - it 'returns all dismissed findings' do - expect(described_class.dismissed).to contain_exactly(finding1) - end - - it 'returns dismissed findings for project' do - expect(project.vulnerability_findings.dismissed).to contain_exactly(finding1) + it 'returns non-dismissed findings for project' do + expect(project2.vulnerability_findings.undismissed).to contain_exactly(finding3) + end end - end - describe '.batch_count_by_project_and_severity' do - let(:pipeline) { create(:ci_pipeline, :success, project: project) } - let(:project) { create(:project) } + describe '.dismissed' do + let_it_be(:project) { create(:project) } + let_it_be(:project2) { create(:project) } - it 'fetches a vulnerability count for the given project and severity' do - create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :high) + let!(:finding1) { create(:vulnerabilities_finding, project: project) } + let!(:finding2) { create(:vulnerabilities_finding, project: project, report_type: :dast) } + let!(:finding3) { create(:vulnerabilities_finding, project: project2) } - count = described_class.batch_count_by_project_and_severity(project.id, 'high') + before do + create( + :vulnerability_feedback, + :dismissal, + project: finding1.project, + project_fingerprint: finding1.project_fingerprint + ) + create( + :vulnerability_feedback, + :dismissal, + project_fingerprint: finding2.project_fingerprint, + project: project2 + ) + create( + :vulnerability_feedback, + :dismissal, + category: :sast, + project_fingerprint: finding2.project_fingerprint, + project: finding2.project + ) + end - expect(count).to be(1) - end + it 'returns all dismissed findings' do + expect(described_class.dismissed).to contain_exactly(finding1) + end - it 'only returns vulnerabilities from the latest successful pipeline' do - old_pipeline = create(:ci_pipeline, :success, project: project) - latest_pipeline = create(:ci_pipeline, :success, project: project) - latest_failed_pipeline = create(:ci_pipeline, :failed, project: project) - create(:vulnerabilities_finding, pipelines: [old_pipeline], project: project, severity: :critical) - create( - :vulnerabilities_finding, - pipelines: [latest_failed_pipeline], - project: project, - severity: :critical - ) - create_list( - :vulnerabilities_finding, 2, - pipelines: [latest_pipeline], - project: project, - severity: :critical - ) + it 'returns dismissed findings for project' do + expect(project.vulnerability_findings.dismissed).to contain_exactly(finding1) + end + end - count = described_class.batch_count_by_project_and_severity(project.id, 'critical') + describe '.batch_count_by_project_and_severity' do + let(:pipeline) { create(:ci_pipeline, :success, project: project) } + let(:project) { create(:project) } - expect(count).to be(2) - end + it 'fetches a vulnerability count for the given project and severity' do + create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :high) - it 'returns 0 when there are no vulnerabilities for that severity level' do - count = described_class.batch_count_by_project_and_severity(project.id, 'high') + count = described_class.batch_count_by_project_and_severity(project.id, 'high') - expect(count).to be(0) - end + expect(count).to be(1) + end - it 'batch loads the counts' do - projects = create_list(:project, 2) + it 'only returns vulnerabilities from the latest successful pipeline' do + old_pipeline = create(:ci_pipeline, :success, project: project) + latest_pipeline = create(:ci_pipeline, :success, project: project) + latest_failed_pipeline = create(:ci_pipeline, :failed, project: project) + create(:vulnerabilities_finding, pipelines: [old_pipeline], project: project, severity: :critical) + create( + :vulnerabilities_finding, + pipelines: [latest_failed_pipeline], + project: project, + severity: :critical + ) + create_list( + :vulnerabilities_finding, 2, + pipelines: [latest_pipeline], + project: project, + severity: :critical + ) - projects.each do |project| - pipeline = create(:ci_pipeline, :success, project: project) + count = described_class.batch_count_by_project_and_severity(project.id, 'critical') - create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :high) - create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :low) + expect(count).to be(2) end - projects_and_severities = [ - [projects.first, 'high'], - [projects.first, 'low'], - [projects.second, 'high'], - [projects.second, 'low'] - ] + it 'returns 0 when there are no vulnerabilities for that severity level' do + count = described_class.batch_count_by_project_and_severity(project.id, 'high') - counts = projects_and_severities.map do |(project, severity)| - described_class.batch_count_by_project_and_severity(project.id, severity) + expect(count).to be(0) end - expect { expect(counts).to all(be 1) }.not_to exceed_query_limit(1) - end + it 'batch loads the counts' do + projects = create_list(:project, 2) - it 'does not include dismissed vulnerabilities in the counts' do - create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :high) - dismissed_vulnerability = create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :high) - create( - :vulnerability_feedback, - project: project, - project_fingerprint: dismissed_vulnerability.project_fingerprint, - feedback_type: :dismissal - ) + projects.each do |project| + pipeline = create(:ci_pipeline, :success, project: project) - count = described_class.batch_count_by_project_and_severity(project.id, 'high') + create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :high) + create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :low) + end - expect(count).to be(1) - end + projects_and_severities = [ + [projects.first, 'high'], + [projects.first, 'low'], + [projects.second, 'high'], + [projects.second, 'low'] + ] - it "does not overwrite one project's counts with another's" do - project1 = create(:project) - project2 = create(:project) - pipeline1 = create(:ci_pipeline, :success, project: project1) - pipeline2 = create(:ci_pipeline, :success, project: project2) - create(:vulnerabilities_finding, pipelines: [pipeline1], project: project1, severity: :critical) - create(:vulnerabilities_finding, pipelines: [pipeline2], project: project2, severity: :high) + counts = projects_and_severities.map do |(project, severity)| + described_class.batch_count_by_project_and_severity(project.id, severity) + end - project1_critical_count = described_class.batch_count_by_project_and_severity(project1.id, 'critical') - project1_high_count = described_class.batch_count_by_project_and_severity(project1.id, 'high') - project2_critical_count = described_class.batch_count_by_project_and_severity(project2.id, 'critical') - project2_high_count = described_class.batch_count_by_project_and_severity(project2.id, 'high') + expect { expect(counts).to all(be 1) }.not_to exceed_query_limit(1) + end - expect(project1_critical_count).to be(1) - expect(project1_high_count).to be(0) - expect(project2_critical_count).to be(0) - expect(project2_high_count).to be(1) - end - end + it 'does not include dismissed vulnerabilities in the counts' do + create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :high) + dismissed_vulnerability = create(:vulnerabilities_finding, pipelines: [pipeline], project: project, severity: :high) + create( + :vulnerability_feedback, + project: project, + project_fingerprint: dismissed_vulnerability.project_fingerprint, + feedback_type: :dismissal + ) - describe '#links' do - let_it_be(:finding, reload: true) do - create( - :vulnerabilities_finding, - raw_metadata: { - links: [{ url: 'https://raw.gitlab.com', name: 'raw_metadata_link' }] - }.to_json - ) - end + count = described_class.batch_count_by_project_and_severity(project.id, 'high') - subject(:links) { finding.links } + expect(count).to be(1) + end - context 'when there are no finding links' do - it 'returns links from raw_metadata' do - expect(links).to eq([{ 'url' => 'https://raw.gitlab.com', 'name' => 'raw_metadata_link' }]) + it "does not overwrite one project's counts with another's" do + project1 = create(:project) + project2 = create(:project) + pipeline1 = create(:ci_pipeline, :success, project: project1) + pipeline2 = create(:ci_pipeline, :success, project: project2) + create(:vulnerabilities_finding, pipelines: [pipeline1], project: project1, severity: :critical) + create(:vulnerabilities_finding, pipelines: [pipeline2], project: project2, severity: :high) + + project1_critical_count = described_class.batch_count_by_project_and_severity(project1.id, 'critical') + project1_high_count = described_class.batch_count_by_project_and_severity(project1.id, 'high') + project2_critical_count = described_class.batch_count_by_project_and_severity(project2.id, 'critical') + project2_high_count = described_class.batch_count_by_project_and_severity(project2.id, 'high') + + expect(project1_critical_count).to be(1) + expect(project1_high_count).to be(0) + expect(project2_critical_count).to be(0) + expect(project2_high_count).to be(1) end end - context 'when there are finding links assigned to given finding' do - let_it_be(:finding_link) { create(:finding_link, name: 'finding_link', url: 'https://link.gitlab.com', finding: finding) } - - it 'returns links from finding link' do - expect(links).to eq([{ 'url' => 'https://link.gitlab.com', 'name' => 'finding_link' }]) + describe '#links' do + let_it_be(:finding, reload: true) do + create( + :vulnerabilities_finding, + raw_metadata: { + links: [{ url: 'https://raw.gitlab.com', name: 'raw_metadata_link' }] + }.to_json + ) end - end - end - describe '#remediations' do - let_it_be(:project) { create_default(:project) } - let_it_be(:finding, refind: true) { create(:vulnerabilities_finding) } + subject(:links) { finding.links } - subject { finding.remediations } + context 'when there are no finding links' do + it 'returns links from raw_metadata' do + expect(links).to eq([{ 'url' => 'https://raw.gitlab.com', 'name' => 'raw_metadata_link' }]) + end + end - context 'when the finding has associated remediation records' do - let_it_be(:persisted_remediation) { create(:vulnerabilities_remediation, findings: [finding]) } - let_it_be(:remediation_hash) { { 'summary' => persisted_remediation.summary, 'diff' => persisted_remediation.diff } } + context 'when there are finding links assigned to given finding' do + let_it_be(:finding_link) { create(:finding_link, name: 'finding_link', url: 'https://link.gitlab.com', finding: finding) } - it { is_expected.to eq([remediation_hash]) } + it 'returns links from finding link' do + expect(links).to eq([{ 'url' => 'https://link.gitlab.com', 'name' => 'finding_link' }]) + end + end 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 } + describe '#remediations' do + let_it_be(:project) { create_default(:project) } + let_it_be(:finding, refind: true) { create(:vulnerabilities_finding) } - before do - raw_metadata = { remediations: [raw_remediation] }.to_json - finding.update!(raw_metadata: raw_metadata) - end + subject { finding.remediations } + + context 'when the finding has associated remediation records' do + 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([raw_remediation]) } + it { is_expected.to eq([remediation_hash]) } end - context 'when the finding does not have remediations in `raw_metadata`' do - before do - finding.update!(raw_metadata: {}.to_json) + 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 - it { is_expected.to be_nil } + context 'when the finding does not have remediations in `raw_metadata`' do + before do + finding.update!(raw_metadata: {}.to_json) + end + + it { is_expected.to be_nil } + end end end - end - - describe 'feedback' do - let_it_be(:project) { create(:project) } - let(:finding) do - create( - :vulnerabilities_finding, - report_type: :dependency_scanning, - project: project - ) - end + describe 'feedback' do + let_it_be(:project) { create(:project) } - describe '#issue_feedback' do - let(:issue) { create(:issue, project: project) } - let!(:issue_feedback) do + let(:finding) do create( - :vulnerability_feedback, - :dependency_scanning, - :issue, - issue: issue, - project: project, - project_fingerprint: finding.project_fingerprint + :vulnerabilities_finding, + report_type: :dependency_scanning, + project: project ) end - let(:vulnerability) { create(:vulnerability, findings: [finding]) } - let!(:issue_link) { create(:vulnerabilities_issue_link, vulnerability: vulnerability, issue: issue)} + describe '#issue_feedback' do + let(:issue) { create(:issue, project: project) } + let!(:issue_feedback) do + create( + :vulnerability_feedback, + :dependency_scanning, + :issue, + issue: issue, + project: project, + project_fingerprint: finding.project_fingerprint + ) + end - it 'returns associated feedback' do - feedback = finding.issue_feedback + let(:vulnerability) { create(:vulnerability, findings: [finding]) } + let!(:issue_link) { create(:vulnerabilities_issue_link, vulnerability: vulnerability, issue: issue)} - expect(feedback).to be_present - expect(feedback[:project_id]).to eq project.id - expect(feedback[:feedback_type]).to eq 'issue' - expect(feedback[:issue_id]).to eq issue.id - end + it 'returns associated feedback' do + feedback = finding.issue_feedback + + expect(feedback).to be_present + expect(feedback[:project_id]).to eq project.id + expect(feedback[:feedback_type]).to eq 'issue' + expect(feedback[:issue_id]).to eq issue.id + end - context 'when there is no feedback for the vulnerability' do - let(:vulnerability_no_feedback) { create(:vulnerability, findings: [finding_no_feedback]) } - let!(:finding_no_feedback) { create(:vulnerabilities_finding, :dependency_scanning, project: project) } + context 'when there is no feedback for the vulnerability' do + let(:vulnerability_no_feedback) { create(:vulnerability, findings: [finding_no_feedback]) } + let!(:finding_no_feedback) { create(:vulnerabilities_finding, :dependency_scanning, project: project) } - it 'does not return unassociated feedback' do - feedback = finding_no_feedback.issue_feedback + it 'does not return unassociated feedback' do + feedback = finding_no_feedback.issue_feedback - expect(feedback).not_to be_present + expect(feedback).not_to be_present + end end - end - context 'when there is no vulnerability associated with the finding' do - let!(:finding_no_vulnerability) { create(:vulnerabilities_finding, :dependency_scanning, project: project) } + context 'when there is no vulnerability associated with the finding' do + let!(:finding_no_vulnerability) { create(:vulnerabilities_finding, :dependency_scanning, project: project) } - it 'does not return feedback' do - feedback = finding_no_vulnerability.issue_feedback + it 'does not return feedback' do + feedback = finding_no_vulnerability.issue_feedback - expect(feedback).not_to be_present + expect(feedback).not_to be_present + end end end - end - describe '#dismissal_feedback' do - let!(:dismissal_feedback) do - create( - :vulnerability_feedback, - :dependency_scanning, - :dismissal, - project: project, - project_fingerprint: finding.project_fingerprint - ) - end + describe '#dismissal_feedback' do + let!(:dismissal_feedback) do + create( + :vulnerability_feedback, + :dependency_scanning, + :dismissal, + project: project, + project_fingerprint: finding.project_fingerprint + ) + end - it 'returns associated feedback' do - feedback = finding.dismissal_feedback + it 'returns associated feedback' do + feedback = finding.dismissal_feedback - expect(feedback).to be_present - expect(feedback[:project_id]).to eq project.id - expect(feedback[:feedback_type]).to eq 'dismissal' + expect(feedback).to be_present + expect(feedback[:project_id]).to eq project.id + expect(feedback[:feedback_type]).to eq 'dismissal' + end end - end - describe '#merge_request_feedback' do - let(:merge_request) { create(:merge_request, source_project: project) } - let!(:merge_request_feedback) do - create( - :vulnerability_feedback, - :dependency_scanning, - :merge_request, - merge_request: merge_request, - project: project, - project_fingerprint: finding.project_fingerprint - ) - end + describe '#merge_request_feedback' do + let(:merge_request) { create(:merge_request, source_project: project) } + let!(:merge_request_feedback) do + create( + :vulnerability_feedback, + :dependency_scanning, + :merge_request, + merge_request: merge_request, + project: project, + project_fingerprint: finding.project_fingerprint + ) + end - it 'returns associated feedback' do - feedback = finding.merge_request_feedback + it 'returns associated feedback' do + feedback = finding.merge_request_feedback - expect(feedback).to be_present - expect(feedback[:project_id]).to eq project.id - expect(feedback[:feedback_type]).to eq 'merge_request' - expect(feedback[:merge_request_id]).to eq merge_request.id + expect(feedback).to be_present + expect(feedback[:project_id]).to eq project.id + expect(feedback[:feedback_type]).to eq 'merge_request' + expect(feedback[:merge_request_id]).to eq merge_request.id + end end end - end - - describe '#load_feedback' do - let_it_be(:project) { create(:project) } - let_it_be(:finding) do - create( - :vulnerabilities_finding, - report_type: :dependency_scanning, - project: project - ) - end - - let_it_be(:feedback) do - create( - :vulnerability_feedback, - :dependency_scanning, - :dismissal, - project: project, - project_fingerprint: finding.project_fingerprint - ) - end - - let(:expected_feedback) { [feedback] } - - subject(:load_feedback) { finding.load_feedback.to_a } - it { is_expected.to eq(expected_feedback) } - - context 'when you have multiple findings' do - let_it_be(:finding_2) do + describe '#load_feedback' do + let_it_be(:project) { create(:project) } + let_it_be(:finding) do create( :vulnerabilities_finding, report_type: :dependency_scanning, @@ -646,367 +627,500 @@ ) end - let_it_be(:feedback_2) do + let_it_be(:feedback) do create( :vulnerability_feedback, :dependency_scanning, :dismissal, project: project, - project_fingerprint: finding_2.project_fingerprint + project_fingerprint: finding.project_fingerprint ) end - let(:expected_feedback) { [[feedback], [feedback_2]] } + let(:expected_feedback) { [feedback] } - subject(:load_feedback) { [finding, finding_2].map(&:load_feedback) } + subject(:load_feedback) { finding.load_feedback.to_a } it { is_expected.to eq(expected_feedback) } - end - end - describe '#state' do - before do - create(:vulnerability, :dismissed, project: finding_with_issue.project, findings: [finding_with_issue]) - end - - let(:unresolved_finding) { create(:vulnerabilities_finding) } - let(:confirmed_finding) { create(:vulnerabilities_finding, :confirmed) } - let(:resolved_finding) { create(:vulnerabilities_finding, :resolved) } - let(:dismissed_finding) { create(:vulnerabilities_finding, :dismissed) } - let(:detected_finding) { create(:vulnerabilities_finding, :detected) } - let(:finding_with_issue) { create(:vulnerabilities_finding, :with_issue_feedback) } - - it 'returns the expected state for a unresolved finding' do - expect(unresolved_finding.state).to eq 'detected' - end + context 'when you have multiple findings' do + let_it_be(:finding_2) do + create( + :vulnerabilities_finding, + report_type: :dependency_scanning, + project: project + ) + end - it 'returns the expected state for a confirmed finding' do - expect(confirmed_finding.state).to eq 'confirmed' - end + let_it_be(:feedback_2) do + create( + :vulnerability_feedback, + :dependency_scanning, + :dismissal, + project: project, + project_fingerprint: finding_2.project_fingerprint + ) + end - it 'returns the expected state for a resolved finding' do - expect(resolved_finding.state).to eq 'resolved' - end + let(:expected_feedback) { [[feedback], [feedback_2]] } - it 'returns the expected state for a dismissed finding' do - expect(dismissed_finding.state).to eq 'dismissed' - end + subject(:load_feedback) { [finding, finding_2].map(&:load_feedback) } - it 'returns the expected state for a detected finding' do - expect(detected_finding.state).to eq 'detected' + it { is_expected.to eq(expected_feedback) } + end end - context 'when a vulnerability present for a dismissed finding' do + describe '#state' do before do - create(:vulnerability, project: dismissed_finding.project, findings: [dismissed_finding]) + create(:vulnerability, :dismissed, project: finding_with_issue.project, findings: [finding_with_issue]) end - it 'still reports a dismissed state' do - expect(dismissed_finding.state).to eq 'dismissed' + let(:unresolved_finding) { create(:vulnerabilities_finding) } + let(:confirmed_finding) { create(:vulnerabilities_finding, :confirmed) } + let(:resolved_finding) { create(:vulnerabilities_finding, :resolved) } + let(:dismissed_finding) { create(:vulnerabilities_finding, :dismissed) } + let(:finding_with_issue) { create(:vulnerabilities_finding, :with_issue_feedback) } + + it 'returns the expected state for a unresolved finding' do + expect(unresolved_finding.state).to eq 'detected' end - end - context 'when a non-dismissal feedback present for a finding belonging to a closed vulnerability' do - before do - create(:vulnerability_feedback, :issue, project: resolved_finding.project) + it 'returns the expected state for a confirmed finding' do + expect(confirmed_finding.state).to eq 'confirmed' end - it 'reports as resolved' do + it 'returns the expected state for a resolved finding' do expect(resolved_finding.state).to eq 'resolved' end - end - end - describe '#scanner_name' do - let(:vulnerabilities_finding) { create(:vulnerabilities_finding) } - - subject(:scanner_name) { vulnerabilities_finding.scanner_name } + it 'returns the expected state for a dismissed finding' do + expect(dismissed_finding.state).to eq 'dismissed' + end - it { is_expected.to eq(vulnerabilities_finding.scanner.name) } - end + context 'when a vulnerability present for a dismissed finding' do + before do + create(:vulnerability, project: dismissed_finding.project, findings: [dismissed_finding]) + end - describe '#description' do - let(:finding) { build(:vulnerabilities_finding) } - let(:expected_description) { finding.metadata['description'] } + it 'still reports a dismissed state' do + expect(dismissed_finding.state).to eq 'dismissed' + end + end - subject { finding.description } + context 'when a non-dismissal feedback present for a finding belonging to a closed vulnerability' do + before do + create(:vulnerability_feedback, :issue, project: resolved_finding.project) + end - context 'when description metadata key is present' do - it { is_expected.to eql(expected_description) } + it 'reports as resolved' do + expect(resolved_finding.state).to eq 'resolved' + end + end end - context 'when description data is present' do - let(:finding) { build(:vulnerabilities_finding, description: 'Vulnerability description') } + describe '#scanner_name' do + let(:vulnerabilities_finding) { create(:vulnerabilities_finding) } - it { is_expected.to eq('Vulnerability description') } - end - end + subject(:scanner_name) { vulnerabilities_finding.scanner_name } - describe '#solution' do - subject { vulnerabilities_finding.solution } + it { is_expected.to eq(vulnerabilities_finding.scanner.name) } + end - context 'when solution metadata key is present' do - let(:vulnerabilities_finding) { build(:vulnerabilities_finding) } + describe '#description' do + let(:finding) { build(:vulnerabilities_finding) } + let(:expected_description) { finding.metadata['description'] } - it { is_expected.to eq(vulnerabilities_finding.metadata['solution']) } - end + subject { finding.description } - context 'when remediations key is present in finding' do - let(:vulnerabilities_finding) do - build(:vulnerabilities_finding_with_remediation, summary: "Test remediation") + context 'when description metadata key is present' do + it { is_expected.to eql(expected_description) } end - it { is_expected.to eq(vulnerabilities_finding.remediations.dig(0, 'summary')) } + context 'when description data is present' do + let(:finding) { build(:vulnerabilities_finding, description: 'Vulnerability description') } + + it { is_expected.to eq('Vulnerability description') } + end end - context 'when solution data is present' do - let(:vulnerabilities_finding) { build(:vulnerabilities_finding, solution: 'Vulnerability solution') } + describe '#solution' do + subject { vulnerabilities_finding.solution } - it { is_expected.to eq('Vulnerability solution') } - end - end + context 'when solution metadata key is present' do + let(:vulnerabilities_finding) { build(:vulnerabilities_finding) } - describe '#location' do - let(:finding) { build(:vulnerabilities_finding) } - let(:expected_location) { finding.metadata['location'] } + it { is_expected.to eq(vulnerabilities_finding.metadata['solution']) } + end - subject { finding.location } + context 'when remediations key is present in finding' do + let(:vulnerabilities_finding) do + build(:vulnerabilities_finding_with_remediation, summary: "Test remediation") + end - context 'when location metadata key is present' do - it { is_expected.to eql(expected_location) } - end + it { is_expected.to eq(vulnerabilities_finding.remediations.dig(0, 'summary')) } + 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) } + context 'when solution data is present' do + let(:vulnerabilities_finding) { build(:vulnerabilities_finding, solution: 'Vulnerability solution') } - it { is_expected.to eq(location) } + it { is_expected.to eq('Vulnerability solution') } + end end - end - describe '#evidence' do - subject { finding.evidence } + describe '#location' do + let(:finding) { build(:vulnerabilities_finding) } + let(:expected_location) { finding.metadata['location'] } - context 'has an evidence fields' do - let(:finding) { create(:vulnerabilities_finding) } - let(:evidence) { finding.metadata['evidence'] } + subject { finding.location } - it do - is_expected.to match a_hash_including( - summary: evidence['summary'], - request: { - headers: [ - { - name: evidence['request']['headers'][0]['name'], - value: evidence['request']['headers'][0]['value'] - } - ], - url: evidence['request']['url'], - method: evidence['request']['method'], - body: evidence['request']['body'] - }, - response: { - headers: [ - { - name: evidence['response']['headers'][0]['name'], - value: evidence['response']['headers'][0]['value'] - } - ], - reason_phrase: evidence['response']['reason_phrase'], - status_code: evidence['response']['status_code'], - body: evidence['request']['body'] - }, - source: { - id: evidence.dig('source', 'id'), - name: evidence.dig('source', 'name'), - url: evidence.dig('source', 'url') - }, - supporting_messages: [ - { - name: evidence.dig('supporting_messages')[0].dig('name'), - request: { - headers: [ - { - name: evidence.dig('supporting_messages')[0].dig('request', 'headers')[0].dig('name'), - value: evidence.dig('supporting_messages')[0].dig('request', 'headers')[0].dig('value') - } - ], - url: evidence.dig('supporting_messages')[0].dig('request', 'url'), - method: evidence.dig('supporting_messages')[0].dig('request', 'method'), - body: evidence.dig('supporting_messages')[0].dig('request', 'body') - }, - response: evidence.dig('supporting_messages')[0].dig('response') + 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 + subject { finding.evidence } + + context 'has an evidence fields' do + let(:finding) { create(:vulnerabilities_finding) } + let(:evidence) { finding.metadata['evidence'] } + + it do + is_expected.to match a_hash_including( + summary: evidence['summary'], + request: { + headers: [ + { + name: evidence['request']['headers'][0]['name'], + value: evidence['request']['headers'][0]['value'] + } + ], + url: evidence['request']['url'], + method: evidence['request']['method'], + body: evidence['request']['body'] + }, + response: { + headers: [ + { + name: evidence['response']['headers'][0]['name'], + value: evidence['response']['headers'][0]['value'] + } + ], + reason_phrase: evidence['response']['reason_phrase'], + status_code: evidence['response']['status_code'], + body: evidence['request']['body'] + }, + source: { + id: evidence.dig('source', 'id'), + name: evidence.dig('source', 'name'), + url: evidence.dig('source', 'url') }, - { - name: evidence.dig('supporting_messages')[1].dig('name'), - request: { - headers: [ - { - name: evidence.dig('supporting_messages')[1].dig('request', 'headers')[0].dig('name'), - value: evidence.dig('supporting_messages')[1].dig('request', 'headers')[0].dig('value') - } - ], - url: evidence.dig('supporting_messages')[1].dig('request', 'url'), - method: evidence.dig('supporting_messages')[1].dig('request', 'method'), - body: evidence.dig('supporting_messages')[1].dig('request', 'body') + supporting_messages: [ + { + name: evidence.dig('supporting_messages')[0].dig('name'), + request: { + headers: [ + { + name: evidence.dig('supporting_messages')[0].dig('request', 'headers')[0].dig('name'), + value: evidence.dig('supporting_messages')[0].dig('request', 'headers')[0].dig('value') + } + ], + url: evidence.dig('supporting_messages')[0].dig('request', 'url'), + method: evidence.dig('supporting_messages')[0].dig('request', 'method'), + body: evidence.dig('supporting_messages')[0].dig('request', 'body') + }, + response: evidence.dig('supporting_messages')[0].dig('response') }, - response: { - headers: [ - { - name: evidence.dig('supporting_messages')[1].dig('response', 'headers')[0].dig('name'), - value: evidence.dig('supporting_messages')[1].dig('response', 'headers')[0].dig('value') - } - ], - reason_phrase: evidence.dig('supporting_messages')[1].dig('response', 'reason_phrase'), - status_code: evidence.dig('supporting_messages')[1].dig('response', 'status_code'), - body: evidence.dig('supporting_messages')[1].dig('response', 'body') + { + name: evidence.dig('supporting_messages')[1].dig('name'), + request: { + headers: [ + { + name: evidence.dig('supporting_messages')[1].dig('request', 'headers')[0].dig('name'), + value: evidence.dig('supporting_messages')[1].dig('request', 'headers')[0].dig('value') + } + ], + url: evidence.dig('supporting_messages')[1].dig('request', 'url'), + method: evidence.dig('supporting_messages')[1].dig('request', 'method'), + body: evidence.dig('supporting_messages')[1].dig('request', 'body') + }, + response: { + headers: [ + { + name: evidence.dig('supporting_messages')[1].dig('response', 'headers')[0].dig('name'), + value: evidence.dig('supporting_messages')[1].dig('response', 'headers')[0].dig('value') + } + ], + reason_phrase: evidence.dig('supporting_messages')[1].dig('response', 'reason_phrase'), + status_code: evidence.dig('supporting_messages')[1].dig('response', 'status_code'), + body: evidence.dig('supporting_messages')[1].dig('response', 'body') + } } - } - ] - ) + ] + ) + end end - end - context 'has no evidence summary when evidence is present, summary is not' do - let(:finding) { create(:vulnerabilities_finding, raw_metadata: { evidence: {} }) } + context 'has no evidence summary when evidence is present, summary is not' do + let(:finding) { create(:vulnerabilities_finding, raw_metadata: { evidence: {} }) } - it do - is_expected.to match a_hash_including( - summary: nil, - source: nil, - supporting_messages: [], - request: nil, - response: nil) + it do + is_expected.to match a_hash_including( + summary: nil, + source: nil, + supporting_messages: [], + request: nil, + response: nil) + end end end - end - describe '#message' do - let(:finding) { build(:vulnerabilities_finding) } - let(:expected_message) { finding.metadata['message'] } + describe '#message' do + let(:finding) { build(:vulnerabilities_finding) } + let(:expected_message) { finding.metadata['message'] } - subject { finding.message } + subject { finding.message } - context 'when message metadata key is present' do - it { is_expected.to eql(expected_message) } - end + 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') } + context 'when message data is present' do + let(:finding) { build(:vulnerabilities_finding, message: 'Vulnerability message') } - it { is_expected.to eq('Vulnerability message') } + it { is_expected.to eq('Vulnerability message') } + end end - end - describe '#cve_value' do - let(:finding) { build(:vulnerabilities_finding) } - let(:expected_cve) { 'CVE-2020-0000' } + describe '#cve_value' do + let(:finding) { build(:vulnerabilities_finding) } + let(:expected_cve) { 'CVE-2020-0000' } - subject { finding.cve_value } + subject { finding.cve_value } - before do - finding.identifiers << build(:vulnerabilities_identifier, external_type: 'cve', name: expected_cve) - end + before do + finding.identifiers << build(:vulnerabilities_identifier, external_type: 'cve', name: expected_cve) + end - context 'when cve metadata key is present' do - it { is_expected.to eql(expected_cve) } - end + 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') } + context 'when cve data is present' do + let(:finding) { build(:vulnerabilities_finding, cve: 'Vulnerability cve') } - it { is_expected.to eq('Vulnerability cve') } + it { is_expected.to eq('Vulnerability cve') } + end end - end - describe '#cwe_value' do - let(:finding) { build(:vulnerabilities_finding) } - let(:expected_cwe) { 'CWE-0000' } + describe '#cwe_value' do + let(:finding) { build(:vulnerabilities_finding) } + let(:expected_cwe) { 'CWE-0000' } - subject { finding.cwe_value } + subject { finding.cwe_value } - before do - finding.identifiers << build(:vulnerabilities_identifier, external_type: 'cwe', name: expected_cwe) + before do + finding.identifiers << build(:vulnerabilities_identifier, external_type: 'cwe', name: expected_cwe) + end + + it { is_expected.to eql(expected_cwe) } end - it { is_expected.to eql(expected_cwe) } - end + describe '#other_identifier_values' do + let(:finding) { build(:vulnerabilities_finding) } + let(:expected_values) { ['ID 1', 'ID 2'] } - describe '#other_identifier_values' do - let(:finding) { build(:vulnerabilities_finding) } - let(:expected_values) { ['ID 1', 'ID 2'] } + subject { finding.other_identifier_values } - subject { finding.other_identifier_values } + before do + finding.identifiers << build(:vulnerabilities_identifier, external_type: 'foo', name: expected_values.first) + finding.identifiers << build(:vulnerabilities_identifier, external_type: 'bar', name: expected_values.second) + end - before do - finding.identifiers << build(:vulnerabilities_identifier, external_type: 'foo', name: expected_values.first) - finding.identifiers << build(:vulnerabilities_identifier, external_type: 'bar', name: expected_values.second) + it { is_expected.to match_array(expected_values) } end - it { is_expected.to match_array(expected_values) } - end + describe "#metadata" do + let(:finding) { build(:vulnerabilities_finding) } - describe "#metadata" do - let(:finding) { build(:vulnerabilities_finding) } + subject { finding.metadata } - subject { finding.metadata } + it "handles bool JSON data" do + allow(finding).to receive(:raw_metadata) { "true" } - it "handles bool JSON data" do - allow(finding).to receive(:raw_metadata) { "true" } + expect(subject).to eq({}) + end - expect(subject).to eq({}) - end + it "handles string JSON data" do + allow(finding).to receive(:raw_metadata) { '"test"' } + + expect(subject).to eq({}) + end - it "handles string JSON data" do - allow(finding).to receive(:raw_metadata) { '"test"' } + it "parses JSON data" do + allow(finding).to receive(:raw_metadata) { '{ "test": true }' } - expect(subject).to eq({}) + expect(subject).to eq({ "test" => true }) + end end - it "parses JSON data" do - allow(finding).to receive(:raw_metadata) { '{ "test": true }' } + describe '#uuid_v5' do + let(:project) { create(:project) } + let(:report_type) { :sast } + let(:identifier_fingerprint) { 'fooo' } + let(:location_fingerprint) { 'zooo' } + let(:identifier) { build(:vulnerabilities_identifier, fingerprint: identifier_fingerprint) } + let(:expected_uuid) { 'this-is-supposed-to-a-uuid' } + let(:finding) do + build(:vulnerabilities_finding, report_type, + uuid: uuid, + project: project, + primary_identifier: identifier, + location_fingerprint: location_fingerprint) + end - expect(subject).to eq({ "test" => true }) - end - end + subject(:uuid_v5) { finding.uuid_v5 } - describe '#uuid_v5' do - let(:project) { create(:project) } - let(:report_type) { :sast } - let(:identifier_fingerprint) { 'fooo' } - let(:location_fingerprint) { 'zooo' } - let(:identifier) { build(:vulnerabilities_identifier, fingerprint: identifier_fingerprint) } - let(:expected_uuid) { 'this-is-supposed-to-a-uuid' } - let(:finding) do - build(:vulnerabilities_finding, report_type, - uuid: uuid, - project: project, - primary_identifier: identifier, - location_fingerprint: location_fingerprint) - end + before do + allow(::Gitlab::UUID).to receive(:v5).and_return(expected_uuid) + end - subject(:uuid_v5) { finding.uuid_v5 } + context 'when the finding has a version 4 uuid' do + let(:uuid) { SecureRandom.uuid } + let(:uuid_name_value) { "#{report_type}-#{identifier_fingerprint}-#{location_fingerprint}-#{project.id}" } - before do - allow(::Gitlab::UUID).to receive(:v5).and_return(expected_uuid) - end + it 'returns the calculated uuid for the finding' do + expect(uuid_v5).to eq(expected_uuid) + expect(::Gitlab::UUID).to have_received(:v5).with(uuid_name_value) + end + end - context 'when the finding has a version 4 uuid' do - let(:uuid) { SecureRandom.uuid } - let(:uuid_name_value) { "#{report_type}-#{identifier_fingerprint}-#{location_fingerprint}-#{project.id}" } + context 'when the finding has a version 5 uuid' do + let(:uuid) { '6756ebb6-8465-5c33-9af9-c5c8b117aefb' } - it 'returns the calculated uuid for the finding' do - expect(uuid_v5).to eq(expected_uuid) - expect(::Gitlab::UUID).to have_received(:v5).with(uuid_name_value) + it 'returns the uuid of the finding' do + expect(uuid_v5).to eq(uuid) + expect(::Gitlab::UUID).not_to have_received(:v5) + end end end - context 'when the finding has a version 5 uuid' do - let(:uuid) { '6756ebb6-8465-5c33-9af9-c5c8b117aefb' } + describe '#eql?' do + let(:project) { create(:project) } + let(:report_type) { :sast } + let(:identifier_fingerprint) { 'fooo' } + let(:identifier) { build(:vulnerabilities_identifier, fingerprint: identifier_fingerprint) } + let(:location_fingerprint1) { 'fingerprint1' } + let(:location_fingerprint2) { 'fingerprint2' } + let(:finding1) do + build(:vulnerabilities_finding, report_type, + project: project, + primary_identifier: identifier, + location_fingerprint: location_fingerprint1) + end + + let(:finding2) do + build(:vulnerabilities_finding, report_type, + project: project, + primary_identifier: identifier, + location_fingerprint: location_fingerprint2) + end - it 'returns the uuid of the finding' do - expect(uuid_v5).to eq(uuid) - expect(::Gitlab::UUID).not_to have_received(:v5) + it 'matches the finding based on enabled tracking methods (if feature flag enabled)' do + signature1 = create( + :vulnerabilities_finding_signature, + finding: finding1 + ) + + signature2 = create( + :vulnerabilities_finding_signature, + finding: finding2, + signature_sha: signature1.signature_sha + ) + + # verify that the signatures do exist and that they match + expect(finding1.signatures.size).to eq(1) + expect(finding2.signatures.size).to eq(1) + expect(signature1.eql?(signature2)).to be(true) + + # now verify that the correct matching method was used for eql? + expect(finding1.eql?(finding2)).to be(vulnerability_finding_signatures_enabled) + end + + context 'short circuits on the highest priority signature match' do + using RSpec::Parameterized::TableSyntax + + let(:same_hash) { false } + let(:same_location) { false } + let(:create_scope_offset) { false } + let(:same_scope_offset) { false} + + let(:create_signatures) do + signature1_hash = create( + :vulnerabilities_finding_signature, + algorithm_type: 'hash', + finding: finding1 + ) + sha = same_hash ? signature1_hash.signature_sha : ::Digest::SHA1.digest(SecureRandom.hex(50)) + create( + :vulnerabilities_finding_signature, + algorithm_type: 'hash', + finding: finding2, + signature_sha: sha + ) + + signature1_location = create( + :vulnerabilities_finding_signature, + algorithm_type: 'location', + finding: finding1 + ) + sha = same_location ? signature1_location.signature_sha : ::Digest::SHA1.digest(SecureRandom.hex(50)) + create( + :vulnerabilities_finding_signature, + algorithm_type: 'location', + finding: finding2, + signature_sha: sha + ) + + signature1_scope_offset = create( + :vulnerabilities_finding_signature, + algorithm_type: 'scope_offset', + finding: finding1 + ) + + if create_scope_offset + sha = same_scope_offset ? signature1_scope_offset.signature_sha : ::Digest::SHA1.digest(SecureRandom.hex(50)) + create( + :vulnerabilities_finding_signature, + algorithm_type: 'scope_offset', + finding: finding2, + signature_sha: sha + ) + end + end + + where(:same_hash, :same_location, :create_scope_offset, :same_scope_offset, :should_match) do + true | true | true | true | true # everything matches + false | false | true | false | false # nothing matches + true | true | true | false | false # highest priority matches alg/priority but not on value + false | false | true | true | true # highest priority matches alg/priority and value + false | true | false | false | true # highest priority is location, matches alg/priority and value + end + with_them do + it 'matches correctly' do + next unless vulnerability_finding_signatures_enabled + + create_signatures + expect(finding1.eql?(finding2)).to be(should_match) + end + end end end end diff --git a/ee/spec/requests/api/vulnerability_findings_spec.rb b/ee/spec/requests/api/vulnerability_findings_spec.rb index 2e352beec72e82758c00aaff911f3a4e15c6b6ba..4dc672f6f0a55c1193304c4b28c33964f28f4d8a 100644 --- a/ee/spec/requests/api/vulnerability_findings_spec.rb +++ b/ee/spec/requests/api/vulnerability_findings_spec.rb @@ -33,7 +33,8 @@ project: project, pipeline: pipeline, project_fingerprint: sast_report.findings.first.project_fingerprint, - vulnerability_data: sast_report.findings.first.raw_metadata + vulnerability_data: sast_report.findings.first.raw_metadata, + finding_uuid: sast_report.findings.first.uuid ) end diff --git a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb index ee9061c95362d15c88ad15e49aeb02474ac5ac88..997db9ab21256bc890823207c1854e50b6576fa8 100644 --- a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb +++ b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb @@ -164,7 +164,7 @@ end context 'when finding_uuid is not present' do - let(:feedback) { build_stubbed(:vulnerability_feedback, :issue, project: project) } + let(:feedback) { build_stubbed(:vulnerability_feedback, :issue, project: project, finding_uuid: nil) } it 'has a nil finding_uuid' do expect(subject[:finding_uuid]).to be_nil diff --git a/ee/spec/services/ci/compare_security_reports_service_spec.rb b/ee/spec/services/ci/compare_security_reports_service_spec.rb index c861b5e3f0f2724bf877cd98d5a61b77c0763954..654c7d76a90aa75f5290dedbd8be4fdf7becd7e7 100644 --- a/ee/spec/services/ci/compare_security_reports_service_spec.rb +++ b/ee/spec/services/ci/compare_security_reports_service_spec.rb @@ -11,265 +11,272 @@ def collect_ids(collection) collection.map { |t| t['identifiers'].first['external_id'] } end - describe '#execute DS' do + where(vulnerability_finding_signatures_enabled: [true, false]) + with_them do before do - stub_licensed_features(dependency_scanning: true) + stub_feature_flags(vulnerability_finding_signatures: vulnerability_finding_signatures_enabled) end - let(:service) { described_class.new(project, current_user, report_type: 'dependency_scanning') } - - subject { service.execute(base_pipeline, head_pipeline) } - - context 'when head pipeline has dependency scanning reports' do - let!(:base_pipeline) { create(:ee_ci_pipeline) } - let!(:head_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_report, project: project) } - - it 'reports new vulnerabilities' do - expect(subject[:status]).to eq(:parsed) - expect(subject[:data]['added'].count).to eq(4) - expect(subject[:data]['fixed'].count).to eq(0) + describe '#execute DS' do + before do + stub_licensed_features(dependency_scanning: true) end - end - - context 'when base and head pipelines have dependency scanning reports' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_report, project: project) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_feature_branch, project: project) } - it 'reports status as parsed' do - expect(subject[:status]).to eq(:parsed) - end + let(:service) { described_class.new(project, current_user, report_type: 'dependency_scanning') } - it 'populates fields based on current_user' do - payload = subject[:data]['added'].first + subject { service.execute(base_pipeline, head_pipeline) } - expect(payload['create_vulnerability_feedback_issue_path']).to be_present - expect(payload['create_vulnerability_feedback_merge_request_path']).to be_present - expect(payload['create_vulnerability_feedback_dismissal_path']).to be_present - expect(payload['create_vulnerability_feedback_issue_path']).to be_present - expect(service.current_user).to eq(current_user) - end + context 'when head pipeline has dependency scanning reports' do + let!(:base_pipeline) { create(:ee_ci_pipeline) } + let!(:head_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_report, project: project) } - it 'reports fixed vulnerability' do - expect(subject[:data]['added'].count).to eq(1) - expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('external_id' => 'CVE-2017-5946')) + it 'reports new vulnerabilities' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]['added'].count).to eq(4) + expect(subject[:data]['fixed'].count).to eq(0) + end end - it 'reports fixed dependency scanning vulnerabilities' do - expect(subject[:data]['fixed'].count).to eq(1) - compare_keys = collect_ids(subject[:data]['fixed']) - expected_keys = %w(06565b64-486d-4326-b906-890d9915804d) - expect(compare_keys).to match_array(expected_keys) + context 'when base and head pipelines have dependency scanning reports' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_report, project: project) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_feature_branch, project: project) } + + it 'reports status as parsed' do + expect(subject[:status]).to eq(:parsed) + end + + it 'populates fields based on current_user' do + payload = subject[:data]['added'].first + + expect(payload['create_vulnerability_feedback_issue_path']).to be_present + expect(payload['create_vulnerability_feedback_merge_request_path']).to be_present + expect(payload['create_vulnerability_feedback_dismissal_path']).to be_present + expect(payload['create_vulnerability_feedback_issue_path']).to be_present + expect(service.current_user).to eq(current_user) + end + + it 'reports fixed vulnerability' do + expect(subject[:data]['added'].count).to eq(1) + expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('external_id' => 'CVE-2017-5946')) + end + + it 'reports fixed dependency scanning vulnerabilities' do + expect(subject[:data]['fixed'].count).to eq(1) + compare_keys = collect_ids(subject[:data]['fixed']) + expected_keys = %w(06565b64-486d-4326-b906-890d9915804d) + expect(compare_keys).to match_array(expected_keys) + end end - end - context 'when head pipeline has corrupted dependency scanning vulnerability reports' do - let_it_be(:base_pipeline) { build(:ee_ci_pipeline, :with_corrupted_dependency_scanning_report, project: project) } - let_it_be(:head_pipeline) { build(:ee_ci_pipeline, :with_corrupted_dependency_scanning_report, project: project) } + context 'when head pipeline has corrupted dependency scanning vulnerability reports' do + let_it_be(:base_pipeline) { build(:ee_ci_pipeline, :with_corrupted_dependency_scanning_report, project: project) } + let_it_be(:head_pipeline) { build(:ee_ci_pipeline, :with_corrupted_dependency_scanning_report, project: project) } - it 'returns status and error message' do - expect(subject[:status]).to eq(:error) - expect(subject[:status_reason]).to include('JSON parsing failed') - end + it 'returns status and error message' do + expect(subject[:status]).to eq(:error) + expect(subject[:status_reason]).to include('JSON parsing failed') + end - it 'returns status and error message when pipeline is nil' do - result = service.execute(nil, head_pipeline) + it 'returns status and error message when pipeline is nil' do + result = service.execute(nil, head_pipeline) - expect(result[:status]).to eq(:error) - expect(result[:status_reason]).to include('JSON parsing failed') + expect(result[:status]).to eq(:error) + expect(result[:status_reason]).to include('JSON parsing failed') + end end end - end - describe '#execute CS' do - before do - stub_licensed_features(container_scanning: true) - end - - let(:service) { described_class.new(project, current_user, report_type: 'container_scanning') } - - subject { service.execute(base_pipeline, head_pipeline) } + describe '#execute CS' do + before do + stub_licensed_features(container_scanning: true) + end - context 'when head pipeline has container scanning reports' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_report, project: project) } + let(:service) { described_class.new(project, current_user, report_type: 'container_scanning') } - it 'reports new and fixed vulnerabilities' do - expect(subject[:status]).to eq(:parsed) - expect(subject[:data]['added'].count).to eq(8) - expect(subject[:data]['fixed'].count).to eq(0) - end - end + subject { service.execute(base_pipeline, head_pipeline) } - context 'when base and head pipelines have container scanning reports' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_report, project: project) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_feature_branch, project: project) } - - it 'populates fields based on current_user' do - payload = subject[:data]['added'].first - expect(payload['create_vulnerability_feedback_issue_path']).to be_present - expect(payload['create_vulnerability_feedback_merge_request_path']).to be_present - expect(payload['create_vulnerability_feedback_dismissal_path']).to be_present - expect(payload['create_vulnerability_feedback_issue_path']).to be_present - expect(service.current_user).to eq(current_user) - end + context 'when head pipeline has container scanning reports' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_report, project: project) } - it 'reports new vulnerability' do - expect(subject[:data]['added'].count).to eq(1) - expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('external_id' => 'CVE-2017-15650')) + it 'reports new and fixed vulnerabilities' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]['added'].count).to eq(8) + expect(subject[:data]['fixed'].count).to eq(0) + end end - it 'reports fixed container scanning vulnerabilities' do - expect(subject[:data]['fixed'].count).to eq(8) - compare_keys = collect_ids(subject[:data]['fixed']) - expected_keys = %w(CVE-2017-16997 CVE-2017-18269 CVE-2018-1000001 CVE-2016-10228 CVE-2010-4052 CVE-2018-18520 CVE-2018-16869 CVE-2018-18311) - expect(compare_keys).to match_array(expected_keys) + context 'when base and head pipelines have container scanning reports' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_report, project: project) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_feature_branch, project: project) } + + it 'populates fields based on current_user' do + payload = subject[:data]['added'].first + expect(payload['create_vulnerability_feedback_issue_path']).to be_present + expect(payload['create_vulnerability_feedback_merge_request_path']).to be_present + expect(payload['create_vulnerability_feedback_dismissal_path']).to be_present + expect(payload['create_vulnerability_feedback_issue_path']).to be_present + expect(service.current_user).to eq(current_user) + end + + it 'reports new vulnerability' do + expect(subject[:data]['added'].count).to eq(1) + expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('external_id' => 'CVE-2017-15650')) + end + + it 'reports fixed container scanning vulnerabilities' do + expect(subject[:data]['fixed'].count).to eq(8) + compare_keys = collect_ids(subject[:data]['fixed']) + expected_keys = %w(CVE-2017-16997 CVE-2017-18269 CVE-2018-1000001 CVE-2016-10228 CVE-2010-4052 CVE-2018-18520 CVE-2018-16869 CVE-2018-18311) + expect(compare_keys).to match_array(expected_keys) + end end end - end - - describe '#execute DAST' do - before do - stub_licensed_features(dast: true) - end - - let(:service) { described_class.new(project, current_user, report_type: 'dast') } - subject { service.execute(base_pipeline, head_pipeline) } - - context 'when head pipeline has DAST reports containing some vulnerabilities' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_dast_report, project: project) } - - it 'reports the new vulnerabilities, while not changing the counts of fixed vulnerabilities' do - expect(subject[:status]).to eq(:parsed) - expect(subject[:data]['added'].count).to eq(20) - expect(subject[:data]['fixed'].count).to eq(0) + describe '#execute DAST' do + before do + stub_licensed_features(dast: true) end - end - context 'when base and head pipelines have DAST reports containing vulnerabilities' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_dast_report, project: project) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_dast_feature_branch, project: project) } + let(:service) { described_class.new(project, current_user, report_type: 'dast') } - it 'populates fields based on current_user' do - payload = subject[:data]['fixed'].first + subject { service.execute(base_pipeline, head_pipeline) } - expect(payload).to be_present - expect(payload['create_vulnerability_feedback_issue_path']).to be_present - expect(payload['create_vulnerability_feedback_merge_request_path']).to be_present - expect(payload['create_vulnerability_feedback_dismissal_path']).to be_present - expect(payload['create_vulnerability_feedback_issue_path']).to be_present - expect(service.current_user).to eq(current_user) - end + context 'when head pipeline has DAST reports containing some vulnerabilities' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_dast_report, project: project) } - it 'reports new vulnerability' do - expect(subject[:data]['added'].count).to eq(1) - expect(subject[:data]['added'].last['identifiers']).to include(a_hash_including('name' => 'CWE-201')) + it 'reports the new vulnerabilities, while not changing the counts of fixed vulnerabilities' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]['added'].count).to eq(20) + expect(subject[:data]['fixed'].count).to eq(0) + end end - it 'reports fixed DAST vulnerabilities' do - expect(subject[:data]['fixed'].count).to eq(19) - expect(subject[:data]['fixed']).to include( - a_hash_including( - { - 'identifiers' => a_collection_including( - a_hash_including( - "name" => "CWE-352" + context 'when base and head pipelines have DAST reports containing vulnerabilities' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_dast_report, project: project) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_dast_feature_branch, project: project) } + + it 'populates fields based on current_user' do + payload = subject[:data]['fixed'].first + + expect(payload).to be_present + expect(payload['create_vulnerability_feedback_issue_path']).to be_present + expect(payload['create_vulnerability_feedback_merge_request_path']).to be_present + expect(payload['create_vulnerability_feedback_dismissal_path']).to be_present + expect(payload['create_vulnerability_feedback_issue_path']).to be_present + expect(service.current_user).to eq(current_user) + end + + it 'reports new vulnerability' do + expect(subject[:data]['added'].count).to eq(1) + expect(subject[:data]['added'].last['identifiers']).to include(a_hash_including('name' => 'CWE-201')) + end + + it 'reports fixed DAST vulnerabilities' do + expect(subject[:data]['fixed'].count).to eq(19) + expect(subject[:data]['fixed']).to include( + a_hash_including( + { + 'identifiers' => a_collection_including( + a_hash_including( + "name" => "CWE-352" + ) ) - ) - }) - ) + }) + ) + end end end - end - - describe '#execute SAST' do - before do - stub_licensed_features(sast: true) - end - - let(:service) { described_class.new(project, current_user, report_type: 'sast') } - subject { service.execute(base_pipeline, head_pipeline) } - - context 'when head pipeline has sast reports' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_sast_report, project: project) } - - it 'reports new vulnerabilities' do - expect(subject[:status]).to eq(:parsed) - expect(subject[:data]['added'].count).to eq(5) - expect(subject[:data]['fixed'].count).to eq(0) + describe '#execute SAST' do + before do + stub_licensed_features(sast: true) end - end - context 'when base and head pipelines have sast reports' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_sast_report, project: project) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_sast_feature_branch, project: project) } + let(:service) { described_class.new(project, current_user, report_type: 'sast') } - it 'populates fields based on current_user' do - payload = subject[:data]['added'].first + subject { service.execute(base_pipeline, head_pipeline) } - expect(payload['create_vulnerability_feedback_issue_path']).to be_present - expect(payload['create_vulnerability_feedback_merge_request_path']).to be_present - expect(payload['create_vulnerability_feedback_dismissal_path']).to be_present - expect(payload['create_vulnerability_feedback_issue_path']).to be_present - expect(service.current_user).to eq(current_user) - end + context 'when head pipeline has sast reports' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_sast_report, project: project) } - it 'reports new vulnerability' do - expect(subject[:data]['added'].count).to eq(1) - expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('name' => 'CWE-327')) + it 'reports new vulnerabilities' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]['added'].count).to eq(5) + expect(subject[:data]['fixed'].count).to eq(0) + end end - it 'reports fixed sast vulnerabilities' do - expect(subject[:data]['fixed'].count).to eq(1) - compare_keys = collect_ids(subject[:data]['fixed']) - expected_keys = %w(CIPHER_INTEGRITY) - expect(compare_keys - expected_keys).to eq([]) + context 'when base and head pipelines have sast reports' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_sast_report, project: project) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_sast_feature_branch, project: project) } + + it 'populates fields based on current_user' do + payload = subject[:data]['added'].first + + expect(payload['create_vulnerability_feedback_issue_path']).to be_present + expect(payload['create_vulnerability_feedback_merge_request_path']).to be_present + expect(payload['create_vulnerability_feedback_dismissal_path']).to be_present + expect(payload['create_vulnerability_feedback_issue_path']).to be_present + expect(service.current_user).to eq(current_user) + end + + it 'reports new vulnerability' do + expect(subject[:data]['added'].count).to eq(1) + expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('name' => 'CWE-327')) + end + + it 'reports fixed sast vulnerabilities' do + expect(subject[:data]['fixed'].count).to eq(1) + compare_keys = collect_ids(subject[:data]['fixed']) + expected_keys = %w(CIPHER_INTEGRITY) + expect(compare_keys - expected_keys).to eq([]) + end end end - end - - describe '#execute SECRET DETECTION' do - before do - stub_licensed_features(secret_detection: true) - end - - let(:service) { described_class.new(project, current_user, report_type: 'secret_detection') } - subject { service.execute(base_pipeline, head_pipeline) } - - context 'when head pipeline has secret_detection reports' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_secret_detection_report, project: project) } - - it 'reports new vulnerabilities' do - expect(subject[:status]).to eq(:parsed) - expect(subject[:data]['added'].count).to eq(1) - expect(subject[:data]['fixed'].count).to eq(0) + describe '#execute SECRET DETECTION' do + before do + stub_licensed_features(secret_detection: true) end - end - context 'when base and head pipelines have secret_detection reports' do - let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_secret_detection_report, project: project) } - let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_secret_detection_feature_branch, project: project) } + let(:service) { described_class.new(project, current_user, report_type: 'secret_detection') } - it 'populates fields based on current_user' do - payload = subject[:data]['added'].first - expect(payload).to be_nil - expect(service.current_user).to eq(current_user) - end + subject { service.execute(base_pipeline, head_pipeline) } + + context 'when head pipeline has secret_detection reports' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_secret_detection_report, project: project) } - it 'does not report any new vulnerability' do - expect(subject[:data]['added'].count).to eq(0) + it 'reports new vulnerabilities' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]['added'].count).to eq(1) + expect(subject[:data]['fixed'].count).to eq(0) + end end - it 'reports fixed secret_detection vulnerabilities' do - expect(subject[:data]['fixed'].count).to eq(1) - compare_keys = collect_ids(subject[:data]['fixed']) - expected_keys = %w(AWS) - expect(compare_keys).to match_array(expected_keys) + context 'when base and head pipelines have secret_detection reports' do + let_it_be(:base_pipeline) { create(:ee_ci_pipeline, :with_secret_detection_report, project: project) } + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :with_secret_detection_feature_branch, project: project) } + + it 'populates fields based on current_user' do + payload = subject[:data]['added'].first + expect(payload).to be_nil + expect(service.current_user).to eq(current_user) + end + + it 'does not report any new vulnerability' do + expect(subject[:data]['added'].count).to eq(0) + end + + it 'reports fixed secret_detection vulnerabilities' do + expect(subject[:data]['fixed'].count).to eq(1) + compare_keys = collect_ids(subject[:data]['fixed']) + expected_keys = %w(AWS) + expect(compare_keys).to match_array(expected_keys) + end end end end diff --git a/ee/spec/services/security/store_report_service_spec.rb b/ee/spec/services/security/store_report_service_spec.rb index 11285ab542f892f5bafa804928f9c9d79a8ae1e1..95264b40a8ab1dd0e17ab29c1171a2283fe8da60 100644 --- a/ee/spec/services/security/store_report_service_spec.rb +++ b/ee/spec/services/security/store_report_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Security::StoreReportService, '#execute' do + using RSpec::Parameterized::TableSyntax + let_it_be(:user) { create(:user) } let(:artifact) { create(:ee_ci_job_artifact, trait) } let(:report_type) { artifact.file_type } @@ -10,456 +12,464 @@ let(:pipeline) { artifact.job.pipeline } let(:report) { pipeline.security_reports.get_report(report_type.to_s, artifact) } - before do - stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true, security_dashboard: true) - allow(Security::AutoFixWorker).to receive(:perform_async) - end - subject { described_class.new(pipeline, report).execute } - context 'without existing data' do - before(:all) do - checksum = 'f00bc6261fa512f0960b7fc3bfcce7fb31997cf32b96fa647bed5668b2c77fee' - create(:vulnerabilities_remediation, checksum: checksum) - end - + where(vulnerability_finding_signatures_enabled: [true, false]) + with_them do before do - project.add_developer(user) - allow(pipeline).to receive(:user).and_return(user) + stub_feature_flags(vulnerability_finding_signatures: vulnerability_finding_signatures_enabled) + stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true, security_dashboard: true) + allow(Security::AutoFixWorker).to receive(:perform_async) end - context 'for different security reports' do - using RSpec::Parameterized::TableSyntax - - where(:case_name, :trait, :scanners, :identifiers, :findings, :finding_identifiers, :finding_pipelines, :remediations, :signatures, :optimize_sql_query_for_security_report_ff) do - 'with SAST report' | :sast | 1 | 6 | 5 | 7 | 5 | 0 | 2 | false - 'with exceeding identifiers' | :with_exceeding_identifiers | 1 | 20 | 1 | 20 | 1 | 0 | 0 | false - 'with Dependency Scanning report' | :dependency_scanning_remediation | 1 | 3 | 2 | 3 | 2 | 1 | 0 | false - 'with Container Scanning report' | :container_scanning | 1 | 8 | 8 | 8 | 8 | 0 | 0 | false - 'with SAST report' | :sast | 1 | 6 | 5 | 7 | 5 | 0 | 2 | true - 'with exceeding identifiers' | :with_exceeding_identifiers | 1 | 20 | 1 | 20 | 1 | 0 | 0 | true - 'with Dependency Scanning report' | :dependency_scanning_remediation | 1 | 3 | 2 | 3 | 2 | 1 | 0 | true - 'with Container Scanning report' | :container_scanning | 1 | 8 | 8 | 8 | 8 | 0 | 0 | true + context 'without existing data' do + before(:all) do + checksum = 'f00bc6261fa512f0960b7fc3bfcce7fb31997cf32b96fa647bed5668b2c77fee' + create(:vulnerabilities_remediation, checksum: checksum) end - with_them do - before do - stub_feature_flags(optimize_sql_query_for_security_report: optimize_sql_query_for_security_report_ff) - end + before do + project.add_developer(user) + allow(pipeline).to receive(:user).and_return(user) + end - it 'inserts all scanners' do - expect { subject }.to change { Vulnerabilities::Scanner.count }.by(scanners) - end + context 'for different security reports' do + using RSpec::Parameterized::TableSyntax - it 'inserts all identifiers' do - expect { subject }.to change { Vulnerabilities::Identifier.count }.by(identifiers) - end + with_them do + before do + stub_feature_flags(optimize_sql_query_for_security_report: optimize_sql_query_for_security_report_ff) + end - it 'inserts all findings' do - expect { subject }.to change { Vulnerabilities::Finding.count }.by(findings) - end + where(:case_name, :trait, :scanners, :identifiers, :findings, :finding_identifiers, :finding_pipelines, :remediations, :signatures) do + 'with SAST report' | :sast | 1 | 6 | 5 | 7 | 5 | 0 | 2 + 'with exceeding identifiers' | :with_exceeding_identifiers | 1 | 20 | 1 | 20 | 1 | 0 | 1 + 'with Dependency Scanning report' | :dependency_scanning_remediation | 1 | 3 | 2 | 3 | 2 | 1 | 2 + 'with Container Scanning report' | :container_scanning | 1 | 8 | 8 | 8 | 8 | 0 | 8 + end - it 'inserts all finding identifiers (join model)' do - expect { subject }.to change { Vulnerabilities::FindingIdentifier.count }.by(finding_identifiers) - end + it 'inserts all scanners' do + expect { subject }.to change { Vulnerabilities::Scanner.count }.by(scanners) + end - it 'inserts all finding pipelines (join model)' do - expect { subject }.to change { Vulnerabilities::FindingPipeline.count }.by(finding_pipelines) - end + it 'inserts all identifiers' do + expect { subject }.to change { Vulnerabilities::Identifier.count }.by(identifiers) + end - it 'inserts all remediations' do - expect { subject }.to change { project.vulnerability_remediations.count }.by(remediations) - end + it 'inserts all findings' do + expect { subject }.to change { Vulnerabilities::Finding.count }.by(findings) + end - it 'inserts all vulnerabilities' do - expect { subject }.to change { Vulnerability.count }.by(findings) - end + it 'inserts all finding identifiers (join model)' do + expect { subject }.to change { Vulnerabilities::FindingIdentifier.count }.by(finding_identifiers) + end + + it 'inserts all finding pipelines (join model)' do + expect { subject }.to change { Vulnerabilities::FindingPipeline.count }.by(finding_pipelines) + end + + it 'inserts all remediations' do + expect { subject }.to change { project.vulnerability_remediations.count }.by(remediations) + end + + it 'inserts all vulnerabilities' do + expect { subject }.to change { Vulnerability.count }.by(findings) + end - it 'inserts all signatures' do - expect { subject }.to change { Vulnerabilities::FindingSignature.count }.by(signatures) + it 'inserts all signatures' do + expect { subject }.to change { Vulnerabilities::FindingSignature.count }.by(signatures) + end end end - end - context 'when there is an exception' do - let(:trait) { :sast } + context 'when there is an exception' do + let(:trait) { :sast } - subject { described_class.new(pipeline, report) } + subject { described_class.new(pipeline, report) } - it 'does not insert any scanner' do - allow(Vulnerabilities::Scanner).to receive(:insert_all).with(anything).and_raise(StandardError) - expect { subject.send(:update_vulnerability_scanners!, report.findings) }.to change { Vulnerabilities::Scanner.count }.by(0) + it 'does not insert any scanner' do + allow(Vulnerabilities::Scanner).to receive(:insert_all).with(anything).and_raise(StandardError) + expect { subject.send(:update_vulnerability_scanners!, report.findings) }.to change { Vulnerabilities::Scanner.count }.by(0) + end end - end - context 'when N+1 database queries have been removed' do - let(:trait) { :sast } - let(:bandit_scanner) { build(:ci_reports_security_scanner, external_id: 'bandit', name: 'Bandit') } + context 'when N+1 database queries have been removed' do + let(:trait) { :sast } + let(:bandit_scanner) { build(:ci_reports_security_scanner, external_id: 'bandit', name: 'Bandit') } - subject { described_class.new(pipeline, report) } + subject { described_class.new(pipeline, report) } - it "avoids N+1 database queries for updating vulnerability scanners", :use_sql_query_cache do - report.add_scanner(bandit_scanner) + it "avoids N+1 database queries for updating vulnerability scanners", :use_sql_query_cache do + report.add_scanner(bandit_scanner) - control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { subject.send(:update_vulnerability_scanners!, report.findings) }.count + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { subject.send(:update_vulnerability_scanners!, report.findings) }.count - 5.times { report.add_finding(build(:ci_reports_security_finding, scanner: bandit_scanner)) } + 5.times { report.add_finding(build(:ci_reports_security_finding, scanner: bandit_scanner)) } - expect { described_class.new(pipeline, report).send(:update_vulnerability_scanners!, report.findings) }.not_to exceed_query_limit(control_count) + expect { described_class.new(pipeline, report).send(:update_vulnerability_scanners!, report.findings) }.not_to exceed_query_limit(control_count) + end end - end - context 'when report data includes all raw_metadata' do - let(:trait) { :dependency_scanning_remediation } + context 'when report data includes all raw_metadata' do + let(:trait) { :dependency_scanning_remediation } - it 'inserts top level finding data', :aggregate_failures do - subject + it 'inserts top level finding data', :aggregate_failures do + subject - finding = Vulnerabilities::Finding.last - finding.raw_metadata = nil + 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 + 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 - end - context 'invalid data' do - let(:artifact) { create(:ee_ci_job_artifact, :sast) } - let(:finding_without_name) { build(:ci_reports_security_finding, name: nil) } - let(:report) { Gitlab::Ci::Reports::Security::Report.new('container_scanning', nil, nil) } + context 'invalid data' do + let(:artifact) { create(:ee_ci_job_artifact, :sast) } + let(:finding_without_name) { build(:ci_reports_security_finding, name: nil) } + let(:report) { Gitlab::Ci::Reports::Security::Report.new('container_scanning', nil, nil) } - before do - allow(Gitlab::ErrorTracking).to receive(:track_and_raise_exception).and_call_original - report.add_finding(finding_without_name) - end + before do + allow(Gitlab::ErrorTracking).to receive(:track_and_raise_exception).and_call_original + report.add_finding(finding_without_name) + end - it 'raises invalid record error' do - expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) - end + it 'raises invalid record error' do + expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) + end - it 'reports the error correctly' do - expected_params = finding_without_name.to_hash.dig(:raw_metadata) - expect { subject.execute }.to raise_error { |error| - expect(Gitlab::ErrorTracking).to have_received(:track_and_raise_exception).with(error, create_params: expected_params) - } + it 'reports the error correctly' do + expected_params = finding_without_name.to_hash.dig(:raw_metadata) + expect { subject.execute }.to raise_error { |error| + expect(Gitlab::ErrorTracking).to have_received(:track_and_raise_exception).with(error, create_params: expected_params) + } + end end end - end - context 'with existing data from previous pipeline' do - let(:finding_identifier_fingerprint) do - build(:ci_reports_security_identifier, external_id: "CIPHER_INTEGRITY").fingerprint - end + context 'with existing data from previous pipeline' do + let(:finding_identifier_fingerprint) do + build(:ci_reports_security_identifier, external_id: "CIPHER_INTEGRITY").fingerprint + end - let(:scanner) { build(:vulnerabilities_scanner, project: project, external_id: 'find_sec_bugs', name: 'Find Security Bugs') } - let(:identifier) { build(:vulnerabilities_identifier, project: project, fingerprint: finding_identifier_fingerprint) } - let(:different_identifier) { build(:vulnerabilities_identifier, project: project) } - let!(:new_artifact) { create(:ee_ci_job_artifact, :sast, job: new_build) } - let(:new_build) { create(:ci_build, pipeline: new_pipeline) } - let(:new_pipeline) { create(:ci_pipeline, project: project) } - let(:new_report) { new_pipeline.security_reports.get_report(report_type.to_s, artifact) } - let(:existing_signature) { create(:vulnerabilities_finding_signature, finding: finding) } - let(:unsupported_signature) do - create(:vulnerabilities_finding_signature, - finding: finding, - algorithm_type: ::Vulnerabilities::FindingSignature.algorithm_types[:location]) - end + let(:scanner) { build(:vulnerabilities_scanner, project: project, external_id: 'find_sec_bugs', name: 'Find Security Bugs') } + let(:identifier) { build(:vulnerabilities_identifier, project: project, fingerprint: finding_identifier_fingerprint) } + let(:different_identifier) { build(:vulnerabilities_identifier, project: project) } + let!(:new_artifact) { create(:ee_ci_job_artifact, :sast, job: new_build) } + let(:new_build) { create(:ci_build, pipeline: new_pipeline) } + let(:new_pipeline) { create(:ci_pipeline, project: project) } + let(:new_report) { new_pipeline.security_reports.get_report(report_type.to_s, artifact) } + let(:existing_signature) { create(:vulnerabilities_finding_signature, finding: finding) } - let(:trait) { :sast } + let(:trait) { :sast } - let(:finding_location_fingerprint) do - build( - :ci_reports_security_locations_sast, - file_path: "groovy/src/main/java/com/gitlab/security_products/tests/App.groovy", - start_line: "29", - end_line: "29" - ).fingerprint - end + let(:finding_location_fingerprint) do + build( + :ci_reports_security_locations_sast, + file_path: "groovy/src/main/java/com/gitlab/security_products/tests/App.groovy", + start_line: "29", + end_line: "29" + ).fingerprint + end - let!(:finding) do - create(:vulnerabilities_finding, - pipelines: [pipeline], - identifiers: [identifier], - primary_identifier: identifier, - scanner: scanner, - project: project, - uuid: "e5388f40-18f5-566d-95c6-d64c6f46a00a", - location_fingerprint: finding_location_fingerprint) - end + let!(:finding) do + created_finding = create(:vulnerabilities_finding, + pipelines: [pipeline], + identifiers: [identifier], + primary_identifier: identifier, + scanner: scanner, + project: project, + uuid: "e5388f40-18f5-566d-95c6-d64c6f46a00a", + location_fingerprint: finding_location_fingerprint) - let!(:vulnerability) { create(:vulnerability, findings: [finding], project: project) } + existing_finding = report.findings.find { |f| f.location.fingerprint == created_finding.location_fingerprint } - let(:desired_uuid) do - Security::VulnerabilityUUID.generate( - report_type: finding.report_type, - primary_identifier_fingerprint: finding.primary_identifier.fingerprint, - location_fingerprint: finding.location_fingerprint, - project_id: finding.project_id - ) - end + create(:vulnerabilities_finding_signature, + finding: created_finding, + algorithm_type: existing_finding.signatures.first.algorithm_type, + signature_sha: existing_finding.signatures.first.signature_sha) - let!(:finding_with_uuidv5) do - create(:vulnerabilities_finding, - pipelines: [pipeline], - identifiers: [different_identifier], - primary_identifier: different_identifier, - scanner: scanner, - project: project, - location_fingerprint: '34661e23abcf78ff80dfcc89d0700437612e3f88') - end + created_finding + end - let!(:vulnerability_with_uuid5) { create(:vulnerability, findings: [finding_with_uuidv5], project: project) } + let!(:vulnerability) { create(:vulnerability, findings: [finding], project: project) } - before do - project.add_developer(user) - allow(new_pipeline).to receive(:user).and_return(user) - end + let(:desired_uuid) do + Security::VulnerabilityUUID.generate( + report_type: finding.report_type, + primary_identifier_fingerprint: finding.primary_identifier.fingerprint, + location_fingerprint: finding.location_fingerprint, + project_id: finding.project_id + ) + end - subject { described_class.new(new_pipeline, new_report).execute } + let!(:finding_with_uuidv5) do + create(:vulnerabilities_finding, + pipelines: [pipeline], + identifiers: [different_identifier], + primary_identifier: different_identifier, + scanner: scanner, + project: project, + location_fingerprint: '34661e23abcf78ff80dfcc89d0700437612e3f88') + end - it 'does not change existing UUIDv5' do - expect { subject }.not_to change(finding_with_uuidv5, :uuid) - end + let!(:vulnerability_with_uuid5) { create(:vulnerability, findings: [finding_with_uuidv5], project: project) } - it 'updates UUIDv4 to UUIDv5' do - subject + before do + project.add_developer(user) + allow(new_pipeline).to receive(:user).and_return(user) + end - expect(finding.reload.uuid).to eq(desired_uuid) - end + subject { described_class.new(new_pipeline, new_report).execute } - it 'reuses existing scanner' do - expect { subject }.not_to change { Vulnerabilities::Scanner.count } - end + it 'does not change existing UUIDv5' do + expect { subject }.not_to change(finding_with_uuidv5, :uuid) + end - it 'inserts only new identifiers and reuse existing ones' do - expect { subject }.to change { Vulnerabilities::Identifier.count }.by(5) - end + it 'updates UUIDv4 to UUIDv5' do + finding.uuid = '00000000-1111-2222-3333-444444444444' + finding.save! - it 'inserts only new findings and reuse existing ones' do - expect { subject }.to change { Vulnerabilities::Finding.count }.by(4) - end + # this report_finding should be used to update the finding's uuid + report_finding = new_report.findings.find { |f| f.location.fingerprint == '0e7d0291d912f56880e39d4fbd80d99dd5d327ba' } + allow(report_finding).to receive(:uuid).and_return(desired_uuid) + report_finding.signatures.pop - it 'inserts all finding pipelines (join model) for this new pipeline' do - expect { subject }.to change { Vulnerabilities::FindingPipeline.where(pipeline: new_pipeline).count }.by(5) - end + subject - it 'inserts new vulnerabilities with data from findings from this new pipeline' do - expect { subject }.to change { Vulnerability.count }.by(4) - end + expect(finding.reload.uuid).to eq(desired_uuid) + end - it 'updates existing findings with new data' do - subject + it 'reuses existing scanner' do + expect { subject }.not_to change { Vulnerabilities::Scanner.count } + end - expect(finding.reload).to have_attributes(severity: 'medium', name: 'Cipher with no integrity') - end + it 'inserts only new identifiers and reuse existing ones' do + expect { subject }.to change { Vulnerabilities::Identifier.count }.by(5) + end - it 'updates signatures to match new values' do - existing_signature - unsupported_signature + it 'inserts only new findings and reuse existing ones' do + expect { subject }.to change { Vulnerabilities::Finding.count }.by(4) + end - expect(finding.signatures.count).to eq(2) - signature_algs = finding.signatures.map(&:algorithm_type).sort - expect(signature_algs).to eq(%w[hash location]) + it 'inserts all finding pipelines (join model) for this new pipeline' do + expect { subject }.to change { Vulnerabilities::FindingPipeline.where(pipeline: new_pipeline).count }.by(5) + end - subject + it 'inserts new vulnerabilities with data from findings from this new pipeline' do + expect { subject }.to change { Vulnerability.count }.by(4) + end - finding.reload - existing_signature.reload + it 'updates existing findings with new data' do + subject - # check that unsupported algorithm is not deleted - expect(finding.signatures.count).to eq(3) - signature_algs = finding.signatures.sort.map(&:algorithm_type) - expect(signature_algs).to eq(%w[hash location scope_offset]) + expect(finding.reload).to have_attributes(severity: 'medium', name: 'Cipher with no integrity') + end - # check that the existing hash signature was updated/reused - expect(existing_signature.id).to eq(finding.signatures.min.id) + it 'updates signatures to match new values' do + next unless vulnerability_finding_signatures_enabled - # check that the unsupported signature was not deleted - expect(::Vulnerabilities::FindingSignature.exists?(unsupported_signature.id)).to eq(true) - end + expect(finding.signatures.count).to eq(1) + expect(finding.signatures.first.algorithm_type).to eq('hash') - it 'updates existing vulnerability with new data' do - subject + existing_signature = finding.signatures.first - expect(vulnerability.reload).to have_attributes(severity: 'medium', title: 'Cipher with no integrity', title_html: 'Cipher with no integrity') - end + subject - context 'when the existing vulnerability is resolved with the latest report' do - let!(:existing_vulnerability) { create(:vulnerability, report_type: report_type, project: project) } + finding.reload + existing_signature.reload - it 'marks the vulnerability as resolved on default branch' do - expect { subject }.to change { existing_vulnerability.reload.resolved_on_default_branch }.from(false).to(true) - end - end + expect(finding.signatures.count).to eq(2) + signature_algs = finding.signatures.sort_by(&:priority).map(&:algorithm_type) + expect(signature_algs).to eq(%w[hash scope_offset]) - context 'when the existing resolved vulnerability is discovered again on the latest report' do - before do - vulnerability.update!(resolved_on_default_branch: true) + # check that the existing hash signature was updated/reused + expect(existing_signature.id).to eq(finding.signatures.find(&:algorithm_hash?).id) end - it 'marks the vulnerability as not resolved on default branch' do - expect { subject }.to change { vulnerability.reload.resolved_on_default_branch }.from(true).to(false) - end - end + it 'updates existing vulnerability with new data' do + subject - context 'when the finding is not valid' do - before do - allow(Gitlab::AppLogger).to receive(:warn) - allow_next_instance_of(::Gitlab::Ci::Reports::Security::Finding) do |finding| - allow(finding).to receive(:valid?).and_return(false) - end + expect(vulnerability.reload).to have_attributes(severity: 'medium', title: 'Cipher with no integrity', title_html: 'Cipher with no integrity') end - it 'does not create a new finding' do - expect { subject }.not_to change { Vulnerabilities::Finding.count } - end + context 'when the existing vulnerability is resolved with the latest report' do + let!(:existing_vulnerability) { create(:vulnerability, report_type: report_type, project: project) } - it 'does not raise an error' do - expect { subject }.not_to raise_error + it 'marks the vulnerability as resolved on default branch' do + expect { subject }.to change { existing_vulnerability.reload.resolved_on_default_branch }.from(false).to(true) + end end - it 'puts a warning log' do - subject + context 'when the existing resolved vulnerability is discovered again on the latest report' do + before do + vulnerability.update_column(:resolved_on_default_branch, true) + end - expect(Gitlab::AppLogger).to have_received(:warn).exactly(new_report.findings.length).times + it 'marks the vulnerability as not resolved on default branch' do + expect { subject }.to change { vulnerability.reload.resolved_on_default_branch }.from(true).to(false) + end end - end - context 'vulnerability issue link' do - context 'when there is no associated issue feedback with finding' do - it 'does not insert issue links from the new pipeline' do - expect { subject }.to change { Vulnerabilities::IssueLink.count }.by(0) + context 'when the finding is not valid' do + before do + allow(Gitlab::AppLogger).to receive(:warn) + allow_next_instance_of(::Gitlab::Ci::Reports::Security::Finding) do |finding| + allow(finding).to receive(:valid?).and_return(false) + end end - end - context 'when there is an associated issue feedback with finding' do - let(:issue) { create(:issue, project: project) } - let!(:issue_feedback) do - create( - :vulnerability_feedback, - :sast, - :issue, - issue: issue, - project: project, - project_fingerprint: new_report.findings.first.project_fingerprint - ) + it 'does not create a new finding' do + expect { subject }.not_to change { Vulnerabilities::Finding.count } end - it 'inserts issue links from the new pipeline' do - expect { subject }.to change { Vulnerabilities::IssueLink.count }.by(1) + it 'does not raise an error' do + expect { subject }.not_to raise_error end - it 'the issue link is valid' do + it 'puts a warning log' do subject - finding = Vulnerabilities::Finding.find_by(uuid: new_report.findings.first.uuid) - vulnerability_id = finding.vulnerability_id - issue_id = issue.id - issue_link = Vulnerabilities::IssueLink.find_by( - vulnerability_id: vulnerability_id, - issue_id: issue_id - ) - - expect(issue_link).not_to be_nil + expect(Gitlab::AppLogger).to have_received(:warn).exactly(new_report.findings.length).times end end - end - end - context 'with existing data from same pipeline' do - let!(:finding) { create(:vulnerabilities_finding, project: project, pipelines: [pipeline]) } - let(:trait) { :sast } + context 'vulnerability issue link' do + context 'when there is no assoiciated issue feedback with finding' do + it 'does not insert issue links from the new pipeline' do + expect { subject }.to change { Vulnerabilities::IssueLink.count }.by(0) + end + end - it 'skips report' do - expect(subject).to eq({ - status: :error, - message: "sast report already stored for this pipeline, skipping..." - }) - end - end + context 'when there is an associated issue feedback with finding' do + let(:issue) { create(:issue, project: project) } + let!(:issue_feedback) do + create( + :vulnerability_feedback, + :sast, + :issue, + issue: issue, + project: project, + project_fingerprint: new_report.findings.first.project_fingerprint + ) + end - context 'start auto_fix' do - before do - stub_licensed_features(vulnerability_auto_fix: true) - end + it 'inserts issue links from the new pipeline' do + expect { subject }.to change { Vulnerabilities::IssueLink.count }.by(1) + end - context 'with auto fix supported report type' do - let(:trait) { :dependency_scanning } + it 'the issue link is valid' do + subject - context 'when auto fix enabled' do - it 'start auto fix worker' do - expect(Security::AutoFixWorker).to receive(:perform_async).with(pipeline.id) + finding = Vulnerabilities::Finding.find_by(uuid: new_report.findings.first.uuid) + vulnerability_id = finding.vulnerability_id + issue_id = issue.id + issue_link = Vulnerabilities::IssueLink.find_by( + vulnerability_id: vulnerability_id, + issue_id: issue_id + ) - subject + expect(issue_link).not_to be_nil + end end end + end - context 'when auto fix disabled' do - context 'when feature flag is disabled' do - before do - stub_feature_flags(security_auto_fix: false) - end + context 'with existing data from same pipeline' do + let!(:finding) { create(:vulnerabilities_finding, project: project, pipelines: [pipeline]) } + let(:trait) { :sast } - it 'does not start auto fix worker' do - expect(Security::AutoFixWorker).not_to receive(:perform_async) + it 'skips report' do + expect(subject).to eq({ + status: :error, + message: "sast report already stored for this pipeline, skipping..." + }) + end + end - subject - end - end + context 'start auto_fix' do + before do + stub_licensed_features(vulnerability_auto_fix: true) + end - context 'when auto fix feature is disabled' do - before do - project.security_setting.update!(auto_fix_dependency_scanning: false) - end + context 'with auto fix supported report type' do + let(:trait) { :dependency_scanning } - it 'does not start auto fix worker' do - expect(Security::AutoFixWorker).not_to receive(:perform_async) + context 'when auto fix enabled' do + it 'start auto fix worker' do + expect(Security::AutoFixWorker).to receive(:perform_async).with(pipeline.id) subject end end - context 'when licensed feature is unavailable' do - before do - stub_licensed_features(vulnerability_auto_fix: false) + context 'when auto fix disabled' do + context 'when feature flag is disabled' do + before do + stub_feature_flags(security_auto_fix: false) + end + + it 'does not start auto fix worker' do + expect(Security::AutoFixWorker).not_to receive(:perform_async) + + subject + end end - it 'does not start auto fix worker' do - expect(Security::AutoFixWorker).not_to receive(:perform_async) + context 'when auto fix feature is disabled' do + before do + project.security_setting.update_column(:auto_fix_dependency_scanning, false) + end - subject + it 'does not start auto fix worker' do + expect(Security::AutoFixWorker).not_to receive(:perform_async) + + subject + end end - end - context 'when security setting is not created' do - before do - project.security_setting.destroy! - project.reload + context 'when licensed feature is unavailable' do + before do + stub_licensed_features(vulnerability_auto_fix: false) + end + + it 'does not start auto fix worker' do + expect(Security::AutoFixWorker).not_to receive(:perform_async) + + subject + end end - it 'does not start auto fix worker' do - expect(Security::AutoFixWorker).not_to receive(:perform_async) - expect(subject[:status]).to eq(:success) + context 'when security setting is not created' do + before do + project.security_setting.destroy! + project.reload + end + + it 'does not start auto fix worker' do + expect(Security::AutoFixWorker).not_to receive(:perform_async) + expect(subject[:status]).to eq(:success) + end end end end - end - context 'with auto fix not supported report type' do - let(:trait) { :sast } + context 'with auto fix not supported report type' do + let(:trait) { :sast } - before do - stub_licensed_features(vulnerability_auto_fix: true) - end + before do + stub_licensed_features(vulnerability_auto_fix: true) + end - it 'does not start auto fix worker' do - expect(Security::AutoFixWorker).not_to receive(:perform_async) + it 'does not start auto fix worker' do + expect(Security::AutoFixWorker).not_to receive(:perform_async) - subject + subject + end end end end