GitLab is able to suggest solutions for security vulnerabilities as part of the details info.
In some cases, it is also possible to get a patch that can be applied to the codebase to fix the problem.
At the moment, users have to download the patch, apply to the local repository, and then push changes back to the remote repository.
Users should still be able to download the patch, since they may want to look at changes before committing into the codebase. We can allow to choose what they want to do via a dropdown button or another similar way.
Proposal
Add a button to automatically create a merge request with the proposed changes.
This should be available in every place where the patch could be downloaded.
Once the button is pressed, the following actions are executed:
a new branch is created
the patch is committed in the new branch
a new merge request is created, with the new branch as the source, and the branch where the security reports belongs as the target
We can also consider to commit the changes and then open the merge request creation page, where users can manually confirm it. This is the same flow we have when changing a file via the UI.
frontend: change the Create issue button in the vulnerability details window into a button dropdown, with Create a merge request (default) and Create issue as possible actions
backend: create a new branch, commit the patch, create (open create page?) a new MR
frontend/backend: usage ping (via snowplow on the button - or via backend when receiving the request)
We should take a look at the Web IDE and figure out if there is something we can reuse.
TODO: The initial backend implementation done with https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9326 covers creation of the merge request targeting the repository's default branch. A follow-up MR is needed to add the more complex logic for changing the target branch to the originating branch of the vulnerability occurrence.
Design
Create MR button selected
Create issue button selected
Initial list state
Hover state - over create issue
Hover state - over create MR
Cases
Issue has been created from this vuln
MR has been created from this vuln
both MR and Issue have been created from this vuln
I identified the following engineering sub-tasks to do:
frontend: change the Download patch button in the vulnerability details window into a button dropdown, with Create a merge request (default) and Download patch as possible actions
backend: create a new branch, commit the patch, create (open create page?) a new MR
frontend/backend: usage ping (via snowplow on the button - or via backend when receiving the request)
frontend: change the Download patch button in the vulnerability details window into a button dropdown, with Create a merge request (default) and Download patch as possible actions
My only concern would be with the button dropdown having the actions Download patch and Create merge request. What if the user wants to create an issue instead? How would this work in the dashboard?
Thinking about how this will be translated into the dashboard: The near-term solution could be:
With, Create merge request and Create issue as the quick actions. If the user opens the modal, they can download the patch from there.
@andyvolpe I think users should be able to create a merge request from the details window too. I expect them to open details, and to create a merge request once details are clear.
Anyway, good point on creating the merge request from the list, so users can avoid one click, and also know if the solution is available.
I'd rather decouple the two aspects, so we can go with the MVC in the details window too.
@andyvolpe that's fair. I was considering using the Download patch button to create the merge request, but it makes more sense to use the issue creation one.
This is what we envisioned some time ago, but I got distracted by the new button
I think we should monitor how many patches are downloaded vs. MRs created. If there is enough evidence that users are comfortable creating the MR rather than downloading a patch, then maybe we can remove the download patch button all-together.
I think we should monitor how many patches are downloaded vs. MRs created. If there is enough evidence that users are comfortable creating the MR rather than downloading a patch, then maybe we can remove the download patch button all-together.
@andyvolpe I see some value in keeping the download button even if users can create the merge request. Probably some flow will require to check changes before any commit, or users will just need to be more confident in the process before using the automated flow.
We also already have it because of the early steps, so removing it would be a breaking change. If we have strong reasons to think that it may create confusion, then we can discuss to remove it. At the moment, I don't see a lot of value by doing that, anyway.
I think we should monitor how many patches are downloaded vs. MRs created
That's something I totally agree on.
@theoretick@samdbeckham we should find a way to measure the usage of both of those two actions. Three actually, if we include the "create issue" one. Snowplow could be an option for GitLab.com, but what about self-managed?
By adding the opportunity to create a MR from a vulnerability I think we should follow the same approach and introduce a merge_request feedback type. This will also allow to link a vulnerability to an existing MR like we currently do with issues. This will have implications on both backend and frontend workload has feedback types are not dynamically handled (due to their nature and different behaviours).
Any preferences on branch name? I'm currently using autoremediate-<timestamp> to avoid conflicts but would be great to use some concatenation of identifiers or something... @gonzoyumo given previous discussions about unique identifiers I don't know if there's a good option here.
cc @samdbeckham Still need to iterate on this a bit but I believe it should be good to start FE work.
Please keep in mind that when creating an issue we are actually creating a Feedback of issue type + a GitLab Issue (this logic is entirely handled in backend).
I am fine using the same template for the MR as the issue does. I do want to change the MR title to be, "Fix vulnerability:[vulnerability name]"
Any preferences on branch name? I'm currently using autoremediate-<timestamp> to avoid conflicts but would be great to use some concatenation of identifiers or something...
maybe remediation-branch-<timestamp> ? but I am open to alternatives. I would stay away from the usage of autoremediate for the moment until we get closer to the "auto" part of that.
@andyvolpe yes, I like the MR title being self explanatory. Otherwise it will be a headache to have a clear vision in MRs lists.
Regarding branch name it could be good to provide some context too. What about sanitizing the vulnerability title into something like lowercased-and-hyphenated-vulnerability-title-<timestamp>?
@gonzoyumo that feels like we would lose the description of the branch if we do use the vulnerability title and cause confustion due to some of the vulnerability names.
Sanitizing the vulnerability title:
nokogiri-gem-via-libxml2-2019-01-30 09:17:57
Remediation naming convention:
vulnerability-remediation-2019-01-30 09:17:57
However if we are creating a new branch everytime, this could become a problem since they will all be the same except for the time stamp.
Maybe we can do a combination with the identifier and remediation naming convention:
Breakman-remediation-2019-01-30 09:17:57
CVE-remediation-2019-01-30 09:17:57
Could lead to less confustion when the user is looking at their branches.
yeah I forgot to prefix the branch name with fix- :) Usually a title should be meaninful about the vulnerability so it should look more like fix-remote-command-execution-in-foo-bar-2019-01-30 09:17:57.
Breakman-remediation-2019-01-30 09:17:57 or CVE-remediation-2019-01-30 09:17:57 doesn't tell much about what it is really about, and you may end up with a lot of these making it hard to distinguish them.
And in case you really want to distinguish branches created by our features from regular ones, then a common prefix like remediation- could be useful to see them at a glance.
@gonzoyumo@andyvolpe here's a couple alternatives I was looking at for branch names.
Assumed requirements
Common prefix identifying this as a remediation fix
timestamp for uniqueness (not necessarily human-readable)
slugified title/identifier
max length 100 chars (totally arbitrary, any preference? We should keep it at least under 260 because windows).
Branch names
title: `"Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js"`identifiers: [ "Gemnasium-9952e574-7b5b-46fa-a270-aeb694198a98", "CVE-2017-11429"]
Please keep in mind that while in these examples the CVE ID might look fine, there are a lot of cases when it will be a less interesting value. E.g. Brakeman Warning Code 1.
That's why I prefer the title approach, or the identifier+title one (but a bit less readable to me).
About the timestamp I have a slight preference for the human readable version, as the cost of taking some space is at least a bit compensated by bringing readable value :)
# title max lenght 100 characters: 1. Remediate/Remote-command-execution-due-to-flaw-in-the-includeParams-attribute-of-URL-and-Anchor-tags-for-org-20190206T1034372. Remediate/ECB-mode-is-insecure-20190206T103437
# title with identifier 1. Remediate/Find-Security-Bugs-ECB-MODE-20190206T1034372. Remediate/CVE-2013-2115-20190206T103437
Please keep in mind that while in these examples the CVE ID might look fine, there are a lot of cases when it will be a less interesting value. E.g. Brakeman Warning Code 1.
There could also be multiple identifiers for a single vulnerability
That's why I prefer the title approach, or the identifier+title one (but a bit less readable to me).
I prefer the title max 100 characters with Remediate/ in the beginning and the human timestamp. Contextually, the title adds more for me than the identifier, since we present the title to the user in more places and with more significance that the identifier.
I also prefer leading with Remediate over Fix because:
Remediate = automated or semi-automated
Fix = something a human would use as a title.
And in case you really want to distinguish branches created by our features from regular ones, then a common prefix like remediation- could be useful to see them at a glance.
Contextually, the title adds more for me than the identifier, since we present the title to the user in more places and with more significance that the identifier
@andyvolpe any idea how our UI will be affected by a 100 char branchname? I imagine the branches page should be fine and hopefully ellipses'd appropriately elsewhere
andyvolpe any idea how our UI will be affected by a 100 char branchname? I imagine the branches page should be fine and hopefully ellipses'd appropriately elsewhere
It looks like we do a good job managing for long branch names everywhere in GitLab.
Pipelines:
Commits / branch dropdown
In MR:
Repository/Branches:
Repository/Compare:
we actually have 124 characters including the timestamp and remediate/ but I am still okay with it.
One suggestion: Can we add a d in front of the time stamp so the user can quickly identify that it's a timestamp instead of having to reread and think about it? maybe capitalizing the d and t here would help too:
Due to complexity of opening the MR towards a dynamic target branch (the one the vulnerability was discovered on), I'm adding a TODO to this issue's description as we will be creating a secondary backend MR to address that specific functionality. The work in https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9326 will cover the initial implementation where MRs will automatically target the repository's default branch.
@theoretick I also think it should be created. Anyone should be able to start a remediation. If we want to check something, we should check that the user is able to create the source branch and merge request (to avoid failures in the process).
Since we want to automate the merge in the future, we can consider to give a visual feedback to users if they cannot merge automatically.
@andyvolpe How are we changing the display when there's already an MR or issue, or both?
Currently we have the two options:
Has issue
Does not have issue
Has Issue
No Issue
But because we're lumping the MR and Issue buttons together we now have to consider both the MR and issue states.
Has issue and Has MR
Has issue and Does not have MR
Does not have issue and Has MR
Does not have issue and Does not have MR
How do we display these four states in the UI?
If they were two seperatre buttons we could just use the same state for the issue button and replicate it for the MR button. But combining them together creates this weirdness.
@samdbeckham I will work on a solution asap. Unfortunately, we can't separate the buttons for now as we have a UX guideline limiting the number of buttons to 3 in the action are of a modal.
@samdbeckham, here are some mocks detailing your questions:
Has an issue and has MR
When both an issue and MR are created we remove the view button altogether and differ the action to view both the MR and the issue to the links in the content area of the modal.
Has an issue and does not have MR
Does not have an issue and has MR
Does not have either:
When the MR is available the create mr button will become the default button.
More complexitiy
We are also introducing some new complexity that I am not sure we can solve for. For instance:
When a user creates an issue from a vulnerability it will be linked to the vuln. What will happen when an MR is created from that issue but not from this area? We risk giving the user the ability to create a duplicate MR in this case.
When a user creates an issue from a vulnerability it will be linked to the vuln. What will happen when an MR is created from that issue but not from this area? We risk giving the user the ability to create a duplicate MR in this case.
@andyvolpe I'm not sure about this. I don't know how it all works in the BE. @theoretick@gonzoyumo could you weigh in on it? In the mean-time, I will follow those steps and just see what happens
When a user creates an issue from a vulnerability it will be linked to the vuln. What will happen when an MR is created from that issue but not from this area? We risk giving the user the ability to create a duplicate MR in this case.
@andyvolpe@samdbeckham I believe that's true and without passing around a whole lot of state I doubt we can easily link an MR created from an issue back to the vulnerability. To do so we would need to store some metadata/state on the issue and pass it conditionally when creating the MR, but there's no such functionality currently and issues are not distinguished by their association with a vulnerability.
The two MRs are also different: one is created as an empty placeholder MR (no diff) and the other one is created with a remediation.
We could look at a possible enhancement to update the issue page's functionality to "Create MR with remediation" but that has quite a bit of additional complexity and won't be possible within %11.9.
This might be too much interface complexity IMO, but I can imagine both scenarios being useful in both cases. Perhaps I want to create an empty WIP MR from the vulnerability pop-up which I will use as a placeholder and start committing to, or perhaps I want to create an auto-remediated MR from the vulnerability issue. This way we could support "Create Merge Request" on non-auto-remediatable vulnerabilities.
Sorry to add yet another branch of logic in both cases but there could be some merit in exploring that in the future.
When a user creates an issue from a vulnerability it will be linked to the vuln. What will happen when an MR is created from that issue but not from this area? We risk giving the user the ability to create a duplicate MR in this case.
We'll need to rethink the way we're linking vulnerabilities with issues and merge requests. Today it is done through the feedback, as the feedback was the first goal in mind. But with the upcoming first-class vulnerabilities we'll certainly build stronger relationship with issues and MRs. Then it could be possible to show an MR created from an issue that is linked to the vulnerability (by transitivity).
So let's keep it this way using the feedback and rethink later I'd say.
PS: it would have been great to have this in a threaded discussion :p
I've added in a WIP MR that should be ready for review later today. I just need to test the UI a little more, add some tests, and review the thing myself.
We're currently waiting on gitlab-ui!190 (merged) to be merged so we can use the split button without it looking like hot garbage. This is about to go into maintainer review so it should be good to go very soon. It's not blocking progress, but it should be in before code freeze anyway :)
Lucas Charlesmarked the checklist item TODO: The initial backend implementation done with https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9326 covers creation of the merge request targeting the repository's default branch. A follow-up MR is needed to add the more complex logic for changing the target branch to the originating branch of the vulnerability occurrence. as completed
marked the checklist item TODO: The initial backend implementation done with https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9326 covers creation of the merge request targeting the repository's default branch. A follow-up MR is needed to add the more complex logic for changing the target branch to the originating branch of the vulnerability occurrence. as completed