Validate directory flag. Unlock on preexecute err.

Ensure -d flag uses relative dirs and doesn't allow for
directory traversal.

Fix bug where if there was an error in PreExecute, we
wouldn't unlock, leaving a possibly abandoned lock.

server/events/apply_executor.go

@@ -46,22 +46,39 @@ func (a *ApplyExecutor) Execute(ctx *CommandContext) CommandResponse {
 	// Plans are stored at project roots by their workspace names. We just
 	// need to find them.
 	var plans []models.Plan
-	err = filepath.Walk(repoDir, func(path string, info os.FileInfo, err error) error {
+	// If they didn't specify a directory, we apply all plans we can find for
+	// this workspace.
+	if ctx.Command.Dir == "" {
+		err = filepath.Walk(repoDir, func(path string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+			// Check if the plan is for the right workspace,
+			if !info.IsDir() && info.Name() == ctx.Command.Workspace+".tfplan" {
+				rel, _ := filepath.Rel(repoDir, filepath.Dir(path))
+				plans = append(plans, models.Plan{
+					Project:   models.NewProject(ctx.BaseRepo.FullName, rel),
+					LocalPath: path,
+				})
+			}
+			return nil
+		})
 		if err != nil {
-			return err
+			return CommandResponse{Error: errors.Wrap(err, "finding plans")}
 		}
-		// Check if the plan is for the right workspace,
-		if !info.IsDir() && info.Name() == ctx.Command.Workspace+".tfplan" {
-			rel, _ := filepath.Rel(repoDir, filepath.Dir(path))
-			plans = append(plans, models.Plan{
-				Project:   models.NewProject(ctx.BaseRepo.FullName, rel),
-				LocalPath: path,
-			})
+	} else {
+		// If they did specify a dir, we apply just the plan in that directory
+		// for this workspace.
+		path := filepath.Join(repoDir, ctx.Command.Dir, ctx.Command.Workspace+".tfplan")
+		stat, err := os.Stat(path)
+		if err != nil || stat.IsDir() {
+			return CommandResponse{Error: errors.Wrapf(err, "finding plan for dir %q and workspace %q", ctx.Command.Dir, ctx.Command.Workspace)}
 		}
-		return nil
-	})
-	if err != nil {
-		return CommandResponse{Error: errors.Wrap(err, "finding plans")}
+		rel, _ := filepath.Rel(repoDir, filepath.Dir(path))
+		plans = append(plans, models.Plan{
+			Project:   models.NewProject(ctx.BaseRepo.FullName, filepath.Dir(rel)),
+			LocalPath: path,
+		})
 	}
 	if len(plans) == 0 {
 		return CommandResponse{Failure: "No plans found for that workspace."}

server/events/event_parser.go

@@ -3,6 +3,7 @@ package events
 import (
 	"errors"
 	"fmt"
+	"path/filepath"
 	"strings"
 
 	"github.com/google/go-github/github"
@@ -21,7 +22,10 @@ type Command struct {
 	Workspace string
 	Verbose   bool
 	Flags     []string
-	Dir       string
+	// Dir is the path relative to the repo root to run the command in.
+	// If empty string then it wasn't specified. "." is the root of the repo.
+	// Dir will never end in "/".
+	Dir string
 }
 
 type EventParsing interface {
@@ -86,19 +90,19 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma
 	var name CommandName
 
 	// Set up the flag parsing depending on the command.
+	const defaultWorkspace = "default"
 	if command == "plan" {
 		name = Plan
 		flagSet = pflag.NewFlagSet("plan", pflag.ContinueOnError)
-		flagSet.StringVarP(&workspace, "workspace", "w", "default", "Switch to this Terraform workspace before planning.")
-		flagSet.StringVarP(&dir, "dir", "d", ".", "Which directory to run plan in. Defaults to the root of the repo.")
+		flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, fmt.Sprintf("Switch to this Terraform workspace before planning. Defaults to '%s'", defaultWorkspace))
+		flagSet.StringVarP(&dir, "dir", "d", "", "Which directory to run plan in relative to root of repo. Use '.' for root. If not specified, will attempt to run plan for all Terraform projects we think were modified in this changeset.")
 		flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.")
 	} else if command == "apply" {
 		name = Apply
 		flagSet = pflag.NewFlagSet("apply", pflag.ContinueOnError)
-		flagSet.StringVarP(&workspace, "workspace", "w", "default", "Apply the plan for this Terraform workspace.")
-		flagSet.StringVarP(&dir, "dir", "d", ".", "Run apply in this directory. Defaults to the root of the repo.")
+		flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, fmt.Sprintf("Apply the plan for this Terraform workspace. Defaults to '%s'", defaultWorkspace))
+		flagSet.StringVarP(&dir, "dir", "d", "", "Run apply in this directory relative to root of repo. Use '.' for root. If not specified, will run apply against all plans created for this workspace.")
 		flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.")
-
 	} else {
 		return nil, fmt.Errorf("unknown command %q – this is a bug", command)
 	}
@@ -116,7 +120,26 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma
 		extraArgs = flagSet.Args()[flagSet.ArgsLenAtDash():]
 	}
 
-	// todo: validate args
+	// If dir is specified, must ensure it's a valid path.
+	if dir != "" {
+		validatedDir := filepath.Clean(dir)
+		// Join with . so the path is relative. This helps us if they use '/',
+		// and is safe to do if their path is relative since it's a no-op.
+		validatedDir = filepath.Join(".", validatedDir)
+		// Need to clean again to resolve relative validatedDirs.
+		validatedDir = filepath.Clean(validatedDir)
+		// Detect relative dirs since they're not allowed.
+		if strings.HasPrefix(validatedDir, "..") {
+			return nil, fmt.Errorf("relative path %q not allowed", dir)
+		}
+
+		dir = validatedDir
+	}
+	// Because we use the workspace name as a file, need to make sure it's
+	// not doing something weird like being a relative dir.
+	if strings.Contains(workspace, "..") {
+		return nil, errors.New("workspace can't contain '..'")
+	}
 
 	c := &Command{Name: name, Verbose: verbose, Workspace: workspace, Dir: dir, Flags: extraArgs}
 	return c, nil
@@ -321,14 +344,3 @@ func (e *EventParser) stringInSlice(a string, list []string) bool {
 	}
 	return false
 }
-
-// nolint: unparam
-func (e *EventParser) removeOccurrences(a string, list []string) []string {
-	var out []string
-	for _, b := range list {
-		if b != a {
-			out = append(out, b)
-		}
-	}
-	return out
-}

server/events/event_parser_test.go

@@ -36,6 +36,15 @@ func TestDetermineCommandInvalid(t *testing.T) {
 		"atlantis slkjd",
 		"@github-user slkjd",
 		"atlantis plans",
+		// relative dirs
+		"atlantis plan -d ..",
+		"atlantis plan -d ../",
+		"atlantis plan -d a/../../",
+		// using .. in workspace
+		"atlantis plan -w a..",
+		"atlantis plan -w ../",
+		"atlantis plan -w ..",
+		"atlantis plan -w a/../b",
 		// misc
 		"related comment mentioning atlantis",
 	}
@@ -94,7 +103,7 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 		{
 			"",
 			"default",
-			".",
+			"",
 			false,
 			"",
 		},
@@ -102,7 +111,7 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 		{
 			"-w workspace",
 			"workspace",
-			".",
+			"",
 			false,
 			"",
 		},
@@ -116,7 +125,7 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 		{
 			"--verbose",
 			"default",
-			".",
+			"",
 			true,
 			"",
 		},
@@ -153,7 +162,7 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 		{
 			"-w workspace -- -d dir --verbose",
 			"workspace",
-			".",
+			"",
 			false,
 			"-d dir --verbose",
 		},
@@ -161,7 +170,7 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 		{
 			"-w -d dir --verbose",
 			"-d",
-			".",
+			"",
 			true,
 			"",
 		},
@@ -169,14 +178,14 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 		{
 			"--",
 			"default",
-			".",
+			"",
 			false,
 			"",
 		},
 		{
 			"abc --",
 			"default",
-			".",
+			"",
 			false,
 			"",
 		},
@@ -202,6 +211,42 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 			true,
 			"arg one -two --three &&",
 		},
+		// Test that the dir string is normalized.
+		{
+			"-d /",
+			"default",
+			".",
+			false,
+			"",
+		},
+		{
+			"-d /adir",
+			"default",
+			"adir",
+			false,
+			"",
+		},
+		{
+			"-d .",
+			"default",
+			".",
+			false,
+			"",
+		},
+		{
+			"-d ./",
+			"default",
+			".",
+			false,
+			"",
+		},
+		{
+			"-d ./adir",
+			"default",
+			"adir",
+			false,
+			"",
+		},
 	}
 	for _, test := range cases {
 		for _, cmdName := range []string{"plan", "apply"} {
@@ -437,15 +482,6 @@ func TestParseGitlabMergeCommentEvent(t *testing.T) {
 	}, user)
 }
 
-func containsVerbose(list []string) bool {
-	for _, b := range list {
-		if b == "--verbose" {
-			return true
-		}
-	}
-	return false
-}
-
 var mergeEventJSON = `{
   "object_kind": "merge_request",
   "user": {

server/events/plan_executor.go

@@ -52,21 +52,29 @@ func (p *PlanExecutor) SetLockURL(f func(id string) (url string)) {
 
 // Execute executes terraform plan for the ctx.
 func (p *PlanExecutor) Execute(ctx *CommandContext) CommandResponse {
-	// Figure out what projects have been modified so we know where to run plan.
-	modifiedFiles, err := p.VCSClient.GetModifiedFiles(ctx.BaseRepo, ctx.Pull, ctx.VCSHost)
-	if err != nil {
-		return CommandResponse{Error: errors.Wrap(err, "getting modified files")}
-	}
-
 	cloneDir, err := p.Workspace.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, ctx.Command.Workspace)
 	if err != nil {
 		return CommandResponse{Error: err}
 	}
 
-	ctx.Log.Info("found %d files modified in this pull request", len(modifiedFiles))
-	projects := p.ProjectFinder.DetermineProjects(ctx.Log, modifiedFiles, ctx.BaseRepo.FullName, cloneDir)
-	if len(projects) == 0 {
-		return CommandResponse{Failure: "No Terraform files were modified."}
+	var projects []models.Project
+	if ctx.Command.Dir == "" {
+		// If they didn't specify a directory to plan in, figure out what
+		// projects have been modified so we know where to run plan.
+		modifiedFiles, err := p.VCSClient.GetModifiedFiles(ctx.BaseRepo, ctx.Pull, ctx.VCSHost)
+		if err != nil {
+			return CommandResponse{Error: errors.Wrap(err, "getting modified files")}
+		}
+		ctx.Log.Info("found %d files modified in this pull request", len(modifiedFiles))
+		projects = p.ProjectFinder.DetermineProjects(ctx.Log, modifiedFiles, ctx.BaseRepo.FullName, cloneDir)
+		if len(projects) == 0 {
+			return CommandResponse{Failure: "No Terraform files were modified."}
+		}
+	} else {
+		projects = []models.Project{{
+			Path:         ctx.Command.Dir,
+			RepoFullName: ctx.BaseRepo.FullName,
+		}}
 	}
 
 	var results []ProjectResult

server/events/plan_executor_test.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/mohae/deepcopy"
 	. "github.com/petergtz/pegomock"
 	"github.com/runatlantis/atlantis/server/events"
 	"github.com/runatlantis/atlantis/server/events/locking"
@@ -22,6 +23,7 @@ var planCtx = events.CommandContext{
 	Command: &events.Command{
 		Name:      events.Plan,
 		Workspace: "workspace",
+		Dir:       "",
 	},
 	Log:      logging.NewNoopLogger(),
 	BaseRepo: models.Repo{},
@@ -63,6 +65,38 @@ func TestExecute_CloneErr(t *testing.T) {
 	Equals(t, "err", r.Error.Error())
 }
 
+func TestExecute_DirectoryAndWorkspaceSet(t *testing.T) {
+	t.Log("Test that we run plan in the right directory and workspace if they're set")
+	p, runner, _ := setupPlanExecutorTest(t)
+	ctx := deepcopy.Copy(planCtx).(events.CommandContext)
+	ctx.Log = logging.NewNoopLogger()
+	ctx.Command.Dir = "dir1/dir2"
+	ctx.Command.Workspace = "workspace-flag"
+
+	When(p.Workspace.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, "workspace-flag")).
+		ThenReturn("/tmp/clone-repo", nil)
+	When(p.ProjectPreExecute.Execute(&ctx, "/tmp/clone-repo", models.Project{RepoFullName: "", Path: "dir1/dir2"})).
+		ThenReturn(events.PreExecuteResult{
+			LockResponse: locking.TryLockResponse{
+				LockKey: "key",
+			},
+		})
+	r := p.Execute(&ctx)
+
+	runner.VerifyWasCalledOnce().RunCommandWithVersion(
+		ctx.Log,
+		"/tmp/clone-repo/dir1/dir2",
+		[]string{"plan", "-refresh", "-no-color", "-out", "/tmp/clone-repo/dir1/dir2/workspace-flag.tfplan", "-var", "atlantis_user=anubhavmishra"},
+		nil,
+		"workspace-flag",
+	)
+	Assert(t, len(r.ProjectResults) == 1, "exp one project result")
+	result := r.ProjectResults[0]
+	Assert(t, result.PlanSuccess != nil, "exp plan success to not be nil")
+	Equals(t, "", result.PlanSuccess.TerraformOutput)
+	Equals(t, "lockurl-key", result.PlanSuccess.LockURL)
+}
+
 func TestExecute_Success(t *testing.T) {
 	t.Log("If there are no errors, the plan should be returned")
 	p, runner, _ := setupPlanExecutorTest(t)

server/events/project_pre_execute.go

@@ -51,14 +51,28 @@ func (p *DefaultProjectPreExecutor) Execute(ctx *CommandContext, repoDir string,
 			lockAttempt.CurrLock.Pull.Num)}}
 	}
 	ctx.Log.Info("acquired lock with id %q", lockAttempt.LockKey)
+	config, tfVersion, err := p.executeWithLock(ctx, repoDir, project)
+	if err != nil {
+		p.Locker.Unlock(lockAttempt.LockKey) // nolint: errcheck
+		return PreExecuteResult{ProjectResult: ProjectResult{Error: err}}
+	}
+	return PreExecuteResult{ProjectConfig: config, TerraformVersion: tfVersion, LockResponse: lockAttempt}
+}
+
+// executeWithLock executes the pre plan/apply tasks after the lock has been
+// acquired. This helper func makes revoking the lock on error easier.
+// Returns the project config, terraform version, or an error.
+func (p *DefaultProjectPreExecutor) executeWithLock(ctx *CommandContext, repoDir string, project models.Project) (ProjectConfig, *version.Version, error) {
+	workspace := ctx.Command.Workspace
 
 	// Check if config file is found, if not we continue the run.
 	var config ProjectConfig
 	absolutePath := filepath.Join(repoDir, project.Path)
 	if p.ConfigReader.Exists(absolutePath) {
+		var err error
 		config, err = p.ConfigReader.Read(absolutePath)
 		if err != nil {
-			return PreExecuteResult{ProjectResult: ProjectResult{Error: err}}
+			return config, nil, err
 		}
 		ctx.Log.Info("parsed atlantis config file in %q", absolutePath)
 	}
@@ -74,25 +88,25 @@ func (p *DefaultProjectPreExecutor) Execute(ctx *CommandContext, repoDir string,
 		if len(config.PreInit) > 0 {
 			_, err := p.Run.Execute(ctx.Log, config.PreInit, absolutePath, workspace, terraformVersion, "pre_init")
 			if err != nil {
-				return PreExecuteResult{ProjectResult: ProjectResult{Error: errors.Wrapf(err, "running %s commands", "pre_init")}}
+				return config, nil, errors.Wrapf(err, "running %s commands", "pre_init")
 			}
 		}
 		_, err := p.Terraform.Init(ctx.Log, absolutePath, workspace, config.GetExtraArguments("init"), terraformVersion)
 		if err != nil {
-			return PreExecuteResult{ProjectResult: ProjectResult{Error: err}}
+			return config, nil, err
 		}
 	} else {
 		ctx.Log.Info("determined that we are running terraform with version < 0.9.0. Running version %s", terraformVersion)
 		if len(config.PreGet) > 0 {
 			_, err := p.Run.Execute(ctx.Log, config.PreGet, absolutePath, workspace, terraformVersion, "pre_get")
 			if err != nil {
-				return PreExecuteResult{ProjectResult: ProjectResult{Error: errors.Wrapf(err, "running %s commands", "pre_get")}}
+				return config, nil, errors.Wrapf(err, "running %s commands", "pre_get")
 			}
 		}
 		terraformGetCmd := append([]string{"get", "-no-color"}, config.GetExtraArguments("get")...)
 		_, err := p.Terraform.RunCommandWithVersion(ctx.Log, absolutePath, terraformGetCmd, terraformVersion, workspace)
 		if err != nil {
-			return PreExecuteResult{ProjectResult: ProjectResult{Error: err}}
+			return config, nil, err
 		}
 	}
 
@@ -106,8 +120,8 @@ func (p *DefaultProjectPreExecutor) Execute(ctx *CommandContext, repoDir string,
 	if len(commands) > 0 {
 		_, err := p.Run.Execute(ctx.Log, commands, absolutePath, workspace, terraformVersion, stage)
 		if err != nil {
-			return PreExecuteResult{ProjectResult: ProjectResult{Error: errors.Wrapf(err, "running %s commands", stage)}}
+			return config, nil, errors.Wrapf(err, "running %s commands", stage)
 		}
 	}
-	return PreExecuteResult{ProjectConfig: config, TerraformVersion: terraformVersion, LockResponse: lockAttempt}
+	return config, terraformVersion, nil
 }