fix(pull): sign the commit when updating a branch by merge (#38441) (#38499)

Updating a branch by merge produced an unsigned commit even when merges
are configured to be signed. Update by rebase was unaffected.

`Update()` builds a fake reverse PR to switch head and base, and it has
no `Index`, so `pr.GetGitHeadRefName()` resolves `refs/pull/0/head`.
Since #36186 `SignMerge` looks that ref up in the base repository
instead of the temporary merge repo. That lookup fails. The caller
dropped the error, so `sign` stayed false.

Sync fork goes through the same fake-PR path.

Pass both sides of the merge to `SignMerge` as refs and evaluate them in
the temp repo, where `base` and `tracking` always exist.

Tests cover a signed and an unsigned update by merge.

Fixes #38066
Backport #38441

routers/web/repo/issue_view.go

@@ -495,7 +495,7 @@ func (prInfo *pullRequestViewInfo) prepareMergeBoxCommitSigning(ctx *context.Con
 
 	wontSignReason := ""
 	if ctx.Doer != nil {
-		sign, key, _, err := asymkey_service.SignMerge(ctx, pull, ctx.Doer, ctx.Repo.GitRepo)
+		sign, key, _, err := asymkey_service.SignMerge(ctx, pull, ctx.Doer, ctx.Repo.GitRepo, pull.BaseBranch, pull.GetGitHeadRefName())
 		data.willSign = sign
 		data.signingKeyMergeDisplay = asymkey_model.GetDisplaySigningKey(key)
 		if err != nil {

services/asymkey/sign.go

@@ -270,19 +270,21 @@ Loop:
 	return true, signingKey, sig, nil
 }
 
-// SignMerge determines if we should sign a PR merge commit to the base repository
+// SignMerge determines if we should sign a PR merge commit to the base repository.
-func SignMerge(ctx context.Context, pr *issues_model.PullRequest, u *user_model.User, gitRepo *git.Repository) (bool, *git.SigningKey, *git.Signature, error) {
+// baseRef and headRef must resolve in gitRepo. Callers pass the temporary merge repo's own
+// refs for an update by merge, whose fake reverse PR has no head ref in the base repository.
+func SignMerge(ctx context.Context, pr *issues_model.PullRequest, u *user_model.User, gitRepo *git.Repository, baseRef, headRef string) (bool, *git.SigningKey, *git.Signature, error) {
 	if err := pr.LoadBaseRepo(ctx); err != nil {
 		log.Error("Unable to get Base Repo for pull request")
 		return false, nil, nil, err
 	}
 	repo := pr.BaseRepo
 
-	baseCommit, err := gitRepo.GetCommit(pr.BaseBranch)
+	baseCommit, err := gitRepo.GetCommit(baseRef)
 	if err != nil {
 		return false, nil, nil, err
 	}
-	headCommit, err := gitRepo.GetCommit(pr.GetGitHeadRefName())
+	headCommit, err := gitRepo.GetCommit(headRef)
 	if err != nil {
 		return false, nil, nil, err
 	}
@@ -338,7 +340,7 @@ Loop:
 				return false, nil, nil, &ErrWontSign{headSigned}
 			}
 		case commitsSigned:
-			verified, err := AllHeadCommitsVerified(ctx, pr, gitRepo)
+			verified, err := allCommitsVerified(ctx, baseCommit, headCommit)
 			if err != nil {
 				return false, nil, nil, err
 			}
@@ -361,11 +363,13 @@ func AllHeadCommitsVerified(ctx context.Context, pr *issues_model.PullRequest, g
 	if err != nil {
 		return false, err
 	}
-	mergeBaseCommit, err := gitrepo.MergeBase(ctx, pr.BaseRepo, baseCommit.ID.String(), headCommit.ID.String())
+	return allCommitsVerified(ctx, baseCommit, headCommit)
-	if err != nil {
-		return false, err
 }
-	commitList, err := headCommit.CommitsBeforeUntil(git.RefNameFromCommit(mergeBaseCommit))
+
+// allCommitsVerified checks the commits a merge would introduce, those reachable from
+// headCommit but not from baseCommit. Both commits must come from the same repository.
+func allCommitsVerified(ctx context.Context, baseCommit, headCommit *git.Commit) (bool, error) {
+	commitList, err := headCommit.CommitsBeforeUntil(baseCommit.ID.RefName())
 	if err != nil {
 		return false, err
 	}

services/pull/check.go

@@ -264,7 +264,7 @@ func checkSigningRequirements(ctx context.Context, pr *issues_model.PullRequest,
 	}
 
 	if mergeStyle != repo_model.MergeStyleFastForwardOnly {
-		if _, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo); err != nil {
+		if _, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo, pr.BaseBranch, pr.GetGitHeadRefName()); err != nil {
 			return err
 		}
 	}

services/pull/merge_prepare.go

@@ -18,7 +18,6 @@ import (
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
 	"gitea.dev/modules/git/gitcmd"
-	"gitea.dev/modules/gitrepo"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/util"
 	asymkey_service "gitea.dev/services/asymkey"
@@ -103,15 +102,18 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
 	mergeCtx.sig = doer.NewGitSig()
 	mergeCtx.committer = mergeCtx.sig
 
-	gitRepo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo)
+	gitRepo, err := git.OpenRepository(ctx, mergeCtx.tmpBasePath)
 	if err != nil {
 		defer cancel()
 		return nil, nil, fmt.Errorf("failed to open temp git repo for pr[%d]: %w", mergeCtx.pr.ID, err)
 	}
 	defer gitRepo.Close()
 
-	// Determine if we should sign
+	// Determine if we should sign, using the temp repo's own refs (see SignMerge for why)
-	sign, key, signer, _ := asymkey_service.SignMerge(ctx, pr, doer, gitRepo)
+	sign, key, signer, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo, git.BranchPrefix+tmpRepoBaseBranch, git.BranchPrefix+tmpRepoTrackingBranch)
+	if err != nil && !asymkey_service.IsErrWontSign(err) {
+		log.Error("%-v SignMerge: %v", mergeCtx.pr, err) // the merge proceeds unsigned regardless, so log it here
+	}
 	if sign {
 		mergeCtx.signKey = key
 		if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {

tests/integration/gpg_ssh_git_test.go

@@ -303,6 +303,84 @@ func testGitSigning(t *testing.T) {
 				assert.True(t, branch.Commit.Verification.Verified)
 			}))
 		})
+
+		t.Run("UpdateMergeSigned", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			testCtx := NewAPITestContext(t, username, "update-merge-signed", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+			t.Run("CreateRepository", doAPICreateRepository(testCtx, false))
+
+			t.Run("CreateFeatureCommit", crudActionCreateFile(
+				t, testCtx, user, "master", "feature", "signed-feature.txt"))
+			pr, err := doAPICreatePullRequest(testCtx, testCtx.Username, testCtx.Reponame, "master", "feature")(t)
+			require.NoError(t, err)
+
+			content := base64.StdEncoding.EncodeToString([]byte("update base"))
+			t.Run("UpdateBase", doAPICreateFile(testCtx, "signed-base.txt", &api.CreateFileOptions{
+				FileOptions: api.FileOptions{
+					BranchName: "master",
+					Message:    "update base",
+					Author: api.Identity{
+						Name:  user.FullName,
+						Email: user.Email,
+					},
+					Committer: api.Identity{
+						Name:  user.FullName,
+						Email: user.Email,
+					},
+				},
+				ContentBase64: content,
+			}))
+
+			req := NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update?style=merge", testCtx.Username, testCtx.Reponame, pr.Index).
+				AddTokenAuth(testCtx.Token)
+			testCtx.Session.MakeRequest(t, req, http.StatusOK)
+
+			t.Run("CheckFeatureBranchSigned", doAPIGetBranch(testCtx, "feature", func(t *testing.T, branch api.Branch) {
+				require.NotNil(t, branch.Commit)
+				require.NotNil(t, branch.Commit.Verification)
+				assert.True(t, branch.Commit.Verification.Verified)
+			}))
+		})
+
+		setting.Repository.Signing.CRUDActions = []string{"never"}
+		t.Run("UpdateMergeUnsigned", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			testCtx := NewAPITestContext(t, username, "update-merge-unsigned", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+			t.Run("CreateRepository", doAPICreateRepository(testCtx, false))
+
+			t.Run("CreateFeatureCommit", crudActionCreateFile(
+				t, testCtx, user, "master", "feature", "unsigned-feature.txt"))
+			pr, err := doAPICreatePullRequest(testCtx, testCtx.Username, testCtx.Reponame, "master", "feature")(t)
+			require.NoError(t, err)
+
+			// the base commit the update merges in is unsigned, so the commitssigned rule must refuse
+			content := base64.StdEncoding.EncodeToString([]byte("update base"))
+			t.Run("UpdateBase", doAPICreateFile(testCtx, "unsigned-base.txt", &api.CreateFileOptions{
+				FileOptions: api.FileOptions{
+					BranchName: "master",
+					Message:    "update base",
+					Author: api.Identity{
+						Name:  user.FullName,
+						Email: user.Email,
+					},
+					Committer: api.Identity{
+						Name:  user.FullName,
+						Email: user.Email,
+					},
+				},
+				ContentBase64: content,
+			}))
+
+			req := NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update?style=merge", testCtx.Username, testCtx.Reponame, pr.Index).
+				AddTokenAuth(testCtx.Token)
+			testCtx.Session.MakeRequest(t, req, http.StatusOK)
+
+			t.Run("CheckFeatureBranchUnsigned", doAPIGetBranch(testCtx, "feature", func(t *testing.T, branch api.Branch) {
+				require.NotNil(t, branch.Commit)
+				require.NotNil(t, branch.Commit.Verification)
+				assert.False(t, branch.Commit.Verification.Verified)
+			}))
+		})
 	})
 }