From 27fa5db3714c241c87325a907c8f13357f014eb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Sun, 15 Jan 2023 14:55:05 +0000 Subject: [PATCH] interp: add ExecHandlers to support exec middlewares The tests and examples were already using a form of middlewares. For example, ExampleExecHandler would handle some specific cases, and fall back to DefaultExecHandler. However, this fall back was hard-coded to DefaultExecHandler. The function wasn't a reusable middleware because of that. Instead, borrow the design of middlewares from go-chi: func (mx *Mux) Use(middlewares ...func(http.Handler) http.Handler) In such an API, each middleware is a function which takes "next", the next handler, and returns its own handler. This way, each middleware can choose whether to handle all calls, or just some of them - while passing on the rest to "next". This makes our API more flexible and our tests less awkward. Most importantly, it enables #93, as a coreutils ExecHandler by design will only be able to handle some coreutil commands and nothing else. For #93. --- interp/api.go | 66 ++++++++++++++-- interp/example_test.go | 34 +++++---- interp/handler_test.go | 168 +++++++++++++++++++++++++++-------------- interp/interp_test.go | 18 +++-- 4 files changed, 201 insertions(+), 85 deletions(-) diff --git a/interp/api.go b/interp/api.go index c55784d98..dd6b23bff 100644 --- a/interp/api.go +++ b/interp/api.go @@ -71,10 +71,15 @@ type Runner struct { // arguments. It may be nil. callHandler CallHandlerFunc - // execHandler is a function responsible for executing programs. It must be non-nil. + // execHandler is responsible for executing programs. It must not be nil. execHandler ExecHandlerFunc - // openHandler is a function responsible for opening files. It must be non-nil. + // execMiddlewares grows with calls to ExecHandlers, + // and is used to construct execHandler when Reset is first called. + // The slice is needed to preserve the relative order of middlewares. + execMiddlewares []func(ExecHandlerFunc) ExecHandlerFunc + + // openHandler is a function responsible for opening files. It must not be nil. openHandler OpenHandlerFunc // readDirHandler is a function responsible for reading directories during @@ -181,7 +186,6 @@ func (r *Runner) optByFlag(flag byte) *bool { func New(opts ...RunnerOption) (*Runner, error) { r := &Runner{ usedNew: true, - execHandler: DefaultExecHandler(2 * time.Second), openHandler: DefaultOpenHandler(), readDirHandler: DefaultReadDirHandler(), statHandler: DefaultStatHandler(), @@ -213,9 +217,11 @@ func New(opts ...RunnerOption) (*Runner, error) { return r, nil } -// RunnerOption is a function which can be passed to New to alter Runner behaviour. -// To apply option to existing Runner call it directly, -// for example interp.Params("-e")(runner). +// RunnerOption can be passed to New to alter a Runner's behaviour. +// It can also be applied directly on an existing Runner, +// such as interp.Params("-e")(runner). +// Note that options cannot be applied once Run or Reset have been called. +// TODO: enforce that rule via didReset. type RunnerOption func(*Runner) error // Env sets the interpreter's environment. If nil, a copy of the current @@ -329,7 +335,10 @@ func CallHandler(f CallHandlerFunc) RunnerOption { } } -// ExecHandler sets the command execution handler. See ExecHandlerFunc for more info. +// ExecHandler sets one command execution handler, +// which replaces DefaultExecHandler(2 * time.Second). +// +// Deprecated: use ExecHandlers instead, which allows for middleware handlers. func ExecHandler(f ExecHandlerFunc) RunnerOption { return func(r *Runner) error { r.execHandler = f @@ -337,6 +346,35 @@ func ExecHandler(f ExecHandlerFunc) RunnerOption { } } +// ExecHandlers appends middlewares to handle command execution. +// The middlewares are chained from first to last, and the first is called by the runner. +// Each middleware is expected to call the "next" middleware at most once. +// +// For example, a middleware may implement only some commands. +// For those commands, it can run its logic and avoid calling "next". +// For any other commands, it can call "next" with the original parameters. +// +// Another common example is a middleware which always calls "next", +// but runs custom logic either before or after that call. +// For instance, a middleware could change the arguments to the "next" call, +// or it could print log lines before or after the call to "next". +// +// The last exec handler is DefaultExecHandler(2 * time.Second). +func ExecHandlers(middlewares ...func(next ExecHandlerFunc) ExecHandlerFunc) RunnerOption { + return func(r *Runner) error { + r.execMiddlewares = append(r.execMiddlewares, middlewares...) + return nil + } +} + +// TODO: consider porting the middleware API in ExecHandlers to OpenHandler, +// ReadDirHandler, and StatHandler. + +// TODO(v4): now that ExecHandlers allows calling a next handler with changed +// arguments, one of the two advantages of CallHandler is gone. The other is the +// ability to work with builtins; if we make ExecHandlers work with builtins, we +// could join both APIs. + // OpenHandler sets file open handler. See OpenHandlerFunc for more info. func OpenHandler(f OpenHandlerFunc) RunnerOption { return func(r *Runner) error { @@ -561,6 +599,19 @@ func (r *Runner) Reset() { r.origStdin = r.stdin r.origStdout = r.stdout r.origStderr = r.stderr + + if r.execHandler != nil && len(r.execMiddlewares) > 0 { + panic("interp.ExecHandler should be replaced with interp.ExecHandlers, not mixed") + } + if r.execHandler == nil { + r.execHandler = DefaultExecHandler(2 * time.Second) + } + // Middlewares are chained from first to last, and each can call the + // next in the chain, so we need to construct the chain backwards. + for i := len(r.execMiddlewares) - 1; i >= 0; i-- { + middleware := r.execMiddlewares[i] + r.execHandler = middleware(r.execHandler) + } } // reset the internal state *r = Runner{ @@ -632,6 +683,7 @@ func (r *Runner) Reset() { r.setVarString("OPTIND", "1") r.dirStack = append(r.dirStack, r.Dir) + r.didReset = true } diff --git a/interp/example_test.go b/interp/example_test.go index b5d9b4b08..348290e3a 100644 --- a/interp/example_test.go +++ b/interp/example_test.go @@ -10,7 +10,6 @@ import ( "os" "runtime" "strings" - "time" "mvdan.cc/sh/v3/expand" "mvdan.cc/sh/v3/interp" @@ -38,28 +37,33 @@ func Example() { // global_value } -func ExampleExecHandler() { +func ExampleExecHandlers() { src := "echo foo; join ! foo bar baz; missing-program bar" file, _ := syntax.NewParser().Parse(strings.NewReader(src), "") - exec := func(ctx context.Context, args []string) error { - hc := interp.HandlerCtx(ctx) - - if args[0] == "join" { - fmt.Fprintln(hc.Stdout, strings.Join(args[2:], args[1])) - return nil + execJoin := func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + hc := interp.HandlerCtx(ctx) + if args[0] == "join" { + fmt.Fprintln(hc.Stdout, strings.Join(args[2:], args[1])) + return nil + } + return next(ctx, args) } - - if _, err := interp.LookPathDir(hc.Dir, hc.Env, args[0]); err != nil { - fmt.Printf("%s is not installed\n", args[0]) - return interp.NewExitStatus(1) + } + execNotInstalled := func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + hc := interp.HandlerCtx(ctx) + if _, err := interp.LookPathDir(hc.Dir, hc.Env, args[0]); err != nil { + fmt.Printf("%s is not installed\n", args[0]) + return interp.NewExitStatus(1) + } + return next(ctx, args) } - - return interp.DefaultExecHandler(2*time.Second)(ctx, args) } runner, _ := interp.New( interp.StdIO(nil, os.Stdout, os.Stdout), - interp.ExecHandler(exec), + interp.ExecHandlers(execJoin, execNotInstalled), ) runner.Run(context.TODO(), file) // Output: diff --git a/interp/handler_test.go b/interp/handler_test.go index 69271253d..e2fb71692 100644 --- a/interp/handler_test.go +++ b/interp/handler_test.go @@ -22,17 +22,21 @@ import ( "mvdan.cc/sh/v3/syntax" ) -func blocklistOneExec(name string) interp.ExecHandlerFunc { - return func(ctx context.Context, args []string) error { - if args[0] == name { - return fmt.Errorf("%s: blocklisted program", name) +func blocklistOneExec(name string) func(interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + if args[0] == name { + return fmt.Errorf("%s: blocklisted program", name) + } + return next(ctx, args) } - return testExecHandler(ctx, args) } } -func blocklistAllExec(ctx context.Context, args []string) error { - return fmt.Errorf("blocklisted: %s", args[0]) +func blocklistAllExec(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + return fmt.Errorf("blocklisted: %s", args[0]) + } } func blocklistNondevOpen(ctx context.Context, path string, flags int, mode os.FileMode) (io.ReadWriteCloser, error) { @@ -47,107 +51,170 @@ func blocklistGlob(ctx context.Context, path string) ([]os.FileInfo, error) { return nil, fmt.Errorf("blocklisted: glob") } +func execPrint(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + hc := interp.HandlerCtx(ctx) + fmt.Fprintf(hc.Stdout, "would run: %s", args) + return nil + } +} + // runnerCtx allows us to give handler functions access to the Runner, if needed. var runnerCtx = new(int) -func execJustPrint(ctx context.Context, args []string) error { - runner, ok := ctx.Value(runnerCtx).(*interp.Runner) - if ok && runner.Exited() { - return fmt.Errorf("would run: %s", args) +func execPrintWouldExec(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + runner, ok := ctx.Value(runnerCtx).(*interp.Runner) + if ok && runner.Exited() { + return fmt.Errorf("would exec via builtin: %s", args) + } + return nil } - return nil } +// TODO: join with TestRunnerOpts? var modCases = []struct { - name string - exec interp.ExecHandlerFunc - open interp.OpenHandlerFunc - call interp.CallHandlerFunc - readdir interp.ReadDirHandlerFunc - src string - want string + name string + opts []interp.RunnerOption + src string + want string }{ { name: "ExecBlocklistOne", - exec: blocklistOneExec("sleep"), + opts: []interp.RunnerOption{ + interp.ExecHandlers(blocklistOneExec("sleep")), + }, src: "echo foo; sleep 1", want: "foo\nsleep: blocklisted program", }, { name: "ExecBlocklistOneSubshell", - exec: blocklistOneExec("faa"), + opts: []interp.RunnerOption{ + interp.ExecHandlers(blocklistOneExec("faa")), + }, src: "a=$(echo foo | sed 's/o/a/g'); echo $a; $a args", want: "faa\nfaa: blocklisted program", }, { name: "ExecBlocklistAllSubshell", - exec: blocklistAllExec, + opts: []interp.RunnerOption{ + interp.ExecHandlers(blocklistAllExec), + }, src: "(malicious)", want: "blocklisted: malicious", }, { name: "ExecPipe", - exec: blocklistAllExec, + opts: []interp.RunnerOption{ + interp.ExecHandlers(blocklistAllExec), + }, src: "malicious | echo foo", want: "foo\nblocklisted: malicious", }, { name: "ExecCmdSubst", - exec: blocklistAllExec, + opts: []interp.RunnerOption{ + interp.ExecHandlers(blocklistAllExec), + }, src: "a=$(malicious)", want: "blocklisted: malicious\n", // TODO: why the newline? }, { name: "ExecBackground", - exec: blocklistAllExec, + opts: []interp.RunnerOption{ + interp.ExecHandlers(blocklistAllExec), + }, src: "{ malicious; true; } & { malicious; true; } & wait", want: "blocklisted: malicious", }, { - name: "ExecBuiltin", - exec: execJustPrint, + name: "ExecPrintWouldExec", + opts: []interp.RunnerOption{ + interp.ExecHandlers(execPrintWouldExec), + }, src: "exec /bin/sh", - want: "would run: [/bin/sh]", + want: "would exec via builtin: [/bin/sh]", + }, + { + name: "ExecPrintAndBlocklist", + opts: []interp.RunnerOption{ + interp.ExecHandlers( + execPrint, + blocklistOneExec("foo"), + ), + }, + src: "foo", + want: "would run: [foo]", + }, + { + name: "ExecPrintAndBlocklistSeparate", + opts: []interp.RunnerOption{ + interp.ExecHandlers(execPrint), + interp.ExecHandlers(blocklistOneExec("foo")), + }, + src: "foo", + want: "would run: [foo]", + }, + { + name: "ExecBlocklistAndPrint", + opts: []interp.RunnerOption{ + interp.ExecHandlers( + blocklistOneExec("foo"), + execPrint, + ), + }, + src: "foo", + want: "foo: blocklisted program", }, { name: "OpenForbidNonDev", - open: blocklistNondevOpen, + opts: []interp.RunnerOption{ + interp.OpenHandler(blocklistNondevOpen), + }, src: "echo foo >/dev/null; echo bar >/tmp/x", want: "non-dev: /tmp/x", }, { name: "CallReplaceWithBlank", - open: blocklistNondevOpen, - call: func(ctx context.Context, args []string) ([]string, error) { - return []string{"echo", "blank"}, nil + opts: []interp.RunnerOption{ + interp.OpenHandler(blocklistNondevOpen), + interp.CallHandler(func(ctx context.Context, args []string) ([]string, error) { + return []string{"echo", "blank"}, nil + }), }, src: "echo foo >/dev/null; { bar; } && baz", want: "blank\nblank\n", }, { name: "CallDryRun", - call: func(ctx context.Context, args []string) ([]string, error) { - return append([]string{"echo", "run:"}, args...), nil + opts: []interp.RunnerOption{ + interp.CallHandler(func(ctx context.Context, args []string) ([]string, error) { + return append([]string{"echo", "run:"}, args...), nil + }), }, src: "cd some-dir; cat foo; exit 1", want: "run: cd some-dir\nrun: cat foo\nrun: exit 1\n", }, { name: "CallError", - call: func(ctx context.Context, args []string) ([]string, error) { - if args[0] == "echo" && len(args) > 2 { - return nil, fmt.Errorf("refusing to run echo builtin with multiple args") - } - return args, nil + opts: []interp.RunnerOption{ + interp.CallHandler(func(ctx context.Context, args []string) ([]string, error) { + if args[0] == "echo" && len(args) > 2 { + return nil, fmt.Errorf("refusing to run echo builtin with multiple args") + } + return args, nil + }), }, src: "echo foo; echo foo bar", want: "foo\nrefusing to run echo builtin with multiple args", }, { - name: "GlobForbid", - readdir: blocklistGlob, - src: "echo *", - want: "blocklisted: glob\n", + name: "GlobForbid", + opts: []interp.RunnerOption{ + interp.ReadDirHandler(blocklistGlob), + }, + src: "echo *", + want: "blocklisted: glob\n", }, } @@ -160,21 +227,12 @@ func TestRunnerHandlers(t *testing.T) { file := parse(t, p, tc.src) var cb concBuffer r, err := interp.New(interp.StdIO(nil, &cb, &cb)) - if tc.exec != nil { - interp.ExecHandler(tc.exec)(r) - } - if tc.open != nil { - interp.OpenHandler(tc.open)(r) - } - if tc.call != nil { - interp.CallHandler(tc.call)(r) - } - if tc.readdir != nil { - interp.ReadDirHandler(tc.readdir)(r) - } if err != nil { t.Fatal(err) } + for _, opt := range tc.opts { + opt(r) + } ctx := context.WithValue(context.Background(), runnerCtx, r) if err := r.Run(ctx, file); err != nil { cb.WriteString(err.Error()) diff --git a/interp/interp_test.go b/interp/interp_test.go index 2351dcfa1..6cfb9b63b 100644 --- a/interp/interp_test.go +++ b/interp/interp_test.go @@ -110,7 +110,7 @@ func TestMain(m *testing.M) { runner, _ := interp.New( interp.StdIO(os.Stdin, os.Stdout, os.Stderr), interp.OpenHandler(testOpenHandler), - interp.ExecHandler(testExecHandler), + interp.ExecHandlers(testExecHandler), ) ctx := context.Background() if err := runner.Run(ctx, file); err != nil { @@ -3407,7 +3407,7 @@ func TestRunnerRun(t *testing.T) { // interp.Env(expand.ListEnviron(append(os.Environ(), // "FOO_INTERP_MISSING_NULL_BAR_INTERP_MISSING=foo_interp_missing\x00bar_interp_missing")...)), interp.OpenHandler(testOpenHandler), - interp.ExecHandler(testExecHandler), + interp.ExecHandlers(testExecHandler), ) if err != nil { t.Fatal(err) @@ -3683,11 +3683,13 @@ var testBuiltinsMap = map[string]func(interp.HandlerContext, []string) error{ }, } -func testExecHandler(ctx context.Context, args []string) error { - if fn := testBuiltinsMap[args[0]]; fn != nil { - return fn(interp.HandlerCtx(ctx), args[1:]) +func testExecHandler(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + if fn := testBuiltinsMap[args[0]]; fn != nil { + return fn(interp.HandlerCtx(ctx), args[1:]) + } + return next(ctx, args) } - return interp.DefaultExecHandler(2*time.Second)(ctx, args) } func testOpenHandler(ctx context.Context, path string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) { @@ -3850,7 +3852,7 @@ func TestRunnerOpts(t *testing.T) { r, err := interp.New(append(c.opts, interp.StdIO(nil, &cb, &cb), interp.OpenHandler(testOpenHandler), - interp.ExecHandler(testExecHandler), + interp.ExecHandlers(testExecHandler), )...) if err != nil { t.Fatal(err) @@ -4064,7 +4066,7 @@ func TestRunnerResetFields(t *testing.T) { interp.Params("-f", "--", "first", tdir, logPath), interp.Dir(tdir), interp.OpenHandler(testOpenHandler), - interp.ExecHandler(testExecHandler), + interp.ExecHandlers(testExecHandler), ) // Check that using option funcs and Runner fields directly is still // kept by Reset.