diff --git a/modules/git/diff.go b/modules/git/diff.go index 7f805478d5..a198695fc0 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -5,6 +5,7 @@ package git import ( "bufio" + "context" "fmt" "io" "regexp" @@ -14,34 +15,64 @@ import ( "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" ) -// RawDiffType type of a raw diff. +// RawDiffType output format: diff or patch type RawDiffType string -// RawDiffType possible values. const ( RawDiffNormal RawDiffType = "diff" RawDiffPatch RawDiffType = "patch" ) // GetRawDiff dumps diff results of repository in given commit ID to io.Writer. -func GetRawDiff(repo *Repository, commitID string, diffType RawDiffType, writer io.Writer) error { - return GetRepoRawDiffForFile(repo, "", commitID, diffType, "", writer) -} - -// GetRepoRawDiffForFile dumps diff results of file in given commit ID to io.Writer according given repository -func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diffType RawDiffType, file string, writer io.Writer) error { - commit, err := repo.GetCommit(endCommit) +func GetRawDiff(repo *Repository, commitID string, diffType RawDiffType, writer io.Writer) (retErr error) { + diffOutput, diffFinish, err := getRepoRawDiffForFile(repo.Ctx, repo, "", commitID, diffType, "") if err != nil { return err } + defer func() { + err := diffFinish() + if retErr == nil { + retErr = err // only return command's error if no previous error + } + }() + _, err = io.Copy(writer, diffOutput) + return err +} + +// GetFileDiffCutAroundLine cuts the old or new part of the diff of a file around a specific line number +func GetFileDiffCutAroundLine( + repo *Repository, startCommit, endCommit, treePath string, + line int64, old bool, numbersOfLine int, +) (_ string, retErr error) { + diffOutput, diffFinish, err := getRepoRawDiffForFile(repo.Ctx, repo, startCommit, endCommit, RawDiffNormal, treePath) + if err != nil { + return "", err + } + defer func() { + err := diffFinish() + if retErr == nil { + retErr = err // only return command's error if no previous error + } + }() + return CutDiffAroundLine(diffOutput, line, old, numbersOfLine) +} + +// getRepoRawDiffForFile returns an io.Reader for the diff results of file in given commit ID +// and a "finish" function to wait for the git command and clean up resources after reading is done. +func getRepoRawDiffForFile(ctx context.Context, repo *Repository, startCommit, endCommit string, diffType RawDiffType, file string) (io.Reader, func() gitcmd.RunStdError, error) { + commit, err := repo.GetCommit(endCommit) + if err != nil { + return nil, nil, err + } var files []string if len(file) > 0 { files = append(files, file) } - cmd := gitcmd.NewCommand() + cmd := gitcmd.NewCommand().WithDir(repo.Path) switch diffType { case RawDiffNormal: if len(startCommit) != 0 { @@ -53,7 +84,7 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff } else { c, err := commit.Parent(0) if err != nil { - return err + return nil, nil, err } cmd.AddArguments("diff"). AddOptionFormat("--find-renames=%s", setting.Git.DiffRenameSimilarityThreshold). @@ -68,18 +99,25 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff } else { c, err := commit.Parent(0) if err != nil { - return err + return nil, nil, err } query := fmt.Sprintf("%s...%s", endCommit, c.ID.String()) cmd.AddArguments("format-patch", "--no-signature", "--stdout").AddDynamicArguments(query).AddDashesAndList(files...) } default: - return fmt.Errorf("invalid diffType: %s", diffType) + return nil, nil, util.NewInvalidArgumentErrorf("invalid diff type: %s", diffType) } - return cmd.WithDir(repo.Path). - WithStdoutCopy(writer). - RunWithStderr(repo.Ctx) + stdoutReader, stdoutReaderClose := cmd.MakeStdoutPipe() + err = cmd.StartWithStderr(ctx) + if err != nil { + stdoutReaderClose() + return nil, nil, err + } + return stdoutReader, func() gitcmd.RunStdError { + stdoutReaderClose() + return cmd.WaitWithStderr() + }, nil } // ParseDiffHunkString parse the diff hunk content and return diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index b072b9c892..b4f0b4f89e 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -2663,7 +2663,7 @@ "repo.branch.new_branch_from": "Create new branch from \"%s\"", "repo.branch.renamed": "Branch %s was renamed to %s.", "repo.branch.rename_default_or_protected_branch_error": "Only admins can rename default or protected branches.", - "repo.branch.rename_protected_branch_failed": "This branch is protected by glob-based protection rules.", + "repo.branch.rename_protected_branch_failed": "Failed to rename branch due to branch protection rules.", "repo.branch.commits_divergence_from": "Commit divergence: %[1]d behind and %[2]d ahead of %[3]s", "repo.branch.commits_no_divergence": "The same as branch %[1]s", "repo.tag.create_tag": "Create tag %s", diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 4624d7e738..9bdc0c76b8 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -515,7 +515,7 @@ func RenameBranch(ctx *context.APIContext) { case repo_model.IsErrUserDoesNotHaveAccessToRepo(err): ctx.APIError(http.StatusForbidden, "User must be a repo or site admin to rename default or protected branches.") case errors.Is(err, git_model.ErrBranchIsProtected): - ctx.APIError(http.StatusForbidden, "Branch is protected by glob-based protection rules.") + ctx.APIError(http.StatusForbidden, "Failed to rename branch due to branch protection rules.") default: ctx.APIErrorInternal(err) } diff --git a/routers/web/repo/editor_cherry_pick.go b/routers/web/repo/editor_cherry_pick.go index ca0e19517a..605a35b100 100644 --- a/routers/web/repo/editor_cherry_pick.go +++ b/routers/web/repo/editor_cherry_pick.go @@ -67,7 +67,7 @@ func CherryPickPost(ctx *context.Context) { if parsed.form.Revert { err = gitrepo.GetReverseRawDiff(ctx, ctx.Repo.Repository, fromCommitID, buf) } else { - err = git.GetRawDiff(ctx.Repo.GitRepo, fromCommitID, "patch", buf) + err = git.GetRawDiff(ctx.Repo.GitRepo, fromCommitID, git.RawDiffPatch, buf) } if err == nil { opts.Content = buf.String() diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index 74a8a127ef..27e540f5cc 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -114,7 +114,7 @@ func checkJobsByRunID(ctx context.Context, runID int64) error { } } if runUpdated { - NotifyWorkflowRunStatusUpdateWithReload(ctx, jobs[0]) + NotifyWorkflowRunStatusUpdateWithReload(ctx, js[0]) } } return nil diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index eeada3100d..6f5b9bb33a 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -895,21 +895,10 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base // SECURITY: The TreePath must be cleaned! use relative path comment.TreePath = util.PathJoinRel(comment.TreePath) - var patch string - reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock - defer func() { - _ = reader.Close() - _ = writer.Close() - }() - go func(comment *base.ReviewComment) { - if err := git.GetRepoRawDiffForFile(g.gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, comment.TreePath, writer); err != nil { - // We should ignore the error since the commit maybe removed when force push to the pull request - log.Warn("GetRepoRawDiffForFile failed when migrating [%s, %s, %s, %s]: %v", g.gitRepo.Path, pr.MergeBase, headCommitID, comment.TreePath, err) - } - _ = writer.Close() - }(comment) - - patch, _ = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) + patch, _ := git.GetFileDiffCutAroundLine( + g.gitRepo, pr.MergeBase, headCommitID, comment.TreePath, + int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines, + ) if comment.CreatedAt.IsZero() { comment.CreatedAt = review.CreatedAt diff --git a/services/pull/review.go b/services/pull/review.go index bcd7392063..acbb620e92 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -8,7 +8,6 @@ import ( "context" "errors" "fmt" - "io" "strings" "code.gitea.io/gitea/models/db" @@ -274,22 +273,12 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo if len(commitID) == 0 { commitID = headCommitID } - reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock - defer func() { - _ = reader.Close() - _ = writer.Close() - }() - go func() { - if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, treePath, writer); err != nil { - _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, headCommitID, treePath, err)) - return - } - _ = writer.Close() - }() - patch, err = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) + patch, err = git.GetFileDiffCutAroundLine( + gitRepo, pr.MergeBase, headCommitID, treePath, + int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines, + ) if err != nil { - log.Error("Error whilst generating patch: %v", err) return nil, err } diff --git a/services/repository/branch.go b/services/repository/branch.go index b076514590..a580208af6 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -442,6 +442,15 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m } } + // We also need to check if "to" matches with a protected branch rule. + rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, to) + if err != nil { + return "", err + } + if rule != nil && !rule.CanUserPush(ctx, doer) { + return "", git_model.ErrBranchIsProtected + } + if err := git_model.RenameBranch(ctx, repo, from, to, func(ctx context.Context, isDefault bool) error { err2 := gitrepo.RenameBranch(ctx, repo, from, to) if err2 != nil { diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index 043aa10c7f..1163c1e8c9 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -237,7 +237,40 @@ func TestAPIRenameBranch(t *testing.T) { MakeRequest(t, req, http.StatusCreated) resp := testAPIRenameBranch(t, "user2", "user2", "repo1", from, "new-branch-name", http.StatusForbidden) - assert.Contains(t, resp.Body.String(), "Branch is protected by glob-based protection rules.") + assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.") + }) + t.Run("RenameBranchToMatchProtectionRulesWithAllowedUser", func(t *testing.T) { + // allow an admin (the owner in this case) to rename a regular branch to one that matches a branch protection rule + repoName := "repo1" + ownerName := "user2" + from := "regular-branch-1" + ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository) + testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated) + + // NOTE: The protected/** branch protection rule was created in a previous test, with push enabled. + testAPIRenameBranch(t, ownerName, ownerName, repoName, from, "protected/2", http.StatusNoContent) + }) + t.Run("RenameBranchToMatchProtectionRulesWithUnauthorizedUser", func(t *testing.T) { + // don't allow renaming a regular branch to a protected branch if the doer is not in the push whitelist + repoName := "repo1" + ownerName := "user2" + pushWhitelist := []string{ownerName} + token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository) + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/branch_protections", ownerName, repoName), + &api.BranchProtection{ + RuleName: "owner-protected/**", + PushWhitelistUsernames: pushWhitelist, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + from := "regular-branch-2" + ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository) + testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated) + + unprivilegedUser := "user40" + resp := testAPIRenameBranch(t, unprivilegedUser, ownerName, repoName, from, "owner-protected/1", http.StatusForbidden) + + assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.") }) t.Run("RenameBranchNormalScenario", func(t *testing.T) { testAPIRenameBranch(t, "user2", "user2", "repo1", "branch2", "new-branch-name", http.StatusNoContent)