From 246f1a2b6b57714a56c5ac3f321399d49243a4eb Mon Sep 17 00:00:00 2001 From: Samuel Manzer Date: Mon, 20 Nov 2023 16:51:19 -0800 Subject: [PATCH] Args: Add `path` arg type to fix working directory handling (#442) Summary: Pull Request resolved: https://github.com/facebookincubator/TTPForge/pull/442 * Add new argument type `path:` * This makes relative paths work correctly when passed as arguments * Previously, if you passed `--arg myarg=foo/bar` the behavior was busted because the TTP will change its working directory to the parent directory of the TTP's YAML file, and `foo/bar` won't exist there, even if it existed in the directory from which you invoked ttpforge :( * Updated all examples to use the new arguments Reviewed By: d0n601 Differential Revision: D51475492 fbshipit-source-id: 4dea0a1b0415ab39580ace902e3bd6e75aab315e --- cmd/run_test.go | 138 +++++++++++++++--- .../test-repo/ttps/args/path/with-path.yaml | 15 ++ .../ttps/args/path/without-path.yaml | 14 ++ .../actions/edit_file/append_delete.yaml | 1 + example-ttps/actions/edit_file/replace.yaml | 1 + .../introduction/dotfile-backdoor-demo.yaml | 6 + example-ttps/tests/with_args.yaml | 1 + pkg/args/path_test.go | 109 ++++++++++++++ pkg/args/spec.go | 8 + pkg/fileutils/fileutils.go | 10 ++ 10 files changed, 281 insertions(+), 22 deletions(-) create mode 100644 cmd/test-resources/repos/test-repo/ttps/args/path/with-path.yaml create mode 100644 cmd/test-resources/repos/test-repo/ttps/args/path/without-path.yaml create mode 100644 pkg/args/path_test.go diff --git a/cmd/run_test.go b/cmd/run_test.go index ab636bf2..b53d5dc8 100644 --- a/cmd/run_test.go +++ b/cmd/run_test.go @@ -21,26 +21,50 @@ package cmd import ( "bytes" + "os" "path/filepath" "testing" + "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +const ( + testResourcesDir = "test-resources" + testRepoName = "test-repo" +) + +type runCmdTestCase struct { + name string + description string + args []string + expectedStdout string + wantError bool +} + +func checkRunCmdTestCase(t *testing.T, tc runCmdTestCase) { + var stdoutBuf, stderrBuf bytes.Buffer + rc := BuildRootCommand(&TestConfig{ + Stdout: &stdoutBuf, + Stderr: &stderrBuf, + }) + rc.SetArgs(append([]string{"run"}, tc.args...)) + err := rc.Execute() + if tc.wantError { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tc.expectedStdout, stdoutBuf.String()) + +} + func TestRun(t *testing.T) { - const testResourcesDir = "test-resources" - const testRepoName = "test-repo" testConfigFilePath := filepath.Join(testResourcesDir, "test-config.yaml") - testCases := []struct { - name string - description string - args []string - expectedStdout string - wantError bool - }{ + testCases := []runCmdTestCase{ { name: "file-step", description: "check that a regular file step works", @@ -127,19 +151,89 @@ func TestRun(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - var stdoutBuf, stderrBuf bytes.Buffer - rc := BuildRootCommand(&TestConfig{ - Stdout: &stdoutBuf, - Stderr: &stderrBuf, - }) - rc.SetArgs(append([]string{"run"}, tc.args...)) - err := rc.Execute() - if tc.wantError { - require.Error(t, err) - } else { - require.NoError(t, err) - } - assert.Equal(t, tc.expectedStdout, stdoutBuf.String()) + checkRunCmdTestCase(t, tc) + }) + } +} + +// TestRunPathArguments checks that +// referencing relative paths in `--arg` values +// when executing `ttpforge run` works as expected. +// One typically needs to specify `type: path` in +// the argument specificiation in order to get desired +// behavior. +func TestRunPathArguments(t *testing.T) { + // in this test, we initially execute every test case from a + // temporary directory, so we need the absolute path to the config + testConfigFilePath, err := filepath.Abs(filepath.Join(testResourcesDir, "test-config.yaml")) + require.NoError(t, err) + // the file we will read from in the test cases + targetFileName := "path-test.txt" + + // setup dummy file(s) for our test cases + fsys := afero.NewOsFs() + targetDir, err := afero.TempDir(fsys, "", "ttpforge-run-test") + targetAbsPath := filepath.Join(targetDir, targetFileName) + require.NoError(t, err) + f, err := fsys.Create(targetAbsPath) + require.NoError(t, err) + defer fsys.Remove(targetFileName) + f.Write([]byte("It worked!\n")) + + testCases := []runCmdTestCase{ + { + name: "Argument with `type: path` - Should Fail", + description: "This should fail because the TTP's argument spec does not have `type: path`", + args: []string{ + "-c", + testConfigFilePath, + testRepoName + "//args/path/without-path.yaml", + "--arg", + "target_path=" + targetFileName, + }, + wantError: true, + }, + { + name: "Argument with `type: path` - Should Succeed", + description: "This should succeed because the TTP's argument spec has `type: path`", + args: []string{ + "-c", + testConfigFilePath, + testRepoName + "//args/path/with-path.yaml", + "--arg", + "target_path=" + targetFileName, + }, + expectedStdout: "It worked!\n", + }, + { + name: "Argument with `type: path` - Absolute Path - Should Succeed", + description: "Extra test case to check that `type: path` works with absolute paths", + args: []string{ + "-c", + testConfigFilePath, + testRepoName + "//args/path/with-path.yaml", + "--arg", + "target_path=" + targetAbsPath, + }, + expectedStdout: "It worked!\n", + }, + } + + // change directory to the target dir so we can use relative paths + // to refer to the target file + wd, err := os.Getwd() + require.NoError(t, err) + err = os.Chdir(targetDir) + require.NoError(t, err) + defer func() { + if err := os.Chdir(wd); err != nil { + panic(err) + } + }() + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + checkRunCmdTestCase(t, tc) }) } } diff --git a/cmd/test-resources/repos/test-repo/ttps/args/path/with-path.yaml b/cmd/test-resources/repos/test-repo/ttps/args/path/with-path.yaml new file mode 100644 index 00000000..dc868409 --- /dev/null +++ b/cmd/test-resources/repos/test-repo/ttps/args/path/with-path.yaml @@ -0,0 +1,15 @@ +name: Testing Path Type Arguments +description: | + This TTP powers the test case + "Argument with `type: path` - Should Succeed" + in `cmd/run_test.go` +args: + - name: target_path + description: | + If a relative path is provided for this argument, it will + be expanded to an absolute path based on the user's current + working directory, NOT the configuration directory of the TTP. + type: path +steps: + - name: cat_target_path + inline: cat {{.Args.target_path}} diff --git a/cmd/test-resources/repos/test-repo/ttps/args/path/without-path.yaml b/cmd/test-resources/repos/test-repo/ttps/args/path/without-path.yaml new file mode 100644 index 00000000..3b6af8d5 --- /dev/null +++ b/cmd/test-resources/repos/test-repo/ttps/args/path/without-path.yaml @@ -0,0 +1,14 @@ +name: Testing Path Type Arguments - Failure +description: | + This TTP powers the test case + "Argument with `type: path` - Should Fail" + in `cmd/run_test.go` +args: + - name: target_path + description: | + If a valid relative path is provided for this argument, the TTP + will still fail because it will run from the TTP's configuration + directory and that path won't exist there +steps: + - name: cat_target_path + inline: cat {{.Args.target_path}} diff --git a/example-ttps/actions/edit_file/append_delete.yaml b/example-ttps/actions/edit_file/append_delete.yaml index 977882b0..812dbcfb 100644 --- a/example-ttps/actions/edit_file/append_delete.yaml +++ b/example-ttps/actions/edit_file/append_delete.yaml @@ -5,6 +5,7 @@ description: | with the edit_file action. args: - name: test_file_path + type: path description: The path at which the temporary test file should be created default: /tmp/ttpforge_edit_file_append_delete steps: diff --git a/example-ttps/actions/edit_file/replace.yaml b/example-ttps/actions/edit_file/replace.yaml index 2ce45f57..b9750240 100644 --- a/example-ttps/actions/edit_file/replace.yaml +++ b/example-ttps/actions/edit_file/replace.yaml @@ -6,6 +6,7 @@ description: | or regexp matches with new strings args: - name: test_file_path + type: path description: The path at which the temporary test file should be created default: /tmp/ttpforge_edit_file_replace steps: diff --git a/example-ttps/introduction/dotfile-backdoor-demo.yaml b/example-ttps/introduction/dotfile-backdoor-demo.yaml index 23ecec0a..5900eed2 100644 --- a/example-ttps/introduction/dotfile-backdoor-demo.yaml +++ b/example-ttps/introduction/dotfile-backdoor-demo.yaml @@ -6,11 +6,16 @@ description: | - Simple but powerful Command-line Argument Support - Last-in-First-Out Cleanup Execution - Checking Conditions at Runtime to Avoid Errors +tests: + - name: default + description: execute with the default setting args: - name: target_file_path + type: path description: The file that we should try to backdoor default: ~/.zshrc - name: payload_file_path + type: path description: | The path to which we should write the payload file. The backdoor we insert into the target file will reference this @@ -21,6 +26,7 @@ args: The shell command that our payload should execute default: echo 'Hello from TTPForge! You have been pwned!' - name: backup_file_path + type: path description: | The file path to which the target file should be backed up default: /tmp/ttpforge-dotfile-backdoor-backup diff --git a/example-ttps/tests/with_args.yaml b/example-ttps/tests/with_args.yaml index f4c1cc71..81b4e2b5 100644 --- a/example-ttps/tests/with_args.yaml +++ b/example-ttps/tests/with_args.yaml @@ -22,6 +22,7 @@ tests: contents: this will not be printed args: - name: target_file_path + type: path description: the path of the file to create - name: contents description: the contents to write to the target file diff --git a/pkg/args/path_test.go b/pkg/args/path_test.go new file mode 100644 index 00000000..10bc186b --- /dev/null +++ b/pkg/args/path_test.go @@ -0,0 +1,109 @@ +/* +Copyright © 2023-present, Meta Platforms, Inc. and affiliates +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: +The above copyright notice and this permission notice shall be included in +all copies or substantial portions of the Software. +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +THE SOFTWARE. +*/ + +package args + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestValidateArgsPath(t *testing.T) { + + // used in some test cases + homedir, err := os.UserHomeDir() + require.NoError(t, err) + + // cd into this directory and use it to verify + // that relative paths are expanded appropriately + tmpDir, err := os.MkdirTemp("", "ttpforge-testing") + require.NoError(t, err) + defer os.RemoveAll(tmpDir) + wd, err := os.Getwd() + require.NoError(t, err) + err = os.Chdir(tmpDir) + require.NoError(t, err) + defer func() { + // this will break a bunch of other tests so be loud about it + if err := os.Chdir(wd); err != nil { + panic(err) + } + }() + // macOS /private/var/... shenanigans + tmpDir, err = filepath.EvalSymlinks(tmpDir) + require.NoError(t, err) + + testCases := []validateTestCase{ + { + name: "Absolute Path", + specs: []Spec{ + { + Name: "mypath", + Type: "path", + }, + }, + argKvStrs: []string{ + "mypath=/tmp/foo", + }, + expectedResult: map[string]any{ + "mypath": "/tmp/foo", + }, + }, + { + name: "Path Containing Tilde", + specs: []Spec{ + { + Name: "mypath", + Type: "path", + }, + }, + argKvStrs: []string{ + "mypath=~/wut", + }, + expectedResult: map[string]any{ + "mypath": filepath.Join(homedir, "wut"), + }, + }, + { + name: "Relative Path", + specs: []Spec{ + { + Name: "mypath", + Type: "path", + }, + }, + argKvStrs: []string{ + "mypath=" + filepath.Join("foo", "bar"), + }, + // can't do a strict equality 0 + expectedResult: map[string]any{ + "mypath": filepath.Join(tmpDir, "foo", "bar"), + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + checkValidateTestCase(t, tc) + }) + } +} diff --git a/pkg/args/spec.go b/pkg/args/spec.go index 30dba44f..ace1dfa9 100644 --- a/pkg/args/spec.go +++ b/pkg/args/spec.go @@ -25,6 +25,8 @@ import ( "regexp" "strconv" "strings" + + "github.com/facebookincubator/ttpforge/pkg/fileutils" ) // Spec defines a CLI argument for the TTP @@ -161,6 +163,12 @@ func (spec Spec) convertArgToType(val string) (any, error) { return nil, errors.New("no-boolean value provided") } return asBool, nil + case "path": + absPath, err := fileutils.AbsPath(val) + if err != nil { + return nil, fmt.Errorf("failed to process argument of type `path`: %w", err) + } + return absPath, nil default: return nil, fmt.Errorf("invalid type %v specified in configuration for argument %v", spec.Type, spec.Name) } diff --git a/pkg/fileutils/fileutils.go b/pkg/fileutils/fileutils.go index baf5094a..de0a9b27 100644 --- a/pkg/fileutils/fileutils.go +++ b/pkg/fileutils/fileutils.go @@ -36,3 +36,13 @@ func ExpandTilde(path string) (string, error) { } return path, nil } + +// AbsPath is a thin wrapper around filepath.Abs +// that we use because it also calls ExpandTilde +func AbsPath(path string) (string, error) { + tmp, err := ExpandTilde(path) + if err != nil { + return "", err + } + return filepath.Abs(tmp) +}