diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index 18ce2c970a2c8..6281ff8f547cb 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -1850,6 +1850,7 @@ "repo.pulls.merge_manually": "Manually merged", "repo.pulls.merge_commit_id": "The merge commit ID", "repo.pulls.require_signed_wont_sign": "The branch requires signed commits but this merge will not be signed", + "repo.pulls.require_signed_head_commits_unverified": "The branch requires signed commits but one or more commits on this pull request are not verified", "repo.pulls.invalid_merge_option": "You cannot use this merge option for this pull request.", "repo.pulls.merge_conflict": "Merge Failed: There was a conflict while merging. Hint: Try a different strategy.", "repo.pulls.merge_conflict_summary": "Error Message", diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index ef8cc6cd9325d..4cde030c4074c 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -965,7 +965,7 @@ func MergePullRequest(ctx *context.APIContext) { } // start with merging by checking - if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil { + if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, repo_model.MergeStyle(form.Do), form.ForceMerge); err != nil { if errors.Is(err, pull_service.ErrIsClosed) { ctx.APIErrorNotFound() } else if errors.Is(err, pull_service.ErrNoPermissionToMerge) { @@ -980,6 +980,8 @@ func MergePullRequest(ctx *context.APIContext) { ctx.APIError(http.StatusMethodNotAllowed, err) } else if asymkey_service.IsErrWontSign(err) { ctx.APIError(http.StatusMethodNotAllowed, err) + } else if errors.Is(err, pull_service.ErrHeadCommitsNotAllVerified) { + ctx.APIError(http.StatusMethodNotAllowed, err) } else { ctx.APIErrorInternal(err) } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index efcdaac6740a5..639ebbf270341 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1100,7 +1100,7 @@ func MergePullRequest(ctx *context.Context) { } // start with merging by checking - if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil { + if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, repo_model.MergeStyle(form.Do), form.ForceMerge); err != nil { switch { case errors.Is(err, pull_service.ErrIsClosed): if issue.IsPull { @@ -1120,6 +1120,8 @@ func MergePullRequest(ctx *context.Context) { ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready")) case asymkey_service.IsErrWontSign(err): ctx.JSONError(err.Error()) // has no translation ... + case errors.Is(err, pull_service.ErrHeadCommitsNotAllVerified): + ctx.JSONError(ctx.Tr("repo.pulls.require_signed_head_commits_unverified")) case errors.Is(err, pull_service.ErrDependenciesLeft): ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked")) default: diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go index cffefe08ae6f6..8c28717e5d0af 100644 --- a/services/asymkey/sign.go +++ b/services/asymkey/sign.go @@ -338,26 +338,41 @@ Loop: return false, nil, nil, &ErrWontSign{headSigned} } case commitsSigned: - verification := ParseCommitWithSignature(ctx, headCommit) - if !verification.Verified { - return false, nil, nil, &ErrWontSign{commitsSigned} - } - // need to work out merge-base - mergeBaseCommit, err := gitrepo.MergeBase(ctx, pr.BaseRepo, baseCommit.ID.String(), headCommit.ID.String()) - if err != nil { - return false, nil, nil, err - } - commitList, err := headCommit.CommitsBeforeUntil(mergeBaseCommit) + verified, err := AllHeadCommitsVerified(ctx, pr, gitRepo) if err != nil { return false, nil, nil, err } - for _, commit := range commitList { - verification := ParseCommitWithSignature(ctx, commit) - if !verification.Verified { - return false, nil, nil, &ErrWontSign{commitsSigned} - } + if !verified { + return false, nil, nil, &ErrWontSign{commitsSigned} } } } return true, signingKey, signer, nil } + +// AllHeadCommitsVerified checks that every new commit in the PR head has a +// verified signature. +func AllHeadCommitsVerified(ctx context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) (bool, error) { + baseCommit, err := gitRepo.GetCommit(pr.BaseBranch) + if err != nil { + return false, err + } + headCommit, err := gitRepo.GetCommit(pr.GetGitHeadRefName()) + 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(mergeBaseCommit) + if err != nil { + return false, err + } + for _, commit := range commitList { + if !ParseCommitWithSignature(ctx, commit).Verified { + return false, nil + } + } + return true, nil +} diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index b3a988320bcf3..b06fc723be150 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -251,7 +251,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - if err := pull_service.CheckPullMergeable(ctx, doer, &perm, pr, pull_service.MergeCheckTypeGeneral, false); err != nil { + if err := pull_service.CheckPullMergeable(ctx, doer, &perm, pr, pull_service.MergeCheckTypeGeneral, scheduledPRM.MergeStyle, false); err != nil { if errors.Is(err, pull_service.ErrNotReadyToMerge) { log.Info("%-v was scheduled to automerge by an unauthorized user", pr) return diff --git a/services/pull/check.go b/services/pull/check.go index 6486ca79df320..996994cea951f 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -38,14 +38,15 @@ import ( var prPatchCheckerQueue *queue.WorkerPoolQueue[string] var ( - ErrIsClosed = errors.New("pull is closed") - ErrNoPermissionToMerge = errors.New("no permission to merge") - ErrNotReadyToMerge = errors.New("not ready to merge") - ErrHasMerged = errors.New("has already been merged") - ErrIsWorkInProgress = errors.New("work in progress PRs cannot be merged") - ErrIsChecking = errors.New("cannot merge while conflict checking is in progress") - ErrNotMergeableState = errors.New("not in mergeable state") - ErrDependenciesLeft = errors.New("is blocked by an open dependency") + ErrIsClosed = errors.New("pull is closed") + ErrNoPermissionToMerge = errors.New("no permission to merge") + ErrNotReadyToMerge = errors.New("not ready to merge") + ErrHasMerged = errors.New("has already been merged") + ErrIsWorkInProgress = errors.New("work in progress PRs cannot be merged") + ErrIsChecking = errors.New("cannot merge while conflict checking is in progress") + ErrNotMergeableState = errors.New("not in mergeable state") + ErrDependenciesLeft = errors.New("is blocked by an open dependency") + ErrHeadCommitsNotAllVerified = errors.New("the branch requires signed commits but not all head commits are verified") ) func markPullRequestStatusAsChecking(ctx context.Context, pr *issues_model.PullRequest) bool { @@ -132,7 +133,13 @@ const ( ) // CheckPullMergeable check if the pull mergeable based on all conditions (branch protection, merge options, ...) -func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, adminForceMerge bool) error { +// mergeStyle tailors the "require signed commits" prechecks: +// - fast-forward-only: no Gitea commit is produced, so Gitea's merge-signing check is skipped; +// only the user's head commits are verified. +// - merge: both the head commits must be verified and Gitea must sign the merge commit. +// - rebase, rebase-merge, squash: Gitea rewrites the commits and signs each, so only Gitea's +// signing ability is checked. +func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, mergeStyle repo_model.MergeStyle, adminForceMerge bool) error { return db.WithTx(stdCtx, func(ctx context.Context) error { if pr.HasMerged { return ErrHasMerged @@ -207,7 +214,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc } } - if _, err := isSignedIfRequired(ctx, pr, doer); err != nil { + if err := checkSigningRequirements(ctx, pr, doer, mergeStyle); err != nil { return err } @@ -221,26 +228,45 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc }) } -// isSignedIfRequired check if merge will be signed if required -func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User) (bool, error) { +// checkSigningRequirements enforces the target branch's RequireSignedCommits rule +// against the selected merge style: +// - fast-forward-only and merge keep the user's commits on the base branch, so +// those commits must all be verified, or the pre-receive hook will reject the +// push with a generic error. +// - fast-forward-only creates no Gitea commit, so Gitea's signing key is not used. +// - merge, rebase, rebase-merge and squash produce a Gitea-signed commit, so +// Gitea must be configured to sign it. +func checkSigningRequirements(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle) error { pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) if err != nil { - return false, err + return err } - if pb == nil || !pb.RequireSignedCommits { - return true, nil + return nil } gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) if err != nil { - return false, err + return err } defer closer.Close() - sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo) + if mergeStyle == repo_model.MergeStyleFastForwardOnly || mergeStyle == repo_model.MergeStyleMerge { + verified, err := asymkey_service.AllHeadCommitsVerified(ctx, pr, gitRepo) + if err != nil { + return err + } + if !verified { + return ErrHeadCommitsNotAllVerified + } + } - return sign, err + if mergeStyle != repo_model.MergeStyleFastForwardOnly { + if _, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo); err != nil { + return err + } + } + return nil } // markPullRequestAsMergeable checks if pull request is possible to leaving checking status, diff --git a/services/pull/check_test.go b/services/pull/check_test.go index 0f39237932436..506cd42301519 100644 --- a/services/pull/check_test.go +++ b/services/pull/check_test.go @@ -9,6 +9,7 @@ import ( "time" "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" @@ -73,6 +74,39 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { prPatchCheckerQueue = nil } +func TestCheckSigningRequirementsHeadCommits(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + ctx := t.Context() + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + require.NoError(t, pr.LoadBaseRepo(ctx)) + require.NoError(t, pr.LoadHeadRepo(ctx)) + + check := func() error { + return checkSigningRequirements(ctx, pr, nil, repo_model.MergeStyleFastForwardOnly) + } + + // No protected branch rule on the base branch: the check must pass. + require.NoError(t, check()) + + // Protected branch without RequireSignedCommits: the check must still pass. + require.NoError(t, git_model.UpdateProtectBranch(ctx, pr.BaseRepo, &git_model.ProtectedBranch{ + RepoID: pr.BaseRepoID, + RuleName: pr.BaseBranch, + RequireSignedCommits: false, + }, git_model.WhitelistOptions{})) + require.NoError(t, check()) + + // With RequireSignedCommits enabled: the test fixture commits have no signatures, + // so the check must report ErrHeadCommitsNotAllVerified. + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + require.NoError(t, err) + require.NotNil(t, pb) + pb.RequireSignedCommits = true + require.NoError(t, git_model.UpdateProtectBranch(ctx, pr.BaseRepo, pb, git_model.WhitelistOptions{})) + require.ErrorIs(t, check(), ErrHeadCommitsNotAllVerified) +} + func TestMarkPullRequestAsMergeable(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index d0213962ee7e2..bf237a4fc6fc9 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -36,6 +36,7 @@ import ( "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/automergequeue" + "code.gitea.io/gitea/services/forms" pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" @@ -510,6 +511,92 @@ func TestFastForwardOnlyMerge(t *testing.T) { }) } +func TestFastForwardOnlyMergeWithRequiredSignedCommits(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + session := loginUser(t, "user1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") + testEditFileToNewBranch(t, session, "user1", "repo1", "master", "update", "README.md", "Hello, signed\n") + + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + createPRReq := NewRequestWithJSON(t, http.MethodPost, "/api/v1/repos/user1/repo1/pulls", &api.CreatePullRequestOption{ + Head: "update", + Base: "master", + Title: "ff-only merge under require-signed-commits", + }).AddTokenAuth(token) + session.MakeRequest(t, createPRReq, http.StatusCreated) + + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user1"}) + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user1.ID, Name: "repo1"}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ + HeadRepoID: repo1.ID, + BaseRepoID: repo1.ID, + HeadBranch: "update", + BaseBranch: "master", + }) + + // Enable require-signed-commits on master. + require.NoError(t, git_model.UpdateProtectBranch(t.Context(), repo1, &git_model.ProtectedBranch{ + RepoID: repo1.ID, + RuleName: "master", + RequireSignedCommits: true, + }, git_model.WhitelistOptions{})) + + prIndex := strconv.FormatInt(pr.Index, 10) + mergeURL := "/user1/repo1/pulls/" + prIndex + "/merge" + + notVerifiedMsg := translation.NewLocale("en-US").TrString("repo.pulls.require_signed_head_commits_unverified") + apiNotVerifiedMsg := pull_service.ErrHeadCommitsNotAllVerified.Error() + // Matches the unexported "wont sign: %s" format and nokey signingMode in + // services/asymkey/sign.go; the test config uses SIGNING_KEY = none. + const wontSignMsg = "wont sign: nokey" + + for _, style := range []repo_model.MergeStyle{repo_model.MergeStyleFastForwardOnly, repo_model.MergeStyleMerge} { + t.Run(string(style)+"/head-commits-unverified", func(t *testing.T) { + mergeReq := NewRequestWithValues(t, http.MethodPost, mergeURL, map[string]string{"do": string(style)}) + resp := session.MakeRequest(t, mergeReq, http.StatusBadRequest) + assert.Equal(t, notVerifiedMsg, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) + }) + } + + for _, style := range []repo_model.MergeStyle{repo_model.MergeStyleRebase, repo_model.MergeStyleRebaseMerge, repo_model.MergeStyleSquash} { + t.Run(string(style)+"/wont-sign", func(t *testing.T) { + mergeReq := NewRequestWithValues(t, http.MethodPost, mergeURL, map[string]string{"do": string(style)}) + resp := session.MakeRequest(t, mergeReq, http.StatusBadRequest) + assert.Equal(t, wontSignMsg, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) + }) + } + + // Admin force-merge must not bypass the unverified-head-commits check, since + // the pre-receive hook would reject the push regardless. + t.Run("fast-forward-only/admin-force-merge-does-not-bypass", func(t *testing.T) { + mergeReq := NewRequestWithValues(t, http.MethodPost, mergeURL, map[string]string{ + "do": string(repo_model.MergeStyleFastForwardOnly), + "force_merge": "true", + }) + resp := session.MakeRequest(t, mergeReq, http.StatusBadRequest) + assert.Equal(t, notVerifiedMsg, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) + }) + + t.Run("api/fast-forward-only/head-commits-unverified", func(t *testing.T) { + apiReq := NewRequestWithJSON(t, http.MethodPost, + fmt.Sprintf("/api/v1/repos/user1/repo1/pulls/%s/merge", prIndex), + &forms.MergePullRequestForm{Do: string(repo_model.MergeStyleFastForwardOnly)}, + ).AddTokenAuth(token) + resp := session.MakeRequest(t, apiReq, http.StatusMethodNotAllowed) + apiBody := DecodeJSON(t, resp, &api.APIError{}) + assert.Equal(t, apiNotVerifiedMsg, apiBody.Message) + }) + + pb, err := git_model.GetFirstMatchProtectedBranchRule(t.Context(), repo1.ID, "master") + require.NoError(t, err) + require.NotNil(t, pb) + pb.RequireSignedCommits = false + require.NoError(t, git_model.UpdateProtectBranch(t.Context(), repo1, pb, git_model.WhitelistOptions{})) + + require.NoError(t, pull_service.Merge(t.Context(), pr, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false)) + }) +} + func TestCantFastForwardOnlyMergeDiverging(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { session := loginUser(t, "user1") // FIXME: don't use admin user for testing