Reconsider the UX of interacting with Vulnerabilities in the MR

Problem

1. Today the Vulnerability Management workflow in the MR is not aligned with the existing code review workflows and is not optimized for engineers and code reviewers. This leads to a fractured experience when teams have to collaborate on vulnerability findings in the MR.
2. Users are unhappy that engineers/developers/non-security team members can dismiss vulnerabilities which, can suppress results from the vulnerability reports, hiding them from the Security team. Users are unhappy that vulnerabilities dismissed this way contain no justification as to why they were dismissed and the security teams often have to add a reason after the fact. It is best practice, and often policy, to document why a vulnerability (that remains in the system) is dismissed.

Hypothesis

1. If we add the ability to create a review from a vulnerability then users will have the ability to block the MR and discuss a resolution, as is the pattern when there is a problem in the changed code.
2. If we remove the ability to dismiss a vulnerability from MR Authors then users will be less likely to misuse the dismissal action.
3. If we allow users to temporarily ignore vulnerabilities in the MR then users will not suppress important but non-blocking vulnerabilities from the Vulnerability Reports.

UX Problem

HMW increase the efficiency of security-centric reviews in the MR while reducing the possibility of misuse/confusion in regards to vulnerability dismissals?

JTBD

Primary: When committing changes to my project, I want to be made aware if I am adding risk through vulnerable code, so that I know my changes can be merged without increasing the risk of my project.

Secondary: When my code has been scanned, I want to resolve the confirmed business-critical vulnerabilities, So I can ensure my changes are compliant with my org's risk policies.

Secondary: When I am ready to release changes into production, I want to verify it is safe to release, So that I can release the changes responsibly.

Proposal

🖍 Figma file

WIP

• Add the ability to start a review from a vulnerability
  • Display a link to the vulnerability modal in the discussion thread
  • Display a link to the link of code in the review thread
  • Display the current status of the vulnerability in the review thread
• Add the ability to temporarily dismiss a vulnerability just for the MR (or branch)
• Restrict Commit Authors from dismissing vulnerabilities in the MR
• Require comments on permanent dismissals
• If user activity = Ignore for this MR and MR is merged, add this activity to the vulnerability detail

Further details

Previous background (relevant but out of date)

Background & Reasoning

Actions

Actions in the MR should be as simple as possible, often binary in nature. They consist of [Must do] and [Don’t need to do] in order to merge. The latter isn’t necessarily an action, rather it is the absence of action. The former can be represented in the form of a review/comment or another type of action that makes the author aware of additional work required prior to merging their changes.

Must-do actions:

Starting a review (in the immediate) is an action that represents the need for action, and matches the current patterns for code review. When a review is started the changes cannot be merged until there is a resolution. In the case of a vulnerability, either the vulnerability needs to be fixed/removed, or further conversation is needed in the form of an issue. Additionally, Creating an issue from a vulnerability is an action that requires subsequent action in the future. The author and reviewer have agreed that immediate action is not required but some level of action is at a later date.

Don't need to do actions:

Dismissing a vulnerability (in the immediate) is an action that represents the need for inaction. This runs counter to the experience in the Merge Request, and there is no equivalent to date on items in the merge checks. Approvals on the other hand do indicate that no further action is required however these operate on a higher level and dismissals and an argument can be made that approvals are akin to dismissals, but again, the effort required and the resulting system response (vuln status=dismissed vs MR=can be merged) are not comparable. Staying on the topic of approvals; users can dismiss any and all vulnerabilities irrespective of the author who introduced them. If the comparison is made to approvals, then users should not be able to dismiss vulnerabilities they introduced as they cannot approve Merge Requests created by their changes.

Domain expertise

More often than not, work in the Merge Request is the engineer's domain. As companies shift newly-detected vulnerability resolution left, (new vulnerability_findings in GitLab) to the responsibility of the developer, we can expect a dropoff in the need/effort required to make a case for remediation as devs typically follow standards provided by their organization. These standards are often primitive but still serve as rules for what can and cannot be merged into any given environment. Additionally, engineers do not need to triage, prioritize, and provide the same level of information as security engineers do when vulnerabilities are detected (or reported) in the application or its infrastructure. This information is necessary as further justification of effort required by the business as a case needs to be made to weigh a feature against a security fix. Once more, this information is required to provide an auditable history of vulnerabilities detected in the application as either a disclosure and/or security release or even a regulated review as is often the policy. In the case of new vulnerability_findings, dismissing them requires an audit trail and justification engineers are generally not trained in providing. Lastly, Security teams still need to review dismissed vulnerabilities no matter when and where they were dismissed, thus allowing dismissals in the merge request is a duplicative effort.

Alerting users to vulnerabilities in the MR

When we alert users, we expect one of two things to happen, and both have the same end result, the alert is no longer alerting. For vulnerabilities detected in the MR, we want users to fix them now, or do nothing if they are not business-critical. Unfortunately, we don't have a mechanism in place at the moment to deem what is and isn't business-critical so we've given the user an additional option to dismiss them, which is the same to them as hitting the off button on an alarm clock. It is human to want to silence an alarm or resolve an app notification, however, these don't have the same consequences as dismissing a vulnerability. Unless there is a good reason to keep it, dismissing a vulnerability should not be an option in the developer workflow.

Approval of the merge request should be the primary action when vulnerabilities are present but are not required to be resolved to merge. This action should be the proverbial "alarm off button" not the dismiss action. The latter should be reserved for those responsible and accountable for the application security in their organization, downstream of the code changes.

Additional benefits

By starting a review from a vulnerability, the MR is blocked from being able to merge. This is, in a sense an additional security gate generated by the user, often a reviewer.

Progress

• UX
  • Define the problem
  • Iterate on a solution
  • Conduct solution validation (link)
    • Iterate on solution with findings (links) from solution validation
  • Suggest MVCs for planning breakdown (links)

Related to #208482 (closed).