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

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 <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>

Author: 4 people

models/fixtures/email_address.yml

@@ -276,4 +276,12 @@
   email: [email protected]
   lower_email: [email protected]
   is_activated: false
-  is_primary: false
+  is_primary: false
+
+-
+  id: 36
+  uid: 36
+  email: [email protected]
+  lower_email: [email protected]
+  is_activated: true
+  is_primary: false

models/fixtures/gpg_key.yml

@@ -1 +1,23 @@
-[] # empty
+-
+  id: 5
+  owner_id: 36
+  key_id: B15431642629B826
+  primary_key_id:
+  content: xsDNBGTrY3UBDAC2HLBqmMplAV15qSnC7g1c4dV406f5EHNhFr95Nup2My6b2eafTlvedv77s8PT/I7F3fy4apOZs5A7w2SsPlLMcQ3ev4uGOsxRtkq5RLy1Yb6SNueX0Da2UVKR5KTC5Q6BWaqxwS0IjKOLZ/xz0Pbe/ClV3bZSKBEY2omkVo3Z0HZ771vB2clPRvGJ/IdeKOsZ3ZytSFXfyiJBdARmeSPmydXLil8+Ibq5iLAeow5PK8hK1TCOnKHzLWNqcNq70tyjoHvcGi70iGjoVEEUgPCLLuU8WmzTJwlvA3BuDzjtaO7TLo/jdE6iqkHtMSS8x+43sAH6hcFRCWAVh/0Uq7n36uGDfNxGnX3YrmX3LR9x5IsBES1rGGWbpxio4o5GIf/Xd+JgDd9rzJCqRuZ3/sW/TxK38htWaVNZV0kMkHUCTc1ctzWpCm635hbFCHBhPYIp+/z206khkAKDbz/CNuU91Wazsh7KO07wrwDtxfDDbInJ8TfHE2TGjzjQzgChfmcAEQEAAQ==
+  verified: true
+  can_sign: true
+  can_encrypt_comms: true
+  can_encrypt_storage: true
+  can_certify: true
+
+-
+  id: 6
+  owner_id: 36
+  key_id: EE3AF48454AFD619
+  primary_key_id: B15431642629B826
+  content: zsDNBGTrY3UBDADsHrzuOicQaPdUQm0+0UNrs92cESm/j/4yBBUk+sfLZAo6J99c4eh4nAQzzZ7al080rYKB0G+7xoRz1eHcQH6zrVcqB8KYtf/sdY47WaMiMyxM+kTSvzp7tsv7QuSQZ0neUEXRyYMz5ttBfIjWUd+3NDItuHyB+MtNWlS3zXgaUbe5VifqKaNmzN0Ye4yXTKcpypE3AOqPVz+iIFv3c6TmsqLHJaR4VoicCleAqLyF/28WsJO7M9dDW+EM3MZVnsVpycTURyHAJGfSk10waQZAaRwmarCN/q0KEJ+aEAK/SRliUneBZoMO5hY5iBeG432tofwaQqAahPv9uXIb1n2JEMKwnMlMA9UGD1AcDbywfj1m/ZGBBw95i4Ekkfn43RvV3THr7uJU/dRqqP+iic4MwpUrOxqELW/kmeHXlBcNbZZhEEvwRoW7U2/9eeuog4nRleRJ0pi/xOP9wmxkKjaIPIK3phdBtEpVk4w/UTAWNdyIIrFggukeAnZFyGJwlm8AEQEAAQ==
+  verified: true
+  can_sign: true
+  can_encrypt_comms: true
+  can_encrypt_storage: true
+  can_certify: true

models/fixtures/user.yml

@@ -1301,7 +1301,7 @@
   lower_name: limited_org36
   name: limited_org36
   full_name: Limited Org 36
-  email: limited_org36@example.com
+  email: abcde@gitea.com
   keep_email_private: false
   email_notifications_preference: enabled
   passwd: ZogKvWdyEx:password
@@ -1320,7 +1320,7 @@
   allow_create_organization: true
   prohibit_login: false
   avatar: avatar22
-  avatar_email: limited_org36@example.com
+  avatar_email: abcde@gitea.com
   use_custom_avatar: false
   num_followers: 0
   num_following: 0

routers/private/hook_verification.go

@@ -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)
 	}

routers/private/hook_verification_test.go

@@ -0,0 +1,43 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package private
+
+import (
+	"context"
+	"testing"
+
+	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/git"
+
+	"github.com/stretchr/testify/assert"
+)
+
+var testReposDir = "tests/repos/"
+
+func TestVerifyCommits(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	gitRepo, err := git.OpenRepository(context.Background(), testReposDir+"repo1_hook_verification")
+	defer gitRepo.Close()
+	assert.NoError(t, err)
+
+	testCases := []struct {
+		base, head string
+		verified   bool
+	}{
+		{"72920278f2f999e3005801e5d5b8ab8139d3641c", "d766f2917716d45be24bfa968b8409544941be32", true},
+		{git.EmptySHA, "93eac826f6188f34646cea81bf426aa5ba7d3bfe", true}, // New branch with verified commit
+		{"9779d17a04f1e2640583d35703c62460b2d86e0a", "72920278f2f999e3005801e5d5b8ab8139d3641c", false},
+		{git.EmptySHA, "9ce3f779ae33f31fce17fac3c512047b75d7498b", false}, // New branch with unverified commit
+	}
+
+	for _, tc := range testCases {
+		err = verifyCommits(tc.base, tc.head, gitRepo, nil)
+		if tc.verified {
+			assert.NoError(t, err)
+		} else {
+			assert.Error(t, err)
+		}
+	}
+}

routers/private/main_test.go

@@ -0,0 +1,17 @@
+// Copyright 2017 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package private
+
+import (
+	"path/filepath"
+	"testing"
+
+	"code.gitea.io/gitea/models/unittest"
+)
+
+func TestMain(m *testing.M) {
+	unittest.MainTest(m, &unittest.TestOptions{
+		GiteaRootPath: filepath.Join("..", ".."),
+	})
+}

routers/private/tests/repos/repo1_hook_verification/HEAD

@@ -0,0 +1 @@
+ref: refs/heads/main

routers/private/tests/repos/repo1_hook_verification/config

@@ -0,0 +1,6 @@
+[core]
+	repositoryformatversion = 0
+	filemode = false
+	bare = true
+	symlinks = false
+	ignorecase = true

routers/private/tests/repos/repo1_hook_verification/info/refs

@@ -0,0 +1 @@
+d766f2917716d45be24bfa968b8409544941be32	refs/heads/main

routers/private/tests/repos/repo1_hook_verification/logs/HEAD

@@ -0,0 +1 @@
+0000000000000000000000000000000000000000 d766f2917716d45be24bfa968b8409544941be32 Gitea <[email protected]> 1693148474 +0800	push