diff --git a/server/events/event_parser.go b/server/events/event_parser.go index 3225e527b..36b2737d1 100644 --- a/server/events/event_parser.go +++ b/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. diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index c8c5bc8ae..218641682 100644 --- a/server/events/event_parser_test.go +++ b/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. { diff --git a/server/events/plan_executor_test.go b/server/events/plan_executor_test.go index a26440eed..168114367 100644 --- a/server/events/plan_executor_test.go +++ b/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)