Security Hotspots are as important as Bugs, Vulnerabilities and Code Smells found on PR.
From a cost perspective, the best moment to tackle Security Hotspots is when the PR is created. At this time, everything is still fresh in the mind of the developer who created the PR (similar to issues) and so it's more efficient to do the review now rather than in 3 months.
For this reason, developers should be aware that Security Hotspots were found on their PR so they can review them immediately.
- We want that developers understand the difference between the changes that are needed to make the PR "qualitative" (ie. issues) and the review that they have to do to make sure that the PR is clean -> it should be clear in the PR that vulnerabilities and security hotspots are not the same thing and do not require the same action.
- We don't want to annotate the PR in GH with Security Hotspots because we cannot give enough context so that developers understand them and we think that they will bring too much noise
As a developer, when my PR is analysed and the diff contains SH :
- I get a notification that my QG successes or fails
- I go to GH and check the summary or the Checks
- I see if my PR contain Security Hotspots and how many they are
- I understand that SH are different from issues and that they require a different action: a review
- I understand that I need to perform this review on SQ
When I review the Security Hotspots of my PR :
- If I identify that the hotspot is not a vulnerability, I mark the hotspot as safe and I expect that the number of hotspots decrease in the PR
- If I identify a vulnerability, I fix it in the code, I mark it as fixed on SQ and I expect that the number of hotspots decreases in the PR
- If I don't know what is the hotspot (and I need another SH reviewer) or if I don't know if it's a vulnerability or not, I expect to see the same number of hotspots
As a reviewer of a PR or as a team lead or quality manager who wants to assess the overall quality of the project/product, when the PR is analysed, I want to check that all Security Hotspots are reviewed meaning that there are no more Security Hotspots in the PR anymore.
As implementation is shared for GitHub and GitLab, the solution will be implemented for both.
In Quality Gate section
If Security Hotsposts Reviewed and Security Review Rating are configured in the quality gate conditions and they are failing, the failing conditions are shown as it is already the case
In Additional Information section
1. We update the information line to tell user to review SH for both QG fails and QG successes
2. Below Vulnerability, we add a new line for Security Hotspots with the following information :
- SH icon, security review rating, number of Hotspots left to review (Security Hotspots Reviewed already reviewed). The number of Hotspots should link to the Security Hotspots review page on SQ for the PR.
(icon)(rating) (24% reviewed)
- When there is no more SH on the PR, ie. 100% SH reviewed :
(icon)(rating) (100% reviewed)
- If there was no SH on the PR, we do not display the %:
- Add the same in the GH summary in the conversation tab
- Check that the notification works correctly
- Check on GHE (Aurelie)
We need to adjust the markdown formatting to display clearly and separately SH, both in the additional informations and in the QG failed condition if any.
This MMF will benefit to GitLab MR decoration, as the markdown generator is shared. So we will need to validate that everything is working well on GitLab as well.
Do the same modification for GH Summary
Here is the formatting target we want to achieve with this MMF :
See mmf-1906.md for the corresponding markdown