7 ms·

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

76 points by sighingnow 4y ago

sighingnow 4y ago
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/v6d-io/v6d/pull/948 - https://github.com/alibaba/GraphScope/pull/1931 https://github.com/alibaba/GraphScope/pull/1931
- gus_massa 4y ago
  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?
  - sighingnow 4y ago
    > 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.
  - masklinn 4y ago
    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 https://github.com/community/community/discussions/12437
    - sighingnow 4y ago
      +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.
- mook 4y ago
  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).
- jerrysievert 4y ago
  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
- chrisfosterelli 4y ago
  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.
  - aliqot 4y ago
    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.
    - majewsky 4y ago
      Unsigned commits won't help in this specific case though, no?
      - henryfjordan 4y ago
        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.
        chrisfosterelli 4y ago
        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".
- stonemetal12 4y ago
  So the whole Github is my resume can be gamed to look like I have contributed to high profile open source projects?
  - gleenn 4y ago
    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
    - chrisfosterelli 4y ago
      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.
    - sighingnow 4y ago
      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.
  - judge2020 4y ago
    https://github.com/gelstudios/gitfiti https://github.com/gelstudios/gitfiti
ctrlmeta 4y ago
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?
- chrisfosterelli 4y ago
  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.
  - dnsmichi 4y ago
    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-create-an-empty-pull-request-for-discussion-prior-to-developing-chan 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 https://gitlab.com/gitlab-org/gitlab-foss/-/issues/28558
Pxtl 4y ago
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.
junon 4y ago
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.
- masklinn 4y ago
  > 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.
  - junon 4y ago
    I mean, yeah. This isn't the only status logic. Just one part I've observed. GitHub is very much event-based.
  - eslaught 4y ago
    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).
- arriu 4y ago
  Could this cause a GitHub action to run?
RicoElectrico 4y ago
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.
- judge2020 4y ago
  settings -> collaborators. If you aren't a owner/admin of the repo you can't see the users with push permission.
jordemort 4y ago
Wonder if you could trigger an Actions run this way
trevcanhuman 4y ago
@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 https://news.ycombinator.com/item?id=32598458 [1] https://news.ycombinator.com/item?id=32598471 https://news.ycombinator.com/item?id=32598471
deathanatos 4y ago
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.)
- Arnavion 4y ago
  >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.
alex-olivier 4y ago
Not the only permissions issue at GitHub https://cerbos.dev/blog/githubs-inconsistent-access-control https://cerbos.dev/blog/githubs-inconsistent-access-control

76 points by sighingnow 4y ago

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/v6d-io/v6d/pull/948 - https://github.com/alibaba/GraphScope/pull/1931 https://github.com/alibaba/GraphScope/pull/1931

gus_massa 4y ago
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?
- sighingnow 4y ago
  > 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.
- masklinn 4y ago
  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 https://github.com/community/community/discussions/12437
  - sighingnow 4y ago
    +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.
mook 4y ago
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).
jerrysievert 4y ago
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
chrisfosterelli 4y ago
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.
- aliqot 4y ago
  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.
  - majewsky 4y ago
    Unsigned commits won't help in this specific case though, no?
    - henryfjordan 4y ago
      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.
      - chrisfosterelli 4y ago
        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".
stonemetal12 4y ago
So the whole Github is my resume can be gamed to look like I have contributed to high profile open source projects?
- gleenn 4y ago
  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
  - chrisfosterelli 4y ago
    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.
  - sighingnow 4y ago
    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.
- judge2020 4y ago
  https://github.com/gelstudios/gitfiti https://github.com/gelstudios/gitfiti

ctrlmeta 4y ago

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?

chrisfosterelli 4y ago
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.
- dnsmichi 4y ago
  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-create-an-empty-pull-request-for-discussion-prior-to-developing-chan 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 https://gitlab.com/gitlab-org/gitlab-foss/-/issues/28558

Pxtl 4y ago

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.

junon 4y ago

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.

masklinn 4y ago
> 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.
- junon 4y ago
  I mean, yeah. This isn't the only status logic. Just one part I've observed. GitHub is very much event-based.
- eslaught 4y ago
  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).
arriu 4y ago
Could this cause a GitHub action to run?

RicoElectrico 4y ago

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.

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

jordemort 4y ago

Wonder if you could trigger an Actions run this way

trevcanhuman 4y ago

@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 https://news.ycombinator.com/item?id=32598458 [1] https://news.ycombinator.com/item?id=32598471 https://news.ycombinator.com/item?id=32598471

deathanatos 4y ago

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.)

Arnavion 4y ago
>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.

alex-olivier 4y ago

Not the only permissions issue at GitHub https://cerbos.dev/blog/githubs-inconsistent-access-control https://cerbos.dev/blog/githubs-inconsistent-access-control