Faster comment feedback. Fix security issue.

server/events/command_handler.go

@@ -42,7 +42,6 @@ type GitlabMergeRequestGetter interface {
 type CommandHandler struct {
 	PlanExecutor             Executor
 	ApplyExecutor            Executor
-	HelpExecutor             Executor
 	LockURLGenerator         LockURLGenerator
 	VCSClient                vcs.ClientProxy
 	GithubPullGetter         GithubPullGetter
@@ -125,7 +124,7 @@ func (c *CommandHandler) run(ctx *CommandContext) {
 
 	if ctx.Pull.State != models.Open {
 		ctx.Log.Info("command was run on closed pull request")
-		c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull, "Atlantis commands can't be run on closed pull requests", ctx.VCSHost) // nolint: errcheck
+		c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, "Atlantis commands can't be run on closed pull requests", ctx.VCSHost) // nolint: errcheck
 		return
 	}
 
@@ -148,8 +147,6 @@ func (c *CommandHandler) run(ctx *CommandContext) {
 		cr = c.PlanExecutor.Execute(ctx)
 	case Apply:
 		cr = c.ApplyExecutor.Execute(ctx)
-	case Help:
-		cr = c.HelpExecutor.Execute(ctx)
 	default:
 		ctx.Log.Err("failed to determine desired command, neither plan nor apply")
 	}
@@ -167,14 +164,14 @@ func (c *CommandHandler) updatePull(ctx *CommandContext, res CommandResponse) {
 	// Update the pull request's status icon and comment back.
 	c.CommitStatusUpdater.UpdateProjectResult(ctx, res) // nolint: errcheck
 	comment := c.MarkdownRenderer.Render(res, ctx.Command.Name, ctx.Log.History.String(), ctx.Command.Verbose)
-	c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull, comment, ctx.VCSHost) // nolint: errcheck
+	c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, comment, ctx.VCSHost) // nolint: errcheck
 }
 
 // logPanics logs and creates a comment on the pull request for panics.
 func (c *CommandHandler) logPanics(ctx *CommandContext) {
 	if err := recover(); err != nil {
 		stack := recovery.Stack(3)
-		c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull, // nolint: errcheck
+		c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, // nolint: errcheck
 			fmt.Sprintf("**Error: goroutine panic. This is a bug.**\n```\n%s\n%s```", err, stack), ctx.VCSHost)
 		ctx.Log.Err("PANIC: %s\n%s", err, stack)
 	}

server/events/command_handler_test.go

@@ -21,7 +21,6 @@ import (
 )
 
 var applier *mocks.MockExecutor
-var helper *mocks.MockExecutor
 var planner *mocks.MockExecutor
 var eventParsing *mocks.MockEventParsing
 var vcsClient *vcsmocks.MockClientProxy
@@ -35,7 +34,6 @@ var logBytes *bytes.Buffer
 func setup(t *testing.T) {
 	RegisterMockTestingT(t)
 	applier = mocks.NewMockExecutor()
-	helper = mocks.NewMockExecutor()
 	planner = mocks.NewMockExecutor()
 	eventParsing = mocks.NewMockEventParsing()
 	ghStatus = mocks.NewMockCommitStatusUpdater()
@@ -49,7 +47,6 @@ func setup(t *testing.T) {
 	ch = events.CommandHandler{
 		PlanExecutor:             planner,
 		ApplyExecutor:            applier,
-		HelpExecutor:             helper,
 		VCSClient:                vcsClient,
 		CommitStatusUpdater:      ghStatus,
 		EventParser:              eventParsing,
@@ -66,7 +63,7 @@ func TestExecuteCommand_LogPanics(t *testing.T) {
 	setup(t)
 	When(ghStatus.Update(fixtures.Repo, fixtures.Pull, vcs.Pending, nil, vcs.Github)).ThenPanic("panic")
 	ch.ExecuteCommand(fixtures.Repo, fixtures.Repo, fixtures.User, 1, nil, vcs.Github)
-	_, _, comment, _ := vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString(), matchers.AnyVcsHost()).GetCapturedArguments()
+	_, _, comment, _ := vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), matchers.AnyVcsHost()).GetCapturedArguments()
 	Assert(t, strings.Contains(comment, "Error: goroutine panic"), "comment should be about a goroutine panic")
 }
 
@@ -125,7 +122,7 @@ func TestExecuteCommand_ClosedPull(t *testing.T) {
 	When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, fixtures.Repo, nil)
 
 	ch.ExecuteCommand(fixtures.Repo, fixtures.Repo, fixtures.User, fixtures.Pull.Num, nil, vcs.Github)
-	vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, modelPull, "Atlantis commands can't be run on closed pull requests", vcs.Github)
+	vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, modelPull.Num, "Atlantis commands can't be run on closed pull requests", vcs.Github)
 }
 
 func TestExecuteCommand_WorkspaceLocked(t *testing.T) {
@@ -150,17 +147,17 @@ func TestExecuteCommand_WorkspaceLocked(t *testing.T) {
 	ghStatus.VerifyWasCalledOnce().Update(fixtures.Repo, fixtures.Pull, vcs.Pending, &cmd, vcs.Github)
 	_, response := ghStatus.VerifyWasCalledOnce().UpdateProjectResult(matchers.AnyPtrToEventsCommandContext(), matchers.AnyEventsCommandResponse()).GetCapturedArguments()
 	Equals(t, msg, response.Failure)
-	vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, fixtures.Pull,
+	vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, fixtures.Pull.Num,
 		"**Plan Failed**: "+msg+"\n\n", vcs.Github)
 }
 
 func TestExecuteCommand_FullRun(t *testing.T) {
-	t.Log("when running a plan, apply or help should comment")
+	t.Log("when running a plan, apply should comment")
 	pull := &github.PullRequest{
 		State: github.String("closed"),
 	}
 	cmdResponse := events.CommandResponse{}
-	for _, c := range []events.CommandName{events.Help, events.Plan, events.Apply} {
+	for _, c := range []events.CommandName{events.Plan, events.Apply} {
 		setup(t)
 		cmd := events.Command{
 			Name:      c,
@@ -170,8 +167,6 @@ func TestExecuteCommand_FullRun(t *testing.T) {
 		When(eventParsing.ParseGithubPull(pull)).ThenReturn(fixtures.Pull, fixtures.Repo, nil)
 		When(workspaceLocker.TryLock(fixtures.Repo.FullName, cmd.Workspace, fixtures.Pull.Num)).ThenReturn(true)
 		switch c {
-		case events.Help:
-			When(helper.Execute(matchers.AnyPtrToEventsCommandContext())).ThenReturn(cmdResponse)
 		case events.Plan:
 			When(planner.Execute(matchers.AnyPtrToEventsCommandContext())).ThenReturn(cmdResponse)
 		case events.Apply:
@@ -183,7 +178,7 @@ func TestExecuteCommand_FullRun(t *testing.T) {
 		ghStatus.VerifyWasCalledOnce().Update(fixtures.Repo, fixtures.Pull, vcs.Pending, &cmd, vcs.Github)
 		_, response := ghStatus.VerifyWasCalledOnce().UpdateProjectResult(matchers.AnyPtrToEventsCommandContext(), matchers.AnyEventsCommandResponse()).GetCapturedArguments()
 		Equals(t, cmdResponse, response)
-		vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString(), matchers.AnyVcsHost())
+		vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), matchers.AnyVcsHost())
 		workspaceLocker.VerifyWasCalledOnce().Unlock(fixtures.Repo.FullName, cmd.Workspace, fixtures.Pull.Num)
 	}
 }

server/events/command_name.go

@@ -6,7 +6,6 @@ type CommandName int
 const (
 	Apply CommandName = iota
 	Plan
-	Help
 	// Adding more? Don't forget to update String() below
 )
 
@@ -17,8 +16,6 @@ func (c CommandName) String() string {
 		return "apply"
 	case Plan:
 		return "plan"
-	case Help:
-		return "help"
 	}
 	return ""
 }

server/events/comment_parser.go

@@ -0,0 +1,221 @@
+package events
+
+import (
+	"fmt"
+	"io/ioutil"
+	"path/filepath"
+	"strings"
+
+	"github.com/runatlantis/atlantis/server/events/vcs"
+	"github.com/spf13/pflag"
+)
+
+//go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_comment_parsing.go CommentParsing
+
+// CommentParsing handles parsing pull request comments.
+type CommentParsing interface {
+	// Parse attempts to parse a pull request comment to see if it's an Atlantis
+	// commmand.
+	Parse(comment string, vcsHost vcs.Host) CommentParseResult
+}
+
+// CommentParser implements CommentParsing
+type CommentParser struct {
+	GithubUser  string
+	GithubToken string
+	GitlabUser  string
+	GitlabToken string
+}
+
+// CommentParseResult describes the result of parsing a comment as a command.
+type CommentParseResult struct {
+	// Command is the successfully parsed command. Will be nil if
+	// CommentResponse or Ignore is set.
+	Command *Command
+	// CommentResponse is set when we should respond immediately to the command
+	// for example for atlantis help.
+	CommentResponse string
+	// Ignore is set to true when we should just ignore this comment.
+	Ignore bool
+}
+
+// Parse parses the comment as an Atlantis command.
+//
+// Valid commands contain:
+// - The initial "executable" name, 'run' or 'atlantis' or '@GithubUser'
+//   where GithubUser is the API user Atlantis is running as.
+// - Then a command, either 'plan', 'apply', or 'help'.
+// - Then optional flags, then an optional separator '--' followed by optional
+//   extra flags to be appended to the terraform plan/apply command.
+//
+// Examples:
+// - atlantis help
+// - run plan
+// - @GithubUser plan -w staging
+// - atlantis plan -w staging -d dir --verbose
+// - atlantis plan --verbose -- -key=value -key2 value2
+//
+// nolint: gocyclo
+func (e *CommentParser) Parse(comment string, vcsHost vcs.Host) CommentParseResult {
+	if multiLineRegex.MatchString(comment) {
+		return CommentParseResult{Ignore: true}
+	}
+
+	// strings.Fields strips out newlines but that's okay since we've removed
+	// multiline strings above.
+	args := strings.Fields(comment)
+	if len(args) < 1 {
+		return CommentParseResult{Ignore: true}
+	}
+
+	// Helpfully warn the user if they're using "terraform" instead of "atlantis"
+	if args[0] == "terraform" {
+		return CommentParseResult{CommentResponse: DidYouMeanAtlantisComment}
+	}
+
+	// Atlantis can be invoked using the name of the VCS host user we're
+	// running under. Need to be able to match against that user.
+	vcsUser := e.GithubUser
+	if vcsHost == vcs.Gitlab {
+		vcsUser = e.GitlabUser
+	}
+	executableNames := []string{"run", "atlantis", "@" + vcsUser}
+
+	// If the comment doesn't start with the name of our 'executable' then
+	// ignore it.
+	if !e.stringInSlice(args[0], executableNames) {
+		return CommentParseResult{Ignore: true}
+	}
+
+	// If they've just typed the name of the executable then give them the help
+	// output.
+	if len(args) == 1 {
+		return CommentParseResult{CommentResponse: HelpComment}
+	}
+	command := args[1]
+
+	// Help output.
+	if e.stringInSlice(command, []string{"help", "-h", "--help"}) {
+		return CommentParseResult{CommentResponse: HelpComment}
+	}
+
+	// Need to have a plan or apply at this point.
+	if !e.stringInSlice(command, []string{"plan", "apply"}) {
+		return CommentParseResult{CommentResponse: fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", command)}
+	}
+
+	var workspace string
+	var dir string
+	var verbose bool
+	var extraArgs []string
+	var flagSet *pflag.FlagSet
+	var name CommandName
+
+	// Set up the flag parsing depending on the command.
+	const defaultWorkspace = "default"
+	switch command {
+	case "plan":
+		name = Plan
+		flagSet = pflag.NewFlagSet("plan", pflag.ContinueOnError)
+		flagSet.SetOutput(ioutil.Discard)
+		flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, "Switch to this Terraform workspace before planning.")
+		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.")
+	case "apply":
+		name = Apply
+		flagSet = pflag.NewFlagSet("apply", pflag.ContinueOnError)
+		flagSet.SetOutput(ioutil.Discard)
+		flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, "Apply the plan for this Terraform workspace.")
+		flagSet.StringVarP(&dir, "dir", "d", "", "Apply the plan for 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.")
+	default:
+		return CommentParseResult{CommentResponse: fmt.Sprintf("Error: unknown command %q – this is a bug", command)}
+	}
+
+	// Now parse the flags.
+	// It's safe to use [2:] because we know there's at least 2 elements in args.
+	err := flagSet.Parse(args[2:])
+	if err == pflag.ErrHelp {
+		return CommentParseResult{CommentResponse: fmt.Sprintf("```\nUsage of %s:\n%s\n```", command, flagSet.FlagUsagesWrapped(usagesCols))}
+	}
+	if err != nil {
+		return CommentParseResult{CommentResponse: fmt.Sprintf("```\nError: %s.\nUsage of %s:\n%s\n```", err.Error(), command, flagSet.FlagUsagesWrapped(usagesCols))}
+	}
+
+	// We only use the extra args after the --. For example given a comment:
+	// "atlantis plan -bad-option -- -target=hi"
+	// we only append "-target=hi" to the eventual command.
+	// todo: keep track of the args we're discarding and include that with
+	//       comment as a warning.
+	if flagSet.ArgsLenAtDash() != -1 {
+		extraArgsUnsafe := flagSet.Args()[flagSet.ArgsLenAtDash():]
+		// Quote all extra args so there isn't a security issue when we append
+		// them to the terraform commands, ex. "; cat /etc/passwd"
+		for _, arg := range extraArgsUnsafe {
+			quotesEscaped := strings.Replace(arg, `"`, `\"`, -1)
+			extraArgs = append(extraArgs, fmt.Sprintf(`"%s"`, quotesEscaped))
+		}
+	}
+
+	dir, err = e.validateDir(dir)
+	if err != nil {
+		return CommentParseResult{CommentResponse: err.Error()}
+	}
+
+	// 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 CommentParseResult{CommentResponse: "Error: Value for -w/--workspace can't contain '..'"}
+	}
+
+	return CommentParseResult{
+		Command: &Command{Name: name, Verbose: verbose, Workspace: workspace, Dir: dir, Flags: extraArgs},
+	}
+}
+
+func (e *CommentParser) validateDir(dir string) (string, error) {
+	if dir == "" {
+		return dir, nil
+	}
+	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 "", fmt.Errorf("Error: Using a relative path %q with -d/--dir is not allowed", dir)
+	}
+
+	return validatedDir, nil
+}
+
+func (e *CommentParser) stringInSlice(a string, list []string) bool {
+	for _, b := range list {
+		if b == a {
+			return true
+		}
+	}
+	return false
+}
+
+var HelpComment = "```cmake\n" +
+	`atlantis
+Terraform automation and collaboration for your team
+
+Usage:
+  atlantis <command> [options]
+
+Commands:
+  plan   Runs 'terraform plan' for the changes in this pull request.
+  apply  Runs 'terraform apply' on the plans generated by 'atlantis plan'.
+  help   View help.
+
+Flags:
+  -h, --help   help for atlantis
+
+Use "atlantis [command] --help" for more information about a command.
+`
+
+var DidYouMeanAtlantisComment = "Did you mean to use `atlantis` instead of `terraform`?"