diff --git a/models/branches.go b/models/branches.go index 20988deed..420a4b663 100644 --- a/models/branches.go +++ b/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) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 4e12181b1..0efcce30a 100644 --- a/models/migrations/migrations.go +++ b/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 diff --git a/models/migrations/v155.go b/models/migrations/v155.go new file mode 100644 index 000000000..58d78b6cf --- /dev/null +++ b/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 +} diff --git a/models/pull.go b/models/pull.go index 9f1f48526..9b6f0830d 100644 --- a/models/pull.go +++ b/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 diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index f7fa5f028..a1a9c624d 100644 --- a/modules/repofiles/update.go +++ b/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 } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 45feaf8c0..f6246c044 100644 --- a/options/locale/locale_en-US.ini +++ b/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 diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 6c63bde94..50ca3977a 100644 --- a/routers/api/v1/repo/branch.go +++ b/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 { diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index afbe7ddab..e2cab70ad 100644 --- a/routers/api/v1/repo/pull.go +++ b/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 diff --git a/routers/private/hook.go b/routers/private/hook.go index 05f0b124c..a2033fc1d 100644 --- a/routers/private/hook.go +++ b/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,146 +177,192 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { }) return } - if protectBranch != nil && protectBranch.IsProtected() { - // detect and prevent deletion - if newCommitID == git.EmptySHA { - log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo) + + // 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{}{ + "err": fmt.Sprintf("branch %s is protected from deletion", branchName), + }) + return + } + + // 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 { + log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Fail to detect force push: %v", err), + }) + return + } else if len(output) > 0 { + log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo) ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("branch %s is protected from deletion", branchName), + "err": fmt.Sprintf("branch %s is protected from force push", branchName), + }) + return + + } + } + + // 3. Enforce require signed commits + if protectBranch.RequireSignedCommits { + err := verifyCommits(oldCommitID, newCommitID, gitRepo, env) + if err != nil { + if !isErrUnverifiedCommit(err) { + log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err), + }) + return + } + unverifiedCommit := err.(*errUnverifiedCommit).sha + log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit), }) return } + } - // detect force push - if git.EmptySHA != oldCommitID { - output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env) - if err != nil { - log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err) + // 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 := 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) ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Fail to detect force push: %v", err), - }) - return - } else if len(output) > 0 { - log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo) - ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("branch %s is protected from force push", branchName), - }) - return - - } - } - - // Require signed commits - if protectBranch.RequireSignedCommits { - err := verifyCommits(oldCommitID, newCommitID, gitRepo, env) - if err != nil { - if !isErrUnverifiedCommit(err) { - log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err), - }) - return - } - unverifiedCommit := err.(*errUnverifiedCommit).sha - log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit) - ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit), + "err": fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err), }) return } - } - // Detect Protected file pattern - globs := protectBranch.GetProtectedFilePatterns() - if len(globs) > 0 { - err := checkFileProtection(oldCommitID, newCommitID, globs, gitRepo, env) - 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) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err), - }) - 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 - if opts.IsDeployKey { - canPush = protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys) - } else { - canPush = protectBranch.CanUserPush(opts.UserID) - } - if !canPush && opts.ProtectedBranchID > 0 { - // Merge (from UI or API) - pr, err := models.GetPullRequestByID(opts.ProtectedBranchID) - if err != nil { - log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err), - }) - return - } - user, err := models.GetUserByID(opts.UserID) - if err != nil { - log.Error("Unable to get User id %d Error: %v", opts.UserID, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err), - }) - return - } - perm, err := models.GetUserRepoPermission(repo, user) - if err != nil { - log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err), - }) - return - } - allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user) - if err != nil { - log.Error("Error calculating if allowed to merge: %v", err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Error calculating if allowed to merge: %v", err), - }) - 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{}{ - "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName), - }) - return - } - // Check all status checks and reviews is ok, unless repo admin which can bypass this. - if !perm.IsAdmin() { - if err := pull_service.CheckPRReadyToMerge(pr); 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{}{ - "err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()), - }) - return - } - log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err) - 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 { + // Or we're simply not able to push to this protected branch 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 } + // 6b. 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) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err), + }) + return + } + user, err := models.GetUserByID(opts.UserID) + if err != nil { + log.Error("Unable to get User id %d Error: %v", opts.UserID, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err), + }) + return + } + perm, err := models.GetUserRepoPermission(repo, user) + if err != nil { + log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err), + }) + 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) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Error calculating if allowed to merge: %v", err), + }) + 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{}{ + "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName), + }) + return + } + + // If we're an admin for the repository we can ignore status checks, reviews and override protected files + 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{}{ + "err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()), + }) + return + } + log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err), + }) + return + } } } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index ef10651aa..797764709 100644 --- a/routers/repo/issue.go +++ b/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 { diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 535bd0cdb..54da7b76b 100644 --- a/routers/repo/pull.go +++ b/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 diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index ab0fd77ee..f864e8a75 100644 --- a/routers/repo/setting_protected_branch.go +++ b/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 { diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 4cea5dd9a..de83b518a 100644 --- a/services/gitdiff/gitdiff.go +++ b/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. diff --git a/services/pull/check.go b/services/pull/check.go index d6817bc81..8665b3e7d 100644 --- a/services/pull/check.go +++ b/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) diff --git a/services/pull/merge.go b/services/pull/merge.go index 7a1964932..e74b4b6b1 100644 --- a/services/pull/merge.go +++ b/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 } diff --git a/services/pull/patch.go b/services/pull/patch.go index 9b894b781..2692d848c 100644 --- a/services/pull/patch.go +++ b/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 fmt.Errorf("git apply --check: %v", err) - } - pr.Status = models.PullRequestStatusMergeable + return true, nil + } + return false, fmt.Errorf("git apply --check: %v", err) + } + 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 } diff --git a/services/pull/pull.go b/services/pull/pull.go index 53de74285..61af7fe9a 100644 --- a/services/pull/pull.go +++ b/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 } diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index d7abd96da..8301dbb05 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -68,6 +68,9 @@ {{$file.Name}}
{{$.i18n.Tr "repo.diff.file_suppressed"}}
+ {{if $file.IsProtected}} + {{$.i18n.Tr "repo.diff.protected"}} + {{end}} {{if and (not $file.IsSubmodule) (not $.PageIsWiki)}} {{if $file.IsDeleted}} {{$.i18n.Tr "repo.diff.view_file"}} @@ -104,6 +107,9 @@ {{end}} {{if $file.IsRenamed}}{{$file.OldName}} → {{end}}{{$file.Name}}{{if .IsLFSFile}} ({{$.i18n.Tr "repo.stored_lfs"}}){{end}} + {{if $file.IsProtected}} + {{$.i18n.Tr "repo.diff.protected"}} + {{end}} {{if and (not $file.IsSubmodule) (not $.PageIsWiki)}} {{if $file.IsDeleted}} {{$.i18n.Tr "repo.diff.view_file"}} diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index b21d53419..d043c269e 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/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 @@ {{svg "octicon-x"}} {{$.i18n.Tr "repo.pulls.blocked_by_outdated_branch"}} + {{else if .IsBlockedByChangedProtectedFiles}} +
+ {{svg "octicon-x" 16}} + {{$.i18n.Tr (TrN $.i18n.Lang $.ChangedProtectedFilesNum "repo.pulls.blocked_by_changed_protected_files_1" "repo.pulls.blocked_by_changed_protected_files_n") | Safe }} +
+ {{range .ChangedProtectedFiles}} +
{{.}}
+ {{end}} +
+
{{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsError .RequiredStatusCheckState.IsFailure)}}
{{svg "octicon-x"}} @@ -165,7 +176,7 @@ {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
{{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}}
@@ -360,6 +371,16 @@ {{svg "octicon-x"}} {{$.i18n.Tr "repo.pulls.blocked_by_outdated_branch"}}
+ {{else if .IsBlockedByChangedProtectedFiles}} +
+ {{svg "octicon-x" 16}} + {{$.i18n.Tr (TrN $.i18n.Lang $.ChangedProtectedFilesNum "repo.pulls.blocked_by_changed_protected_files_1" "repo.pulls.blocked_by_changed_protected_files_n") | Safe }} +
+ {{range .ChangedProtectedFiles}} +
{{.}}
+ {{end}} +
+
{{else if and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess)}}
{{svg "octicon-x"}}