From cd48a007bb85ba071ed175a6376b9e392e2c49b6 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 15 Oct 2022 14:24:39 +0200 Subject: [PATCH] improve code quality (#21464) (#21463) Backport #21464 and #21465 Co-authored-by: wxiaoguang --- modules/git/command.go | 28 +++++++++++++++++++++++++++- modules/git/command_test.go | 15 +++++++++++++++ modules/git/commit.go | 2 +- modules/git/repo_commit.go | 19 ++++++++++--------- modules/git/repo_stats.go | 8 ++++---- modules/gitgraph/graph.go | 19 +++++++------------ 6 files changed, 64 insertions(+), 27 deletions(-) diff --git a/modules/git/command.go b/modules/git/command.go index b24d32dbe..ed06dd9b0 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -40,6 +40,7 @@ type Command struct { parentContext context.Context desc string globalArgsLength int + brokenArgs []string } func (c *Command) String() string { @@ -50,6 +51,7 @@ func (c *Command) String() string { } // NewCommand creates and returns a new Git Command based on given command and arguments. +// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead. func NewCommand(ctx context.Context, args ...string) *Command { // Make an explicit copy of globalCommandArgs, otherwise append might overwrite it cargs := make([]string, len(globalCommandArgs)) @@ -63,11 +65,13 @@ func NewCommand(ctx context.Context, args ...string) *Command { } // NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args +// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead. func NewCommandNoGlobals(args ...string) *Command { return NewCommandContextNoGlobals(DefaultContext, args...) } // NewCommandContextNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args +// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead. func NewCommandContextNoGlobals(ctx context.Context, args ...string) *Command { return &Command{ name: GitExecutable, @@ -89,12 +93,28 @@ func (c *Command) SetDescription(desc string) *Command { return c } -// AddArguments adds new argument(s) to the command. +// AddArguments adds new argument(s) to the command. Each argument must be safe to be trusted. +// User-provided arguments should be passed to AddDynamicArguments instead. func (c *Command) AddArguments(args ...string) *Command { c.args = append(c.args, args...) return c } +// AddDynamicArguments adds new dynamic argument(s) to the command. +// The arguments may come from user input and can not be trusted, so no leading '-' is allowed to avoid passing options +func (c *Command) AddDynamicArguments(args ...string) *Command { + for _, arg := range args { + if arg != "" && arg[0] == '-' { + c.brokenArgs = append(c.brokenArgs, arg) + } + } + if len(c.brokenArgs) != 0 { + return c + } + c.args = append(c.args, args...) + return c +} + // RunOpts represents parameters to run the command. If UseContextTimeout is specified, then Timeout is ignored. type RunOpts struct { Env []string @@ -138,8 +158,14 @@ func CommonCmdServEnvs() []string { return commonBaseEnvs() } +var ErrBrokenCommand = errors.New("git command is broken") + // Run runs the command with the RunOpts func (c *Command) Run(opts *RunOpts) error { + if len(c.brokenArgs) != 0 { + log.Error("git command is broken: %s, broken args: %s", c.String(), strings.Join(c.brokenArgs, " ")) + return ErrBrokenCommand + } if opts == nil { opts = &RunOpts{} } diff --git a/modules/git/command_test.go b/modules/git/command_test.go index 67d4ca388..52d25c9c7 100644 --- a/modules/git/command_test.go +++ b/modules/git/command_test.go @@ -26,4 +26,19 @@ func TestRunWithContextStd(t *testing.T) { assert.Contains(t, err.Error(), "exit status 129 - unknown option:") assert.Empty(t, stdout) } + + cmd = NewCommand(context.Background()) + cmd.AddDynamicArguments("-test") + assert.ErrorIs(t, cmd.Run(&RunOpts{}), ErrBrokenCommand) + + cmd = NewCommand(context.Background()) + cmd.AddDynamicArguments("--test") + assert.ErrorIs(t, cmd.Run(&RunOpts{}), ErrBrokenCommand) + + subCmd := "version" + cmd = NewCommand(context.Background()).AddDynamicArguments(subCmd) // for test purpose only, the sub-command should never be dynamic for production + stdout, stderr, err = cmd.RunStdString(&RunOpts{}) + assert.NoError(t, err) + assert.Empty(t, stderr) + assert.Contains(t, stdout, "git version") } diff --git a/modules/git/commit.go b/modules/git/commit.go index 82b5e0b25..6a99d7b45 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -163,7 +163,7 @@ func AllCommitsCount(ctx context.Context, repoPath string, hidePRRefs bool, file // CommitsCountFiles returns number of total commits of until given revision. func CommitsCountFiles(ctx context.Context, repoPath string, revision, relpath []string) (int64, error) { cmd := NewCommand(ctx, "rev-list", "--count") - cmd.AddArguments(revision...) + cmd.AddDynamicArguments(revision...) if len(relpath) > 0 { cmd.AddArguments("--") cmd.AddArguments(relpath...) diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index e6fec4d1a..8d848076b 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -158,7 +158,7 @@ func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Co // add previous arguments except for --grep and --all hashCmd.AddArguments(args...) // add keyword as - hashCmd.AddArguments(v) + hashCmd.AddDynamicArguments(v) // search with given constraints for commit matching sha hash of v hashMatching, _, err := hashCmd.RunStdBytes(&RunOpts{Dir: repo.Path}) @@ -208,14 +208,15 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) ( }() go func() { stderr := strings.Builder{} - err := NewCommand(repo.Ctx, "log", revision, "--follow", - "--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize*page), - prettyLogFormat, "--", file). - Run(&RunOpts{ - Dir: repo.Path, - Stdout: stdoutWriter, - Stderr: &stderr, - }) + gitCmd := NewCommand(repo.Ctx, "log", prettyLogFormat, "--follow", + "--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize*page)) + gitCmd.AddDynamicArguments(revision) + gitCmd.AddArguments("--", file) + err := gitCmd.Run(&RunOpts{ + Dir: repo.Path, + Stdout: stdoutWriter, + Stderr: &stderr, + }) if err != nil { _ = stdoutWriter.CloseWithError(ConcatenateError(err, (&stderr).String())) } else { diff --git a/modules/git/repo_stats.go b/modules/git/repo_stats.go index c0c91c6fc..164ae0732 100644 --- a/modules/git/repo_stats.go +++ b/modules/git/repo_stats.go @@ -59,15 +59,15 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) _ = stdoutWriter.Close() }() - args := []string{"log", "--numstat", "--no-merges", "--pretty=format:---%n%h%n%aN%n%aE%n", "--date=iso", fmt.Sprintf("--since='%s'", since)} + gitCmd := NewCommand(repo.Ctx, "log", "--numstat", "--no-merges", "--pretty=format:---%n%h%n%aN%n%aE%n", "--date=iso", fmt.Sprintf("--since='%s'", since)) if len(branch) == 0 { - args = append(args, "--branches=*") + gitCmd.AddArguments("--branches=*") } else { - args = append(args, "--first-parent", branch) + gitCmd.AddArguments("--first-parent").AddDynamicArguments(branch) } stderr := new(strings.Builder) - err = NewCommand(repo.Ctx, args...).Run(&RunOpts{ + err = gitCmd.Run(&RunOpts{ Env: []string{}, Dir: repo.Path, Stdout: stdoutWriter, diff --git a/modules/gitgraph/graph.go b/modules/gitgraph/graph.go index 271382525..0f3c02134 100644 --- a/modules/gitgraph/graph.go +++ b/modules/gitgraph/graph.go @@ -24,19 +24,17 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo page = 1 } - args := make([]string, 0, 12+len(branches)+len(files)) - - args = append(args, "--graph", "--date-order", "--decorate=full") + graphCmd := git.NewCommand(r.Ctx, "log", "--graph", "--date-order", "--decorate=full") if hidePRRefs { - args = append(args, "--exclude="+git.PullPrefix+"*") + graphCmd.AddArguments("--exclude=" + git.PullPrefix + "*") } if len(branches) == 0 { - args = append(args, "--all") + graphCmd.AddArguments("--all") } - args = append(args, + graphCmd.AddArguments( "-C", "-M", fmt.Sprintf("-n %d", setting.UI.GraphMaxCommitNum*page), @@ -44,15 +42,12 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo fmt.Sprintf("--pretty=format:%s", format)) if len(branches) > 0 { - args = append(args, branches...) + graphCmd.AddDynamicArguments(branches...) } - args = append(args, "--") if len(files) > 0 { - args = append(args, files...) + graphCmd.AddArguments("--") + graphCmd.AddArguments(files...) } - - graphCmd := git.NewCommand(r.Ctx, "log") - graphCmd.AddArguments(args...) graph := NewGraph() stderr := new(strings.Builder)