From 677af6ac57295a9e8795a8ab3f29e284f2247fb6 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Sat, 15 Oct 2022 20:18:31 +0800
Subject: [PATCH] Follow improve code quality (#21465)
After some discussion, introduce a new slice `brokenArgs` to make
`gitCmd.Run()` return errors if any dynamic argument is invalid.
Co-authored-by: delvh <dev.lh@web.de>
---
modules/git/command.go | 20 +++++++++++++++++---
modules/git/command_test.go | 14 ++++++--------
2 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/modules/git/command.go b/modules/git/command.go
index 8ebd8898fb..ed06dd9b08 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,20 +93,24 @@ 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.
-// If the argument is invalid (it shouldn't happen in real life), it panics to caller
+// 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] == '-' {
- panic("invalid argument: " + arg)
+ c.brokenArgs = append(c.brokenArgs, arg)
}
}
+ if len(c.brokenArgs) != 0 {
+ return c
+ }
c.args = append(c.args, args...)
return c
}
@@ -150,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 c965ea230d..52d25c9c74 100644
--- a/modules/git/command_test.go
+++ b/modules/git/command_test.go
@@ -27,15 +27,13 @@ func TestRunWithContextStd(t *testing.T) {
assert.Empty(t, stdout)
}
- assert.Panics(t, func() {
- cmd = NewCommand(context.Background())
- cmd.AddDynamicArguments("-test")
- })
+ cmd = NewCommand(context.Background())
+ cmd.AddDynamicArguments("-test")
+ assert.ErrorIs(t, cmd.Run(&RunOpts{}), ErrBrokenCommand)
- assert.Panics(t, func() {
- cmd = NewCommand(context.Background())
- cmd.AddDynamicArguments("--test")
- })
+ 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