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: - [17.0 notice](https://docs.gitlab.com/ee/update/deprecations.html#sast-analyzer-coverage-changing-in-gitlab-170) - [16.0 notice](https://docs.gitlab.com/ee/update/deprecations.html#sast-analyzer-coverage-changing-in-gitlab-160) - [15.4 notice](https://docs.gitlab.com/ee/update/deprecations.html#sast-analyzer-consolidation-and-cicd-template-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. [x] Create a [Batched background migration](https://docs.gitlab.com/ee/development/database/batched_background_migrations.html) 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](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/ee/lib/api/vulnerabilities.rb#L57-57), which does the following: 1. [Inserts](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/ee/app/services/vulnerabilities/base_state_transition_service.rb#L15) a new `vulnerability_state_transitions` record, containing the current state and the new [resolved](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/app/models/concerns/enums/vulnerability.rb#L45-45) state. 1. [Updates](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/ee/app/services/vulnerabilities/base_service.rb#L26) the `vulnerabilities` record and sets the state to [resolved](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/app/models/concerns/enums/vulnerability.rb#L45-45). There's a [trigger](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/db/structure.sql#L31589) 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. 1. [Updates](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/ee/app/services/vulnerabilities/base_state_transition_service.rb#L37) the `vulnerabilities_reads` table and sets the `dismissal_reason` to `null`. 1. [Inserts](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/app/services/system_notes/base_service.rb#L19) a new record into the `notes` table with a comment explaining why the vulnerability was resolved. 1. [Inserts](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/app/services/system_notes/base_service.rb#L17) a new `system_note_metadata` record indicating that the vulnerability was resolved. 1. [Deletes](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/app/models/concerns/cache_markdown_field.rb#L180) records from the `vulnerability_user_mentions` table matching the `vulnerability.id` and the `note.id` for the `note` record created in step `4` above. 1. [Updates](https://gitlab.com/gitlab-org/gitlab/blob/18dc5fe8566e/ee/app/services/vulnerabilities/base_service.rb#L22) 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](https://gitlab.com/gitlab-org/gitlab/blob/ca5c4cccf19c/lib/users/internal.rb#L40-40). The comment for resolving vulnerabilities should be: ```shell 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](https://docs.gitlab.com/ee/development/migration_style_guide.html#using-application-code-in-migrations-discouraged): <details><summary>Click to expand</summary> ```ruby 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 ``` </details> 1. [x] Add unit tests for the above code. 1. [x] Create query plans and optimize the queries, adding indexes where necessary. 1. [x] Add docs for this migration. - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/163586+s 1. [x] Add a release post item for this migration. - https://gitlab.com/gitlab-com/www-gitlab-com/-/merge_requests/135930+s 1. [ ] Remove `tmp_index_vulnerability_reads_where_state_is_detected` index as part of https://gitlab.com/gitlab-org/gitlab/-/issues/475161+s. To be completed in %"17.6". See [this discussion](https://gitlab.com/gitlab-org/gitlab/-/issues/475161#note_2081655373) for details. 1. [ ] https://gitlab.com/gitlab-org/gitlab/-/issues/481944+s To be completed in %"17.6". **Note:** the [MR](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/157776) 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](https://gitlab.com/gitlab-org/gitlab/-/issues/444926#note_2045796963) 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](https://docs.gitlab.com/ee/user/application_security/sast/#automatic-vulnerability-resolution), which cleans up findings when rules are disabled or deleted.