domi/forgejo-domi
1
0
Fork 0
forked from forgejo/forgejo
Code Pull requests Activity

Fix verifyCommits error when push a new branch (#26664)

Browse source 
> ### 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>
This commit is contained in:
CaiCandong 2023-08-30 10:27:53 +08:00 • committed by GitHub
parent 508de3a58d
commit 815d267c80
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
43 changed files with 270 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

40
routers/private/hook_verification.go
View file

@ -28,23 +28,31 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []
_ = stdoutWriter.Close()
}()
var command *git.Command
if oldCommitID == git.EmptySHA {
// When creating a new branch, the oldCommitID is empty, by using "newCommitID --not --all":
// List commits that are reachable by following the newCommitID, exclude "all" existing heads/tags commits
// So, it only lists the new commits received, doesn't list the commits already present in the receiving repository
command = git.NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(newCommitID).AddArguments("--not", "--all")
} else {
command = git.NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(oldCommitID + "..." + newCommitID)
}
// This is safe as force pushes are already forbidden
err = git.NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(oldCommitID + "..." + newCommitID).
Run(&git.RunOpts{
Env: env,
Dir: repo.Path,
Stdout: stdoutWriter,
PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error {
_ = stdoutWriter.Close()
err := readAndVerifyCommitsFromShaReader(stdoutReader, repo, env)
if err != nil {
log.Error("%v", err)
cancel()
}
_ = stdoutReader.Close()
return err
},
})
err = command.Run(&git.RunOpts{
Env: env,
Dir: repo.Path,
Stdout: stdoutWriter,
PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error {
_ = stdoutWriter.Close()
err := readAndVerifyCommitsFromShaReader(stdoutReader, repo, env)
if err != nil {
log.Error("%v", err)
cancel()
}
_ = stdoutReader.Close()
return err
},
})
if err != nil && !isErrUnverifiedCommit(err) {
log.Error("Unable to check commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err)
}