Allow fast-forward-only merge when signed commits are required (#37335)
Fast-forward-only creates no Gitea commit, so skip the "can Gitea sign" precheck for it. Pre-check head-commit verification for styles that preserve user commits on the target (merge, fast-forward-only) so a PR with unsigned commits surfaces a localized error instead of a 500 at the pre-receive hook. The dropdown still shows every configured style; the avatar and signing warning toggle per selection via data-pull-merge-style. Fixes #12272 **Note**: Admin force-merge does not bypass the new head-commits check. This matches the existing `isSignedIfRequired` behavior. Signed-off-by: Nikita Vakula <programmistov.programmist@gmail.com> Signed-off-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: silverwind <me@silverwind.io> Co-authored-by: Claude (Opus 4.7) <noreply@anthropic.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user