Git hooks check old commits when creating branch from another branch

We're having an issue where when creating a branch from another branch the member_check git hook fails. This is an issue for us, as we have many imported projects which contain existing commits from people who never had an account in GitLab.

Create a new project, with member_check git_hook disabled. Clone the empty project, and do the following:

git config --local user.email "john.doe@example.com"
echo ${RANDOM} > README; git add README; git commit -m"Testing" README
git push
git checkout -b new-branch-1
echo ${RANDOM} > README; git commit -m"Testing" README
git push -u origin new-branch-1

Now enable the member check and continue as follows:

git config --local user.email "<Your valid member e-mail>"
git checkout master
git checkout -b new-branch-2
git push -u origin new-branch-2
git checkout new-branch-1
git checkout -b new-branch-3
git push -u origin new-branch-3

With the branch created from new-branch-1 instead of master, the commit hook denies the push. Even though there are no new commits and it should be no different than with new-branch-2 which was created from master.

I suspect (I'm not a developer) it's going wrong in lib/gitlab/git_access.rb on line 191 where oldrev is actively set to the default branch:

oldrev = project.default_branch if Gitlab::Git.blank_ref?(oldrev)

Probably either oldrev should be set to the branch the new branch is created from or the rev-list should be retrieved as in:

git rev-list newrev --not --all

See this StackOverflow post: http://stackoverflow.com/a/22547375/114983

Kind regards, Bartosz

Added ~91797 label

Added ~113221 label

This is very important for the customer, would you mind taking a look @vsizov?

ZD: https://gitlab.zendesk.com/agent/tickets/15464

sure I'll take a look right now.

Reassigned to @vsizov

@boudekerk I just tried to create repo like in your example but command git rev-list newrev --not --all returns commits from master as well, so it does not work as you expect. @DouweM @rspeicher Do you have some ideas?

In my opinion, current behaviour is totally right. But as a user who enable git hook you need to know that you should make sure that every branch is valid and meets project requirements.

Hi @vsizov,

I don't agree that the current behaviour is in any way correct. A hook should only apply to new commits, but at the very least there should not be a difference in behaviour between branching from master or another branch.

BTW, for me the rev-list doesn't return any commits (it shouldn't) using the reproduction path above. Please try with the following pre-receive hook:

#!/bin/bash
INPUT="$(cat -)"
echo "======================================================================="
echo \"${INPUT}\"
echo "======================================================================="
git rev-list $(echo "${INPUT}" | awk '{print $2}') --not --all
echo "======================================================================="

The output should be similar to the below:

remote: =======================================================================
remote: "0000000000000000000000000000000000000000 17d9e4b5441655499c2f541309789505bebe5a0b refs/heads/new-branch-3"
remote: =======================================================================
remote: =======================================================================

Kind regards, Bartosz

For reference I just performed my reproduction path above with this hook enabled for each step:

• git push

remote: =======================================================================
remote: "0000000000000000000000000000000000000000 d64552d5a0fba16b14b52dd4f87bc296f1ff128a refs/heads/master"
remote: =======================================================================
remote: d64552d5a0fba16b14b52dd4f87bc296f1ff128a
remote: =======================================================================

• git push -u origin new-branch-1

remote: =======================================================================
remote: "0000000000000000000000000000000000000000 d7d0892ae7b93df3270d56e89bfd4a3856818e39 refs/heads/new-branch-1"
remote: =======================================================================
remote: d7d0892ae7b93df3270d56e89bfd4a3856818e39
remote: =======================================================================

• git push -u origin new-branch-2

remote: =======================================================================
remote: "0000000000000000000000000000000000000000 d64552d5a0fba16b14b52dd4f87bc296f1ff128a refs/heads/new-branch-2"
remote: =======================================================================
remote: =======================================================================

• git push -u origin new-branch-3

remote: =======================================================================
remote: "0000000000000000000000000000000000000000 d7d0892ae7b93df3270d56e89bfd4a3856818e39 refs/heads/new-branch-3"
remote: =======================================================================
remote: =======================================================================

Note how the rev-list command only produces output for the first two pushes, the ones which actually have new commits, and that it only outputs those new commits. Also note that the newrev for the branch pushes of new-branch 2 & 3 reference the commits/newrevs of master and new-branch-1 respectively.

Please do note that the order of the parameters in the rev-list command is relevant.

Kind regards, Bartosz

@boudekerk Try to create a commit in new-branch-3 branch and then run git rev-list {newrev} --not --all and you won't see any commit, but we should check this commit somehow, right?. In my comment above I used a bit different git repo and I tested another case. I just wanted to say that command you mentioned does not work.

Let me summarise. We have commit ref A with all valid commits, we also have ref B based on A with invalid commit. Desired behaviour is an ability to push new ref C which is based on ref B(with invalid commit). For now it's impossible because you can't push invalid commits anymore since you enabled git hook. It's totally correct behaviour to me.

To be honest I partially agree with you as it would be great if GitLab can check commits that actually created by you.

Hi @vsizov,

I'll reply to your comments in order.

• When doing a commit in new-branch-3, you will see the commit with the rev-list command in your pre-receive hook. If you don't see it, something is seriously wrong with your setup.

git checkout new-branch-1
git checkout -b new-branch-4
echo ${RANDOM} > README; git commit -m"Testing" README
git push -u origin new-branch-4
...
remote: =======================================================================
remote: "0000000000000000000000000000000000000000 938c47a88a8fbf42ffc08cc1d8280a24c23c38c0 refs/heads/new-branch-4"
remote: =======================================================================
remote: 938c47a88a8fbf42ffc08cc1d8280a24c23c38c0
remote: =======================================================================

commit 938c47a88a8fbf42ffc08cc1d8280a24c23c38c0 is the only new commit being pushed, and as such, in my opinion, the only one that should be checked. If you do multiple commits, you'll see them all listed.

Anyway, the rev-list command should work, it's basic git. If you can list your exact steps perhaps I can help you figure out why it's not behaving as it should on your remote.

@vsizov As to your summary, I'd like to add that all commits (valid & invalid) have already been pushed. (With my reproduction path there's no valid commits BTW)

To rewrite your summary including the above, and slightly modified to match my reproduction path for consistency in case of other people getting involved in this issue:

Both are already present on the remote.
Now we create ref C based on A, and ref D based on B.
Pushing ref C succeeds, and pushing ref D does not.```

Note that both are based of a ref containing one or more invalid commits.

You say: "you can't push invalid commits". Please note that I'm not pushing any invalid commits, everything I'm pushing is valid. It's failing on those invalid commits that are already present in the remote repository.

And I don't want GitLab to only check commits created by me. What I want is GitLab to check *all* commits (by anyone) being pushed, but not any older commits already present on the remote. This is how all git hooks I've ever come across worked in my experience. To use the member check as an example, the behaviour you describe as correct means that when an employee leaves we have to leave their account active forever as otherwise people won't be able to push new branches?

But, even if you disagree on which behaviour is correct, I assume you do agree that the behaviour should at least be consistent.
Now we have different behaviour depending on which branch the branch being pushed has been created from, which in my opinion is a bug in itself.

@boudekerk Thanks for your explanation, I really appreciate that. I read your comments one more time with fresh head after morning coffee and now I totally agree with you. Yesterday I was playing with commands you provided and it did not work for me, maybe I did something wrong. Today and tomorrow I have days off so I will work on it on Monday (most probably). Thanks one more time.

@vsizov No problem. Just let me know if there's anything I can do to help.

Title changed from Member check checks old commits when creating branch from another branch to Git hooks check old commits when creating branch from another branch

Hi @vsizov,

it seems this issue, or at least a very similar one, also occurs when merging. It seems to check previously pushed commits from the other side of the merge. Here as well you can use the same rev-list command as above to only return the commits that should be checked.

git config --local user.email "doe@example.com"
echo ${RANDOM} > README && git add README && git commit -m"Testing 1" README && git push
git checkout -b test-branch && git push -u origin test-branch
git config --local user.email "john.doe@example.com"
echo ${RANDOM} > README.tb && git add README.tb && git commit -m"Testing tb 1" README.tb && git push
git checkout master
git config --local user.email "jane.doe@example.com"
echo ${RANDOM} > README.m && git add README.m && git commit -m"Testing m 1" README.m && git push
git checkout test-branch

Now enable the member_check:

git config --local user.email "<Your valid member e-mail>"
git merge -m"Test merge" master
git push

It will fail on "jane.doe@example.com", which was the author on master. When doing the merge the other way, it will fail on "john.doe@example.com" which was the author on test-branch.

Kind regards, Bartosz

P.S. We had similar issues in another (non GitLab) hook. Not sure how much use this is, but see here: https://github.com/gnustavo/Git-Hooks/issues/24 https://github.com/gnustavo/Git-Hooks/issues/26

sure, as it is using the same piece of code.

I'm looking at this issue.

@boudekerk good timing thanks for expanding.

@vsizov thank you very much for following up.

OK, that was not hard to implement it locally, the only problem left is that it does not work when we create commit by rugged like web-editor. The reason is obvious but it should be handled properly anyway. I will brainstorm it tomorrow morning.

@vsizov Glad to hear you've found a (partial) solution. Let me know if you'd like me to test anything.

@boudekerk Unfortunately I have not, because my fix fixes HTTP and SSH pushes but breaks Web-editor pushes (to be specific, I mean git hook checks for pushes).

The problem is that when we edit repo with rugged we fire pre-receive hook when ref is already updated, yes, not actual ref but temporary ref, anyway repo is already updated with new commit and it makes a difference from what we have when we receive changes with git-receive-pack. This is why git rev-list newrev --not --all does not work in this case and this is why I was confused and said that it does not work at all (I tested it with web-editor). So there is two possible solutions here: a) improve hook triggering mechanism for rugged editor, b) determine how did we receive the changes which is kinda hack.

@dzaporozhets What is your opinion on this?

Assignee removed

I will return to this task a bit later as I have more important tasks in my task list but this task pretty important as well so I don't mind if someone pick it up. I think there are a lot of reports about this, at least I heard about it from @patricio as well.

improve hook triggering mechanism for rugged editor

@vsizov how can we improve it?

@dzaporozhets I don't know yet, some deep research should be done.

@vsizov ok I think we will go with most easy/simple option

Milestone changed to 8.6

Thanks @vsizov for looking into this. Could you share update when you start on this please?

@balameb Sure, this issue is in my pinned tabs in Chrome :)

@vsizov excellent thanks.

I still have other important issues, so it's available to pick up by someone else.

Ping @jameslopez @rspeicher with cc to @DouweM . Would appreciate an updated estimate of if this will make it into 8.6

mentioned in issue #40 (closed)

I'll have a look at this one...

Reassigned to @jameslopez

Thanks @jameslopez, curious for updates when you have them.

mentioned in merge request !281 (merged)

@ernstvn this one is a bit tricky and I was also distracted with other MRs so I'm still trying a few solutions... I'll keep this thread updated.

working on another more important issue - will resume my work on this after.

Milestone changed to 8.7

@ernstvn This will go into 8.7

@DouweM Thanks for the update despite the unfortunate nature. Any chance it can make it into an 8.6 patch? It is causing considerable pain to our customer. \cc @balameb

@boudekerk Sorry about this being pushed back another release. Unfortunately we didn't have the developer capacity to properly investigate this until a week before release, and then it was pushed back in the queue again when a security issue took precedence.

It's quite a tricky issue, but the aim is to release it in one of the first few patch releases (we did one already today, so there could be a number of them)

@jameslopez is currently finishing up that security issue, and will continue here afterwards.

@DouweM Thank you for the update. I'll make sure to keep an eye on the coming patch releases.

Status changed to closed by merge request !281 (merged)

mentioned in commit 9310e6ea

Hi @jameslopez @rspeicher @DouweM,

I just (manually) applied the changes from the merge request linked above to lib/gitlab/git_access.rb. So perhaps it's possible I missed something, but while this does fix the reproduction path in the description, the reproduction path I added later (merge) still fails. So unless I made a mistake applying the changes, this only seems to be a partial fix covering only the first case.

Kind regards,
Bartosz

Status changed to reopened

@boudekerk thanks for testing this - I haven't actually create a test for that particular path. I assumed from the discussion it was the same piece of code but it looks like it's a separate issue. My suggestion would be to close this and open a new issue for that particular case ? /cc @DouweM @rspeicher

TODO: Also fix the milestone here if we decide to close this as this went to 8.6.2

mentioned in commit 35e0ee1f

@jameslopez @rspeicher I'm not a developer, but retrieving new commits (to check) from a push will happen in a single piece of code regarless of whether it's branching or merging I presume? I'm not sure you can even determine whether it's a merge, new branch or regular commit without looking at those first. Correct me if I'm wrong.

I'm happy to create a new issue, it's just that functionally it's pretty similar. With this fix in place we'd be able to branch, but further down the line wouldn't be able merge that branch, which is limiting our workflow. And there's a lot of information in this issue that would have to be duplicated. Please let me know whether you'd still prefer us to create a new issue.

BTW, please don't get me wrong. I really appreciate all the effort that was put into nailing down and fixing the issue so far.

@jameslopez If this particular issue is not solved why would we need to close this issue?

@vsizov happy to keep this issue open. Strictly speaking the description / title of this issue has been fixed. Perhaps we should add the problem with merging to it.

@jameslopez Ah, OK.

@jameslopez RE: new issue/re-using this one -- up to you. It might focus the discussion more to open a new one.

mentioned in issue #428 (closed)

Status changed to closed

Cool. Created new issue https://gitlab.com/gitlab-org/gitlab-ee/issues/428

mentioned in issue #429

mentioned in issue #497 (closed)

mentioned in issue #587 (closed)

mentioned in issue #815 (closed)

Mentioned in commit pfjason/gitlab-ce@9310e6ea

mentioned in issue #1736 (closed)

mentioned in commit 5994b3b6