From a77752502e520d4e2c16f7b2a95068a4f1c2d9f3 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 17 Sep 2019 21:50:25 +0000 Subject: Git command DSL --- internal/git/safecmd.go | 213 +++++++++++++++++++++++++++++++++++++++++++ internal/git/safecmd_test.go | 158 ++++++++++++++++++++++++++++++++ 2 files changed, 371 insertions(+) create mode 100644 internal/git/safecmd.go create mode 100644 internal/git/safecmd_test.go (limited to 'internal/git') diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go new file mode 100644 index 000000000..352ee2ffe --- /dev/null +++ b/internal/git/safecmd.go @@ -0,0 +1,213 @@ +package git + +import ( + "context" + "fmt" + "io" + "regexp" + "strings" + + "github.com/prometheus/client_golang/prometheus" + "gitlab.com/gitlab-org/gitaly/internal/command" + "gitlab.com/gitlab-org/gitaly/internal/git/repository" +) + +var invalidationTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "gitaly_invalid_commands_total", + Help: "Total number of invalid arguments tried to execute", + }, + []string{"command"}, +) + +func init() { + prometheus.MustRegister(invalidationTotal) +} + +func incrInvalidArg(subcmdName string) { + invalidationTotal.WithLabelValues(subcmdName).Inc() +} + +// SubCmd represents a specific git command +type SubCmd struct { + Name string // e.g. "log", or "cat-file", or "worktree" + Flags []Option // optional flags before the positional args + Args []string // positional args after all flags + PostSepArgs []string // post separator (i.e. "--") positional args +} + +var subCmdNameRegex = regexp.MustCompile(`^[[:alnum:]]+(-[[:alnum:]]+)*$`) + +// ValidateArgs checks all arguments in the sub command and validates them +func (sc SubCmd) ValidateArgs() ([]string, error) { + var safeArgs []string + + if !subCmdNameRegex.MatchString(sc.Name) { + return nil, &invalidArgErr{ + msg: fmt.Sprintf("invalid sub command name %q", sc.Name), + } + } + safeArgs = append(safeArgs, sc.Name) + + for _, o := range sc.Flags { + args, err := o.ValidateArgs() + if err != nil { + return nil, err + } + safeArgs = append(safeArgs, args...) + } + + for _, a := range sc.Args { + if err := validatePositionalArg(a); err != nil { + return nil, err + } + safeArgs = append(safeArgs, a) + } + + if len(sc.PostSepArgs) > 0 { + safeArgs = append(safeArgs, "--") + } + + // post separator args do not need any validation + safeArgs = append(safeArgs, sc.PostSepArgs...) + + return safeArgs, nil +} + +// Option is a git command line flag with validation logic +type Option interface { + IsOption() + ValidateArgs() ([]string, error) +} + +// Flag is a single token optional command line argument that enables or +// disables functionality (e.g. "-L") +type Flag struct { + Name string +} + +// IsOption is a method present on all Flag interface implementations +func (Flag) IsOption() {} + +// ValidateArgs returns an error if the flag is not sanitary +func (f Flag) ValidateArgs() ([]string, error) { + if !flagRegex.MatchString(f.Name) { + return nil, &invalidArgErr{ + msg: fmt.Sprintf("flag %q failed regex validation", f.Name), + } + } + return []string{f.Name}, nil +} + +// ValueFlag is an optional command line argument that is comprised of pair of +// tokens (e.g. "-n 50") +type ValueFlag struct { + Name string + Value string +} + +// IsOption is a method present on all Flag interface implementations +func (ValueFlag) IsOption() {} + +// ValidateArgs returns an error if the flag is not sanitary +func (vf ValueFlag) ValidateArgs() ([]string, error) { + if !flagRegex.MatchString(vf.Name) { + return nil, &invalidArgErr{ + msg: fmt.Sprintf("value flag %q failed regex validation", vf.Name), + } + } + return []string{vf.Name, vf.Value}, nil +} + +var flagRegex = regexp.MustCompile(`^(-|--)[[:alnum:]]`) + +type invalidArgErr struct { + msg string +} + +func (iae *invalidArgErr) Error() string { return iae.msg } + +// IsInvalidArgErr relays if the error is due to an argument validation failure +func IsInvalidArgErr(err error) bool { + _, ok := err.(*invalidArgErr) + return ok +} + +func validatePositionalArg(arg string) error { + if strings.HasPrefix(arg, "-") { + return &invalidArgErr{ + msg: fmt.Sprintf("positional arg %q cannot start with dash '-'", arg), + } + } + return nil +} + +// SafeCmd creates a git.Command with the given args and Repository. It +// validates the arguments in the command before executing. +func SafeCmd(ctx context.Context, repo repository.GitRepo, globals []Option, sc SubCmd) (*command.Command, error) { + args, err := combineArgs(globals, sc) + if err != nil { + return nil, err + } + + return Command(ctx, repo, args...) +} + +// SafeBareCmd creates a git.Command with the given args, stdin/stdout/stderr, +// and env. It validates the arguments in the command before executing. +func SafeBareCmd(ctx context.Context, stdin io.Reader, stdout, stderr io.Writer, env []string, globals []Option, sc SubCmd) (*command.Command, error) { + args, err := combineArgs(globals, sc) + if err != nil { + return nil, err + } + + return BareCommand(ctx, stdin, stdout, stderr, env, args...) +} + +// SafeStdinCmd creates a git.Command with the given args and Repository that is +// suitable for Write()ing to. It validates the arguments in the command before +// executing. +func SafeStdinCmd(ctx context.Context, repo repository.GitRepo, globals []Option, sc SubCmd) (*command.Command, error) { + args, err := combineArgs(globals, sc) + if err != nil { + return nil, err + } + + return StdinCommand(ctx, repo, args...) +} + +// SafeCmdWithoutRepo works like Command but without a git repository. It +// validates the arugments in the command before executing. +func SafeCmdWithoutRepo(ctx context.Context, globals []Option, sc SubCmd) (*command.Command, error) { + args, err := combineArgs(globals, sc) + if err != nil { + return nil, err + } + + return CommandWithoutRepo(ctx, args...) +} + +func combineArgs(globals []Option, sc SubCmd) (_ []string, err error) { + defer func() { + if err != nil && IsInvalidArgErr(err) { + incrInvalidArg(sc.Name) + } + }() + + var args []string + + for _, g := range globals { + gargs, err := g.ValidateArgs() + if err != nil { + return nil, err + } + args = append(args, gargs...) + } + + scArgs, err := sc.ValidateArgs() + if err != nil { + return nil, err + } + + return append(args, scArgs...), nil +} diff --git a/internal/git/safecmd_test.go b/internal/git/safecmd_test.go new file mode 100644 index 000000000..e646c7d4d --- /dev/null +++ b/internal/git/safecmd_test.go @@ -0,0 +1,158 @@ +package git_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/testhelper" + "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" +) + +func TestFlagValidation(t *testing.T) { + for _, tt := range []struct { + option git.Option + valid bool + }{ + // valid Flag inputs + {option: git.Flag{"-k"}, valid: true}, + {option: git.Flag{"-K"}, valid: true}, + {option: git.Flag{"--asdf"}, valid: true}, + {option: git.Flag{"--asdf-qwer"}, valid: true}, + {option: git.Flag{"--asdf=qwerty"}, valid: true}, + {option: git.Flag{"-D=A"}, valid: true}, + {option: git.Flag{"-D="}, valid: true}, + + // valid ValueFlag inputs + {option: git.ValueFlag{"-k", "adsf"}, valid: true}, + {option: git.ValueFlag{"-k", "--anything"}, valid: true}, + {option: git.ValueFlag{"-k", ""}, valid: true}, + + // valid FlagCombo inputs + + // invalid Flag inputs + {option: git.Flag{"-*"}}, // invalid character + {option: git.Flag{"a"}}, // missing dash + {option: git.Flag{"[["}}, // suspicious characters + {option: git.Flag{"||"}}, // suspicious characters + {option: git.Flag{"asdf=qwerty"}}, // missing dash + + // invalid ValueFlag inputs + {option: git.ValueFlag{"k", "asdf"}}, // missing dash + } { + args, err := tt.option.ValidateArgs() + if tt.valid { + require.NoError(t, err) + } else { + require.Error(t, err, + "expected error, but args %v passed validation", args) + require.True(t, git.IsInvalidArgErr(err)) + } + } +} + +func TestSafeCmdInvalidArg(t *testing.T) { + for _, tt := range []struct { + globals []git.Option + subCmd git.SubCmd + errMsg string + }{ + { + subCmd: git.SubCmd{Name: "--meow"}, + errMsg: "invalid sub command name \"--meow\"", + }, + { + subCmd: git.SubCmd{ + Name: "meow", + Flags: []git.Option{git.Flag{"woof"}}, + }, + errMsg: "flag \"woof\" failed regex validation", + }, + { + subCmd: git.SubCmd{ + Name: "meow", + Args: []string{"--tweet"}, + }, + errMsg: "positional arg \"--tweet\" cannot start with dash '-'", + }, + } { + _, err := git.SafeCmd( + context.Background(), + &gitalypb.Repository{}, + tt.globals, + tt.subCmd, + ) + require.EqualError(t, err, tt.errMsg) + require.True(t, git.IsInvalidArgErr(err)) + } +} + +func TestSafeCmdValid(t *testing.T) { + testRepo, _, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + for _, tt := range []struct { + globals []git.Option + subCmd git.SubCmd + expectArgs []string + }{ + { + subCmd: git.SubCmd{Name: "meow"}, + expectArgs: []string{"meow"}, + }, + { + globals: []git.Option{ + git.Flag{"--aaaa-bbbb"}, + }, + subCmd: git.SubCmd{Name: "cccc"}, + expectArgs: []string{"--aaaa-bbbb", "cccc"}, + }, + { + subCmd: git.SubCmd{ + Name: "meow", + Args: []string{""}, + PostSepArgs: []string{"-woof", ""}, + }, + expectArgs: []string{"meow", "", "--", "-woof", ""}, + }, + { + globals: []git.Option{ + git.Flag{"-a"}, + git.ValueFlag{"-b", "c"}, + }, + subCmd: git.SubCmd{ + Name: "d", + Flags: []git.Option{ + git.Flag{"-e"}, + git.ValueFlag{"-f", "g"}, + git.Flag{"-h=i"}, + }, + Args: []string{"1", "2"}, + PostSepArgs: []string{"3", "4", "5"}, + }, + expectArgs: []string{"-a", "-b", "c", "d", "-e", "-f", "g", "-h=i", "1", "2", "--", "3", "4", "5"}, + }, + } { + cmd, err := git.SafeCmd(ctx, testRepo, tt.globals, tt.subCmd) + require.NoError(t, err) + // ignore first 3 indeterministic args (executable path and repo args) + require.Equal(t, tt.expectArgs, cmd.Args()[3:]) + + cmd, err = git.SafeStdinCmd(ctx, testRepo, tt.globals, tt.subCmd) + require.NoError(t, err) + require.Equal(t, tt.expectArgs, cmd.Args()[3:]) + + cmd, err = git.SafeBareCmd(ctx, nil, nil, nil, nil, tt.globals, tt.subCmd) + require.NoError(t, err) + // ignore first indeterministic arg (executable path) + require.Equal(t, tt.expectArgs, cmd.Args()[1:]) + + cmd, err = git.SafeCmdWithoutRepo(ctx, tt.globals, tt.subCmd) + require.NoError(t, err) + require.Equal(t, tt.expectArgs, cmd.Args()[1:]) + } +} -- cgit v1.2.3