stacklok
Not all commits being added to `main` are checked during PR
Describe the issue
It is currently possible to have commits prior to HEAD of an incoming branch merged into main that contain vulnerabilities
minder seemingly does not evaluate all commits in a diff allowing for a commit with a vulnerable version to be included when merging a PR.
Although this wont cause HEAD of main (or the target) to have a vulnerability pulled in, there should be checks against being able to introduce any commits onto main that include unsafe dependencies.
To Reproduce
- Create a branch off of main
- Change a dependency's version to include a vulnerability and commit (commit A)
- Change this dependency back to a safe version in a new commit (commit B)
- Create a PR and see how
minderwill evaluate this as safe despite commit A being present and potentially introducing a vulnerability to the version history
What version are you using?
No response
Hey Greg,
I'm trying to understand the vulnerability here -- is the concern that there could be a merge commit on the history of main which includes a commit where the code is not in a safe state? Is that a different concern than (for example) that a commit in the history of main contains the code in a non-working state, even though the merged commit was working and passed unit tests?
Hey Evan!
If I recall, the main concern is when rebasing/merging multiple commits into main -- not all commits are checked for vulnerabiltiies.
For example:
main contains commits A -> B -> C
feature-branch contains commits D -> E* -> F
Now if commit E* has vulnerabilities (let's assume a log4j issue) then merging in ALL commits of feature-branch onto main such that main looks like A -> B -> C -> D -> E* -> F then there exists a commit on main (E*) with some vulnerability -- despite HEAD of main (F) being secure.
Here is a diagram of the scenario as I explained it to Jakub.
Ah, so this is primarily a concern when using rebase merges, as the other merge types will roll the changes to main up under a single commit which can't be partially stepped into.
This is pretty interesting, because I imagine that a lot of projects don't care about the intermediate states of PRs (because people as a whole aren't great at rewriting branch history to remove the vulnerable commits), but it would be important if rebase merges are allowed.
Yeah exactly. Perhaps Minder should rather just recommend changes be squashed when merging or perhaps even provide a simple git command to safely drop/squash the vulnerable commit.
Personally, I would want to know whether any of the commits I am rebasing onto my main branch contain vulnerabilities since I'm sure there is a scenario where a bad-actor could exploit this.
I think most /upstream/ projects are not interested in that, but..having worked in the past for a company that used to make a lot of money on essentially backporting fixes and features to old versions, sometimes you do care that /a/ patch introduced a vulnerability because someone might cherry-pick just that patch later and not the follow-up that bumps the version..
