Skip to content

Do not show Findings dismissed on the default branch in the Security scanning MR widget

Why are we doing this work?

The discussion on Dismissed vulnerabilities showing up in Merge R... (#330042 - closed) yielded the following description of the desired behavior of the Security scanning MR widget best described as follows:

  • finding dismissed in default branch: always hide in MR widget
  • finding dismissed on branch: show in MR widget with a badge/strikethrough

Essentially we need to hide Findings that are present on the default branch and have been dismissed and expose Findings that are not present on the default branch and have been dismissed

Steps to reproduce

See #330042 (comment 1263880687)

1st Implementation plan

Read More
  1. Adjust lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb to:
    • Gather UUIDs of added and fixed vulnerabilities
    • SELECT vulnerability_id INNER JOIN vulnerabilties ON vulnerabilties_occurrences.vulnerability_id = vulnerabilities.id AND vulnerabilities.present_on_default_branch = true FROM vulnerabilties_occurrences WHERE uuid IN (list of UUIDS here) AND vulnerabilities.state = 2
    • Remove rows matching the above query from the result

Explanation: both app/models/merge_request.rb and ee/app/models/ee/merge_request.rb call ee/app/services/ci/compare_security_reports_service.rb which uses lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb under the hood. This class has added and fixed methods which could be changed to filter out dismissed Vulnerabilities.

I don't think we can do it further down the stack because we use Vulnerabilities::FindingEntity inside of Vulnerabilities::FindingReportsComparerEntity to serialize the results and the serializer does the expose :dismissal_feedback, using: Vulnerabilities::FeedbackEntity for us.

1st Implementation Result

Upon delivery of the first implementation in which we would take the UUID's of the vulnerabilities taken from the vulnerability scan job artifacts, and check them against known dismissed UUID's to filter them from the sets that would be presented in the MR Secuirty comparison widget.

Unfortunately that fix proved to be unsuccessful due to a separate problem with the current implementation of the vulnerability comparison report in which the UUID's of vulnerabilities detected in job artifacts do not match with the UUID's of persisted vulnerabilities in the database, meaning the prior fixes's implementation for filtering dismissed vulnerabilities will not work in all instances. Unfortunately, to reconcile all the possible UUID's for a report and then filter off the dismissed instances would be extremely expensive and prohibitive to calculate with the current architecture. As a result, we need to rework the vulnerability_reports_comparison to use persisted vulnerability information to avoid this expensive and make this filtering appropriately feasible.

This was unfortunately not an expected result of our first attempt at fixing this.

2nd Implementation Plan

Current expectation is that the implementation of Use security_findings for security MR widget re... (#390185 - closed) • Rushik Subba • 17.9 • On track will probably fix this implicitly.

Edited by Gregory Havenga