Escape additional comment args.

Remove extra quoting and instead add a backslach to each character in
the extra args before appending it to the command.
ex. atlantis plan -- -var key=val will result in:
  sh -c "atlantis plan \-\v\a\r \k\e\y\=\v\a\l"
Fixes #697.

Author: lkysow

server/events/comment_parser.go

@@ -21,7 +21,7 @@ import (
 	"regexp"
 	"strings"
 
-	shlex "github.com/flynn-archive/go-shlex"
+	"github.com/flynn-archive/go-shlex"
 	"github.com/runatlantis/atlantis/server/events/models"
 	"github.com/runatlantis/atlantis/server/events/yaml"
 	"github.com/spf13/pflag"
@@ -162,7 +162,6 @@ func (e *CommentParser) Parse(comment string, vcsHost models.VCSHostType) Commen
 	var dir string
 	var project string
 	var verbose bool
-	var extraArgs []string
 	var flagSet *pflag.FlagSet
 	var name models.CommandName
 
@@ -208,14 +207,9 @@ func (e *CommentParser) Parse(comment string, vcsHost models.VCSHostType) Commen
 		return CommentParseResult{CommentResponse: e.errMarkdown(fmt.Sprintf("unknown argument(s) – %s", strings.Join(unusedArgs, " ")), command, flagSet)}
 	}
 
+	var extraArgs []string
 	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))
-		}
+		extraArgs = flagSet.Args()[flagSet.ArgsLenAtDash():]
 	}
 
 	dir, err = e.validateDir(dir)

server/events/comment_parser_test.go

@@ -418,15 +418,15 @@ func TestParse_Parsing(t *testing.T) {
 			"workspace",
 			"dir",
 			false,
-			"\"--verbose\"",
+			"--verbose",
 			"",
 		},
 		{
 			"-w workspace -- -d dir --verbose",
 			"workspace",
 			"",
 			false,
-			"\"-d\" \"dir\" \"--verbose\"",
+			"-d dir --verbose",
 			"",
 		},
 		// Test the extra args parsing.
@@ -438,21 +438,12 @@ func TestParse_Parsing(t *testing.T) {
 			"",
 			"",
 		},
-		// Test trying to escape quoting
-		{
-			"-- \";echo \"hi",
-			"",
-			"",
-			false,
-			`";echo hi"`,
-			"",
-		},
 		{
 			"-w workspace -d dir --verbose -- arg one -two --three &&",
 			"workspace",
 			"dir",
 			true,
-			"\"arg\" \"one\" \"-two\" \"--three\" \"&&\"",
+			"arg one -two --three &&",
 			"",
 		},
 		// Test whitespace.
@@ -461,15 +452,15 @@ func TestParse_Parsing(t *testing.T) {
 			"workspace",
 			"dir",
 			true,
-			"\"arg\" \"one\" \"-two\" \"--three\" \"&&\"",
+			"arg one -two --three &&",
 			"",
 		},
 		{
 			"   -w   workspace   -d   dir   --verbose   --   arg   one   -two   --three   &&",
 			"workspace",
 			"dir",
 			true,
-			"\"arg\" \"one\" \"-two\" \"--three\" \"&&\"",
+			"arg one -two --three &&",
 			"",
 		},
 		// Test that the dir string is normalized.

server/events/models/models.go

@@ -305,9 +305,11 @@ type ProjectCommandContext struct {
 	AutoplanEnabled bool
 	// BaseRepo is the repository that the pull request will be merged into.
 	BaseRepo Repo
-	// CommentArgs are the extra arguments appended to comment,
-	// ex. atlantis plan -- -target=resource
-	CommentArgs []string
+	// EscapedCommentArgs are the extra arguments that were added to the atlantis
+	// command, ex. atlantis plan -- -target=resource. We then escape them
+	// by adding a \ before each character so that they can be used within
+	// sh -c safely, i.e. sh -c "terraform plan $(touch bad)".
+	EscapedCommentArgs []string
 	// HeadRepo is the repository that is getting merged into the BaseRepo.
 	// If the pull request branch is from the same repository then HeadRepo will
 	// be the same as BaseRepo.

server/events/project_command_builder.go

@@ -382,24 +382,36 @@ func (p *DefaultProjectCommandBuilder) buildCtx(ctx *CommandContext,
 	}
 
 	return models.ProjectCommandContext{
-		ApplyCmd:          p.CommentBuilder.BuildApplyComment(projCfg.RepoRelDir, projCfg.Workspace, projCfg.Name),
-		BaseRepo:          ctx.BaseRepo,
-		CommentArgs:       commentArgs,
-		AutomergeEnabled:  automergeEnabled,
-		AutoplanEnabled:   projCfg.AutoplanEnabled,
-		Steps:             steps,
-		HeadRepo:          ctx.HeadRepo,
-		Log:               ctx.Log,
-		PullMergeable:     ctx.PullMergeable,
-		Pull:              ctx.Pull,
-		ProjectName:       projCfg.Name,
-		ApplyRequirements: projCfg.ApplyRequirements,
-		RePlanCmd:         p.CommentBuilder.BuildPlanComment(projCfg.RepoRelDir, projCfg.Workspace, projCfg.Name, commentArgs),
-		RepoRelDir:        projCfg.RepoRelDir,
-		RepoConfigVersion: projCfg.RepoCfgVersion,
-		TerraformVersion:  projCfg.TerraformVersion,
-		User:              ctx.User,
-		Verbose:           verbose,
-		Workspace:         projCfg.Workspace,
+		ApplyCmd:           p.CommentBuilder.BuildApplyComment(projCfg.RepoRelDir, projCfg.Workspace, projCfg.Name),
+		BaseRepo:           ctx.BaseRepo,
+		EscapedCommentArgs: p.escapeArgs(commentArgs),
+		AutomergeEnabled:   automergeEnabled,
+		AutoplanEnabled:    projCfg.AutoplanEnabled,
+		Steps:              steps,
+		HeadRepo:           ctx.HeadRepo,
+		Log:                ctx.Log,
+		PullMergeable:      ctx.PullMergeable,
+		Pull:               ctx.Pull,
+		ProjectName:        projCfg.Name,
+		ApplyRequirements:  projCfg.ApplyRequirements,
+		RePlanCmd:          p.CommentBuilder.BuildPlanComment(projCfg.RepoRelDir, projCfg.Workspace, projCfg.Name, commentArgs),
+		RepoRelDir:         projCfg.RepoRelDir,
+		RepoConfigVersion:  projCfg.RepoCfgVersion,
+		TerraformVersion:   projCfg.TerraformVersion,
+		User:               ctx.User,
+		Verbose:            verbose,
+		Workspace:          projCfg.Workspace,
 	}
 }
+
+func (p *DefaultProjectCommandBuilder) escapeArgs(args []string) []string {
+	var escaped []string
+	for _, arg := range args {
+		var escapedArg string
+		for i := range arg {
+			escapedArg += "\\" + string(arg[i])
+		}
+		escaped = append(escaped, escapedArg)
+	}
+	return escaped
+}