Auto merge: do not consider skipped checks as failed

[AbhyudayaSharma]

Package manage/ecosystem
javascript

Manifest contents prior to update
package.json and package-lock.json

Updated dependency

• @typescript-eslint/eslint-plugin from 2.27.0 to 2.30.0 (PR)
• @types/react-dom from 16.9.6 to 16.9.7 (PR)

What you expected to see, versus what you actually saw
It looks like GitHub Actions recently starting publishing skipped build checks on PR builds. Ever since, Dependabot is not auto merging any pull requests. Also interesting to note here is that when some other checks fail, the skipped check is also listed as failed. See this comment for example. Commenting @dependabot merge also has no effect as in this pull request: AbhyudayaSharma/abhyudayasharma.github.io#111

Dependabot configuration:

Images of the diff or a link to the PR, issue or logs

• Bump @types/node from 13.13.0 to 13.13.4 AbhyudayaSharma/abhyudayasharma.github.io#120
• Bump @types/react-router-dom from 5.1.4 to 5.1.5 AbhyudayaSharma/abhyudayasharma.github.io#121
• Bump @typescript-eslint/parser from 2.27.0 to 2.30.0 AbhyudayaSharma/abhyudayasharma.github.io#122
• Bump @typescript-eslint/eslint-plugin from 2.27.0 to 2.30.0 AbhyudayaSharma/abhyudayasharma.github.io#123
• Bump @types/react-dom from 16.9.6 to 16.9.7 AbhyudayaSharma/abhyudayasharma.github.io#124

Status as reported by GitHub:

[agustinprod]

Yeah it's really annoying to merge to manually merge PR one by one, the automatic merge was a time saver.

[janhartigan]

This has made merging in a lot of dependabot PRs super painful for us

[tsimbalar]

Just so both end up linked. I believe this is the same behavior described in
dependabot/feedback#910

[janhartigan]

@hsyn @jurre sorry to ping you directly, but wondering if there's a plan to address this issue.

[jurre]

We're currently working on a github native version of dependabot, which is in beta, and we have decided to drop automerging support there entirely.

The reasoning is that we feel there's some security risk associated with automatically merging pull requests without having a human review them, and if you do want this behavior there are GitHub Actions out there that will let you implement automerging rules for more than just Dependabot PR's.

[mforman1]

When there are skipped checks the @dependabot merge command is not working either, any advise?

[mbrevda]

we feel there's some security risk

@jurre do you feel it's Github's place to be deciding this or would it perhaps be more appropriate for each user to decide what's best for themselves?

[AbhyudayaSharma]

When there are skipped checks the @dependabot merge command is not working either, any advise?

@jurre Even if you consider auto-merge to be a security risk, @dependabot merge comments on a PR from a user with write-access to the repository should be considered as an approval. Skipped checks should not cause a failure to merge.

[jurre]

Skipped checks should not cause a failure to merge

You're right, we have an issue to track this here: dependabot/feedback#948. It's on our radar, but it might be a while before we can get to it.

[janhartigan]

@jurre is there something I can do to help get this sorted? Just need to be pointed in the right direction and I can submit a PR. Any idea where the offending code is? Any immediate thoughts on how to tackle this?

[jurre]

I appreciate the offer to help @janhartigan, unfortunately the code that powers this feature is not part of dependabot-core, but lives in a private backend service that we maintain internally.

[tuukka]

we have decided to drop automerging support

Thanks for letting your users and customers who relied on this know so everyone can decide whether to move elsewhere.

if you do want this behavior there are GitHub Actions out there

In this issue like in many others, GitHub Actions is the problem not the solution - definitely not something to rely on in its current state.

[infin8x]

Hi 👋! Alex from the Dependabot team here.

Thanks @AbhyudayaSharma @janhartigan @tuukka @mbrevda @killgt @tsimbalar @mforman1 for your feedback on the removal of auto-merge in GitHub-native Dependabot. We understand the frustration. Whether to support auto-merge was the subject of many debates internally, and I’d like to provide some context for why we made the decision.

You may be familiar with the event-stream attack in late 2018, where an attacker took over a popular library (event-stream) and added a new dependency. That dependency had obfuscated malicious code that attempted to steal bitcoin from users with a certain bitcoin wallet installed. That code went undetected for 2.5 months. Before it was removed by npm, the malicious package was downloaded nearly 8 million times.

Pre-acquisition Dependabot averaged about 34 thousand repositories that merged a PR in any given month. By comparison, GitHub saw 44 million+ repositories created in 2019. With Dependabot natively available on GitHub, if even a small fraction of those repositories were to enable Dependabot and auto-merge, we could easily become an unwitting and rapid spreader of supply chain attacks like event-stream.

We are investigating longer-term solutions that would allow us to bring back auto-merge, like the abilities to add more trust into ecosystems like npm, highlight unexpected new dependencies like the malicious dependency added to event-stream, and provide stronger traceability between a package’s sources, its build, and the package registry. But until we can provide sufficient validation of the security and authenticity of vulnerable dependencies, we don’t anticipate bringing back an auto-merge feature to GitHub-native Dependabot.

Separately from auto-merge, others in this thread have raised valid bugs about @dependabot merge and some check validations not working as expected. I’ve taken those back to the team for us to address, and will share updates as we have them.

As always, I appreciate your input for Dependabot. Please let me know if you have other questions or feedback.

[janhartigan]

@infin8x thank you for taking the time to write that, super helpful to understand the core issue.

So if I'm reading this correctly, the primary concern is in having dependabot merges happen purely automatically. I'm a bit confused why that would be an issue with @dependabot merge or the merge on approval features. In either case a repo contributor with write access gave the explicit go-ahead to merge the upgrade, so it seems to me like it takes the burden off your shoulders and places it squarely on the repo owners. I'm sure you guys have thought about this, but I think the key thing here is that when I have a bunch of open PRs, and I give explicit approval to them, I don't want to spend a few hours going back, merging in master, waiting for CI to pass, and pressing the merge button. Please let me know if I'm thinking about this wrong! Thanks again for taking the time.

[infin8x]

@janhartigan thanks!

A few issues got conflated together in this thread - sorry about that. On the @dependabot merge and "merge on approval" issues specifically: those are bugs. I've asked the team to take a look and will share updates.

[mbrevda]

@infin8x thanks for that.

As you mentioned, during the event-stream incident the malicious code went undetected for 2.5 months. How many users actually audit a package before using it or review a diff every time they merge an update? Even if PRs are being merged manually, chances are user would still merge a malicious update in the "next event-stream" and hence not allowing automerge is merely a hindrance to an otherwise elegant workflow.

Finally, I don't believe my original question was answered: who appointed GitHub the knower of what's best? Why not let the user decide on the level of safety/security they want for their own repo?? Will you next decide which versions are WordPress are safe for use? Or which programing languages?

GitHubs job is to provide a set of tools for it's users, not to dictate how and when those tools may or may not be used.

[infin8x]

Hi @mbrevda,

Thanks for your response. I hear your frustration. All of us on the Dependabot team work to build features with GitHub’s massive worldwide community in mind. We know from past experience that the options we provide and the defaults we choose can have a large impact on that community.

It’s a hard balance - obviously applying updates helps keep you secure, and we want to make that easy - but there's also the drawback of making it possible to easily propagate malicious content. At this point, we’re erring on the side of caution.

Again, we are investigating longer-term solutions that would allow us to bring back auto-merge, once we can provide sufficient validation of the security and authenticity of vulnerable dependencies.

[mbrevda]

@infin8x thanks, again, for taking the time to respond. I can appreciate that being a product manager for the world's largest code sharing platform is a huge responsibility. In all likelihood, being in your position, I'd similarly look to find myself "on the side of caution".

The implications of totally removing the auto-merge option seem to be way more far-reaching than just suggesting "the defaults" as the defaults were never to auto-merge. As these recent changes went as far as totally disabling the bots ability to merge (even when specifically requested by a human!), "the defaults" are no longer a choice but an imposition on the community at large.

Additionally, as "the options [you] provide" still enable auto-merging as @jurre pointed out (albeit via slightly more steps) the ultimate results are simply more complexion and indirection - not "caution"! And this doesn't even include the Dependabot competitors that are available as Github apps - all which allow for auto merging.

Finally, it would seem that the root issue here really is the easy accessibility to "propagate malicious content" via npm (which Github recently acquired) and that remains unaddressed.

In conclusion: it's easy to see why Github feels the need to apply safe and sane defaults and I'm glad there are bright and experienced minds looking out for the community. However, this precedent of enforcing a specific agenda is a slippery slope and troubling at best.

---

Out of respect to OP and the legitimate issue at hand, I'd like to "yield my time" and continue this conversation in a more appropriate forum if you can suggest one.

[lesair]

@infin8x from an end-user perspective, I concur with @mbrevda, this move feels like we're being coerced to follow corporate security guidelines without agreeing on them or even having a saying in the matter.

On the other hand, I totally understand your perspective that security should be paramount. So I support the decision of phasing auto merges out, with the hopes that you will roll the feature out again in the future in a secure manner.

[userhas404d]

For those that still want auto-merge while this is getting sorted dependabot side there's always mergify, which is free for open source repos.

[feelepxyz]

Pushed a fix for merging/automerging when there are skipped check runs. Rolling out fix for both versions of Dependabot.

[dwilkie]

Awesome thanks. Interesting side affect - Now we have around 200 PR merging, one after another which causes all other open PRs to rebase, which ate up all of our GitHub actions minutes.

[tuukka]

Should Dependabot rebase, CI and merge one PR at a time?

[feelepxyz]

Now we have around 200 PR merging, one after another which causes all other open PRs to rebase, which ate up all of our GitHub actions minutes.

Ouch! Yeah, we'd like to improve or make the rebase-strategy configurable. We've got some plans for this but will be a while before we can get to it.

[jrjohnson]

I wish the alternatives to auto-merge were just more annoying routes to the same destination.

Unfortunately only dependabot is able to determine the package and type of update. I want to merge things like eslint which are, by definition, covered completely by my tests. I also want to merge semver patch versions of packages where I know and trust the maintainers to follow good release practices. This kind of granular control isn't possible with a merge bot. It doesn't have enough information to make those determinations without parsing the content of the PR.

Dependabot also had additional security controls in that it would never auto-merge something with a new maintainer. So doing this with another bot would actually be a step back and make it more likely that the type of attack that @infin8x laid out in #1823 (comment) would succeed.

I hate to pile on here, but I have to agree with @mbrevda. Removing auto-merge doesn't add any security (as manual review is not in any way guaranteed by requiring a human to click the merge button), it may decrease security (where github could intervene in a merge when specific criteria were met), and it makes keeping dependencies up to date much more difficult (an OWASP top ten vulnerability).

[sandstrom]

@feelepxyz I noticed this was fixed in dependabot recently, which is great! (dependabot/feedback#948)

Is there any plan to add this to Github itself, i.e. the branch protection rules where certain checks can be required before merge is allowed?

See also https://gh.neting.ccmunity/t/github-actions-and-required-checks-in-a-monorepo-using-paths-to-limit-execution/16586/5

[stevebaros]

Still fails on my github action
Run ridedott/merge-me-action@v1
Automatic merges enabled for GitHub login: dependabot[bot].
Pull request created by stevebaros, not dependabot[bot], skipping.

[Mattsi-Jansky]

Adding my +1 to the choir - I'm disappointed by this decision.

The reasoning is that we feel there's some security risk associated with automatically merging pull requests without having a human review them

I have comprehensive testing in place that can do a far better job than I ever could of reviewing the change. I can look at the number changing from one digit to another in package.json, but what good is that going to do? I don't even know what half these dependencies do. I don't have the time to read through all their source code changes, and even if I did there is little guarantee that I could tell a well-obfuscated bitcoin miner apart from the regular mess of a codebase I'm not familiar with. Especially when their updates include dependency updates- I'd have to read the source code changes of every dependency recursively down the entire tree.

Meanwhile my automated tests can prove that the site generates correctly, my React components are behaving as expected, and even notice the tiniest unexpected visual change using browser automation with visual regression tests. If I were to review a dependency update all I'd do is run the tests, which has already been done by a pipeline. There is no benefit to human oversight here.

Trying to look at it from GitHub's perspective, I can see valid reasoning for this change- to reduce the blast radius of malicious changes to dependencies. If a major library gets taken over by a malicious actor you don't want it to near-instantaneously update across thousands, perhaps even tens of thousands of repositories. But there are other ways we could achieve this- perhaps if you enable automerge your automatic PRs are delayed by a certain amount, to give the community some time to catch any malicious changes. It could even be a random amount, some repositories getting updated faster than others. That way a small number test the change while the rest wait to see if there are any problems. I wouldn't mind my dependabot PRs being a few days late if I can automerge them.

[tuukka]

But there are other ways we could achieve this- perhaps if you enable automerge your automatic PRs are delayed by a certain amount, to give the community some time to catch any malicious changes.

Yes! And in the future this could evolve into a distributed review process, where the merge delay is skipped if several major projects have explicitly ok'd the change. This could be a huge improvement to the process of distributing critical security fixes: as soon as a few major projects have applied the upgrade, all the small projects and people on vacation would get the fix merged automatically.