From 7641fc3a8c908e9399b40c2927c58b4b5d95707e Mon Sep 17 00:00:00 2001 From: bircni Date: Sat, 3 Oct 2026 19:54:55 +0200 Subject: [PATCH] fix(actions): restore pushes to protected branches (#39564) Use the Actions token's loaded write permission when checking protected-branch pushes. Preserve push and force-push allowlists and add regression coverage. Fixes https://github.com/go-gitea/gitea/issues/39563 Co-authored-by: wxiaoguang Co-authored-by: silverwind Co-authored-by: Giteabot --- models/git/protected_branch.go | 22 +++------- routers/private/hook_pre_receive.go | 4 +- routers/private/hook_pre_receive_test.go | 51 ++++++++++++++++++++++++ services/context/repo.go | 2 +- services/convert/convert.go | 2 +- services/pull/update.go | 4 +- services/repository/branch.go | 2 +- services/repository/files/patch.go | 7 +++- services/repository/files/update.go | 7 +++- 9 files changed, 76 insertions(+), 25 deletions(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 9eed8bb013a..1ad08736c1f 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -123,23 +123,13 @@ func (protectBranch *ProtectedBranch) LoadRepo(ctx context.Context) (err error) } // CanUserPush returns if some user could push to this protected branch -func (protectBranch *ProtectedBranch) CanUserPush(ctx context.Context, user *user_model.User) bool { +func (protectBranch *ProtectedBranch) CanUserPush(ctx context.Context, user *user_model.User, permissionInRepo access_model.Permission) bool { if !protectBranch.CanPush { return false } if !protectBranch.EnableWhitelist { - if err := protectBranch.LoadRepo(ctx); err != nil { - log.Error("LoadRepo: %v", err) - return false - } - - writeAccess, err := access_model.HasAccessUnit(ctx, user, protectBranch.Repo, unit.TypeCode, perm.AccessModeWrite) - if err != nil { - log.Error("HasAccessUnit: %v", err) - return false - } - return writeAccess + return permissionInRepo.CanWrite(unit.TypeCode) } if slices.Contains(protectBranch.WhitelistUserIDs, user.ID) { @@ -160,17 +150,17 @@ func (protectBranch *ProtectedBranch) CanUserPush(ctx context.Context, user *use // CanUserForcePush returns if some user could force push to this protected branch // Since force-push extends normal push, we also check if user has regular push access -func (protectBranch *ProtectedBranch) CanUserForcePush(ctx context.Context, user *user_model.User) bool { +func (protectBranch *ProtectedBranch) CanUserForcePush(ctx context.Context, user *user_model.User, permissionInRepo access_model.Permission) bool { if !protectBranch.CanForcePush { return false } if !protectBranch.EnableForcePushAllowlist { - return protectBranch.CanUserPush(ctx, user) + return protectBranch.CanUserPush(ctx, user, permissionInRepo) } if slices.Contains(protectBranch.ForcePushAllowlistUserIDs, user.ID) { - return protectBranch.CanUserPush(ctx, user) + return protectBranch.CanUserPush(ctx, user, permissionInRepo) } if len(protectBranch.ForcePushAllowlistTeamIDs) == 0 { @@ -182,7 +172,7 @@ func (protectBranch *ProtectedBranch) CanUserForcePush(ctx context.Context, user log.Error("IsUserInTeams: %v", err) return false } - return in && protectBranch.CanUserPush(ctx, user) + return in && protectBranch.CanUserPush(ctx, user, permissionInRepo) } // IsUserMergeWhitelisted checks if some user is whitelisted to merge to this branch diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 82e1cd78053..519f033bad5 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -229,9 +229,9 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r } } else { if isForcePush { - canPush = !changedProtectedfiles && protectBranch.CanUserForcePush(ctx, ctx.Doer) + canPush = !changedProtectedfiles && protectBranch.CanUserForcePush(ctx, ctx.Doer, ctx.Repo.Permission) } else { - canPush = !changedProtectedfiles && protectBranch.CanUserPush(ctx, ctx.Doer) + canPush = !changedProtectedfiles && protectBranch.CanUserPush(ctx, ctx.Doer, ctx.Repo.Permission) } } diff --git a/routers/private/hook_pre_receive_test.go b/routers/private/hook_pre_receive_test.go index 60d5513522f..4cfacadf8a0 100644 --- a/routers/private/hook_pre_receive_test.go +++ b/routers/private/hook_pre_receive_test.go @@ -6,16 +6,67 @@ package private import ( "testing" + "gitea.dev/models/db" + git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" repo_model "gitea.dev/models/repo" "gitea.dev/models/unittest" + user_model "gitea.dev/models/user" "gitea.dev/modules/git" + "gitea.dev/modules/private" "gitea.dev/services/contexttest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestPreReceiveActionsProtectedBranch(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + for _, tc := range []struct { + name string + protection git_model.ProtectedBranch + forcePush bool + allowed bool + }{ + {name: "push", protection: git_model.ProtectedBranch{CanPush: true}, allowed: true}, + {name: "push allowlist", protection: git_model.ProtectedBranch{CanPush: true, EnableWhitelist: true}}, + {name: "force push", protection: git_model.ProtectedBranch{CanPush: true, CanForcePush: true}, forcePush: true, allowed: true}, + {name: "force push allowlist", protection: git_model.ProtectedBranch{CanPush: true, CanForcePush: true, EnableForcePushAllowlist: true}, forcePush: true}, + } { + t.Run(tc.name, func(t *testing.T) { + mockCtx, resp := contexttest.MockPrivateContext(t, "/") + ctx := &preReceiveContext{PrivateContext: mockCtx, opts: &private.HookOptions{UserID: user_model.ActionsUserID}} + ctx.SetPathParam("owner", "user2") + ctx.SetPathParam("repo", "repo2") + RepoAssignment(ctx.PrivateContext) + require.False(t, ctx.Written()) + defer ctx.Repo.GitRepo.Close() + + doer := user_model.NewActionsUserWithTaskID(53) + loadContextDoerPermission(ctx.PrivateContext, doer.ID, doer.ExtDoerData.EncodeToString()) + + protection := tc.protection + protection.RepoID = ctx.Repo.Repository.ID + protection.RuleName = "probe" + require.NoError(t, db.Insert(t.Context(), &protection)) + defer func() { + require.NoError(t, git_model.DeleteProtectedBranch(t.Context(), ctx.Repo.Repository, protection.ID)) + }() + + oldCommitID, newCommitID := "205ac761f3326a7ebe416e8673760016450b5cec", "1032bbf17fbc0d9c95bb5418dabe8f8c99278700" + if tc.forcePush { + oldCommitID, newCommitID = newCommitID, oldCommitID + } + preReceiveBranch(ctx, oldCommitID, newCommitID, git.RefNameFromBranch("probe")) + if tc.allowed { + assert.False(t, ctx.Written(), resp.Body.String()) + } else { + assert.Contains(t, resp.Body.String(), "Not allowed to") + } + }) + } +} + // TestPreReceiveCanWriteCodePerBranch ensures the maintainer-edit write grant is evaluated against // the exact ref being pushed on every call, derived from that ref rather than shared mutable state. // Otherwise, a per-branch grant (an open PR with "allow edits from maintainers") could be batched diff --git a/services/context/repo.go b/services/context/repo.go index 1226459d47a..8389a499142 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -190,7 +190,7 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r protectionRequireSigned := false if protectedBranch != nil { protectedBranch.Repo = targetRepo - canPushWithProtection = protectedBranch.CanUserPush(ctx, doer) + canPushWithProtection = protectedBranch.CanUserPush(ctx, doer, doerRepoPerm) protectionRequireSigned = protectedBranch.RequireSignedCommits // If branch-wide push is restricted, allow direct commit when the // URL-derived tree path matches an unprotected file pattern. The diff --git a/services/convert/convert.go b/services/convert/convert.go index c3c710951e7..a89d6a89f3b 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -113,7 +113,7 @@ func ToBranch(ctx context.Context, repo *repo_model.Repository, branchName strin return nil, err } bp.Repo = repo - branch.UserCanPush = bp.CanUserPush(ctx, user) + branch.UserCanPush = bp.CanUserPush(ctx, user, permission) branch.UserCanMerge = git_model.IsUserMergeWhitelisted(ctx, bp, user.ID, permission) } diff --git a/services/pull/update.go b/services/pull/update.go index 2af773d3874..732a475e72b 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -123,8 +123,8 @@ func isUserAllowedToPushOrForcePushInRepoBranch(ctx context.Context, user *user_ } if pb != nil { // override previous results if there is a branch protection rule pb.Repo = repo - pushAllowed = pb.CanUserPush(ctx, user) - forcePushAllowed = pb.CanUserForcePush(ctx, user) + pushAllowed = pb.CanUserPush(ctx, user, repoPerm) + forcePushAllowed = pb.CanUserForcePush(ctx, user, repoPerm) } return pushAllowed, forcePushAllowed, nil } diff --git a/services/repository/branch.go b/services/repository/branch.go index 5f5fa885f98..0598e7c83a4 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -447,7 +447,7 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m if err != nil { return "", err } - if rule != nil && !rule.CanUserPush(ctx, doer) { + if rule != nil && !rule.CanUserPush(ctx, doer, perm) { return "", git_model.ErrBranchIsProtected } diff --git a/services/repository/files/patch.go b/services/repository/files/patch.go index f50cef0c860..f670d1f5010 100644 --- a/services/repository/files/patch.go +++ b/services/repository/files/patch.go @@ -9,6 +9,7 @@ import ( "strings" git_model "gitea.dev/models/git" + "gitea.dev/models/perm/access" repo_model "gitea.dev/models/repo" user_model "gitea.dev/models/user" "gitea.dev/modules/git" @@ -88,7 +89,11 @@ func (opts *ApplyDiffPatchOptions) Validate(ctx context.Context, repo *repo_mode } if protectedBranch != nil { protectedBranch.Repo = repo - if !protectedBranch.CanUserPush(ctx, doer) { + perm, err := access.GetDoerRepoPermission(ctx, repo, doer) + if err != nil { + return err + } + if !protectedBranch.CanUserPush(ctx, doer, perm) { return ErrUserCannotCommit{ UserName: doer.LowerName, } diff --git a/services/repository/files/update.go b/services/repository/files/update.go index 159b63f103c..d746de7262b 100644 --- a/services/repository/files/update.go +++ b/services/repository/files/update.go @@ -13,6 +13,7 @@ import ( "time" git_model "gitea.dev/models/git" + "gitea.dev/models/perm/access" repo_model "gitea.dev/models/repo" user_model "gitea.dev/models/user" "gitea.dev/modules/git" @@ -667,7 +668,11 @@ func VerifyBranchProtection(ctx context.Context, repo *repo_model.Repository, gi protectedBranch.Repo = repo globUnprotected := protectedBranch.GetUnprotectedFilePatterns() globProtected := protectedBranch.GetProtectedFilePatterns() - canUserPush := protectedBranch.CanUserPush(ctx, doer) + perm, err := access.GetDoerRepoPermission(ctx, repo, doer) + if err != nil { + return err + } + canUserPush := protectedBranch.CanUserPush(ctx, doer, perm) for _, treePath := range treePaths { isUnprotectedFile := false if len(globUnprotected) != 0 {