Security MR widget shows all findings as new when default branch has missing security reports (including when the CI is configured to make multiple pipelines)

Why are we doing this work

Whenever the latest pipeline on the default branch is missing security report artifacts that are present on an up-to-date MR, the security widget on the MR shows all findings from the missing security report as new.

Examples of when this can happen:

  • Scheduled pipeline that do not include security scans
  • Use of scan execution policies to scan the default branch
  • Use of conditional includes for pipeline optimization, such as skipping scans when the latest commit only changed *.md files

See internal note for an example.

Steps to reproduce:

  1. Run pipeline on the default branch that produces security reports.
  2. Create a branch with no changes to security scans, push, and create an MR.
  3. Run a pipeline on the default branch without security reports; e.g.: set DEPENDENCY_SCANNING_DISABLED=1 on the "run pipeline" UI.
  4. (optional) run another pipeline disabling all security reports; e.g. as above, and SAST_DISABLED=1.

Screenshots

After step 2 above

image

After step 3 above

image

After step 4 above

image

Relevant links

Non-functional requirements

  • Documentation:
  • Feature flag:
  • Performance:
  • Testing:

Implementation plan

Essentially the issue here is that we select the most recent pipeline with the sha of the merge-base commit of the MR branch regardless of whether that pipeline has run any scans of the type we are comparing. See https://gitlab.com/gitlab-org/gitlab/-/blob/b789c566db51ad451a7ed6c686d32551502e0abf/app/models/merge_request.rb#L1890-1894 called from https://gitlab.com/gitlab-org/gitlab/-/blob/b789c566db51ad451a7ed6c686d32551502e0abf/app/models/merge_request.rb#L1733-1742

We would eventually like to move towards a solution that directly uses findings instead of security reports for the MR comparison, as is discussed in the comments re using the same solution as groupsecurity policies. We have an issue already for this work Use security_findings for security MR widget re... (#390185 - closed) however this is blocked on some data migrations that need to occur before we are ready to implement.

Therefore the recommended implementation plan for this issue is to continue using the approach we have now, but instead of selecting the most recent pipeline for the merge-base commit, select the most recent pipeline for the merge-base commit that has a scan of the required type.

  • Create new methods to select the latest pipeline with a report of the required type, e.g. MergeRequest#latest_base_pipeline_with_report_type(report_type) and MergeRequest#latest_merge_base_pipeline_with_report_type(report_type). Here's a naive proof of concept as an example:
mr = MergeRequest.last

# Get all of the pipelines for the merge-base.  ID 803 does not have a dependency_scanning artifact.
mr.project.ci_pipelines.order(id: :desc).where(sha: mr.diff_base_sha, ref: mr.target_branch).map(&:id)
# => [803, 800, 799]

# Get all of the pipelines for the merge-base that have a dependency_scanning artifact.  Note the list does not contain 803.
mr.project.ci_pipelines.order(id: :desc).where(sha: mr.diff_base_sha, ref: mr.target_branch).select do |pipeline|
  pipeline.job_artifacts.with_file_types(['dependency_scanning']).present?
end.map(&:id)
# => [800, 799]
  • Modify MergeRequest#comparison_base_pipeline to call these methods when appropriate - e.g. when the service_class is CompareSecurityReportsService.

Verification steps

See 'steps to reproduce' above.

Testing

  • Make sure e2e: package-and-test is triggered in the MR and govern specs are green so that there's no regression.
Edited by Harsha Muralidhar