From cf59b557b2255aaee486168656e9801642052c16 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Thu, 11 Jul 2019 17:02:36 +0100 Subject: [PATCH] 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. --- server/events/comment_parser.go | 12 +- server/events/comment_parser_test.go | 19 +- server/events/models/models.go | 8 +- server/events/project_command_builder.go | 50 ++-- .../project_command_builder_internal_test.go | 282 +++++++++--------- server/events/project_command_builder_test.go | 66 +++- server/events/runtime/apply_step_runner.go | 6 +- .../events/runtime/apply_step_runner_test.go | 48 +-- server/events/runtime/plan_step_runner.go | 4 +- .../events/runtime/plan_step_runner_test.go | 78 ++--- server/events/runtime/run_step_runner.go | 2 +- server/events/runtime/run_step_runner_test.go | 12 +- 12 files changed, 324 insertions(+), 263 deletions(-) diff --git a/server/events/comment_parser.go b/server/events/comment_parser.go index 431d103492..b6bf149499 100644 --- a/server/events/comment_parser.go +++ b/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) diff --git a/server/events/comment_parser_test.go b/server/events/comment_parser_test.go index 460b3c2dda..297bc9b39c 100644 --- a/server/events/comment_parser_test.go +++ b/server/events/comment_parser_test.go @@ -418,7 +418,7 @@ func TestParse_Parsing(t *testing.T) { "workspace", "dir", false, - "\"--verbose\"", + "--verbose", "", }, { @@ -426,7 +426,7 @@ func TestParse_Parsing(t *testing.T) { "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,7 +452,7 @@ func TestParse_Parsing(t *testing.T) { "workspace", "dir", true, - "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", + "arg one -two --three &&", "", }, { @@ -469,7 +460,7 @@ func TestParse_Parsing(t *testing.T) { "workspace", "dir", true, - "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", + "arg one -two --three &&", "", }, // Test that the dir string is normalized. diff --git a/server/events/models/models.go b/server/events/models/models.go index 2fe503f534..36c7d938e8 100644 --- a/server/events/models/models.go +++ b/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. diff --git a/server/events/project_command_builder.go b/server/events/project_command_builder.go index 629317ffa2..bedd17098f 100644 --- a/server/events/project_command_builder.go +++ b/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 +} diff --git a/server/events/project_command_builder_internal_test.go b/server/events/project_command_builder_internal_test.go index 47f6a625e9..eb020ec3a6 100644 --- a/server/events/project_command_builder_internal_test.go +++ b/server/events/project_command_builder_internal_test.go @@ -49,22 +49,22 @@ workflows: - apply`, repoCfg: "", expCtx: models.ProjectCommandContext{ - ApplyCmd: "atlantis apply -d project1 -w myworkspace", - BaseRepo: baseRepo, - CommentArgs: []string{"flag"}, - AutomergeEnabled: false, - AutoplanEnabled: true, - HeadRepo: models.Repo{}, - Log: nil, - PullMergeable: true, - Pull: models.PullRequest{}, - ProjectName: "", - ApplyRequirements: []string{}, - RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", - RepoRelDir: "project1", - User: models.User{}, - Verbose: true, - Workspace: "myworkspace", + ApplyCmd: "atlantis apply -d project1 -w myworkspace", + BaseRepo: baseRepo, + EscapedCommentArgs: []string{`\f\l\a\g`}, + AutomergeEnabled: false, + AutoplanEnabled: true, + HeadRepo: models.Repo{}, + Log: nil, + PullMergeable: true, + Pull: models.PullRequest{}, + ProjectName: "", + ApplyRequirements: []string{}, + RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", + RepoRelDir: "project1", + User: models.User{}, + Verbose: true, + Workspace: "myworkspace", }, expPlanSteps: []string{"init", "plan"}, expApplySteps: []string{"apply"}, @@ -98,24 +98,24 @@ projects: terraform_version: v10.0 `, expCtx: models.ProjectCommandContext{ - ApplyCmd: "atlantis apply -d project1 -w myworkspace", - BaseRepo: baseRepo, - CommentArgs: []string{"flag"}, - AutomergeEnabled: true, - AutoplanEnabled: true, - HeadRepo: models.Repo{}, - Log: nil, - PullMergeable: true, - Pull: models.PullRequest{}, - ProjectName: "", - ApplyRequirements: []string{}, - RepoConfigVersion: 3, - RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", - RepoRelDir: "project1", - TerraformVersion: mustVersion("10.0"), - User: models.User{}, - Verbose: true, - Workspace: "myworkspace", + ApplyCmd: "atlantis apply -d project1 -w myworkspace", + BaseRepo: baseRepo, + EscapedCommentArgs: []string{`\f\l\a\g`}, + AutomergeEnabled: true, + AutoplanEnabled: true, + HeadRepo: models.Repo{}, + Log: nil, + PullMergeable: true, + Pull: models.PullRequest{}, + ProjectName: "", + ApplyRequirements: []string{}, + RepoConfigVersion: 3, + RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", + RepoRelDir: "project1", + TerraformVersion: mustVersion("10.0"), + User: models.User{}, + Verbose: true, + Workspace: "myworkspace", }, expPlanSteps: []string{"init", "plan"}, expApplySteps: []string{"apply"}, @@ -149,24 +149,24 @@ projects: terraform_version: v10.0 `, expCtx: models.ProjectCommandContext{ - ApplyCmd: "atlantis apply -d project1 -w myworkspace", - BaseRepo: baseRepo, - CommentArgs: []string{"flag"}, - AutomergeEnabled: true, - AutoplanEnabled: true, - HeadRepo: models.Repo{}, - Log: nil, - PullMergeable: true, - Pull: models.PullRequest{}, - ProjectName: "", - ApplyRequirements: []string{"approved", "mergeable"}, - RepoConfigVersion: 3, - RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", - RepoRelDir: "project1", - TerraformVersion: mustVersion("10.0"), - User: models.User{}, - Verbose: true, - Workspace: "myworkspace", + ApplyCmd: "atlantis apply -d project1 -w myworkspace", + BaseRepo: baseRepo, + EscapedCommentArgs: []string{`\f\l\a\g`}, + AutomergeEnabled: true, + AutoplanEnabled: true, + HeadRepo: models.Repo{}, + Log: nil, + PullMergeable: true, + Pull: models.PullRequest{}, + ProjectName: "", + ApplyRequirements: []string{"approved", "mergeable"}, + RepoConfigVersion: 3, + RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", + RepoRelDir: "project1", + TerraformVersion: mustVersion("10.0"), + User: models.User{}, + Verbose: true, + Workspace: "myworkspace", }, expPlanSteps: []string{"init", "plan"}, expApplySteps: []string{"apply"}, @@ -208,24 +208,24 @@ projects: terraform_version: v10.0 `, expCtx: models.ProjectCommandContext{ - ApplyCmd: "atlantis apply -d project1 -w myworkspace", - BaseRepo: baseRepo, - CommentArgs: []string{"flag"}, - AutomergeEnabled: true, - AutoplanEnabled: true, - HeadRepo: models.Repo{}, - Log: nil, - PullMergeable: true, - Pull: models.PullRequest{}, - ProjectName: "", - ApplyRequirements: []string{"approved"}, - RepoConfigVersion: 3, - RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", - RepoRelDir: "project1", - TerraformVersion: mustVersion("10.0"), - User: models.User{}, - Verbose: true, - Workspace: "myworkspace", + ApplyCmd: "atlantis apply -d project1 -w myworkspace", + BaseRepo: baseRepo, + EscapedCommentArgs: []string{`\f\l\a\g`}, + AutomergeEnabled: true, + AutoplanEnabled: true, + HeadRepo: models.Repo{}, + Log: nil, + PullMergeable: true, + Pull: models.PullRequest{}, + ProjectName: "", + ApplyRequirements: []string{"approved"}, + RepoConfigVersion: 3, + RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", + RepoRelDir: "project1", + TerraformVersion: mustVersion("10.0"), + User: models.User{}, + Verbose: true, + Workspace: "myworkspace", }, expPlanSteps: []string{"plan"}, expApplySteps: []string{}, @@ -354,24 +354,24 @@ workflows: - apply `, expCtx: models.ProjectCommandContext{ - ApplyCmd: "atlantis apply -d project1 -w myworkspace", - BaseRepo: baseRepo, - CommentArgs: []string{"flag"}, - AutomergeEnabled: true, - AutoplanEnabled: true, - HeadRepo: models.Repo{}, - Log: nil, - PullMergeable: true, - Pull: models.PullRequest{}, - ProjectName: "", - ApplyRequirements: []string{}, - RepoConfigVersion: 3, - RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", - RepoRelDir: "project1", - TerraformVersion: mustVersion("10.0"), - User: models.User{}, - Verbose: true, - Workspace: "myworkspace", + ApplyCmd: "atlantis apply -d project1 -w myworkspace", + BaseRepo: baseRepo, + EscapedCommentArgs: []string{`\f\l\a\g`}, + AutomergeEnabled: true, + AutoplanEnabled: true, + HeadRepo: models.Repo{}, + Log: nil, + PullMergeable: true, + Pull: models.PullRequest{}, + ProjectName: "", + ApplyRequirements: []string{}, + RepoConfigVersion: 3, + RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", + RepoRelDir: "project1", + TerraformVersion: mustVersion("10.0"), + User: models.User{}, + Verbose: true, + Workspace: "myworkspace", }, expPlanSteps: []string{"plan"}, expApplySteps: []string{"apply"}, @@ -409,24 +409,24 @@ projects: workflow: custom `, expCtx: models.ProjectCommandContext{ - ApplyCmd: "atlantis apply -d project1 -w myworkspace", - BaseRepo: baseRepo, - CommentArgs: []string{"flag"}, - AutomergeEnabled: true, - AutoplanEnabled: true, - HeadRepo: models.Repo{}, - Log: nil, - PullMergeable: true, - Pull: models.PullRequest{}, - ProjectName: "", - ApplyRequirements: []string{}, - RepoConfigVersion: 3, - RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", - RepoRelDir: "project1", - TerraformVersion: mustVersion("10.0"), - User: models.User{}, - Verbose: true, - Workspace: "myworkspace", + ApplyCmd: "atlantis apply -d project1 -w myworkspace", + BaseRepo: baseRepo, + EscapedCommentArgs: []string{`\f\l\a\g`}, + AutomergeEnabled: true, + AutoplanEnabled: true, + HeadRepo: models.Repo{}, + Log: nil, + PullMergeable: true, + Pull: models.PullRequest{}, + ProjectName: "", + ApplyRequirements: []string{}, + RepoConfigVersion: 3, + RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", + RepoRelDir: "project1", + TerraformVersion: mustVersion("10.0"), + User: models.User{}, + Verbose: true, + Workspace: "myworkspace", }, expPlanSteps: []string{"plan"}, expApplySteps: []string{"apply"}, @@ -467,24 +467,24 @@ workflows: steps: [] `, expCtx: models.ProjectCommandContext{ - ApplyCmd: "atlantis apply -d project1 -w myworkspace", - BaseRepo: baseRepo, - CommentArgs: []string{"flag"}, - AutomergeEnabled: true, - AutoplanEnabled: true, - HeadRepo: models.Repo{}, - Log: nil, - PullMergeable: true, - Pull: models.PullRequest{}, - ProjectName: "", - ApplyRequirements: []string{}, - RepoConfigVersion: 3, - RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", - RepoRelDir: "project1", - TerraformVersion: mustVersion("10.0"), - User: models.User{}, - Verbose: true, - Workspace: "myworkspace", + ApplyCmd: "atlantis apply -d project1 -w myworkspace", + BaseRepo: baseRepo, + EscapedCommentArgs: []string{`\f\l\a\g`}, + AutomergeEnabled: true, + AutoplanEnabled: true, + HeadRepo: models.Repo{}, + Log: nil, + PullMergeable: true, + Pull: models.PullRequest{}, + ProjectName: "", + ApplyRequirements: []string{}, + RepoConfigVersion: 3, + RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", + RepoRelDir: "project1", + TerraformVersion: mustVersion("10.0"), + User: models.User{}, + Verbose: true, + Workspace: "myworkspace", }, expPlanSteps: []string{}, expApplySteps: []string{}, @@ -509,23 +509,23 @@ projects: workspace: myworkspace `, expCtx: models.ProjectCommandContext{ - ApplyCmd: "atlantis apply -d project1 -w myworkspace", - BaseRepo: baseRepo, - CommentArgs: []string{"flag"}, - AutomergeEnabled: false, - AutoplanEnabled: true, - HeadRepo: models.Repo{}, - Log: nil, - PullMergeable: true, - Pull: models.PullRequest{}, - ProjectName: "", - ApplyRequirements: []string{"approved"}, - RepoConfigVersion: 3, - RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", - RepoRelDir: "project1", - User: models.User{}, - Verbose: true, - Workspace: "myworkspace", + ApplyCmd: "atlantis apply -d project1 -w myworkspace", + BaseRepo: baseRepo, + EscapedCommentArgs: []string{`\f\l\a\g`}, + AutomergeEnabled: false, + AutoplanEnabled: true, + HeadRepo: models.Repo{}, + Log: nil, + PullMergeable: true, + Pull: models.PullRequest{}, + ProjectName: "", + ApplyRequirements: []string{"approved"}, + RepoConfigVersion: 3, + RePlanCmd: "atlantis plan -d project1 -w myworkspace -- flag", + RepoRelDir: "project1", + User: models.User{}, + Verbose: true, + Workspace: "myworkspace", }, expPlanSteps: []string{"plan"}, expApplySteps: []string{"apply"}, diff --git a/server/events/project_command_builder_test.go b/server/events/project_command_builder_test.go index 2464662946..cb2bf521a7 100644 --- a/server/events/project_command_builder_test.go +++ b/server/events/project_command_builder_test.go @@ -3,6 +3,7 @@ package events_test import ( "io/ioutil" "path/filepath" + "strings" "testing" . "github.com/petergtz/pegomock" @@ -180,7 +181,7 @@ func TestDefaultProjectCommandBuilder_BuildSinglePlanApplyCommand(t *testing.T) Workspace: "myworkspace", }, AtlantisYAML: "", - ExpCommentArgs: []string{"commentarg"}, + ExpCommentArgs: []string{`\c\o\m\m\e\n\t\a\r\g`}, ExpWorkspace: "myworkspace", ExpDir: ".", ExpApplyReqs: []string{}, @@ -382,7 +383,7 @@ projects: actCtx := actCtxs[0] Equals(t, c.ExpDir, actCtx.RepoRelDir) Equals(t, c.ExpWorkspace, actCtx.Workspace) - Equals(t, c.ExpCommentArgs, actCtx.CommentArgs) + Equals(t, c.ExpCommentArgs, actCtx.EscapedCommentArgs) Equals(t, c.ExpProjectName, actCtx.ProjectName) Equals(t, c.ExpApplyReqs, actCtx.ApplyRequirements) }) @@ -654,3 +655,64 @@ projects: }) ErrEquals(t, "running commands in workspace \"notconfigured\" is not allowed because this directory is only configured for the following workspaces: default, staging", err) } + +// Test that extra comment args are escaped. +func TestDefaultProjectCommandBuilder_EscapeArgs(t *testing.T) { + cases := []struct { + ExtraArgs []string + ExpEscapedArgs []string + }{ + { + ExtraArgs: []string{"arg1", "arg2"}, + ExpEscapedArgs: []string{`\a\r\g\1`, `\a\r\g\2`}, + }, + { + ExtraArgs: []string{"-var=$(touch bad)"}, + ExpEscapedArgs: []string{`\-\v\a\r\=\$\(\t\o\u\c\h\ \b\a\d\)`}, + }, + { + ExtraArgs: []string{"-- ;echo bad"}, + ExpEscapedArgs: []string{`\-\-\ \;\e\c\h\o\ \b\a\d`}, + }, + } + + for _, c := range cases { + t.Run(strings.Join(c.ExtraArgs, " "), func(t *testing.T) { + RegisterMockTestingT(t) + tmpDir, cleanup := DirStructure(t, map[string]interface{}{ + "main.tf": nil, + }) + defer cleanup() + + workingDir := mocks.NewMockWorkingDir() + When(workingDir.Clone(matchers.AnyPtrToLoggingSimpleLogger(), matchers.AnyModelsRepo(), matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString())).ThenReturn(tmpDir, nil) + When(workingDir.GetWorkingDir(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString())).ThenReturn(tmpDir, nil) + vcsClient := vcsmocks.NewMockClient() + When(vcsClient.GetModifiedFiles(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest())).ThenReturn([]string{"main.tf"}, nil) + + builder := &events.DefaultProjectCommandBuilder{ + WorkingDirLocker: events.NewDefaultWorkingDirLocker(), + WorkingDir: workingDir, + ParserValidator: &yaml.ParserValidator{}, + VCSClient: vcsClient, + ProjectFinder: &events.DefaultProjectFinder{}, + CommentBuilder: &events.CommentParser{}, + GlobalCfg: valid.NewGlobalCfg(true, false, false), + } + + var actCtxs []models.ProjectCommandContext + var err error + actCtxs, err = builder.BuildPlanCommands(&events.CommandContext{}, &events.CommentCommand{ + RepoRelDir: ".", + Flags: c.ExtraArgs, + Name: models.PlanCommand, + Verbose: false, + Workspace: "default", + }) + Ok(t, err) + Equals(t, 1, len(actCtxs)) + actCtx := actCtxs[0] + Equals(t, c.ExpEscapedArgs, actCtx.EscapedCommentArgs) + }) + } +} diff --git a/server/events/runtime/apply_step_runner.go b/server/events/runtime/apply_step_runner.go index 1fbf61bbb0..2570714ec2 100644 --- a/server/events/runtime/apply_step_runner.go +++ b/server/events/runtime/apply_step_runner.go @@ -38,7 +38,7 @@ func (a *ApplyStepRunner) Run(ctx models.ProjectCommandContext, extraArgs []stri var out string if a.isRemotePlan(contents) { - args := append(append(append([]string{"apply", "-input=false", "-no-color"}, extraArgs...), ctx.CommentArgs...)) + args := append(append(append([]string{"apply", "-input=false", "-no-color"}, extraArgs...), ctx.EscapedCommentArgs...)) out, err = a.runRemoteApply(ctx, args, path, planPath, ctx.TerraformVersion) if err == nil { out = a.cleanRemoteApplyOutput(out) @@ -46,7 +46,7 @@ func (a *ApplyStepRunner) Run(ctx models.ProjectCommandContext, extraArgs []stri } else { // NOTE: we need to quote the plan path because Bitbucket Server can // have spaces in its repo owner names which is part of the path. - args := append(append(append([]string{"apply", "-input=false", "-no-color"}, extraArgs...), ctx.CommentArgs...), fmt.Sprintf("%q", planPath)) + args := append(append(append([]string{"apply", "-input=false", "-no-color"}, extraArgs...), ctx.EscapedCommentArgs...), fmt.Sprintf("%q", planPath)) out, err = a.TerraformExecutor.RunCommandWithVersion(ctx.Log, path, args, ctx.TerraformVersion, ctx.Workspace) } @@ -78,7 +78,7 @@ func (a *ApplyStepRunner) hasTargetFlag(ctx models.ProjectCommandContext, extraA return split[0] == "-target" } - for _, arg := range ctx.CommentArgs { + for _, arg := range ctx.EscapedCommentArgs { if isTargetFlag(arg) { return true } diff --git a/server/events/runtime/apply_step_runner_test.go b/server/events/runtime/apply_step_runner_test.go index 8c279aa5ae..c43a9b58f1 100644 --- a/server/events/runtime/apply_step_runner_test.go +++ b/server/events/runtime/apply_step_runner_test.go @@ -63,9 +63,9 @@ func TestRun_Success(t *testing.T) { When(terraform.RunCommandWithVersion(matchers.AnyPtrToLoggingSimpleLogger(), AnyString(), AnyStringSlice(), matchers2.AnyPtrToGoVersionVersion(), AnyString())). ThenReturn("output", nil) output, err := o.Run(models.ProjectCommandContext{ - Workspace: "workspace", - RepoRelDir: ".", - CommentArgs: []string{"comment", "args"}, + Workspace: "workspace", + RepoRelDir: ".", + EscapedCommentArgs: []string{"comment", "args"}, }, []string{"extra", "args"}, tmpDir) Ok(t, err) Equals(t, "output", output) @@ -91,10 +91,10 @@ func TestRun_AppliesCorrectProjectPlan(t *testing.T) { When(terraform.RunCommandWithVersion(matchers.AnyPtrToLoggingSimpleLogger(), AnyString(), AnyStringSlice(), matchers2.AnyPtrToGoVersionVersion(), AnyString())). ThenReturn("output", nil) output, err := o.Run(models.ProjectCommandContext{ - Workspace: "default", - RepoRelDir: ".", - ProjectName: "projectname", - CommentArgs: []string{"comment", "args"}, + Workspace: "default", + RepoRelDir: ".", + ProjectName: "projectname", + EscapedCommentArgs: []string{"comment", "args"}, }, []string{"extra", "args"}, tmpDir) Ok(t, err) Equals(t, "output", output) @@ -120,10 +120,10 @@ func TestRun_UsesConfiguredTFVersion(t *testing.T) { When(terraform.RunCommandWithVersion(matchers.AnyPtrToLoggingSimpleLogger(), AnyString(), AnyStringSlice(), matchers2.AnyPtrToGoVersionVersion(), AnyString())). ThenReturn("output", nil) output, err := o.Run(models.ProjectCommandContext{ - Workspace: "workspace", - RepoRelDir: ".", - CommentArgs: []string{"comment", "args"}, - TerraformVersion: tfVersion, + Workspace: "workspace", + RepoRelDir: ".", + EscapedCommentArgs: []string{"comment", "args"}, + TerraformVersion: tfVersion, }, []string{"extra", "args"}, tmpDir) Ok(t, err) Equals(t, "output", output) @@ -197,9 +197,9 @@ func TestRun_UsingTarget(t *testing.T) { } output, err := step.Run(models.ProjectCommandContext{ - Workspace: "workspace", - RepoRelDir: ".", - CommentArgs: c.commentFlags, + Workspace: "workspace", + RepoRelDir: ".", + EscapedCommentArgs: c.commentFlags, }, c.extraArgs, tmpDir) Equals(t, "", output) if c.expErr { @@ -240,11 +240,11 @@ Plan: 0 to add, 0 to change, 1 to destroy.` } tfVersion, _ := version.NewVersion("0.11.0") ctx := models.ProjectCommandContext{ - Log: logging.NewSimpleLogger("testing", false, logging.Debug), - Workspace: "workspace", - RepoRelDir: ".", - CommentArgs: []string{"comment", "args"}, - TerraformVersion: tfVersion, + Log: logging.NewSimpleLogger("testing", false, logging.Debug), + Workspace: "workspace", + RepoRelDir: ".", + EscapedCommentArgs: []string{"comment", "args"}, + TerraformVersion: tfVersion, } output, err := o.Run(ctx, []string{"extra", "args"}, tmpDir) <-tfExec.DoneCh @@ -302,11 +302,11 @@ Plan: 0 to add, 0 to change, 1 to destroy.` tfVersion, _ := version.NewVersion("0.11.0") output, err := o.Run(models.ProjectCommandContext{ - Log: logging.NewSimpleLogger("testing", false, logging.Debug), - Workspace: "workspace", - RepoRelDir: ".", - CommentArgs: []string{"comment", "args"}, - TerraformVersion: tfVersion, + Log: logging.NewSimpleLogger("testing", false, logging.Debug), + Workspace: "workspace", + RepoRelDir: ".", + EscapedCommentArgs: []string{"comment", "args"}, + TerraformVersion: tfVersion, }, []string{"extra", "args"}, tmpDir) <-tfExec.DoneCh ErrEquals(t, `Plan generated during apply phase did not match plan generated during plan phase. diff --git a/server/events/runtime/plan_step_runner.go b/server/events/runtime/plan_step_runner.go index 43e6b44208..46e6bf9e6c 100644 --- a/server/events/runtime/plan_step_runner.go +++ b/server/events/runtime/plan_step_runner.go @@ -73,7 +73,7 @@ func (p *PlanStepRunner) remotePlan(ctx models.ProjectCommandContext, extraArgs argList := [][]string{ {"plan", "-input=false", "-refresh", "-no-color"}, extraArgs, - ctx.CommentArgs, + ctx.EscapedCommentArgs, } args := p.flatten(argList) output, err := p.runRemotePlan(ctx, args, path, tfVersion) @@ -174,7 +174,7 @@ func (p *PlanStepRunner) buildPlanCmd(ctx models.ProjectCommandContext, extraArg {"plan", "-input=false", "-refresh", "-no-color", "-out", fmt.Sprintf("%q", planFile)}, tfVars, extraArgs, - ctx.CommentArgs, + ctx.EscapedCommentArgs, envFileArgs, } diff --git a/server/events/runtime/plan_step_runner_test.go b/server/events/runtime/plan_step_runner_test.go index ebbd2f1d23..eebd803bf5 100644 --- a/server/events/runtime/plan_step_runner_test.go +++ b/server/events/runtime/plan_step_runner_test.go @@ -39,11 +39,11 @@ func TestRun_NoWorkspaceIn08(t *testing.T) { When(terraform.RunCommandWithVersion(matchers.AnyPtrToLoggingSimpleLogger(), AnyString(), AnyStringSlice(), matchers2.AnyPtrToGoVersionVersion(), AnyString())). ThenReturn("output", nil) output, err := s.Run(models.ProjectCommandContext{ - Log: logger, - CommentArgs: []string{"comment", "args"}, - Workspace: workspace, - RepoRelDir: ".", - User: models.User{Username: "username"}, + Log: logger, + EscapedCommentArgs: []string{"comment", "args"}, + Workspace: workspace, + RepoRelDir: ".", + User: models.User{Username: "username"}, Pull: models.PullRequest{ Num: 2, }, @@ -166,11 +166,11 @@ func TestRun_SwitchesWorkspace(t *testing.T) { When(terraform.RunCommandWithVersion(matchers.AnyPtrToLoggingSimpleLogger(), AnyString(), AnyStringSlice(), matchers2.AnyPtrToGoVersionVersion(), AnyString())). ThenReturn("output", nil) output, err := s.Run(models.ProjectCommandContext{ - Log: logger, - Workspace: "workspace", - RepoRelDir: ".", - User: models.User{Username: "username"}, - CommentArgs: []string{"comment", "args"}, + Log: logger, + Workspace: "workspace", + RepoRelDir: ".", + User: models.User{Username: "username"}, + EscapedCommentArgs: []string{"comment", "args"}, Pull: models.PullRequest{ Num: 2, }, @@ -286,11 +286,11 @@ func TestRun_CreatesWorkspace(t *testing.T) { When(terraform.RunCommandWithVersion(logger, "/path", expPlanArgs, tfVersion, "workspace")).ThenReturn("output", nil) output, err := s.Run(models.ProjectCommandContext{ - Log: logger, - Workspace: "workspace", - RepoRelDir: ".", - User: models.User{Username: "username"}, - CommentArgs: []string{"comment", "args"}, + Log: logger, + Workspace: "workspace", + RepoRelDir: ".", + User: models.User{Username: "username"}, + EscapedCommentArgs: []string{"comment", "args"}, Pull: models.PullRequest{ Num: 2, }, @@ -346,11 +346,11 @@ func TestRun_NoWorkspaceSwitchIfNotNecessary(t *testing.T) { When(terraform.RunCommandWithVersion(logger, "/path", expPlanArgs, tfVersion, "workspace")).ThenReturn("output", nil) output, err := s.Run(models.ProjectCommandContext{ - Log: logger, - Workspace: "workspace", - RepoRelDir: ".", - User: models.User{Username: "username"}, - CommentArgs: []string{"comment", "args"}, + Log: logger, + Workspace: "workspace", + RepoRelDir: ".", + User: models.User{Username: "username"}, + EscapedCommentArgs: []string{"comment", "args"}, Pull: models.PullRequest{ Num: 2, }, @@ -417,11 +417,11 @@ func TestRun_AddsEnvVarFile(t *testing.T) { When(terraform.RunCommandWithVersion(logger, tmpDir, expPlanArgs, tfVersion, "workspace")).ThenReturn("output", nil) output, err := s.Run(models.ProjectCommandContext{ - Log: logger, - Workspace: "workspace", - RepoRelDir: ".", - User: models.User{Username: "username"}, - CommentArgs: []string{"comment", "args"}, + Log: logger, + Workspace: "workspace", + RepoRelDir: ".", + User: models.User{Username: "username"}, + EscapedCommentArgs: []string{"comment", "args"}, Pull: models.PullRequest{ Num: 2, }, @@ -476,12 +476,12 @@ func TestRun_UsesDiffPathForProject(t *testing.T) { When(terraform.RunCommandWithVersion(logger, "/path", expPlanArgs, tfVersion, "default")).ThenReturn("output", nil) output, err := s.Run(models.ProjectCommandContext{ - Log: logger, - Workspace: "default", - RepoRelDir: ".", - User: models.User{Username: "username"}, - CommentArgs: []string{"comment", "args"}, - ProjectName: "projectname", + Log: logger, + Workspace: "default", + RepoRelDir: ".", + User: models.User{Username: "username"}, + EscapedCommentArgs: []string{"comment", "args"}, + ProjectName: "projectname", Pull: models.PullRequest{ Num: 2, }, @@ -627,10 +627,10 @@ func TestRun_NoOptionalVarsIn012(t *testing.T) { AnyString())).ThenReturn("output", nil) output, err := s.Run(models.ProjectCommandContext{ - Workspace: "default", - RepoRelDir: ".", - User: models.User{Username: "username"}, - CommentArgs: []string{"comment", "args"}, + Workspace: "default", + RepoRelDir: ".", + User: models.User{Username: "username"}, + EscapedCommentArgs: []string{"comment", "args"}, Pull: models.PullRequest{ Num: 2, }, @@ -719,10 +719,10 @@ plan locally at this time. // Now that mocking is set up, we're ready to run the plan. ctx := models.ProjectCommandContext{ - Workspace: "default", - RepoRelDir: ".", - User: models.User{Username: "username"}, - CommentArgs: []string{"comment", "args"}, + Workspace: "default", + RepoRelDir: ".", + User: models.User{Username: "username"}, + EscapedCommentArgs: []string{"comment", "args"}, Pull: models.PullRequest{ Num: 2, }, diff --git a/server/events/runtime/run_step_runner.go b/server/events/runtime/run_step_runner.go index 54ab6a805d..7c4eaf18ee 100644 --- a/server/events/runtime/run_step_runner.go +++ b/server/events/runtime/run_step_runner.go @@ -29,7 +29,7 @@ func (r *RunStepRunner) Run(ctx models.ProjectCommandContext, command string, pa "BASE_BRANCH_NAME": ctx.Pull.BaseBranch, "BASE_REPO_NAME": ctx.BaseRepo.Name, "BASE_REPO_OWNER": ctx.BaseRepo.Owner, - "COMMENT_ARGS": strings.Join(ctx.CommentArgs, ","), + "COMMENT_ARGS": strings.Join(ctx.EscapedCommentArgs, ","), "DIR": path, "HEAD_BRANCH_NAME": ctx.Pull.HeadBranch, "HEAD_REPO_NAME": ctx.HeadRepo.Name, diff --git a/server/events/runtime/run_step_runner_test.go b/server/events/runtime/run_step_runner_test.go index 32f6207a77..ebf8eb7afa 100644 --- a/server/events/runtime/run_step_runner_test.go +++ b/server/events/runtime/run_step_runner_test.go @@ -97,12 +97,12 @@ func TestRunStepRunner_Run(t *testing.T) { User: models.User{ Username: "acme-user", }, - Log: logging.NewNoopLogger(), - Workspace: "myworkspace", - RepoRelDir: "mydir", - TerraformVersion: projVersion, - ProjectName: c.ProjectName, - CommentArgs: []string{"-target=resource1", "-target=resource2"}, + Log: logging.NewNoopLogger(), + Workspace: "myworkspace", + RepoRelDir: "mydir", + TerraformVersion: projVersion, + ProjectName: c.ProjectName, + EscapedCommentArgs: []string{"-target=resource1", "-target=resource2"}, } out, err := r.Run(ctx, c.Command, tmpDir) if c.ExpErr != "" {