Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pushing new branch fails if new branch is covered by rule requiring signed commits #25565

Closed
Craig-Holmquist-NTI opened this issue Jun 28, 2023 · 8 comments · Fixed by #26664
Closed
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Milestone

Comments

@Craig-Holmquist-NTI
Copy link

Description

If a new branch is pushed, and the repository has a rule that would require signed commits for the new branch, the commit is rejected with a 500 error regardless of whether it's signed.

When pushing a new branch, the "old" commit is the empty ID (0000000000000000000000000000000000000000). verifyCommits has no provision for this and passes an invalid commit range to git rev-list. Prior to 1.19 this wasn't an issue because only pre-existing individual branches could be protected.

I was able to reproduce with https://try.gitea.io/CraigTest/test, which is set up with a blanket rule to require commits on all branches.

Gitea Version

1.19.3

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

https://gist.github.com/Craig-Holmquist-NTI/1267366c5beb28eb38b8c49cd3de7f5c

Screenshots

No response

Git Version

2.41.0.windows.1

Operating System

Windows 10

How are you running Gitea?

gitea-1.19.3-windows-4.0-amd64.exe binary downloaded from gitea.com, run from command line in a simple test environment

Database

SQLite

@theanurin
Copy link

+1 on docker-based gitea/gitea:1.20.2-rootless

Server Log

2023/08/18 10:26:58 ...hook_verification.go:49:verifyCommits() [E] Unable to check commits from 0000000000000000000000000000000000000000 to 8cba4b5f8f4d2411c39d1474b4e24b0aa82880a7 in /var/lib/gitea/git/repositories/xxxxx/xxxxx.git: exit status 128

2023/08/18 10:26:58 .../hook_pre_receive.go:209:preReceiveBranch() [E] Unable to check commits from 0000000000000000000000000000000000000000 to 8cba4b5f8f4d2411c39d1474b4e24b0aa82880a7 in <Repository 21:xxxxx/xxxxx>: exit status 128

Client Log

git push ......
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
remote: 
remote: Gitea: Internal Server Error (no message for end users)
To ssh://git.xxxxxxxxx.xxx:xxxxx/xxxxx/xxxxx.git
 ! [remote rejected]     dev -> dev (pre-receive hook declined)
error: failed to push some refs to 'ssh://git.xxxxxxxxx.xxx:xxxxx/xxxxx/xxxxx.git'

@CaiCandong
Copy link
Member

CaiCandong commented Aug 21, 2023

I can reproduce this bug too.
The command git rev-list old...new can't handle the case of old is an empty ID. Does anyone have a solution to list the sha of G and H.
~E5CZAL 0CUQV`KXDJB0A7H

@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Aug 21, 2023
@Craig-Holmquist-NTI
Copy link
Author

One solution (per https://stackoverflow.com/a/34902814) is to use:

git rev-list newCommitID --not --all

whenever the old commit ID is empty. This works because the new branch doesn't exist when the pre-receive hook is run, so it finds commits reachable from newCommitID but not from any pre-existing ref.

@CaiCandong
Copy link
Member

git rev-list newCommitID --not --all

no work

@Craig-Holmquist-NTI
Copy link
Author

git rev-list newCommitID --not --all

no work

You'll have to provide a little more detail. It works in a fork my company's been using.

@CaiCandong
Copy link
Member

@Craig-Holmquist-NTI
I modified the Gitea code to look like this :

image

but it did not work, the result of this command git rev-list newCommitID --not --all is empty

@Craig-Holmquist-NTI
Copy link
Author

@CaiCandong
I think that code will run git rev-list --not --all newCommitID, not git rev-list newCommitID --not --all. newCommitID has to come before --not --all; otherwise the --not negates it too.

What ours looks like:

var cmd *git.Command
	
if oldCommitID == git.EmptySHA {
    cmd = git.NewCommand(repo.Ctx, "rev-list", newCommitID, "--not", "--all")
} else {
    cmd = git.NewCommand(repo.Ctx, "rev-list", oldCommitID+"..."+newCommitID) 
}

@CaiCandong
Copy link
Member

I think that code will run git rev-list --not --all newCommitID

It worked, thank you very much!

@lunny lunny added this to the 1.20.4 milestone Aug 22, 2023
silverwind pushed a commit that referenced this issue Aug 30, 2023
> ### Description
> If a new branch is pushed, and the repository has a rule that would
require signed commits for the new branch, the commit is rejected with a
500 error regardless of whether it's signed.
> 
> When pushing a new branch, the "old" commit is the empty ID
(0000000000000000000000000000000000000000). verifyCommits has no
provision for this and passes an invalid commit range to git rev-list.
Prior to 1.19 this wasn't an issue because only pre-existing individual
branches could be protected.
> 
> I was able to reproduce with
[try.gitea.io/CraigTest/test](https://try.gitea.io/CraigTest/test),
which is set up with a blanket rule to require commits on all branches.


Fix #25565
Very thanks to @Craig-Holmquist-NTI for reporting the bug and suggesting
an valid solution!

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue Aug 30, 2023
> ### Description
> If a new branch is pushed, and the repository has a rule that would
require signed commits for the new branch, the commit is rejected with a
500 error regardless of whether it's signed.
> 
> When pushing a new branch, the "old" commit is the empty ID
(0000000000000000000000000000000000000000). verifyCommits has no
provision for this and passes an invalid commit range to git rev-list.
Prior to 1.19 this wasn't an issue because only pre-existing individual
branches could be protected.
> 
> I was able to reproduce with
[try.gitea.io/CraigTest/test](https://try.gitea.io/CraigTest/test),
which is set up with a blanket rule to require commits on all branches.


Fix go-gitea#25565
Very thanks to @Craig-Holmquist-NTI for reporting the bug and suggesting
an valid solution!

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
silverwind pushed a commit that referenced this issue Aug 31, 2023
Backport #26664 by @CaiCandong

> ### Description
> If a new branch is pushed, and the repository has a rule that would
require signed commits for the new branch, the commit is rejected with a
500 error regardless of whether it's signed.
> 
> When pushing a new branch, the "old" commit is the empty ID
(0000000000000000000000000000000000000000). verifyCommits has no
provision for this and passes an invalid commit range to git rev-list.
Prior to 1.19 this wasn't an issue because only pre-existing individual
branches could be protected.
> 
> I was able to reproduce with
[try.gitea.io/CraigTest/test](https://try.gitea.io/CraigTest/test),
which is set up with a blanket rule to require commits on all branches.


Fix #25565
Very thanks to @Craig-Holmquist-NTI for reporting the bug and suggesting
an valid solution!

Co-authored-by: CaiCandong <50507092+CaiCandong@users.noreply.github.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants