Prevent git commands in dangerous dirs (#222)

* Prevent git commands in dangerous dirs

* Try E2E

* Go fix

* Fix vendoring and lint

* Tweak message

* lint

* Tweak message

* Fix fmt

* Add escape hatch

e2e/bitrise.yml

@@ -502,6 +502,26 @@ workflows:
     - _check_outputs
     - _teardown
 
+  test_dangerous_clone_dir:
+    steps:
+    - script:
+        inputs:
+        - content: |-
+            #/bin/env bash
+            set -x
+            bitrise run --config=./e2e/bitrise.yml utility_dangerous_clone_dir
+            if [ $? == 0 ]; then
+              exit 1
+            fi
+
+  utility_dangerous_clone_dir:
+    envs:
+    - TEST_REPO_URL: https://github.com/websitebot/git-repo-fixture-private.git
+    - BRANCH: main
+    - WORKDIR: $HOME
+    after_run:
+    - _run
+
   _check_changelog:
     steps:
     - generate-changelog:

main.go

@@ -10,6 +10,7 @@ import (
 	"github.com/bitrise-io/go-utils/v2/errorutil"
 	. "github.com/bitrise-io/go-utils/v2/exitcode"
 	"github.com/bitrise-io/go-utils/v2/log"
+	"github.com/bitrise-io/go-utils/v2/pathutil"
 	"github.com/bitrise-steplib/steps-git-clone/gitclone/tracker"
 	"github.com/bitrise-steplib/steps-git-clone/step"
 )
@@ -53,6 +54,7 @@ func createStep(logger log.Logger) step.GitCloneStep {
 	tracker := tracker.NewStepTracker(envRepo, logger)
 	inputParser := stepconf.NewInputParser(envRepo)
 	cmdFactory := command.NewFactory(envRepo)
+	pathModififer := pathutil.NewPathModifier()
 
-	return step.NewGitCloneStep(logger, tracker, inputParser, cmdFactory)
+	return step.NewGitCloneStep(logger, tracker, inputParser, envRepo, cmdFactory, pathModififer)
 }

step/step.go

@@ -6,7 +6,10 @@ import (
 	"github.com/bitrise-io/go-steputils/v2/stepconf"
 	"github.com/bitrise-io/go-utils/retry"
 	"github.com/bitrise-io/go-utils/v2/command"
+	"github.com/bitrise-io/go-utils/v2/env"
 	"github.com/bitrise-io/go-utils/v2/log"
+	"github.com/bitrise-io/go-utils/v2/log/colorstring"
+	"github.com/bitrise-io/go-utils/v2/pathutil"
 	"github.com/bitrise-steplib/steps-git-clone/gitclone"
 	"github.com/bitrise-steplib/steps-git-clone/gitclone/bitriseapi"
 	"github.com/bitrise-steplib/steps-git-clone/gitclone/tracker"
@@ -47,18 +50,22 @@ type Config struct {
 }
 
 type GitCloneStep struct {
-	logger      log.Logger
-	tracker     tracker.StepTracker
-	inputParser stepconf.InputParser
-	cmdFactory  command.Factory
+	logger       log.Logger
+	tracker      tracker.StepTracker
+	inputParser  stepconf.InputParser
+	envRepo      env.Repository
+	cmdFactory   command.Factory
+	pathModifier pathutil.PathModifier
 }
 
-func NewGitCloneStep(logger log.Logger, tracker tracker.StepTracker, inputParser stepconf.InputParser, cmdFactory command.Factory) GitCloneStep {
+func NewGitCloneStep(logger log.Logger, tracker tracker.StepTracker, inputParser stepconf.InputParser, envRepo env.Repository, cmdFactory command.Factory, pathModifier pathutil.PathModifier) GitCloneStep {
 	return GitCloneStep{
-		logger:      logger,
-		tracker:     tracker,
-		inputParser: inputParser,
-		cmdFactory:  cmdFactory,
+		logger:       logger,
+		tracker:      tracker,
+		inputParser:  inputParser,
+		envRepo:      envRepo,
+		cmdFactory:   cmdFactory,
+		pathModifier: pathModifier,
 	}
 }
 
@@ -69,6 +76,20 @@ func (g GitCloneStep) ProcessConfig() (Config, error) {
 	}
 	stepconf.Print(input)
 
+	if g.isCloneDirDangerous(input.CloneIntoDir) && g.envRepo.Get("BITRISE_GIT_CLONE_FORCE_RUN") != "true" {
+		g.logger.Println()
+		g.logger.Println()
+		g.logger.Errorf("BEWARE: The git clone directory is set to %s", input.CloneIntoDir)
+		g.logger.Errorf("This is probably not what you want, as the step could overwrite files in the directory.")
+		g.logger.Printf("To update the path, you have a few options:")
+		g.logger.Printf("1. Change the %s step input", colorstring.Cyan("clone_into_dir"))
+		g.logger.Printf("2. If not specified, %s defaults to %s. Check the value of this env var.", colorstring.Cyan("clone_into_dir"), colorstring.Cyan("$BITRISE_SOURCE_DIR"))
+		g.logger.Printf("3. When using self-hosted agents, you can customize %s and other important values in the %s file.", colorstring.Cyan("$BITRISE_SOURCE_DIR"), colorstring.Cyan("~/.bitrise/agent-config.yml"))
+		g.logger.Printf("If you are sure you want to proceed, you can set the %s env var to force the step to run.", colorstring.Cyan("BITRISE_GIT_CLONE_FORCE_RUN=true"))
+
+		return Config{}, fmt.Errorf("dangerous clone directory detected")
+	}
+
 	return Config{input}, nil
 }
 
@@ -99,6 +120,46 @@ func (g GitCloneStep) ExportOutputs(runResult gitclone.CheckoutStateResult) erro
 	return nil
 }
 
+func (g GitCloneStep) isCloneDirDangerous(path string) bool {
+	blocklist := []string{
+		"~",
+		"~/Downloads",
+		"~/Documents",
+		"~/Desktop",
+		"/bin",
+		"/usr/bin",
+		"/etc",
+		"/Applications",
+		"/Library",
+		"~/Library",
+		"~/.config",
+		"~/.bitrise",
+		"~/.ssh",
+	}
+
+	absClonePath, err := g.pathModifier.AbsPath(path)
+	if err != nil {
+		g.logger.Warnf("Failed to get absolute path of clone directory: %s", err)
+		// The path could be incorrect for many reasons, but we don't want to cause a false positive.
+		// A true positive will be caught by the git command anyway.
+		return false
+	}
+
+	for _, dangerousPath := range blocklist {
+		absDangerousPath, err := g.pathModifier.AbsPath(dangerousPath)
+		if err != nil {
+			// Not all blocklisted paths are valid on all systems, so we ignore this error.
+			continue
+		}
+
+		if absClonePath == absDangerousPath {
+			return true
+		}
+	}
+
+	return false
+}
+
 func convertConfig(config Config) gitclone.Config {
 	return gitclone.Config{
 		ShouldMergePR:         config.ShouldMergePR,

step/step_test.go

@@ -0,0 +1,83 @@
+package step
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/bitrise-io/go-steputils/v2/stepconf"
+	"github.com/bitrise-io/go-utils/v2/command"
+	"github.com/bitrise-io/go-utils/v2/env"
+	"github.com/bitrise-io/go-utils/v2/log"
+	"github.com/bitrise-io/go-utils/v2/pathutil"
+	"github.com/bitrise-steplib/steps-git-clone/gitclone/tracker"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_GitCloneStep_IsCloneDirDangerous(t *testing.T) {
+	home, err := os.UserHomeDir()
+	require.NoError(t, err)
+
+	tests := []struct {
+		name     string
+		path     string
+		expected bool
+	}{
+		{
+			name:     "Safe path in temp dir",
+			path:     "$TMPDIR/clone",
+			expected: false,
+		},
+		{
+			name:     "Safe path in home",
+			path:     filepath.Join(home, "clone"),
+			expected: false,
+		},
+		{
+			name:     "Home as env var",
+			path:     "$HOME",
+			expected: true,
+		},
+		{
+			name:     "Home as tilde",
+			path:     "~",
+			expected: true,
+		},
+		{
+			name:     "Home as absolute path",
+			path:     home,
+			expected: true,
+		},
+		{
+			name:     "Dangerous path with tilde",
+			path:     "~/Documents",
+			expected: true,
+		},
+		{
+			name:     "Dangerous absolute path",
+			path:     filepath.Join(home, ".ssh"),
+			expected: true,
+		},
+		{
+			name:     "Nonexistent env var only",
+			path:     "$NONEXISTENT",
+			expected: false,
+		},
+	}
+
+	logger := log.NewLogger()
+	envRepo := env.NewRepository()
+	tracker := tracker.NewStepTracker(envRepo, logger)
+	inputParser := stepconf.NewInputParser(envRepo)
+	cmdFactory := command.NewFactory(envRepo)
+	pathModifier := pathutil.NewPathModifier()
+
+	gitCloneStep := NewGitCloneStep(logger, tracker, inputParser, envRepo, cmdFactory, pathModifier)
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result := gitCloneStep.isCloneDirDangerous(test.path)
+			require.Equal(t, test.expected, result)
+		})
+	}
+}