Enable Mergify for dev workflow

[jd]

Hey team!

The current pull request configuration is pretty nice and has coverage, CI run, etc and uses code review. It seems to me there could be a large piece of automation that could be setup, and that's automatic merging based on those criteria.

I'd suggest to use Mergify for that on this repository: https://mergify.io/ which allows to do that easily.

WDYT?

(Full disclosure: I'm behind the service :)

[actionless]

can the service also take a PR label into account?

Is this documented?

do you know any such tools which could be appliable to the project? it could be quite nice to see not only tests- but also documentation-coverage

[jd]

can the service also take a PR label into account?

It does. Right now it's used to ignore PR. What would you like to do with a label?

Everything's documented at http://doc.mergify.io/

If doc are covered by the current jobs, that can be included in the rules.

[actionless]

What would you like to do with a label?

right now we have a label "PRs to be merged soon", i think having that label on a PR could be a good condition, meant what a PR confirmed by a human-review but pending some auto-checks or so

If doc are covered by the current jobs,

that's what i was asking -- if you know any tools for doc coverage which we could apply to awesome. it will be really amazing to start using something like that

[jd]

right now we have a label "PRs to be merged soon", i think having that label on a PR could be a good condition, meant what a PR confirmed by a human-review but pending some auto-checks or so

You actually don't need that. Those conditions are already configured on the Mergify side: you'd just have to write some rules like "2 humans must approve + all CI tests must pass" and that'll be enough.

that's what i was asking -- if you know any tools for doc coverage which we could apply to awesome. it will be really amazing to start using something like that

I don't – though I don't think it's related to Mergify usage though here.

[actionless]

I don't – though I don't think it's related to Mergify usage though here.

it's indeed unrelated, i just thought you did some kind of research of the "market" of such tools so could give some guidance on that

[jd]

@actionless do you want to go ahead and enable it or do you want me to do it or help?

[actionless]

let's wait for other feedback, my role in the project is too minimal for making such kind of decisions

[jd]

Ping @awesomeWM/write-access :)

[blueyed]

+0 from me.
Often I think it's good for somebody with write access to edit and then squash-merge the PR, i.e. some minimal human intervention often makes sense.

[psychon]

+0.5 from me since I imagine this makes me less important.

I took a look at mergify and came up with the following configuration. This basically requires "everything has to be green" and I imagine that requiring codecov/patch (i.e.: The new code is covered by tests) means that will not fire quite often currently. Oh and requiring two approvals is also more than we usually have. This does NOT require the "pull requests to be merged soon"-label and that is actually not possible currently.

rules:
  default:
    protection:
      required_status_checks:
        strict: True
        contexts:
          - continuous-integration/travis-ci
          - codacy/pr
          - codecov/patch
          - coverage/coveralls
      required_pull_request_reviews:
        required_approving_review_count: 2

So, what do you think about this? Anyone strongly against this? Any suggestions for a different configuration? (What does codacy do again for us?)

Edit: Added coverage/coveralls since it is marked as required on GitHub
Edit: Added strict: True

[jd]

@psychon that sounds like a good config, it's the same one we use for https://github.com/gnocchixyz projects for example.
We also enable the strict workflow (https://doc.mergify.io/strict-workflow.html) which maybe something you'll like.

And if there are some missing features that could help you guys with your PRs, feel free to ping me or open issues on https://github.com/mergifyio/mergify-engine/issues

[actionless]

@jd and what will happen if a contributor will modify the settings in .mergify.yml ?

[actionless]

also, is it possible to choose to use .mergify.yml from branch "to" which you are going to merge, not "from"?

UPD: my bad, i saw a message in my PR (#2129):
mergify/pr — .mergify.yml is missing , so i thought that file is present on master branch but missing on mine, so it was going to use the config from my branch, not from master

[jd]

@actionless The GitHub pull request determines what the target branch is; if you want to merge to a different branch, you have to open a different pull request. It's not related to Mergify (AFAIU).

[psychon]

@jd I think the question is: Which .mergify.yml is used for a PR that modifies this file? The file from the target branch or the one from the branch from which a pull should be made?

[jd]

@psychon The one from the pull request is taken into account.

[Elv13]

@psychon The one from the pull request is taken into account.

Umm, did I miss something? So the PR can disables everything and allow itself to be self-merged with malicious code?

[psychon]

So the PR can disables everything and allow itself to be self-merged with malicious code?

The PR can disable everything to make sure it is merged and once it is in the master branch Travis will run the changes to .travis.yml and you can steal encrypted Travis secrets. (E.g. by triggering another PR with a special branch name that causes this)

I agree that taking .mergify.yml from the target branch makes more sense.

[Elv13]

and you can steal encrypted Travis secrets.

And from there if a JS framework with a catchy name uses this, you can replace the NPM package, lock everyone out of the github project and shut down half of the internet.

[jd]

You always need at least one review to merge a PR. :)

That being said, @sileht is working on improving that initial workflow around .mergify.yml to be less confusing.

[Elv13]

You always need at least one review to merge a PR. :)

By who? According to your doc I can just rewrite the CODEOWNERS or put require_code_owner_reviews=false in the attacker branch and bypass it.

IMO, as long as Mergify allows Mergify config to be modified using Mergify, that's big no from me. Any changes to .mergify.yml, CODEOWNERS or any files having an effect what-so-ever on the merging process should turn off mergify and require the PR to be handled manually.

[Elv13]

According to your doc, CODEOWNERS refers to names as @octocat. GitHub allows names to be re-used. If you use its search API to get all CODEOWNERS files and poll the existence of the user account, you can re-create the account within seconds of it being deleted (maliciously or not using a yet undiscovered XSS/OAUTH2/PrivigeleEscalation vector by another web service) and take over a project. That's another security vulnerability that I found at 5AM while reading the doc in diagonal. Users must be listed using their UUID, not usernames.

From IRC:

[05:41] <psychon> what did I do this time? ;-)
[05:41] <psychon> jd_: where is "you need at least one review to merge a PR" implemented? neither compute_approvals nor compute_approved seems to do that
[05:42] <jd_> psychon: on GitHub side
[05:42] <psychon> heh, and here I was chasing through unknown python code...
[05:42] <jd_> psychon: I just love how you jump right on things like you do, poking at the engine code :)
[05:43] <psychon> yesterday I lost half a day learning about internals of PGP due to the efail vulnerability ;-)
[05:45] <psychon> feel free to just close that issue with "you are wrong" if you want (and btw thanks for reviewing my PR, I'll just wait some days and then test if I can review my own PR, too... hehe)
[06:07] <Elv13> jd_: Ok, so GitHub is the one that prevents your web service from merging unreviewed PRs? Still, you might want to fix that CODEOWNER thing anyway
[06:09] <jd_> psychon: i'll let sileht reply :)
[06:09] <jd_> Elv13: yup
[06:09] <jd_> thanks guys for the help and feedback, it's great :)
[06:12] <Elv13> jd_: I am leaving, but could you please add the relevant GitHub API documentation link for this in the issue? Given the nature of the service, double checking a couple thing up front seems a good idea. As far as i can see, that part isn't open source, right?
[06:15] <Elv13> https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button doesn't seem to talk about this at all

[actionless]

so please disable it until @sileht work will be merged in

UPD: because even it will require only one review, it still feeling not fine what it will use .mergify.yml not from the destination branch

[jd]

@actionless is has been merged, see the issue @psychon opened Mergifyio/mergify#27

[actionless]

also i was thinking about the case when code reviews are already approved but there are new commits:

again in this PR: #2129
you can see what i did more commits after psychon's review but at the bottom of the page his review is still marked as approved

[jd]

@actionless That's handled (on GitHub side) by thedismiss_stale_reviews setting that you can (should) set in .mergify.yml.

[actionless]

shouldn't it be forced by default?

[psychon]

@actionless It is the default: https://doc.mergify.io/configuration.html#default
I am not sure what you mean exactly with "forced".

[actionless]

i meant to always dismiss stale reviews disregard of that option if author of PR doesn't have write access

[Elv13]

@actionless has a point. Any PR that modifies .mergify.yml should always dismiss stale reviews. Even if the project allows them. Otherwise it's a blatant exploit vector / timing attack. If the attacker has control of someone who can edit the pull request (and remember, it can be anyone from the organization it comes from, not necessarily the submitter him/herself), then it can poll the API to know if the review is done. Then try to update the PR now and win the race before Mergify merges it.

As I said in #2260 (comment) , good default security values are not enough. Anything that touches the file affecting how /.***.yml or other bot related files should be permanently blacklisted from triggering Mergify.

[sileht]

@Elv13 That looks a good feature, the Github branch protection system doesn't help to do that.
The API to remove approvals looks accessible by GithubAPP but require special right, that I'm not sure we can have.

But if I can't use this API, I can implement this like disabling_label https://doc.mergify.io/configuration.html#rule
but calling it disabling_files.
So if the PR changes some files, Mergify will just be disabled for this PR.

[Elv13]

the Github branch protection system doesn't help to do that.

Thanks for considering it, but I think I have to be a bit more insistent on how critical security is. Take those issues with a grain of salt. I didn't try to setup the service and pen-test it. Nor did I really review the code. I just looked up yesterday at the same time as @psychon did after @jd comment on the .mergify.yml critical security vulnerability and project takeover risks (note: This was really, really bad). However for a project that literally is all about remote code injection, it has to be rock solid.

1. You should not rely on GitHub branch protection. It is dependent on settings out of your control and the API may change. You have access to the raw data (the checks) and should base the decision only on that and GH own protection as the fallback. If GitHub ever changes its API behavior or branch protection behavior on a Monday morning and that breaks Mergify and let PRs be merged by accident, I hope you have serious Legal insurances. Really, we talk about millions of euros of lawsuits if you have paying customers. I looked at the code and found quite a lot of places where security doesn't seem to be the priority (normal for startups, but still not good). I planned on doing some bug reports and PRs, but instead I will list them here. I looked at the GitHub doc and the behavior that were said to prevent the takeover issue are not documented to behave as they behave, so I take that into the "trusting undefined behavior" kind of bugs. First and foremost, do not rely on 3rd party half undocumented branch protection behaviors, implement your own using the more complete data.

2. There is some mild potential (future) cache poisoning vectors on your redis. You should hash all the strings used in the keys. Currently, the GitHub usernames cannot have ~, neither the repository names nor the git branches. So for now this is fine. But if some law are passed (and here in Quebec, governments tend to pass idiotic laws when they need the nationalist votes) and force the business to support UTF8 in usernames, then it will open some attack vector on your existing cache. If you sha1[0:12] the username/repository/branch name and use this as cahce keys, then you protect yourself against the 3rd party changes. You should not use 3rd party uninitialized data (and never use strings under the potential attacker controls) in the concatenation or string.split. I am aware that the current code would still not allow the wrong PR being merged, but the cache poisoning itselt could still cause a denial of service.

3. The branch names are sometime used along with + to create URLs. I havn't check how the GitHub API encode them in the json, but watch out for XSS and URL manipulation. The branch names can have funky names like git checkout -b 'test./branchlol=foobar><script>careful_code</script>'. You should at least encode the values or use a proper URL builder instead of +.

4. As said above and pointed out by @actionless, seriously, disable Mergify when a PR modify the config, bypass the project settings and just disable it. You legally expose yourself for no valid reasons. If there is ever a major bug and the issue raised above regress, then at least it wont be as easy to exploit.

5. The outdated PR approval, as mentioned above, can open the door for a timing attack. You should remove the settings and permanently disallow merging PRs that have changed since they were approved. There is (IMHO) no clean way to mitigate the attack vector beside preventing it from existing.

Do you want me to open issues for those things or you will take care of them? Thanks for your useful project. Don't take those criticism negatively. It's just that I care.

[jd]

@Elv13 This is an awesome feedback, thank you. We're not taking anything negatively. I came here for awesome to be one of the first users because I knew I'd find such great feedback ;) We committed to build this engine in open source for that kind of reason!

Everything you point seems valid, and some points we already aware. As you noticed, we're just starting up from our MVP so we took shortcuts obviously. I think it'd be great if you'd open issues for all of that so we can keep track and discuss them in the "proper" place. If you don't want to bother, let me know and I'll take care of it — you already did a lot.

Thanks again!

[actionless]

one more question, is it possible to choose if the merged branch commits will be squashed or rebased before the merge?

[jd]

@actionless Right now you can choose that on a per-repository basis, not on a per-pr basis. Feel free to open an issue/PR on mergify-engine if you need that.

[actionless]

no, i was just asking to understand better the possibilities

[blueyed]

Yes, would be nice to allow for commands to the bot that it would act on.

Right now you can choose that on a per-repository basis

Could not find it in the docs at https://doc.mergify.io/getting-started.html#configuration ?!

And aren't commits rebased always in strict mode?

[jd]

@blueyed

This is what you're looking for I think: https://doc.mergify.io/configuration.html#merge-strategy

And aren't commits rebased always in strict mode?

Not necessarily. Strict mode makes sure that if you have 2 PR ready to be merged, Mergify does: Merge PR#1 by rebasing -> Rebase PR#2 on top of master (which now includes PR#1) -> Wait for CI -> Merge PR#2.

Whereas without strict mode it'll do:
Merge PR#1 by rebasing -> Merge PR#2 by rebasing

[blueyed]

Not necessarily.

Both cases you mention appear to rebase the PR?!
But the question is about strict mode only, so only the first case matters, where it rebases both of them.

https://doc.mergify.io/configuration.html#merge-strategy

rebase_fallback: If method is set to rebase, but the pull request cannot be rebased, the method defined in rebase_fallback will be used instead. Possible values are merge, squash, none. Default is merge.

When would a "merge" work while a "rebase" does not?

[jd]

Both cases you mention appear to rebase the PR?!

That is if you set the merge method to rebase in both cases. :)

When would a "merge" work while a "rebase" does not?

You can have a merge that have no conflict at all; imagine you have a branch b1 with a huge commit and its own revert. Your branch b1 is therefore in the same state as master.

Now, rebasing b1 on top of this new master will break because master changes and the first huge commit of b1 is not applicable anymore.

However, merging b1 into master works; b1 is in the state of master~, so it's easy to merge, there's no file to change, just the history of b1 to attach.

Example:

1. Preparing a repo named t with master and a newbranch

$ cd t
$ git init
Initialized empty Git repository in /private/tmp/t/.git/
$ echo "hello" > world
$ git add world
$ git commit -m 'initial commit'
[master (root-commit) d82756b] initial commit
 1 file changed, 1 insertion(+)
 create mode 100644 world
$ git checkout -b newfeature
Switched to a new branch 'newfeature'
$ echo "goodbye" > world
$ ➔ git commit world -m 'changed'
[newfeature d87ddbb] changed
 1 file changed, 1 insertion(+), 1 deletion(-)
$ EDITOR=cat git revert HEAD
hint: Waiting for your editor to close the file... Revert "changed"

This reverts commit d87ddbb8a84cecd6584c811ff00358d70e98d9fd.

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch newfeature
# Changes to be committed:
#	modified:   world
#
[newfeature f01d603] Revert "changed"
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git checkout master
Switched to branch 'master'
$ echo breakingit > world
$ git commit world -m broken
[master 47b639b] broken
 1 file changed, 1 insertion(+), 1 deletion(-)
$ cd ../
$ cp -a t t2

2. Merging:

$ cd t
$ git merge newfeature
Already up to date!
hint: Waiting for your editor to close the file... Waiting for Emacs...
Merge made by the 'recursive' strategy.
$ cat world
breakingit

3. Rebasing:

$ cd ../t2
$ git checkout newfeature
Switched to branch 'newfeature'
$ git rebase master
 world | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
First, rewinding head to replay your work on top of it...
Applying: changed
Using index info to reconstruct a base tree...
M	world
Falling back to patching base and 3-way merge...
Auto-merging world
CONFLICT (content): Merge conflict in world
error: Failed to merge in the changes.
Patch failed at 0001 changed
Use 'git am --show-current-patch' to see the failed patch

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".