From dcd9fc7ee894700f702f3847d7d2a41d6a009b7e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 23 Oct 2022 22:44:45 +0800 Subject: Refactor git command arguments and make all arguments to be safe to be used (#21535) Follow #21464 Make all git command arguments strictly safe. Most changes are one-to-one replacing, keep all existing logic. --- services/gitdiff/gitdiff.go | 28 +++++++++++++++------------- services/gitdiff/gitdiff_test.go | 2 +- 2 files changed, 16 insertions(+), 14 deletions(-) (limited to 'services/gitdiff') diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index e687d9ae91..3c8c5c81a5 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1056,7 +1056,7 @@ type DiffOptions struct { MaxLines int MaxLineCharacters int MaxFiles int - WhitespaceBehavior string + WhitespaceBehavior git.CmdArg DirectComparison bool } @@ -1082,7 +1082,7 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff argsLength += len(files) + 1 } - diffArgs := make([]string, 0, argsLength) + diffArgs := make([]git.CmdArg, 0, argsLength) if (len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == git.EmptySHA) && commit.ParentCount() == 0 { diffArgs = append(diffArgs, "diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M") if len(opts.WhitespaceBehavior) != 0 { @@ -1090,7 +1090,7 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff } // append empty tree ref diffArgs = append(diffArgs, "4b825dc642cb6eb9a060e54bf8d69288fbee4904") - diffArgs = append(diffArgs, opts.AfterCommitID) + diffArgs = append(diffArgs, git.CmdArgCheck(opts.AfterCommitID)) } else { actualBeforeCommitID := opts.BeforeCommitID if len(actualBeforeCommitID) == 0 { @@ -1101,8 +1101,8 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff if len(opts.WhitespaceBehavior) != 0 { diffArgs = append(diffArgs, opts.WhitespaceBehavior) } - diffArgs = append(diffArgs, actualBeforeCommitID) - diffArgs = append(diffArgs, opts.AfterCommitID) + diffArgs = append(diffArgs, git.CmdArgCheck(actualBeforeCommitID)) + diffArgs = append(diffArgs, git.CmdArgCheck(opts.AfterCommitID)) opts.BeforeCommitID = actualBeforeCommitID } @@ -1111,13 +1111,15 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff // the skipping for us parsePatchSkipToFile := opts.SkipTo if opts.SkipTo != "" && git.CheckGitVersionAtLeast("2.31") == nil { - diffArgs = append(diffArgs, "--skip-to="+opts.SkipTo) + diffArgs = append(diffArgs, git.CmdArg("--skip-to="+opts.SkipTo)) parsePatchSkipToFile = "" } if len(files) > 0 { diffArgs = append(diffArgs, "--") - diffArgs = append(diffArgs, files...) + for _, file := range files { + diffArgs = append(diffArgs, git.CmdArg(file)) // it's safe to cast it to CmdArg because there is a "--" before + } } reader, writer := io.Pipe() @@ -1126,7 +1128,7 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff _ = writer.Close() }() - go func(ctx context.Context, diffArgs []string, repoPath string, writer *io.PipeWriter) { + go func(ctx context.Context, diffArgs []git.CmdArg, repoPath string, writer *io.PipeWriter) { cmd := git.NewCommand(ctx, diffArgs...) cmd.SetDescription(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) if err := cmd.Run(&git.RunOpts{ @@ -1199,15 +1201,15 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff separator = ".." } - shortstatArgs := []string{opts.BeforeCommitID + separator + opts.AfterCommitID} + shortstatArgs := []git.CmdArg{git.CmdArgCheck(opts.BeforeCommitID + separator + opts.AfterCommitID)} if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == git.EmptySHA { - shortstatArgs = []string{git.EmptyTreeSHA, opts.AfterCommitID} + shortstatArgs = []git.CmdArg{git.EmptyTreeSHA, git.CmdArgCheck(opts.AfterCommitID)} } diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, shortstatArgs...) if err != nil && strings.Contains(err.Error(), "no merge base") { // git >= 2.28 now returns an error if base and head have become unrelated. // previously it would return the results of git diff --shortstat base head so let's try that... - shortstatArgs = []string{opts.BeforeCommitID, opts.AfterCommitID} + shortstatArgs = []git.CmdArg{git.CmdArgCheck(opts.BeforeCommitID), git.CmdArgCheck(opts.AfterCommitID)} diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, shortstatArgs...) } if err != nil { @@ -1322,7 +1324,7 @@ func CommentMustAsDiff(c *issues_model.Comment) *Diff { } // GetWhitespaceFlag returns git diff flag for treating whitespaces -func GetWhitespaceFlag(whitespaceBehavior string) string { +func GetWhitespaceFlag(whitespaceBehavior string) git.CmdArg { whitespaceFlags := map[string]string{ "ignore-all": "-w", "ignore-change": "-b", @@ -1331,7 +1333,7 @@ func GetWhitespaceFlag(whitespaceBehavior string) string { } if flag, ok := whitespaceFlags[whitespaceBehavior]; ok { - return flag + return git.CmdArg(flag) } log.Warn("unknown whitespace behavior: %q, default to 'show-all'", whitespaceBehavior) return "" diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index dfdd4df9c4..a7fd677fef 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -627,7 +627,7 @@ func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) { return } defer gitRepo.Close() - for _, behavior := range []string{"-w", "--ignore-space-at-eol", "-b", ""} { + for _, behavior := range []git.CmdArg{"-w", "--ignore-space-at-eol", "-b", ""} { diffs, err := GetDiff(gitRepo, &DiffOptions{ AfterCommitID: "bd7063cc7c04689c4d082183d32a604ed27a24f9", -- cgit v1.2.3