improve code quality (#21464) (#21463)

Backport #21464 and #21465

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

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{}
 	}

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")
 }

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...)

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 <commit>
-				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",
+		gitCmd := NewCommand(repo.Ctx, "log", prettyLogFormat, "--follow",
-			"--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize*page),
+			"--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize*page))
-			prettyLogFormat, "--", file).
+		gitCmd.AddDynamicArguments(revision)
-			Run(&RunOpts{
+		gitCmd.AddArguments("--", file)
-				Dir:    repo.Path,
+		err := gitCmd.Run(&RunOpts{
-				Stdout: stdoutWriter,
+			Dir:    repo.Path,
-				Stderr: &stderr,
+			Stdout: stdoutWriter,
-			})
+			Stderr: &stderr,
+		})
 		if err != nil {
 			_ = stdoutWriter.CloseWithError(ConcatenateError(err, (&stderr).String()))
 		} else {

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,

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))
+	graphCmd := git.NewCommand(r.Ctx, "log", "--graph", "--date-order", "--decorate=full")
-
-	args = append(args, "--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)