Quote extra comment args.

To avoid an attacker prepending something like
atlantis plan -- ; cat /etc/passwd

server/events/event_parser.go

@@ -117,7 +117,12 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma
 	// todo: keep track of the args we're discarding and include that with
 	//       comment as a warning.
 	if flagSet.ArgsLenAtDash() != -1 {
-		extraArgs = flagSet.Args()[flagSet.ArgsLenAtDash():]
+		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 {
+			extraArgs = append(extraArgs, fmt.Sprintf(`"%s"`, arg))
+		}
 	}
 
 	// If dir is specified, must ensure it's a valid path.

server/events/event_parser_test.go

@@ -157,14 +157,14 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 			"workspace",
 			"dir",
 			false,
-			"--verbose",
+			"\"--verbose\"",
 		},
 		{
 			"-w workspace -- -d dir --verbose",
 			"workspace",
 			"",
 			false,
-			"-d dir --verbose",
+			"\"-d\" \"dir\" \"--verbose\"",
 		},
 		// Test missing arguments.
 		{
@@ -194,7 +194,7 @@ func TestDetermineCommand_Parsing(t *testing.T) {
 			"workspace",
 			"dir",
 			true,
-			"arg one -two --three &&",
+			"\"arg\" \"one\" \"-two\" \"--three\" \"&&\"",
 		},
 		// Test whitespace.
 		{
@@ -202,14 +202,14 @@ func TestDetermineCommand_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/plan_executor_test.go

@@ -97,6 +97,54 @@ func TestExecute_DirectoryAndWorkspaceSet(t *testing.T) {
 	Equals(t, "lockurl-key", result.PlanSuccess.LockURL)
 }
 
+func TestExecute_AddedArgs(t *testing.T) {
+	t.Log("Test that we include extra-args added to the comment in the plan command")
+	p, runner, _ := setupPlanExecutorTest(t)
+	ctx := deepcopy.Copy(planCtx).(events.CommandContext)
+	ctx.Log = logging.NewNoopLogger()
+	ctx.Command.Flags = []string{"\"-target=resource\"", "\"-var\"", "\"a=b\"", "\";\"", "\"echo\"", "\"hi\""}
+
+	When(p.VCSClient.GetModifiedFiles(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), matchers.AnyVcsHost())).ThenReturn([]string{"file.tf"}, nil)
+	When(p.Workspace.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, "workspace")).
+		ThenReturn("/tmp/clone-repo", nil)
+	When(p.ProjectPreExecute.Execute(&ctx, "/tmp/clone-repo", models.Project{RepoFullName: "", Path: "."})).
+		ThenReturn(events.PreExecuteResult{
+			LockResponse: locking.TryLockResponse{
+				LockKey: "key",
+			},
+		})
+	r := p.Execute(&ctx)
+
+	runner.VerifyWasCalledOnce().RunCommandWithVersion(
+		ctx.Log,
+		"/tmp/clone-repo",
+		[]string{
+			"plan",
+			"-refresh",
+			"-no-color",
+			"-out",
+			"/tmp/clone-repo/workspace.tfplan",
+			"-var",
+			"atlantis_user=anubhavmishra",
+			// NOTE: extra args should be quoted to prevent an attacker from
+			// appending malicious commands.
+			"\"-target=resource\"",
+			"\"-var\"",
+			"\"a=b\"",
+			"\";\"",
+			"\"echo\"",
+			"\"hi\"",
+		},
+		nil,
+		"workspace",
+	)
+	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)