[Enhancement] Allow admin to merge pr with protected file changes (#12078)

* [Enhancement] Allow admin to merge pr with protected file changes

As tilte, show protected message in diff page and merge box.

Signed-off-by: a1012112796 <1012112796@qq.com>

* remove unused ver

* Update options/locale/locale_en-US.ini

Co-authored-by: Cirno the Strongest <1447794+CirnoT@users.noreply.github.com>

* Add TrN

* Apply suggestions from code review

* fix lint

* Update options/locale/locale_en-US.ini

Co-authored-by: zeripath <art27@cantab.net>

* Apply suggestions from code review

* move pr proteced files check to TestPatch
* Call TestPatch when protected branches settings changed

* Apply review suggestion @CirnoT

* move to service @lunny

* slightly restructure routers/private/hook.go

Adds a lot of comments and simplifies the logic

Signed-off-by: Andrew Thornton <art27@cantab.net>

* placate lint

Signed-off-by: Andrew Thornton <art27@cantab.net>

* skip duplicate protected files check

* fix check logic

* slight refactor of TestPatch

Signed-off-by: Andrew Thornton <art27@cantab.net>

* When checking for protected files changes in TestPatch use the temporary repository

Signed-off-by: Andrew Thornton <art27@cantab.net>

* fix introduced issue with hook

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Remove the check on PR index being greater than 0 as it unnecessary

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <matti@mdranta.net>
Co-authored-by: Cirno the Strongest <1447794+CirnoT@users.noreply.github.com>
Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>

models/branches.go

@@ -209,6 +209,38 @@ func (protectBranch *ProtectedBranch) GetProtectedFilePatterns() []glob.Glob {
 	return extarr
 }
 
+// MergeBlockedByProtectedFiles returns true if merge is blocked by protected files change
+func (protectBranch *ProtectedBranch) MergeBlockedByProtectedFiles(pr *PullRequest) bool {
+	glob := protectBranch.GetProtectedFilePatterns()
+	if len(glob) == 0 {
+		return false
+	}
+
+	return len(pr.ChangedProtectedFiles) > 0
+}
+
+// IsProtectedFile return if path is protected
+func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path string) bool {
+	if len(patterns) == 0 {
+		patterns = protectBranch.GetProtectedFilePatterns()
+		if len(patterns) == 0 {
+			return false
+		}
+	}
+
+	lpath := strings.ToLower(strings.TrimSpace(path))
+
+	r := false
+	for _, pat := range patterns {
+		if pat.Match(lpath) {
+			r = true
+			break
+		}
+	}
+
+	return r
+}
+
 // GetProtectedBranchByRepoID getting protected branch by repo ID
 func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) {
 	protectedBranches := make([]*ProtectedBranch, 0)

models/migrations/migrations.go

@@ -244,6 +244,8 @@ var migrations = []Migration{
 	NewMigration("add Team review request support", addTeamReviewRequestSupport),
 	// v154 > v155
 	NewMigration("add timestamps to Star, Label, Follow, Watch and Collaboration", addTimeStamps),
+	// v155 -> v156
+	NewMigration("add changed_protected_files column for pull_request table", addChangedProtectedFilesPullRequestColumn),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v155.go

@@ -0,0 +1,22 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"fmt"
+
+	"xorm.io/xorm"
+)
+
+func addChangedProtectedFilesPullRequestColumn(x *xorm.Engine) error {
+	type PullRequest struct {
+		ChangedProtectedFiles []string `xorm:"TEXT JSON"`
+	}
+
+	if err := x.Sync2(new(PullRequest)); err != nil {
+		return fmt.Errorf("Sync2: %v", err)
+	}
+	return nil
+}

models/pull.go

@@ -45,6 +45,8 @@ type PullRequest struct {
 	CommitsAhead    int
 	CommitsBehind   int
 
+	ChangedProtectedFiles []string `xorm:"TEXT JSON"`
+
 	IssueID int64  `xorm:"INDEX"`
 	Issue   *Issue `xorm:"-"`
 	Index   int64

modules/repofiles/update.go

@@ -123,7 +123,7 @@ func detectEncodingAndBOM(entry *git.TreeEntry, repo *models.Repository) (string
 
 // CreateOrUpdateRepoFile adds or updates a file in the given repository
 func CreateOrUpdateRepoFile(repo *models.Repository, doer *models.User, opts *UpdateRepoFileOptions) (*structs.FileResponse, error) {
-	// If no branch name is set, assume master
+	// If no branch name is set, assume default branch
 	if opts.OldBranch == "" {
 		opts.OldBranch = repo.DefaultBranch
 	}

options/locale/locale_en-US.ini

@@ -1232,6 +1232,8 @@ pulls.required_status_check_administrator = As an administrator, you may still m
 pulls.blocked_by_approvals = "This Pull Request doesn't have enough approvals yet. %d of %d approvals granted."
 pulls.blocked_by_rejection = "This Pull Request has changes requested by an official reviewer."
 pulls.blocked_by_outdated_branch = "This Pull Request is blocked because it's outdated."
+pulls.blocked_by_changed_protected_files_1= "This Pull Request is blocked because it changes a protected file:"
+pulls.blocked_by_changed_protected_files_n= "This Pull Request is blocked because it changes protected files:"
 pulls.can_auto_merge_desc = This pull request can be merged automatically.
 pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically due to conflicts.
 pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts.
@@ -1779,6 +1781,7 @@ diff.review.comment = Comment
 diff.review.approve = Approve
 diff.review.reject = Request changes
 diff.committed_by = committed by
+diff.protected = Protected
 
 releases.desc = Track project versions and downloads.
 release.releases = Releases

routers/api/v1/repo/branch.go

@@ -16,6 +16,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	api "code.gitea.io/gitea/modules/structs"
+	pull_service "code.gitea.io/gitea/services/pull"
 	repo_service "code.gitea.io/gitea/services/repository"
 )
 
@@ -545,6 +546,11 @@ func CreateBranchProtection(ctx *context.APIContext, form api.CreateBranchProtec
 		return
 	}
 
+	if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil {
+		ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err)
+		return
+	}
+
 	// Reload from db to get all whitelists
 	bp, err := models.GetProtectedBranchBy(ctx.Repo.Repository.ID, form.BranchName)
 	if err != nil {
@@ -768,6 +774,11 @@ func EditBranchProtection(ctx *context.APIContext, form api.EditBranchProtection
 		return
 	}
 
+	if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil {
+		ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err)
+		return
+	}
+
 	// Reload from db to ensure get all whitelists
 	bp, err := models.GetProtectedBranchBy(repo.ID, bpName)
 	if err != nil {

routers/api/v1/repo/pull.go

@@ -774,7 +774,7 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
 		return
 	}
 
-	if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
+	if err := pull_service.CheckPRReadyToMerge(pr, false); err != nil {
 		if !models.IsErrNotAllowedToMerge(err) {
 			ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err)
 			return

routers/private/hook.go

@@ -25,7 +25,6 @@ import (
 
 	"gitea.com/macaron/macaron"
 	"github.com/go-git/go-git/v5/plumbing"
-	"github.com/gobwas/glob"
 )
 
 func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []string) error {
@@ -59,53 +58,6 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []
 	return err
 }
 
-func checkFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, repo *git.Repository, env []string) error {
-
-	stdoutReader, stdoutWriter, err := os.Pipe()
-	if err != nil {
-		log.Error("Unable to create os.Pipe for %s", repo.Path)
-		return err
-	}
-	defer func() {
-		_ = stdoutReader.Close()
-		_ = stdoutWriter.Close()
-	}()
-
-	// This use of ...  is safe as force-pushes have already been ruled out.
-	err = git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).
-		RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path,
-			stdoutWriter, nil, nil,
-			func(ctx context.Context, cancel context.CancelFunc) error {
-				_ = stdoutWriter.Close()
-
-				scanner := bufio.NewScanner(stdoutReader)
-				for scanner.Scan() {
-					path := strings.TrimSpace(scanner.Text())
-					if len(path) == 0 {
-						continue
-					}
-					lpath := strings.ToLower(path)
-					for _, pat := range patterns {
-						if pat.Match(lpath) {
-							cancel()
-							return models.ErrFilePathProtected{
-								Path: path,
-							}
-						}
-					}
-				}
-				if err := scanner.Err(); err != nil {
-					return err
-				}
-				_ = stdoutReader.Close()
-				return err
-			})
-	if err != nil && !models.IsErrFilePathProtected(err) {
-		log.Error("Unable to check file protection for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err)
-	}
-	return err
-}
-
 func readAndVerifyCommitsFromShaReader(input io.ReadCloser, repo *git.Repository, env []string) error {
 	scanner := bufio.NewScanner(input)
 	for scanner.Scan() {
@@ -202,6 +154,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 			private.GitQuarantinePath+"="+opts.GitQuarantinePath)
 	}
 
+	// Iterate across the provided old commit IDs
 	for i := range opts.OldCommitIDs {
 		oldCommitID := opts.OldCommitIDs[i]
 		newCommitID := opts.NewCommitIDs[i]
@@ -224,8 +177,17 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 			})
 			return
 		}
-		if protectBranch != nil && protectBranch.IsProtected() {
+
-			// detect and prevent deletion
+		// Allow pushes to non-protected branches
+		if protectBranch == nil || !protectBranch.IsProtected() {
+			continue
+		}
+
+		// This ref is a protected branch.
+		//
+		// First of all we need to enforce absolutely:
+		//
+		// 1. Detect and prevent deletion of the branch
 		if newCommitID == git.EmptySHA {
 			log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo)
 			ctx.JSON(http.StatusForbidden, map[string]interface{}{
@@ -234,7 +196,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 			return
 		}
 
-			// detect force push
+		// 2. Disallow force pushes to protected branches
 		if git.EmptySHA != oldCommitID {
 			output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env)
 			if err != nil {
@@ -253,7 +215,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 			}
 		}
 
-			// Require signed commits
+		// 3. Enforce require signed commits
 		if protectBranch.RequireSignedCommits {
 			err := verifyCommits(oldCommitID, newCommitID, gitRepo, env)
 			if err != nil {
@@ -273,10 +235,15 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 			}
 		}
 
-			// Detect Protected file pattern
+		// Now there are several tests which can be overridden:
+		//
+		// 4. Check protected file patterns - this is overridable from the UI
+		changedProtectedfiles := false
+		protectedFilePath := ""
+
 		globs := protectBranch.GetProtectedFilePatterns()
 		if len(globs) > 0 {
-				err := checkFileProtection(oldCommitID, newCommitID, globs, gitRepo, env)
+			_, err := pull_service.CheckFileProtection(oldCommitID, newCommitID, globs, 1, env, gitRepo)
 			if err != nil {
 				if !models.IsErrFilePathProtected(err) {
 					log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
@@ -285,23 +252,45 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 					})
 					return
 				}
-					protectedFilePath := err.(models.ErrFilePathProtected).Path
+
+				changedProtectedfiles = true
+				protectedFilePath = err.(models.ErrFilePathProtected).Path
+			}
+		}
+
+		// 5. Check if the doer is allowed to push
+		canPush := false
+		if opts.IsDeployKey {
+			canPush = !changedProtectedfiles && protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys)
+		} else {
+			canPush = !changedProtectedfiles && protectBranch.CanUserPush(opts.UserID)
+		}
+
+		// 6. If we're not allowed to push directly
+		if !canPush {
+			// Is this is a merge from the UI/API?
+			if opts.ProtectedBranchID == 0 {
+				// 6a. If we're not merging from the UI/API then there are two ways we got here:
+				//
+				// We are changing a protected file and we're not allowed to do that
+				if changedProtectedfiles {
 					log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
 					ctx.JSON(http.StatusForbidden, map[string]interface{}{
 						"err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
 					})
 					return
 				}
-			}
 
-			canPush := false
+				// Or we're simply not able to push to this protected branch
-			if opts.IsDeployKey {
+				log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo)
-				canPush = protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys)
+				ctx.JSON(http.StatusForbidden, map[string]interface{}{
-			} else {
+					"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
-				canPush = protectBranch.CanUserPush(opts.UserID)
+				})
+				return
 			}
-			if !canPush && opts.ProtectedBranchID > 0 {
+			// 6b. Merge (from UI or API)
-				// Merge (from UI or API)
+
+			// Get the PR, user and permissions for the user in the repository
 			pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
 			if err != nil {
 				log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err)
@@ -326,6 +315,8 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 				})
 				return
 			}
+
+			// Now check if the user is allowed to merge PRs for this repository
 			allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user)
 			if err != nil {
 				log.Error("Error calculating if allowed to merge: %v", err)
@@ -334,6 +325,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 				})
 				return
 			}
+
 			if !allowedMerge {
 				log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v and is not allowed to merge pr #%d", opts.UserID, branchName, repo, pr.Index)
 				ctx.JSON(http.StatusForbidden, map[string]interface{}{
@@ -341,9 +333,23 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 				})
 				return
 			}
-				// Check all status checks and reviews is ok, unless repo admin which can bypass this.
+
-				if !perm.IsAdmin() {
+			// If we're an admin for the repository we can ignore status checks, reviews and override protected files
-					if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
+			if perm.IsAdmin() {
+				continue
+			}
+
+			// Now if we're not an admin - we can't overwrite protected files so fail now
+			if changedProtectedfiles {
+				log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
+				ctx.JSON(http.StatusForbidden, map[string]interface{}{
+					"err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
+				})
+				return
+			}
+
+			// Check all status checks and reviews are ok
+			if err := pull_service.CheckPRReadyToMerge(pr, true); err != nil {
 				if models.IsErrNotAllowedToMerge(err) {
 					log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error())
 					ctx.JSON(http.StatusForbidden, map[string]interface{}{
@@ -355,13 +361,6 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 				ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
 					"err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err),
 				})
-					}
-				}
-			} else if !canPush {
-				log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo)
-				ctx.JSON(http.StatusForbidden, map[string]interface{}{
-					"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
-				})
 				return
 			}
 		}

routers/repo/issue.go

@@ -1426,6 +1426,9 @@ func ViewIssue(ctx *context.Context) {
 			ctx.Data["IsBlockedByOutdatedBranch"] = pull.ProtectedBranch.MergeBlockedByOutdatedBranch(pull)
 			ctx.Data["GrantedApprovals"] = cnt
 			ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits
+			ctx.Data["ChangedProtectedFiles"] = pull.ChangedProtectedFiles
+			ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0
+			ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles)
 		}
 		ctx.Data["WillSign"] = false
 		if ctx.User != nil {

routers/repo/pull.go

@@ -624,6 +624,20 @@ func ViewPullFiles(ctx *context.Context) {
 		return
 	}
 
+	if err = pull.LoadProtectedBranch(); err != nil {
+		ctx.ServerError("LoadProtectedBranch", err)
+		return
+	}
+
+	if pull.ProtectedBranch != nil {
+		glob := pull.ProtectedBranch.GetProtectedFilePatterns()
+		if len(glob) != 0 {
+			for _, file := range diff.Files {
+				file.IsProtected = pull.ProtectedBranch.IsProtectedFile(glob, file.Name)
+			}
+		}
+	}
+
 	ctx.Data["Diff"] = diff
 	ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0
 
@@ -772,7 +786,7 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
 		return
 	}
 
-	if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
+	if err := pull_service.CheckPRReadyToMerge(pr, false); err != nil {
 		if !models.IsErrNotAllowedToMerge(err) {
 			ctx.ServerError("Merge PR status", err)
 			return

routers/repo/setting_protected_branch.go

@@ -16,6 +16,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	pull_service "code.gitea.io/gitea/services/pull"
 )
 
 // ProtectedBranch render the page to protect the repository
@@ -262,6 +263,10 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
 			ctx.ServerError("UpdateProtectBranch", err)
 			return
 		}
+		if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil {
+			ctx.ServerError("CheckPrsForBaseBranch", err)
+			return
+		}
 		ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", branch))
 		ctx.Redirect(fmt.Sprintf("%s/settings/branches/%s", ctx.Repo.RepoLink, branch))
 	} else {

services/gitdiff/gitdiff.go

@@ -351,6 +351,7 @@ type DiffFile struct {
 	IsSubmodule        bool
 	Sections           []*DiffSection
 	IsIncomplete       bool
+	IsProtected        bool
 }
 
 // GetType returns type of diff file.

services/pull/check.go

@@ -62,7 +62,7 @@ func checkAndUpdateStatus(pr *models.PullRequest) {
 	}
 
 	if !has {
-		if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files"); err != nil {
+		if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil {
 			log.Error("Update[%d]: %v", pr.ID, err)
 		}
 	}
@@ -228,6 +228,20 @@ func handle(data ...queue.Data) {
 	}
 }
 
+// CheckPrsForBaseBranch check all pulls with bseBrannch
+func CheckPrsForBaseBranch(baseRepo *models.Repository, baseBranchName string) error {
+	prs, err := models.GetUnmergedPullRequestsByBaseInfo(baseRepo.ID, baseBranchName)
+	if err != nil {
+		return err
+	}
+
+	for _, pr := range prs {
+		AddToTaskQueue(pr)
+	}
+
+	return nil
+}
+
 // Init runs the task queue to test all the checking status pull requests
 func Init() error {
 	prQueue = queue.CreateUniqueQueue("pr_patch_checker", handle, "").(queue.UniqueQueue)

services/pull/merge.go

@@ -559,7 +559,7 @@ func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *mod
 }
 
 // CheckPRReadyToMerge checks whether the PR is ready to be merged (reviews and status checks)
-func CheckPRReadyToMerge(pr *models.PullRequest) (err error) {
+func CheckPRReadyToMerge(pr *models.PullRequest, skipProtectedFilesCheck bool) (err error) {
 	if err = pr.LoadBaseRepo(); err != nil {
 		return fmt.Errorf("LoadBaseRepo: %v", err)
 	}
@@ -598,5 +598,15 @@ func CheckPRReadyToMerge(pr *models.PullRequest) (err error) {
 		}
 	}
 
+	if skipProtectedFilesCheck {
+		return nil
+	}
+
+	if pr.ProtectedBranch.MergeBlockedByProtectedFiles(pr) {
+		return models.ErrNotAllowedToMerge{
+			Reason: "Changed protected files",
+		}
+	}
+
 	return nil
 }

services/pull/patch.go

@@ -18,6 +18,8 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/util"
+
+	"github.com/gobwas/glob"
 )
 
 // DownloadDiffOrPatch will write the patch for the pr to the writer
@@ -66,6 +68,7 @@ func TestPatch(pr *models.PullRequest) error {
 	}
 	defer gitRepo.Close()
 
+	// 1. update merge base
 	pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath)
 	if err != nil {
 		var err2 error
@@ -75,10 +78,32 @@ func TestPatch(pr *models.PullRequest) error {
 		}
 	}
 	pr.MergeBase = strings.TrimSpace(pr.MergeBase)
+
+	// 2. Check for conflicts
+	if conflicts, err := checkConflicts(pr, gitRepo, tmpBasePath); err != nil || conflicts {
+		return err
+	}
+
+	// 3. Check for protected files changes
+	if err = checkPullFilesProtection(pr, gitRepo); err != nil {
+		return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err)
+	}
+
+	if len(pr.ChangedProtectedFiles) > 0 {
+		log.Trace("Found %d protected files changed", len(pr.ChangedProtectedFiles))
+	}
+
+	pr.Status = models.PullRequestStatusMergeable
+
+	return nil
+}
+
+func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath string) (bool, error) {
+	// 1. Create a plain patch from head to base
 	tmpPatchFile, err := ioutil.TempFile("", "patch")
 	if err != nil {
 		log.Error("Unable to create temporary patch file! Error: %v", err)
-		return fmt.Errorf("Unable to create temporary patch file! Error: %v", err)
+		return false, fmt.Errorf("Unable to create temporary patch file! Error: %v", err)
 	}
 	defer func() {
 		_ = util.Remove(tmpPatchFile.Name())
@@ -87,38 +112,43 @@ func TestPatch(pr *models.PullRequest) error {
 	if err := gitRepo.GetDiff(pr.MergeBase, "tracking", tmpPatchFile); err != nil {
 		tmpPatchFile.Close()
 		log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
-		return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
+		return false, fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
 	}
 	stat, err := tmpPatchFile.Stat()
 	if err != nil {
 		tmpPatchFile.Close()
-		return fmt.Errorf("Unable to stat patch file: %v", err)
+		return false, fmt.Errorf("Unable to stat patch file: %v", err)
 	}
 	patchPath := tmpPatchFile.Name()
 	tmpPatchFile.Close()
 
+	// 1a. if the size of that patch is 0 - there can be no conflicts!
 	if stat.Size() == 0 {
 		log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID)
 		pr.Status = models.PullRequestStatusMergeable
 		pr.ConflictedFiles = []string{}
-		return nil
+		return false, nil
 	}
 
 	log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath)
 
+	// 2. preset the pr.Status as checking (this is not save at present)
 	pr.Status = models.PullRequestStatusChecking
 
+	// 3. Read the base branch in to the index of the temporary repository
 	_, err = git.NewCommand("read-tree", "base").RunInDir(tmpBasePath)
 	if err != nil {
-		return fmt.Errorf("git read-tree %s: %v", pr.BaseBranch, err)
+		return false, fmt.Errorf("git read-tree %s: %v", pr.BaseBranch, err)
 	}
 
+	// 4. Now get the pull request configuration to check if we need to ignore whitespace
 	prUnit, err := pr.BaseRepo.GetUnit(models.UnitTypePullRequests)
 	if err != nil {
-		return err
+		return false, err
 	}
 	prConfig := prUnit.PullRequestsConfig()
 
+	// 5. Prepare the arguments to apply the patch against the index
 	args := []string{"apply", "--check", "--cached"}
 	if prConfig.IgnoreWhitespaceConflicts {
 		args = append(args, "--ignore-whitespace")
@@ -126,26 +156,44 @@ func TestPatch(pr *models.PullRequest) error {
 	args = append(args, patchPath)
 	pr.ConflictedFiles = make([]string, 0, 5)
 
+	// 6. Prep the pipe:
+	//   - Here we could do the equivalent of:
+	//  `git apply --check --cached patch_file > conflicts`
+	//     Then iterate through the conflicts. However, that means storing all the conflicts
+	//     in memory - which is very wasteful.
+	//   - alternatively we can do the equivalent of:
+	//  `git apply --check ... | grep ...`
+	//     meaning we don't store all of the conflicts unnecessarily.
 	stderrReader, stderrWriter, err := os.Pipe()
 	if err != nil {
 		log.Error("Unable to open stderr pipe: %v", err)
-		return fmt.Errorf("Unable to open stderr pipe: %v", err)
+		return false, fmt.Errorf("Unable to open stderr pipe: %v", err)
 	}
 	defer func() {
 		_ = stderrReader.Close()
 		_ = stderrWriter.Close()
 	}()
+
+	// 7. Run the check command
 	conflict := false
 	err = git.NewCommand(args...).
 		RunInDirTimeoutEnvFullPipelineFunc(
 			nil, -1, tmpBasePath,
 			nil, stderrWriter, nil,
 			func(ctx context.Context, cancel context.CancelFunc) error {
+				// Close the writer end of the pipe to begin processing
 				_ = stderrWriter.Close()
+				defer func() {
+					// Close the reader on return to terminate the git command if necessary
+					_ = stderrReader.Close()
+				}()
+
 				const prefix = "error: patch failed:"
 				const errorPrefix = "error: "
+
 				conflictMap := map[string]bool{}
 
+				// Now scan the output from the command
 				scanner := bufio.NewScanner(stderrReader)
 				for scanner.Scan() {
 					line := scanner.Text()
@@ -170,25 +218,111 @@ func TestPatch(pr *models.PullRequest) error {
 						break
 					}
 				}
+
 				if len(conflictMap) > 0 {
 					pr.ConflictedFiles = make([]string, 0, len(conflictMap))
 					for key := range conflictMap {
 						pr.ConflictedFiles = append(pr.ConflictedFiles, key)
 					}
 				}
-				_ = stderrReader.Close()
+
 				return nil
 			})
 
+	// 8. If there is a conflict the `git apply` command will return a non-zero error code - so there will be a positive error.
 	if err != nil {
 		if conflict {
 			pr.Status = models.PullRequestStatusConflict
 			log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles)
-			return nil
+
+			return true, nil
 		}
-		return fmt.Errorf("git apply --check: %v", err)
+		return false, fmt.Errorf("git apply --check: %v", err)
 	}
-	pr.Status = models.PullRequestStatusMergeable
+	return false, nil
+}
+
+// CheckFileProtection check file Protection
+func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, env []string, repo *git.Repository) ([]string, error) {
+	// 1. If there are no patterns short-circuit and just return nil
+	if len(patterns) == 0 {
+		return nil, nil
+	}
+
+	// 2. Prep the pipe
+	stdoutReader, stdoutWriter, err := os.Pipe()
+	if err != nil {
+		log.Error("Unable to create os.Pipe for %s", repo.Path)
+		return nil, err
+	}
+	defer func() {
+		_ = stdoutReader.Close()
+		_ = stdoutWriter.Close()
+	}()
+
+	changedProtectedFiles := make([]string, 0, limit)
+
+	// 3. Run `git diff --name-only` to get the names of the changed files
+	err = git.NewCommand("diff", "--name-only", oldCommitID, newCommitID).
+		RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path,
+			stdoutWriter, nil, nil,
+			func(ctx context.Context, cancel context.CancelFunc) error {
+				// Close the writer end of the pipe to begin processing
+				_ = stdoutWriter.Close()
+				defer func() {
+					// Close the reader on return to terminate the git command if necessary
+					_ = stdoutReader.Close()
+				}()
 
+				// Now scan the output from the command
+				scanner := bufio.NewScanner(stdoutReader)
+				for scanner.Scan() {
+					path := strings.TrimSpace(scanner.Text())
+					if len(path) == 0 {
+						continue
+					}
+					lpath := strings.ToLower(path)
+					for _, pat := range patterns {
+						if pat.Match(lpath) {
+							changedProtectedFiles = append(changedProtectedFiles, path)
+							break
+						}
+					}
+					if len(changedProtectedFiles) >= limit {
+						break
+					}
+				}
+
+				if len(changedProtectedFiles) > 0 {
+					return models.ErrFilePathProtected{
+						Path: changedProtectedFiles[0],
+					}
+				}
+				return scanner.Err()
+			})
+	// 4. log real errors if there are any...
+	if err != nil && !models.IsErrFilePathProtected(err) {
+		log.Error("Unable to check file protection for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err)
+	}
+
+	return changedProtectedFiles, err
+}
+
+// checkPullFilesProtection check if pr changed protected files and save results
+func checkPullFilesProtection(pr *models.PullRequest, gitRepo *git.Repository) error {
+	if err := pr.LoadProtectedBranch(); err != nil {
+		return err
+	}
+
+	if pr.ProtectedBranch == nil {
+		pr.ChangedProtectedFiles = nil
+		return nil
+	}
+
+	var err error
+	pr.ChangedProtectedFiles, err = CheckFileProtection(pr.MergeBase, "tracking", pr.ProtectedBranch.GetProtectedFilePatterns(), 10, os.Environ(), gitRepo)
+	if err != nil && !models.IsErrFilePathProtected(err) {
+		return err
+	}
 	return nil
 }

services/pull/pull.go

@@ -173,7 +173,7 @@ func ChangeTargetBranch(pr *models.PullRequest, doer *models.User, targetBranch
 	pr.CommitsAhead = divergence.Ahead
 	pr.CommitsBehind = divergence.Behind
 
-	if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files", "base_branch", "commits_ahead", "commits_behind"); err != nil {
+	if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files", "changed_protected_files", "base_branch", "commits_ahead", "commits_behind"); err != nil {
 		return err
 	}
 

templates/repo/diff/box.tmpl

@@ -68,6 +68,9 @@
 						</div>
 						<span class="file">{{$file.Name}}</span>
 						<div>{{$.i18n.Tr "repo.diff.file_suppressed"}}</div>
+						{{if $file.IsProtected}}
+							<span class="ui right basic label">{{$.i18n.Tr "repo.diff.protected"}}</span>
+						{{end}}
 						{{if and (not $file.IsSubmodule) (not $.PageIsWiki)}}
 							{{if $file.IsDeleted}}
 								<a class="ui basic grey tiny button" rel="nofollow" href="{{EscapePound $.BeforeSourcePath}}/{{EscapePound .Name}}">{{$.i18n.Tr "repo.diff.view_file"}}</a>
@@ -104,6 +107,9 @@
 							{{end}}
 						</div>
 						<span class="file">{{if $file.IsRenamed}}{{$file.OldName}} &rarr; {{end}}{{$file.Name}}{{if .IsLFSFile}} ({{$.i18n.Tr "repo.stored_lfs"}}){{end}}</span>
+						{{if $file.IsProtected}}
+							<span class="ui right basic label">{{$.i18n.Tr "repo.diff.protected"}}</span>
+						{{end}}
 						{{if and (not $file.IsSubmodule) (not $.PageIsWiki)}}
 							{{if $file.IsDeleted}}
 								<a class="ui basic grey tiny button" rel="nofollow" href="{{EscapePound $.BeforeSourcePath}}/{{EscapePound .Name}}">{{$.i18n.Tr "repo.diff.view_file"}}</a>

templates/repo/issue/view_content/pull.tmpl

@@ -67,6 +67,7 @@
 	{{- else if .IsBlockedByApprovals}}red
 	{{- else if .IsBlockedByRejection}}red
 	{{- else if .IsBlockedByOutdatedBranch}}red
+	{{- 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
@@ -145,6 +146,16 @@
 						<i class="icon icon-octicon">{{svg "octicon-x"}}</i>
 					{{$.i18n.Tr "repo.pulls.blocked_by_outdated_branch"}}
 					</div>
+				{{else if .IsBlockedByChangedProtectedFiles}}
+					<div class="item text red">
+						<i class="icon icon-octicon">{{svg "octicon-x" 16}}</i>
+						{{$.i18n.Tr (TrN $.i18n.Lang $.ChangedProtectedFilesNum "repo.pulls.blocked_by_changed_protected_files_1" "repo.pulls.blocked_by_changed_protected_files_n") | Safe }}
+						<div class="ui ordered list">
+							{{range .ChangedProtectedFiles}}
+								<div data-value="-" class="item">{{.}}</div>
+							{{end}}
+						</div>
+					</div>
 				{{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsError .RequiredStatusCheckState.IsFailure)}}
 					<div class="item text red">
 						<i class="icon icon-octicon">{{svg "octicon-x"}}</i>
@@ -165,7 +176,7 @@
 						{{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
 					</div>
 				{{end}}
-				{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOutdatedBranch (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}}
+				{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}}
 				{{if and (or $.IsRepoAdmin (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not .RequireSigned) .WillSign)}}
 					{{if $notAllOverridableChecksOk}}
 						<div class="item text yellow">
@@ -360,6 +371,16 @@
 						<i class="icon icon-octicon">{{svg "octicon-x"}}</i>
 					{{$.i18n.Tr "repo.pulls.blocked_by_outdated_branch"}}
 					</div>
+				{{else if .IsBlockedByChangedProtectedFiles}}
+					<div class="item text red">
+						<i class="icon icon-octicon">{{svg "octicon-x" 16}}</i>
+						{{$.i18n.Tr (TrN $.i18n.Lang $.ChangedProtectedFilesNum "repo.pulls.blocked_by_changed_protected_files_1" "repo.pulls.blocked_by_changed_protected_files_n") | Safe }}
+						<div class="ui ordered list">
+							{{range .ChangedProtectedFiles}}
+								<div data-value="-" class="item">{{.}}</div>
+							{{end}}
+						</div>
+					</div>
 				{{else if and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess)}}
 					<div class="item text red">
 						{{svg "octicon-x"}}