Skip to content

Create data migration to automatically resolve findings from removed SAST analyzers

As part of the %17.0 consolidation of SAST analyzers, we plan to have a data migration clean up findings from analyzers that have been removed. This is to mitigate confusion related to any findings that are not produced by the new analyzer. The new analyzer may not create the same finding because:

  • The relevant rule cannot, for technical reasons, be migrated to Semgrep-based scanning.
  • The relevant rule has been removed from the ruleset or not converted/translated/migrated because we have judged that it doesn't provide sufficient security value.
  • The old analyzer stopped creating the finding due to a previous update (unrelated to analyzer consolidation) and the finding just still remains.

Note that this migration will resolve findings, not dismiss them. By design, resolved findings will be reopened if an analyzer finds the same problem again at the same location. (Dismissed findings don't do this.)

Scope

The migration should resolve findings in all analyzers that have been removed, including those removed in previous releases before 17.0. See the deprecation notices for a list of previous and upcoming changes:

Timing

This migration must only be released in 17.0, not an earlier version. It also must be timed such that it does not take effect on GitLab.com until the SAST.gitlab-ci.yml template is updated to remove the affected analyzers.

Implementation Plan

  1. Create a Batched background migration to resolve all vulnerbilities in a detected state belonging to the following scanners:

    • eslint
    • gosec
    • bandit
    • security_code_scan
    • brakeman
    • flawfinder
    • mobsf
    • njsscan
    • nodejs-scan
    • nodejs_scan
    • phpcs_security_audit

    The batched background migration must implement the same functionality as Vulnerabilities::ResolveService, which does the following:

    1. Inserts a new vulnerability_state_transitions record, containing the current state and the new resolved state.

    2. Updates the vulnerabilities record and sets the state to resolved.

      There's a trigger on the vulnerabilities table which automatically updates the vulnerability_reads table after the vulnerabilities table is updated, so we don't need to explicitly add an SQL call to do this for us.

    3. Updates the vulnerabilities_reads table and sets the dismissal_reason to null.

    4. Inserts a new record into the notes table with a comment explaining why the vulnerability was resolved.

    5. Inserts a new system_note_metadata record indicating that the vulnerability was resolved.

    6. Deletes records from the vulnerability_user_mentions table matching the vulnerability.id and the note.id for the note record created in step 4 above.

    7. Updates the vulnerability_statistics entry for the project_id related to the vulnerability being resolved.

    The user/author for resolving vulnerabilities and creating system notes should be Users::Internal#security_bot.

    The comment for resolving vulnerabilities should be:

    This vulnerability was automatically resolved because it was created by an analyzer that has been removed from GitLab SAST.

    The following is a rough implementation of the migration, bearing in mind that we need to replace Vulnerabilities::ResolveService(...).execute with code that implements the same functionality in the migration, since using application code in migrations is discouraged:

    Click to expand 
    user = Users::Internal.security_bot
comment = 'This vulnerability was automatically resolved because it was created by an analyzer that has been removed from GitLab SAST.'

removed_scanners = %w[
  eslint
  gosec
  bandit
  security_code_scan
  brakeman
  flawfinder
  mobsf
  njsscan
  nodejs-scan
  nodejs_scan
  phpcs_security_audit
]

Vulnerabilities::Read
  .with_scanner_external_ids(scanner_external_ids)
  .with_states([Enums::Vulnerability.vulnerability_states[:detected]]).each_batch(of: 100) do |vulnerability_read_batch|
  vulnerability_read_batch.each do |vulnerability_read|
    ::Vulnerabilities::ResolveService.new(user, vulnerability_read.vulnerability, comment).execute
  end
end

  2. Add unit tests for the above code.

  3. Create query plans and optimize the queries, adding indexes where necessary.

  4. Add docs for this migration.

  5. Add a release post item for this migration.

  6. Remove tmp_index_vulnerability_reads_where_state_is_detected index as part of Remove temporary index added by vulnerability r... (#475161 - closed) • Unassigned • 17.6.

    To be completed in %17.6. See this discussion for details.

  7. Finalize migration for resolving vulnerabilitie... (#481944 - closed) • Brian Williams • 17.6

    To be completed in %17.6.

Note: the MR containing the migration was merged in %17.3, however, there was a bug which prevented the migration from working properly. By the time the bug was fixed, %17.3 had already shipped, so we had to backport the bugfix to 17.3.1. See this comment for more details.

Note: This is a type of automatic resolution, but it is not the same as the SAST feature that's specifically called automatic vulnerability resolution, which cleans up findings when rules are disabled or deleted.

Edited by Adam Cohen