Improve how we show security reports on the Merge Request view
Description
Currently we display the security reports (sast+sast:container+dast and codeclimate) in the merge request widget:
When we expand any of them, it's hard to focus on something:
Problems
- Can have a huge scrollable area in the MR widget;
- Hard to navigate, user has to click the link of each alert and then scroll back to the MR widget to check the next problem or open the links in new tabs
- DAST report's links open a modal box with the detailed explanation
- Too many report blocks in the MR widget
There is an issue to group all security reports: https://gitlab.com/gitlab-org/gitlab-ee/issues/4310 And some other issues to improve UX: https://gitlab.com/gitlab-org/gitlab-ee/issues/4337 https://gitlab.com/gitlab-org/gitlab-ee/issues/4991 And an issue to improve the code base: https://gitlab.com/gitlab-org/gitlab-ee/issues/4455
Note This components used in the MR widget are now being used in the Pipeline View inside a tab.
Proposal
(From Slack)
@plafoucriere suggested:
plafoucriere
- We should have almost everything already available. (edited)
- It makes sense to report security issues in the `Changes` pane too, so users can comment on it
- It allows to add buttons in the future (Open issue, Ignore, Try to fix -yeah, why not-, …)
- it’s integrated in the review process
- We have space to explain the issue to the user
- it brings sec issues closer to the codeAnd here's the rest of discussion:
plafoucriere
but in the demo, it’s just a warning, and it’s not pushing the user to fix it. By integrating sast directly in the MR, we encourage the team to fix the issues before merging
Brendan
And/or as a “discussion” created by the security scan right? The analogy being - if I as a human reviewer see something I don’t like, I comment on the line starting a discussion.
In the same vein it could start a discussion automatically for lines like you show. And then the “x of y” discussions resolved directly relates
Brendan
And for merge requests, you can optionally require all discussions to be resolved to merge it. AND you can “close discussions to a new issue”
Brendan
All of those patterns now apply to human and machine code reviews…
plafoucriere
Exactly. That’s what we want! SAST is a helper for human review. We can also imagine in the future something smarter, where we gather some feedback to train a “proxy” between the tools and the code.
plafoucriere
This program could be able to give a confidence score on alerts after some training
plafoucriere
Want to go even further?
plafoucriere
It is also possible to train a model with user code. If we can link an alert with an issue, we can analyze how users fixed it, and suggest a smart update to other users. That’s where gitlab.com is important, we need DATA
fcatteau
Opening a discussion is indeed a great idea! Actually that's a feature we wanted to implement in Gemnasium back then for the same reasons: users may discuss the security issue and conclude it is irrelevant in that particular case. And it's very important to keep track of that discussion!
jschatz
I've always felt that the MR widget has turned into a sort of dumping grounds. I feel there is def better ways to organize it.
plafoucriere
Every journey starts with a first step ;)
ogonzalez
note that showing issues in the "code changes" tab won't be suitable for all kind of issues. e.g. a 3rd party lib with new vuln even without having changed its requirement (there is no change in the source code then). Same for container scanning.
Though, this is great for other cases :+1: and it leverages existing components of GitLab.
Depending on how we want to prioritize this, we may then discard this one: https://gitlab.com/gitlab-org/gitlab-ee/issues/5043
Or mix them together and define a roadmap with multiple iterations to achieve that goal. (edited)
Brendan
I guess my dream for that case would be GitLab updating the 3rd party lib (in gemfile, package.json, etc) and open an MR for that.
filipa
I only have one concern regarding my usage of gitlab: Could it be a lot harder to review an MR with that? :thinking_face:
filipa
But I have to agree the widget is getting a little crowded and if I need to inspect each code line it will take me forever!
We can always collapse those discussions, maybe include a button to collapse all report discussions would solve my first concern
fcatteau
Or maybe we need a way to turn a security alert into a discussion. Obviously discussions are not a good fit when there are many open security issues, which is likely to happen where SAST is activated for the first time on a old/legacy/poorly maintained project.Edited by Filipa Lacerda