From a49c89384259ac8a1610104a465ccafd06ccb8e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Fri, 17 Jul 2026 00:38:39 +0300 Subject: [PATCH] fix(pull): sign the commit when updating a branch by merge (#38441) 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 --- routers/web/repo/issue_view.go | 2 +- services/asymkey/sign.go | 24 +++++---- services/pull/check.go | 2 +- services/pull/merge_prepare.go | 10 ++-- tests/integration/gpg_ssh_git_test.go | 78 +++++++++++++++++++++++++++ 5 files changed, 100 insertions(+), 16 deletions(-) diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index 6b97f1b0cd3..f78d04efa93 100644 --- a/routers/web/repo/issue_view.go +++ b/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 { diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go index 820e633201a..ba405d824f7 100644 --- a/services/asymkey/sign.go +++ b/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 -func SignMerge(ctx context.Context, pr *issues_model.PullRequest, u *user_model.User, gitRepo *git.Repository) (bool, *git.SigningKey, *git.Signature, error) { +// SignMerge determines if we should sign a PR merge commit to the base repository. +// 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, gitRepo, 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()) - if err != nil { - return false, err - } - commitList, err := headCommit.CommitsBeforeUntil(gitRepo, git.RefNameFromCommit(mergeBaseCommit)) + return allCommitsVerified(ctx, gitRepo, baseCommit, headCommit) +} + +// allCommitsVerified checks the commits a merge would introduce, those reachable from +// headCommit but not from baseCommit. Both commits must come from gitRepo. +func allCommitsVerified(ctx context.Context, gitRepo *git.Repository, baseCommit, headCommit *git.Commit) (bool, error) { + commitList, err := headCommit.CommitsBeforeUntil(gitRepo, baseCommit.ID.RefName()) if err != nil { return false, err } diff --git a/services/pull/check.go b/services/pull/check.go index 92021b7d1c4..145eff5d5ea 100644 --- a/services/pull/check.go +++ b/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 } } diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index 37f5593e5d7..c50bb3ccb2d 100644 --- a/services/pull/merge_prepare.go +++ b/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 - sign, key, signer, _ := asymkey_service.SignMerge(ctx, pr, doer, gitRepo) + // Determine if we should sign, using the temp repo's own refs (see SignMerge for why) + 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 { diff --git a/tests/integration/gpg_ssh_git_test.go b/tests/integration/gpg_ssh_git_test.go index 3b9bc29d1df..130cfb65341 100644 --- a/tests/integration/gpg_ssh_git_test.go +++ b/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) + })) + }) }) }