Escape additional comment args.

Fixes #697.
runatlantis · Jul 11, 2019 · a17e84f · a17e84f
1 parent dd02dea
commit a17e84f
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 13 deletions.
diff --git a/server/events/comment_parser.go b/server/events/comment_parser.go
@@ -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,13 +207,20 @@ 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))
+			var escapedArg string
+			// Add a backslash to each character so they're escaped when appended
+			// to the terraform command and used in sh -c.
+			// Golang strings are bytes so this isn't the proper treatment for
+			// UTF-8 characters however I don't want to expose us to any weird
+			// UTF-8 escape so to be safe I'm adding the backslash to every byte.
+			for i := range arg {
+				escapedArg += "\\" + string(arg[i])
+			}
+			extraArgs = append(extraArgs, escapedArg)
 		}
 	}
 

diff --git a/server/events/comment_parser_test.go b/server/events/comment_parser_test.go
@@ -418,15 +418,15 @@ func TestParse_Parsing(t *testing.T) {
 			"workspace",
 			"dir",
 			false,
-			"\"--verbose\"",
+			`\-\-\v\e\r\b\o\s\e`,
 			"",
 		},
 		{
 			"-w workspace -- -d dir --verbose",
 			"workspace",
 			"",
 			false,
-			"\"-d\" \"dir\" \"--verbose\"",
+			`\-\d \d\i\r \-\-\v\e\r\b\o\s\e`,
 			"",
 		},
 		// Test the extra args parsing.
@@ -440,19 +440,19 @@ func TestParse_Parsing(t *testing.T) {
 		},
 		// Test trying to escape quoting
 		{
-			"-- \";echo \"hi",
+			`-- ";echo "hi`,
 			"",
 			"",
 			false,
-			`";echo hi"`,
+			`\;\e\c\h\o\ \h\i`,
 			"",
 		},
 		{
 			"-w workspace -d dir --verbose -- arg one -two --three &&",
 			"workspace",
 			"dir",
 			true,
-			"\"arg\" \"one\" \"-two\" \"--three\" \"&&\"",
+			`\a\r\g \o\n\e \-\t\w\o \-\-\t\h\r\e\e \&\&`,
 			"",
 		},
 		// Test whitespace.
@@ -461,15 +461,15 @@ func TestParse_Parsing(t *testing.T) {
 			"workspace",
 			"dir",
 			true,
-			"\"arg\" \"one\" \"-two\" \"--three\" \"&&\"",
+			`\a\r\g \o\n\e \-\t\w\o \-\-\t\h\r\e\e \&\&`,
 			"",
 		},
 		{
 			"   -w   workspace   -d   dir   --verbose   --   arg   one   -two   --three   &&",
 			"workspace",
 			"dir",
 			true,
-			"\"arg\" \"one\" \"-two\" \"--three\" \"&&\"",
+			`\a\r\g \o\n\e \-\t\w\o \-\-\t\h\r\e\e \&\&`,
 			"",
 		},
 		// Test that the dir string is normalized.
@@ -521,6 +521,23 @@ func TestParse_Parsing(t *testing.T) {
 			"",
 			"",
 		},
+		// Extra args with shell characters
+		{
+			"-- -var=$(touch bad)",
+			"",
+			"",
+			false,
+			`\-\v\a\r\=\$\(\t\o\u\c\h \b\a\d\)`,
+			"",
+		},
+		{
+			"-- ;echo bad",
+			"",
+			"",
+			false,
+			`\;\e\c\h\o \b\a\d`,
+			"",
+		},
 	}
 	for _, test := range cases {
 		for _, cmdName := range []string{"plan", "apply"} {
@@ -532,7 +549,7 @@ func TestParse_Parsing(t *testing.T) {
 				Assert(t, test.expWorkspace == r.Command.Workspace, "exp workspace to equal %q but was %q for comment %q", test.expWorkspace, r.Command.Workspace, comment)
 				Assert(t, test.expVerbose == r.Command.Verbose, "exp verbose to equal %v but was %v for comment %q", test.expVerbose, r.Command.Verbose, comment)
 				actExtraArgs := strings.Join(r.Command.Flags, " ")
-				Assert(t, test.expExtraArgs == actExtraArgs, "exp extra args to equal %v but got %v for comment %q", test.expExtraArgs, actExtraArgs, comment)
+				Assert(t, test.expExtraArgs == actExtraArgs, "exp extra args to equal %q but got %q for comment %q", test.expExtraArgs, actExtraArgs, comment)
 				if cmdName == "plan" {
 					Assert(t, r.Command.Name == models.PlanCommand, "did not parse comment %q as plan command", comment)
 				}