GitHub “allows” unauthorized users “merging” PRs, bypass write permission check? | Hacker News Reader

GitHub “allows” unauthorized users “merging” PRs, bypass write permission check?

github.com

76 pointssighingnow43 comments

We recently noticed some strange email notifications from Github with content "Merged #948 into main.", which told that pull requests on our repo has been merged by stranger who actually doesn't have the write permission to our repo!

After further inspection, we found that the merge event is triggered by the creator of the pull request pushing current main commits to the PR's "from" branch.

Moreover, when pushing current main to a pull request,

- The pull request is displayed as "Merged"

- A "PR merged into main" email is sent to all subscribers (mainly the repo owners)

- A "PR merged" contribution is displayed on the creator's Github profile

Closing dangling pull requests is a quite resonable design, but mark it as "Merged" rather than "Closed" would confuse people to let them think they are hacked at the first glance (note that there even an email notification "Merged #xxxx into main" to repo's owners).

If such a feature is misused, it may lead to chaos to more open source repos in the futuer, especially those famous ones.

See some example links in

- https://github.com/v6d-io/v6d/pull/948

- https://github.com/alibaba/GraphScope/pull/1931

This does strike me as kind of weird but in the same way it's weird that github lets you author commits with other people's email address and it shows up with a reference to their github account. A lot of github has very strange UI edge cases which come from the fact their features are a leaky abstraction over core git operations. Since the PR diff shows no changes were made and the merged commit is one that's authored by an actual contributor, I'm not sure it's as much a security vulnerability as a curiosity.

Git allows this. Github is just doing what it's told with the data it has. If you don't like this, ignore unsigned commits.

Unsigned commits won't help in this specific case though, no?

Git and Github both allow you to put whatever email you want. If you care about being certain who is committing to your repo, you should ignore the email and only look at the commit signature.

If you're talking about the issue OP is discussing, it should still be possible even if it's a signed commit. 61f3741 is a signed commit in the linked PR.

This just re-uses existing commits on the repository. The commits can be signed and github will still show "merged by X" if neither X nor the author of the signed commit merged the PR.

So really it's "if you care about being certain who is committing to your repo, you should ignore who github says is committing to your repo", which, to my earlier point, is technically understandable when you dig into it but nonetheless a little weird from a UX perspective.

If you're talking about forging the commit author, that's also weird. It makes sense in the decentralized context of git, but not in how most people use github. Nobody is saying that it isn't allowed, but the fact that github allows it is really an artifact of the fact that git allows it. In the github web app, your account is email verified, so it's weird that someone can generate commits which (in the UI) link to your email verified github account that were not actually created by you. Most people don't expect webapps to work this way, even if git might. It'd be similarly weird if facebook allowed people to create posts on your behalf and we told users "oh that's not weird, you should really verify the GPG key of your posts".

The concern here is not about who "commit" this, the concern is who "click the merge button" in this case.

https://news.ycombinator.com/item?id=10005577

GitHub ties "things" to a commit hash - which causes all sorts of interesting/unwanted behaviors.

    * create an action that only runs on branches with a specific pattern
    * create another action that only runs on branches with a different pattern
    * push a commit to a branch with the first pattern, create a PR
    * create a new branch with the second pattern
    * push the same commit on the second branch
    * watch things get confused on the PR that was originally created

I had seen similar things, but slightly different: a user forked our repo (with open PRs), then merged one of the existing PRs from their fork into their fork. GitHub "helpfully" sent mail to the original PR author about it, with the link pointing to the fork (so it shows up (correctly) as _merged_ — in the fork).

Just to be 100% sure:

1) Is it a real problem that allows anyone to push into any repository?

2) Is it just a very confusing message?

> 1) Is it a real problem that allows anyone to push into any repository?

It is not a real problem that allows anyone to push into any repo, but is a real problem that shows a incorrect/unexpected "Merged" pull request status on any repository.

> 2) Is it just a very confusing message?

From the links in paste in the comments you could see that github shows the unauthorized user "merged" the pull request into main, and, the repo's owner received a email says:

FROM: XXXXX Content: Merged #xxx into main.

It is exactly the SAME as email notification of a normal authorized merge event.

It's the latter.

Basically, aside from github's merge button (which does magic inaccessible to mere mortal[0]) the signal github looks for to know whether a PR is merged is whether the PR's head commit is in the target branch.

So if you reset the PR's branch to the target (or any of its commit), as far as github is concerned it's as if the PR had been merged.

[0] the ability to close PRs as merged was requested 3 years ago on the old discourse forum, which was deleted when github deployed the new community thingie, the request was reposted on the new site https://github.com/community/community/discussions/12437

+1 for the feature of marking as merged, as in many projects, e.g., apache arrow, the pull requests are merged in a different way and all pull requests are showed as "closed" rather than "merged".

It would cause some confusion for project management, I guess.

So the whole Github is my resume can be gamed to look like I have contributed to high profile open source projects?

Only if you get something actually accepted on a big OSS project can you make it look like someone else did it. You can't just magically get stuff committed to Linux or Chrome or whatever, that project would need to accept the PR. You could make people look bad/weird by making and accepting PRs for your own project though. But I think most people seem aware of this nature of unsigned git commits having any email address that the author wants

This issue is specifically that you can make github think you merged a PR to a project for which you do not have merge permission. Your PR would be empty but github would show that you were a contributor (potentially anyway, I don't know it's been verified it gives you contributor status).

EDIT: Since bifenglin didn't get marked as a contributor in the second example from GP above I wonder if it's not actually possible. It could require a commit physically in the repo with an email linked to your account.

You don't need to get accept to "merge" a PR. When pushing the main branch to your PR, the PR would get closed as "Merged" and there will be a "Merged" contribution badage showed in your github profile.

https://github.com/gelstudios/gitfiti

Can someone explain what we are looking at? Perhaps a simple point by point explanation of what was done, what the expected behavior is and what we observe instead?

Do I understand correctly that these are the events that happened:

* septicmk forked v6d-io/v6d to septicmk/v6d

* septicmk created a pull request from septicmk/v6d:main to v6d-io/v6d:main

* septicmk merged sighingnow's commit 61f3741 to septicmk/v6d:main (perhaps while updating their fork?)

* GitHub says septicmk merged commit 61f3741 to septicmk/v6d:main?

If so is that not right? They did merge a commit in their fork at septicmk/v6d:main, didn't they?

Does GitHub say anywhere that they merged commit to v6d-io/v6d:main? That would be very interesting and a real issue!

I agree the big "Merged" button on top of the pull request is very misleading, perhaps a UX bug! But no unauthorized merge actually happened here, did it?

What happened here (if I understand right) is:

* septicmk forked v6d-io/v6d to septicmk/v6d

* septicmk added commits to septicmk/v6d:main

* septicmk created a pull request from septicmk/v6d:main to v6d-io/v6d:main

* septicmk force pushed septicmk/v6d:main with the head commit of v6d-io/v6d:main

* Github closed the pull request and displayed a "merge" notification

The UX issue is that septicmk did not have write permission to v6d-io and did not actually merge any commit into it, but GitHub says "septicmk merged commit 61f3741 into v6d-io:main". Commit 61f3741 was authored by sighingnow and was already on v6d-io:main.

So in this case no authorized merge actually happened, the PR was just updated to have no difference from the actual repository's main branch, but GitHub makes it seem like a merge happened.

GitLab team member here. It seems related to the problem that you cannot open an empty PR https://stackoverflow.com/questions/46577500/why-cant-i-crea... and force overriding an old branch base is a workaround to land at an empty diff. Another workaround could be git commit —-allow-empty, making the GitHub UI think that no changes between branches is equal to the merged state.

The behavior is not reproducible in GitLab, where empty merge requests can be created, and remain in the open state, also with zero changes at some later point. Added in GitLab 9.2 as part of the issue to new branch and wip/draft merge request flow: https://gitlab.com/gitlab-org/gitlab-foss/-/issues/28558

I think that Gitlab behaviour makes more intuitive sense. Thanks for chiming in.

Allowing "zero changes" state for a merge request does make sense, as during the development workflow developers may doing some rebase & force push work and they may forget commit changes after reset to main and before push to the pull requests.

Closing the "zero changes" merge request have confused in my previous experience when I continue to push re-added commits to the original pull request branch.

Looks like they did a force-push that replaced the PR's from-branch (septicmk/v6d:main) with the state of the target branch. This effectively removes all commits from the PR, leaving the PR with zero remaining commits. All of the zero commits on the PR are fully merged into v6d-io/v6d:main, thus marking the PR as merged (technically correct, but confusing).

So in effect, unauthorized merges (PR looking merged; email notifications being sent) are possible if they are "zero-commit-merges" that don't actually change any state in the git repository.

The "zero-commit-merges" should be treated as a "Closed" event better than being treated as a "Merged" event.

From Git's perspective, the two "forks" share the same HEAD commit, e.g. they are merged. That is what a merge does. A branch is a name that exists independently of the commit and points to it, so the commit history is not actually a property of the branch, despite what Git UIs tell you.

Thus, in the underlying data structure behind the GitHub interface, there really isn't an "event" here to identify. The PR branch points to the same commit as the base branch, therefore both branches are in the state of existing as "merged" with one another.

So GitHub would have to track changes to the PR branch that result in this state separately from the existing "merge from GUI" and "push PR branch to master" changes, which I could imagine is fraught with edge cases that could result in what you consider a merge event ending up as a closed event.

indeed, I could reproduce it, just force push `master` into a PR, github UI will close the PR, marked it as merged, delete the source branch ... but of course nothing happens in the git repo :/

GitHub automatically marks a PR as merged if its HEAD commit hash matches the BASE commit hash. Usually it happens when the PR code goes into the main branch, but it can also happen if the PR branch is forcibly reset to match the main branch. The bug here is that it closes the discussion prematurely (a discussion on a zero diff, at that specific moment in time). There's no unauthorized source code modification, as you noted. Awkward but meh.

    if (baseBranchCommits.includesAll(prBranchCommits)) status = merged;

That's pretty much the Github logic. The PR branch was updated to include the commits of the base branch and force-pushed, which caused github to run the above merge logic.

This is useful in many cases, but is still very confusing UX in such cases. Nothing was exploited, the PR update messages are just... well, poorly designed.

> That's pretty much the Github logic.

Of note: github can actually override this logic internally, in a way which they have not made accessible through the API: if you use github's merge button to squash or rebase-merge a PR, it'll be marked as merged, despite the PR's commits not being part of the target branch.

I think the current behavior is useful: if the repo owner decides to merge a PR manually (e.g., with the git command line), the PR ends up in a merged state as expected. Otherwise the owner has to do two actions: first on the command line, and then (to avoid duplicate merge commits) close (not merge!) on the web interface. Beyond the extra work, it's ugly because all the PRs in the repo are (incorrectly) marked as closed, not merged.

The issue seems to be that there is no provenance tracking. If a commit appears first in the contributed repo, and then makes its way into the base branch, I think that can reasonably be considered merged; if the contributed branch gets overwritten with commits already in the base branch, that is no longer a contribution at all and just the contributor resetting the branch (perhaps with malicious intent, or else through sheer ignorance).

I mean, yeah. This isn't the only status logic. Just one part I've observed. GitHub is very much event-based.

Could this cause a GitHub action to run?

For the target repo, only on the pull_request event[0] would run; nothing is being `push`ed to the repo, so it doesn't allow something like pushing an actions script that exfiltrates secrets.

0: https://docs.github.com/en/actions/using-workflows/events-th...

It looks like default branch was force-pushed to the last commit on the branch of the PR? If you have permission to push … that's not "unauthorized"?

That a PR gets "merged" if the base branch becomes equal with the PR's last commit is … a bit weird, but it makes perfect sense in cases like where one might want to FF the base to keep things clean. The force-push muddles it a bit. Surprising to new users, maybe, but "unauthorized" seems like a stretch unless I'm missing something.

There are cases where I think Github's controls could be much clearer. E.g., in some cases, you can "require" reviews, yet still change the branch after the approval. So once you have approval … it's like carte blanche to merge whatever. It is sometimes useful — if the repo is not security critical, and you trust your reviewee, it lets me say "these three changes please, then LGTM", approve it, and just trust they'll do the right thing. But if you need tight controls, then not so much. There's also Github Actions & required checks … if a "required check" is skipped, Github treats that as if the check passed! (That, to me, is a bug. To Github's support reps … that's just how it works.)

>It looks like default branch was force-pushed to the last commit on the branch of the PR? If you have permission to push … that's not "unauthorized"?

No, other way around. upstream was originally at 61f3741 . PR was at a31b8dd which was a commit on top of 61f3741 . Then they force-pushed the PR to 61f3741 . It's still not a problem because it's just the PR branch that was modified. And as OP says elsewhere in this thread, it's definitely confusing to mark it as "Merged" even though it's technically correct.

@dang i'd recommend changing the title on this one. I think the title is somewhat misleading, since it doesn't really explain what is happening. See [0][1]

[0] https://news.ycombinator.com/item?id=32598458

[1] https://news.ycombinator.com/item?id=32598471

Reminds me of how in Azure DevOps, every merge commit or PR to any branch with any work-item in its history would then mark the work item as modified and add the PR/merge to its links. We were maintainging a lot of branches and ended up confusing the heck out of project managers as to why their work items kept getting modified.

Not the only permissions issue at GitHub

https://cerbos.dev/blog/githubs-inconsistent-access-control

Wonder if you could trigger an Actions run this way

Tangential question - how do I check a list of people who can write to a public GitHub repo and merge a PR? Especially so if said repo is under a personal profile, not an organization.

settings -> collaborators. If you aren't a owner/admin of the repo you can't see the users with push permission.

Reply on news.ycombinator.com