peezy-tech/jojo
2
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2026-05-13 06:20:24 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

fix: apply signed-merge checks by merge style (#11403)

Browse source 
Fixes #6438

When a protected branch requires signed commits and no signing key is available, fast-forward-only merges should still be allowed because they do not create a new commit.

This patch applies signing checks by merge behaviour/style instead of one global gate:

- pass `mergeStyle` through `CheckPullMergeable(...)` in web/API/automerge paths
- require signing for commit-creating styles (`merge`, `rebase`, `rebase-merge`, `squash`)
- bypass signing precheck only for `fast-forward-only`
- align merge UI options with backend behaviour so signing-dependent styles are unavailable when signing cannot happen
- add Go unit tests for merge-style signing requirements
- add frontend unit coverage for the no-allowed-merge-styles guard

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11403
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: abdo <dev@abdo.wtf>
Co-committed-by: abdo <dev@abdo.wtf>
This commit is contained in:
abdo 2026-04-09 20:26:27 +02:00 • committed by Gusted
parent 4e6a782a89
commit e16dc2ebfd
8 changed files with 129 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

5
routers/api/v1/repo/pull.go
View file

@ -922,7 +922,8 @@ func MergePullRequest(ctx *context.APIContext) {
}
}
manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
mergeStyle := repo_model.MergeStyle(form.Do)
manuallyMerged := mergeStyle == repo_model.MergeStyleManuallyMerged
mergeCheckType := pull_service.MergeCheckTypeGeneral
if form.MergeWhenChecksSucceed {
@ -933,7 +934,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, form.ForceMerge, mergeStyle); err != nil {
if errors.Is(err, pull_service.ErrIsClosed) {
ctx.NotFound()
} else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) {

5
routers/web/repo/pull.go
View file

@ -1317,7 +1317,8 @@ func MergePullRequest(ctx *context.Context) {
pr.Issue = issue
pr.Issue.Repo = ctx.Repo.Repository
manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
mergeStyle := repo_model.MergeStyle(form.Do)
manuallyMerged := mergeStyle == repo_model.MergeStyleManuallyMerged
mergeCheckType := pull_service.MergeCheckTypeGeneral
if form.MergeWhenChecksSucceed {
@ -1328,7 +1329,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, form.ForceMerge, mergeStyle); err != nil {
switch {
case errors.Is(err, pull_service.ErrIsClosed):
if issue.IsPull {

2
services/automerge/automerge.go
View file

@ -215,7 +215,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, false, scheduledPRM.MergeStyle); err != nil {
if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) {
log.Info("%-v was scheduled to automerge by an unauthorized user", pr)
return

17
services/pull/check.go
View file

@ -65,7 +65,7 @@ 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, adminSkipProtectionCheck bool) error {
func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, adminSkipProtectionCheck bool, mergeStyle repo_model.MergeStyle) error {
return db.WithTx(stdCtx, func(ctx context.Context) error {
if pr.HasMerged {
return ErrHasMerged
@ -136,7 +136,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc
}
}
if _, err := isSignedIfRequired(ctx, pr, doer); err != nil {
if _, err := isSignedIfRequired(ctx, pr, doer, mergeStyle); err != nil {
return err
}
@ -151,7 +151,7 @@ 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) {
func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle) (bool, error) {
pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
if err != nil {
return false, err
@ -161,11 +161,22 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer
return true, nil
}
if !isMergeSigningRequired(mergeStyle) {
return true, nil
}
sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitRefName())
return sign, err
}
func isMergeSigningRequired(mergeStyle repo_model.MergeStyle) bool {
// Only fast-forward-only is guaranteed not to create a new commit. Rebase
// rewrites commits when the pull request is behind, and it can also amend
// the tip commit when a REBASE_TEMPLATE is configured.
return mergeStyle != repo_model.MergeStyleFastForwardOnly
}
// checkAndUpdateStatus checks if pull request is possible to leaving checking status,
// and set to be either conflict or mergeable.
func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) bool {

51
services/pull/check_test.go
View file

@ -11,6 +11,7 @@ import (
"forgejo.org/models/db"
issues_model "forgejo.org/models/issues"
repo_model "forgejo.org/models/repo"
"forgejo.org/models/unittest"
"forgejo.org/modules/queue"
"forgejo.org/modules/setting"
@ -67,3 +68,53 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) {
prPatchCheckerQueue.ShutdownWait(5 * time.Second)
prPatchCheckerQueue = nil
}
func TestIsMergeSigningRequired(t *testing.T) {
testCases := []struct {
name string
mergeStyle repo_model.MergeStyle
expected bool
}{
{
name: "fast-forward never requires signing",
mergeStyle: repo_model.MergeStyleFastForwardOnly,
expected: false,
},
{
name: "rebase requires signing even when up to date",
mergeStyle: repo_model.MergeStyleRebase,
expected: true,
},
{
name: "rebase-merge requires signing",
mergeStyle: repo_model.MergeStyleRebaseMerge,
expected: true,
},
{
name: "squash commits require signing",
mergeStyle: repo_model.MergeStyleSquash,
expected: true,
},
{
name: "merge commits require signing",
mergeStyle: repo_model.MergeStyleMerge,
expected: true,
},
{
name: "rebase-update style still requires signing",
mergeStyle: repo_model.MergeStyleRebaseUpdate,
expected: true,
},
{
name: "empty merge style requires signing",
mergeStyle: "",
expected: true,
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
assert.Equal(t, testCase.expected, isMergeSigningRequired(testCase.mergeStyle))
})
}
}

33
templates/repo/issue/view_content/pull.tmpl
View file

@ -1,3 +1,9 @@
{{$prUnit := .Repository.MustGetUnit $.Context $.UnitTypePullRequests}}
{{$prConfig := $prUnit.PullRequestsConfig}}
{{$fastForwardStyleAllowed := and $prConfig.AllowFastForwardOnly (eq .Issue.PullRequest.CommitsBehind 0)}}
{{$manualMergeStyleAllowed := $prConfig.AllowManualMerge}}
{{$hasUnsignedMergeStyle := or $fastForwardStyleAllowed $manualMergeStyleAllowed}}
{{$signingBlocksAllMergeStyles := and .AllowMerge .RequireSigned (not .WillSign) (not $hasUnsignedMergeStyle)}}
{{if and .Issue.PullRequest.HasMerged (not .IsPullBranchDeletable)}}
{{/* Then the merge box will not be displayed because this page already contains enough information */}}
{{else}}
@ -14,7 +20,7 @@
{{- else if .IsBlockedByChangedProtectedFiles}}red
{{- else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsFailure .RequiredStatusCheckState.IsError)}}red
{{- else if and .EnableStatusCheck (or (not $.LatestCommitStatus) .RequiredStatusCheckState.IsPending .RequiredStatusCheckState.IsWarning)}}yellow
{{- else if and .AllowMerge .RequireSigned (not .WillSign)}}red
{{- else if $signingBlocksAllMergeStyles}}red
{{- else if .Issue.PullRequest.IsChecking}}yellow
{{- else if .Issue.PullRequest.IsEmpty}}grey
{{- else if .Issue.PullRequest.CanAutoMerge}}green
@ -148,7 +154,7 @@
{{svg "octicon-x"}}
{{ctx.Locale.Tr "repo.pulls.required_status_check_missing"}}
</div>
{{else if and .AllowMerge .RequireSigned (not .WillSign)}}
{{else if $signingBlocksAllMergeStyles}}
<div class="item">
{{svg "octicon-x"}}
{{ctx.Locale.Tr "repo.pulls.require_signed_wont_sign"}}
@ -162,7 +168,7 @@
{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}}
{{/* admin can merge without checks, writer can merge when checks succeed */}}
{{$canMergeNow := and (or (and $.IsRepoAdmin (not .ProtectedBranch.ApplyToAdmins)) (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not .RequireSigned) .WillSign)}}
{{$canMergeNow := and (or (and $.IsRepoAdmin (not .ProtectedBranch.ApplyToAdmins)) (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not $signingBlocksAllMergeStyles))}}
{{/* admin and writer both can make an auto merge schedule */}}
{{if $canMergeNow}}
@ -201,8 +207,7 @@
{{end}}
{{if .AllowMerge}} {{/* user is allowed to merge */}}
{{$prUnit := .Repository.MustGetUnit $.Context $.UnitTypePullRequests}}
{{if or $prUnit.PullRequestsConfig.AllowMerge $prUnit.PullRequestsConfig.AllowRebase $prUnit.PullRequestsConfig.AllowRebaseMerge $prUnit.PullRequestsConfig.AllowSquash $prUnit.PullRequestsConfig.AllowFastForwardOnly $prUnit.PullRequestsConfig.AllowManualMerge}}
{{if or $prConfig.AllowMerge $prConfig.AllowRebase $prConfig.AllowRebaseMerge $prConfig.AllowSquash $prConfig.AllowFastForwardOnly $prConfig.AllowManualMerge}}
{{$hasPendingPullRequestMergeTip := ""}}
{{if .HasPendingPullRequestMerge}}
{{$createdPRMergeStr := DateUtils.TimeSince .PendingPullRequestMerge.CreatedUnix}}
@ -231,7 +236,7 @@
'pullHeadCommitID': {{.PullHeadCommitID}},
'isPullBranchDeletable': {{.IsPullBranchDeletable}},
'defaultMergeStyle': {{.MergeStyle}},
'defaultDeleteBranchAfterMerge': {{$prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}},
'defaultDeleteBranchAfterMerge': {{$prConfig.DefaultDeleteBranchAfterMerge}},
'mergeMessageFieldPlaceHolder': {{ctx.Locale.Tr "repo.editor.commit_message_desc"}},
'defaultMergeMessage': defaultMergeMessage,
@ -243,7 +248,7 @@
mergeForm['mergeStyles'] = [
{
'name': 'merge',
'allowed': {{$prUnit.PullRequestsConfig.AllowMerge}},
'allowed': {{and $prConfig.AllowMerge (or (not .RequireSigned) .WillSign)}},
'textDoMerge': {{ctx.Locale.Tr "repo.pulls.merge_pull_request"}},
'mergeTitleFieldText': defaultMergeTitle,
'mergeMessageFieldText': defaultMergeMessage,
@ -251,14 +256,14 @@
},
{
'name': 'rebase',
'allowed': {{$prUnit.PullRequestsConfig.AllowRebase}},
'allowed': {{and $prConfig.AllowRebase (or (not .RequireSigned) .WillSign)}},
'textDoMerge': {{ctx.Locale.Tr "repo.pulls.rebase_merge_pull_request"}},
'hideMergeMessageTexts': true,
'hideAutoMerge': generalHideAutoMerge,
},
{
'name': 'rebase-merge',
'allowed': {{$prUnit.PullRequestsConfig.AllowRebaseMerge}},
'allowed': {{and $prConfig.AllowRebaseMerge (or (not .RequireSigned) .WillSign)}},
'textDoMerge': {{ctx.Locale.Tr "repo.pulls.rebase_merge_commit_pull_request"}},
'mergeTitleFieldText': defaultMergeTitle,
'mergeMessageFieldText': defaultMergeMessage,
@ -266,7 +271,7 @@
},
{
'name': 'squash',
'allowed': {{$prUnit.PullRequestsConfig.AllowSquash}},
'allowed': {{and $prConfig.AllowSquash (or (not .RequireSigned) .WillSign)}},
'textDoMerge': {{ctx.Locale.Tr "repo.pulls.squash_merge_pull_request"}},
'mergeTitleFieldText': defaultSquashMergeTitle,
'mergeMessageFieldText': {{.GetCommitMessages}} + defaultSquashMergeMessage,
@ -274,14 +279,14 @@
},
{
'name': 'fast-forward-only',
'allowed': {{and $prUnit.PullRequestsConfig.AllowFastForwardOnly (eq .Issue.PullRequest.CommitsBehind 0)}},
'allowed': {{$fastForwardStyleAllowed}},
'textDoMerge': {{ctx.Locale.Tr "repo.pulls.fast_forward_only_merge_pull_request"}},
'hideMergeMessageTexts': true,
'hideAutoMerge': generalHideAutoMerge,
},
{
'name': 'manually-merged',
'allowed': {{$prUnit.PullRequestsConfig.AllowManualMerge}},
'allowed': {{$manualMergeStyleAllowed}},
'textDoMerge': {{ctx.Locale.Tr "repo.pulls.merge_manually"}},
'hideMergeMessageTexts': true,
'hideAutoMerge': true,
@ -293,7 +298,7 @@
{{$showGeneralMergeForm = true}}
<div id="pull-request-merge-form"></div>
{{else}}
{{/* no merge style was set in repo setting: not or ($prUnit.PullRequestsConfig.AllowMerge ...) */}}
{{/* no merge style was set in repo setting: not or ($prConfig.AllowMerge ...) */}}
<div class="divider"></div>
<div class="item text red">
{{svg "octicon-x"}}
@ -349,7 +354,7 @@
{{svg "octicon-x"}}
{{ctx.Locale.Tr "repo.pulls.required_status_check_failed"}}
</div>
{{else if and .RequireSigned (not .WillSign)}}
{{else if $signingBlocksAllMergeStyles}}
<div class="item text red">
{{svg "octicon-x"}}
{{ctx.Locale.Tr "repo.pulls.require_signed_wont_sign"}}

32
web_src/js/components/PullRequestMergeForm.test.js
View file

@ -32,3 +32,35 @@ test('renders escaped branch name', async () => {
mergeform = await renderMergeForm('<script class="evil">alert("evil message");</script>');
expect(mergeform.get('label[for="delete-branch-after-merge"]').text()).toBe('Delete branch "<script class="evil">alert("evil message");</script>"');
});
test('hides merge controls when no merge style is allowed', () => {
window.config.pageData.pullRequestMergeForm = {
textDeleteBranch: 'Delete branch',
textAutoMergeButtonWhenSucceed: 'when checks succeed',
textAutoMergeWhenSucceed: 'Auto merge when checks succeed',
textAutoMergeCancelSchedule: 'Cancel schedule',
textCancel: 'Cancel',
defaultDeleteBranchAfterMerge: false,
defaultMergeMessage: '',
defaultMergeStyle: 'merge',
emptyCommit: false,
hasPendingPullRequestMerge: false,
hasPendingPullRequestMergeTip: '',
isPullBranchDeletable: false,
canMergeNow: true,
allOverridableChecksOk: true,
pullHeadCommitID: 'abc123',
mergeStyles: [{
name: 'merge',
allowed: false,
textDoMerge: 'Merge',
mergeTitleFieldText: '',
mergeMessageFieldText: '',
hideAutoMerge: false,
}],
};
const mergeform = mount(PullRequestMergeForm);
expect(mergeform.find('.merge-button').exists()).toBe(false);
expect(mergeform.find('form.form-fetch-action').exists()).toBe(false);
});

9
web_src/js/components/PullRequestMergeForm.vue
View file

@ -38,7 +38,8 @@ export default {
},
watch: {
mergeStyle(val) {
this.mergeStyleDetail = this.mergeForm.mergeStyles.find((e) => e.name === val);
const mergeStyleDetail = this.mergeForm.mergeStyles.find((e) => e.name === val);
if (mergeStyleDetail) this.mergeStyleDetail = mergeStyleDetail;
for (const elem of document.querySelectorAll('[data-pull-merge-style]')) {
toggleElem(elem, elem.getAttribute('data-pull-merge-style') === val);
}
@ -49,6 +50,7 @@ export default {
let mergeStyle = this.mergeForm.mergeStyles.find((e) => e.allowed && e.name === this.mergeForm.defaultMergeStyle)?.name;
if (!mergeStyle) mergeStyle = this.mergeForm.mergeStyles.find((e) => e.allowed)?.name;
if (!mergeStyle) return;
this.switchMergeStyle(mergeStyle, !this.mergeForm.canMergeNow);
},
mounted() {
@ -62,6 +64,7 @@ export default {
this.showMergeStyleMenu = false;
},
toggleActionForm(show) {
if (show && this.mergeStyleAllowedCount === 0) return;
this.showActionForm = show;
if (!show) return;
this.deleteBranchAfterMerge = this.mergeForm.defaultDeleteBranchAfterMerge;
@ -134,7 +137,7 @@ export default {
</div>
</form>
<div v-if="!showActionForm" class="tw-flex">
<div v-if="!showActionForm && mergeStyleAllowedCount > 0" class="tw-flex">
<!-- the merge button -->
<div class="ui buttons merge-button" :class="[mergeForm.emptyCommit ? 'grey' : mergeForm.allOverridableChecksOk ? 'primary' : 'red']" @click="toggleActionForm(true)">
<button class="ui button">
@ -146,7 +149,7 @@ export default {
</template>
</span>
</button>
<div class="ui dropdown icon button" @click.stop="showMergeStyleMenu = !showMergeStyleMenu" v-if="mergeStyleAllowedCount>1">
<div class="ui dropdown icon button" @click.stop="showMergeStyleMenu = !showMergeStyleMenu" v-if="mergeStyleAllowedCount > 1">
<svg-icon name="octicon-triangle-down" :size="14"/>
<div class="menu" :class="{'show':showMergeStyleMenu}">
<template v-for="msd in mergeForm.mergeStyles">