From f8763bc68ae977a0c7b680d55ea434a903373d44 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Oct 2022 18:58:50 +0800 Subject: [PATCH 1/5] fix2 --- modules/git/command.go | 14 +++++++++++--- modules/git/command_test.go | 16 +++++++--------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/modules/git/command.go b/modules/git/command.go index 8ebd8898fba55..20c5a739fc387 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 + broken bool } func (c *Command) String() string { @@ -89,18 +90,19 @@ 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 trusted. 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.broken = true + return c } } c.args = append(c.args, args...) @@ -150,8 +152,14 @@ func CommonCmdServEnvs() []string { return commonBaseEnvs() } +var ErrBrokenCommand = errors.New("git command is command") + // Run runs the command with the RunOpts func (c *Command) Run(opts *RunOpts) error { + if c.broken { + log.Error("git command is broken: %s", c.String()) + return ErrBrokenCommand + } if opts == nil { opts = &RunOpts{} } diff --git a/modules/git/command_test.go b/modules/git/command_test.go index c965ea230d89c..52d25c9c74820 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") - }) - - 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) + + 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 From cdbe6d3aa219b55a0e96a928b43754b11ac63c0c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Oct 2022 19:01:45 +0800 Subject: [PATCH 2/5] Update modules/git/command.go --- modules/git/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/command.go b/modules/git/command.go index 20c5a739fc387..bc76d3368ee60 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -90,7 +90,7 @@ func (c *Command) SetDescription(desc string) *Command { return c } -// AddArguments adds new argument(s) to the command. Each argument must be safe trusted. +// AddArguments adds new argument(s) to the command. Each argument must be safe to be trusted. func (c *Command) AddArguments(args ...string) *Command { c.args = append(c.args, args...) return c From b59fff50b75ae57d4a98e7c1e4d52bd4d615c04a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Oct 2022 18:58:50 +0800 Subject: [PATCH 3/5] fix2 --- modules/git/command.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/git/command.go b/modules/git/command.go index bc76d3368ee60..8bc184cfdcfd2 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -51,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)) @@ -64,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, @@ -91,6 +94,7 @@ func (c *Command) SetDescription(desc string) *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 From ee8cbc07083d6988aa5887707ed85f7fe206be3e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Oct 2022 19:19:43 +0800 Subject: [PATCH 4/5] fix --- modules/git/command.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/modules/git/command.go b/modules/git/command.go index 8bc184cfdcfd2..20e7499ae7ad9 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -40,7 +40,7 @@ type Command struct { parentContext context.Context desc string globalArgsLength int - broken bool + brokenArgs []string } func (c *Command) String() string { @@ -105,10 +105,12 @@ func (c *Command) AddArguments(args ...string) *Command { func (c *Command) AddDynamicArguments(args ...string) *Command { for _, arg := range args { if arg != "" && arg[0] == '-' { - c.broken = true - return c + c.brokenArgs = append(c.brokenArgs, arg) } } + if len(c.brokenArgs) != 0 { + return c + } c.args = append(c.args, args...) return c } @@ -160,8 +162,8 @@ var ErrBrokenCommand = errors.New("git command is command") // Run runs the command with the RunOpts func (c *Command) Run(opts *RunOpts) error { - if c.broken { - log.Error("git command is broken: %s", c.String()) + 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 { From ae10111f89a0c834d41efb113d3abf9c30c60b77 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Oct 2022 19:23:22 +0800 Subject: [PATCH 5/5] Update modules/git/command.go Co-authored-by: delvh --- modules/git/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/command.go b/modules/git/command.go index 20e7499ae7ad9..ed06dd9b08de9 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -158,7 +158,7 @@ func CommonCmdServEnvs() []string { return commonBaseEnvs() } -var ErrBrokenCommand = errors.New("git command is command") +var ErrBrokenCommand = errors.New("git command is broken") // Run runs the command with the RunOpts func (c *Command) Run(opts *RunOpts) error {