fix(multienv): allow commas and quoted values (#3542)

* Make code more Go-idiomatic While at it makes it more readable. Signed-off-by: Leandro López (inkel) <[email protected]> * Add internal function to parse multienv step input This new function properly deals with quotes and commas in values. Signed-off-by: Leandro López (inkel) <[email protected]> * Add regression test for multienv output with comma in values See #2765 for an issue report. Signed-off-by: Leandro López (inkel) <[email protected]> * Use parseMultienvLine for parsing multienv steps output Signed-off-by: Leandro López (inkel) <[email protected]> * Add internal function to parse multienv step input This new function properly deals with quotes and commas in values. Signed-off-by: Leandro López (inkel) <[email protected]> --------- Signed-off-by: Leandro López (inkel) <[email protected]> Co-authored-by: PePe Amengual <[email protected]>
runatlantis · Oct 6, 2023 · bb18da2 · bb18da2
1 parent d083aa1
commit bb18da2
Show file tree

Hide file tree

Showing 3 changed files with 208 additions and 20 deletions.
diff --git a/server/core/runtime/multienv_step_runner.go b/server/core/runtime/multienv_step_runner.go
@@ -1,6 +1,7 @@
 package runtime
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
@@ -17,27 +18,124 @@ type MultiEnvStepRunner struct {
 // The command must return a json string containing the array of name-value pairs that are being added as extra environment variables
 func (r *MultiEnvStepRunner) Run(ctx command.ProjectContext, command string, path string, envs map[string]string) (string, error) {
 	res, err := r.RunStepRunner.Run(ctx, command, path, envs, false, valid.PostProcessRunOutputShow)
-	if err == nil {
-		if len(res) > 0 {
-			var sb strings.Builder
-			sb.WriteString("Dynamic environment variables added:\n")
-
-			envVars := strings.Split(res, ",")
-			for _, item := range envVars {
-				// Only split after the first = found in case the environment variable value has
-				// = in it (as might be the case with access tokens)
-				nameValue := strings.SplitN(strings.TrimRight(item, "\n"), "=", 2)
-				if len(nameValue) == 2 {
-					envs[nameValue[0]] = nameValue[1]
-					sb.WriteString(nameValue[0])
-					sb.WriteString("\n")
-				} else {
-					return "", fmt.Errorf("Invalid environment variable definition: %s", item)
-				}
+	if err != nil {
+		return "", err
+	}
+
+	if len(res) == 0 {
+		return "No dynamic environment variable added", nil
+	}
+
+	var sb strings.Builder
+	sb.WriteString("Dynamic environment variables added:\n")
+
+	vars, err := parseMultienvLine(res)
+	if err != nil {
+		return "", fmt.Errorf("Invalid environment variable definition: %s (%w)", res, err)
+	}
+
+	for i := 0; i < len(vars); i += 2 {
+		key := vars[i]
+		envs[key] = vars[i+1]
+		sb.WriteString(key)
+		sb.WriteRune('\n')
+	}
+
+	return sb.String(), nil
+}
+
+func parseMultienvLine(in string) ([]string, error) {
+	in = strings.TrimSpace(in)
+	if in == "" {
+		return nil, nil
+	}
+	if len(in) < 3 {
+		return nil, errors.New("invalid syntax") // TODO
+	}
+
+	var res []string
+	var inValue, dquoted, squoted, escaped bool
+	var i int
+
+	for j, r := range in {
+		if !inValue {
+			if r == '=' {
+				inValue = true
+				res = append(res, in[i:j])
+				i = j + 1
+			}
+			if r == ' ' || r == '\t' {
+				return nil, errInvalidKeySyntax
 			}
-			return sb.String(), nil
+			if r == ',' && len(res) > 0 {
+				i = j + 1
+			}
+			continue
+		}
+
+		if r == '"' && !squoted {
+			if j == i && !dquoted { // value is double quoted
+				dquoted = true
+				i = j + 1
+			} else if dquoted && in[j-1] != '\\' {
+				res = append(res, unescape(in[i:j], escaped))
+				i = j + 1
+				dquoted = false
+				inValue = false
+			} else if in[j-1] != '\\' {
+				return nil, errMisquoted
+			} else if in[j-1] == '\\' {
+				escaped = true
+			}
+			continue
+		}
+
+		if r == '\'' && !dquoted {
+			if j == i && !squoted { // value is double quoted
+				squoted = true
+				i = j + 1
+			} else if squoted && in[j-1] != '\\' {
+				res = append(res, in[i:j])
+				i = j + 1
+				squoted = false
+				inValue = false
+			}
+			continue
+		}
+
+		if r == ',' && !dquoted && !squoted && inValue {
+			res = append(res, in[i:j])
+			i = j + 1
+			inValue = false
 		}
-		return "No dynamic environment variable added", nil
 	}
-	return "", err
+
+	if i < len(in) {
+		if !inValue {
+			return nil, errRemaining
+		}
+		res = append(res, unescape(in[i:], escaped))
+		inValue = false
+	}
+	if dquoted || squoted {
+		return nil, errMisquoted
+	}
+	if inValue {
+		return nil, errRemaining
+	}
+
+	return res, nil
 }
+
+func unescape(s string, escaped bool) string {
+	if escaped {
+		return strings.ReplaceAll(strings.ReplaceAll(s, `\\`, `\`), `\"`, `"`)
+	}
+	return s
+}
+
+var (
+	errInvalidKeySyntax = errors.New("invalid key syntax")
+	errMisquoted        = errors.New("misquoted")
+	errRemaining        = errors.New("remaining unparsed data")
+)
diff --git a/server/core/runtime/multienv_step_runner_internal_test.go b/server/core/runtime/multienv_step_runner_internal_test.go
@@ -0,0 +1,85 @@
+package runtime
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestMultiEnvStepRunner_Run_parser(t *testing.T) {
+	t.Run("success", func(t *testing.T) {
+		tests := map[string][]string{
+			"":                nil,
+			"KEY=value":       {"KEY", "value"},
+			`KEY="value"`:     {"KEY", "value"},
+			"KEY==":           {"KEY", "="},
+			`KEY="'"`:         {"KEY", "'"},
+			`KEY=""`:          {"KEY", ""},
+			`KEY=a\"b`:        {"KEY", `a"b`},
+			`KEY="va\"l\"ue"`: {"KEY", `va"l"ue`},
+
+			"KEY='value'":   {"KEY", "value"},
+			`KEY='va"l"ue'`: {"KEY", `va"l"ue`},
+			`KEY='"'`:       {"KEY", `"`},
+			"KEY=a'b":       {"KEY", "a'b"},
+			"KEY=''":        {"KEY", ""},
+			"KEY='a\\'b'":   {"KEY", "a\\'b"},
+
+			"FOO=bar,QUUX=baz":     {"FOO", "bar", "QUUX", "baz"},
+			"FOO='bar',QUUX=baz":   {"FOO", "bar", "QUUX", "baz"},
+			"FOO=bar,QUUX='baz'":   {"FOO", "bar", "QUUX", "baz"},
+			`FOO="bar",QUUX=baz`:   {"FOO", "bar", "QUUX", "baz"},
+			`FOO=bar,QUUX="baz"`:   {"FOO", "bar", "QUUX", "baz"},
+			`FOO="bar",QUUX='baz'`: {"FOO", "bar", "QUUX", "baz"},
+			`FOO='bar',QUUX="baz"`: {"FOO", "bar", "QUUX", "baz"},
+
+			"FOO=\"bar\nbaz\"": {"FOO", "bar\nbaz"},
+
+			`KEY="foo='bar',lorem=ipsum"`: {"KEY", "foo='bar',lorem=ipsum"},
+			`FOO=bar,QUUX="lorem ipsum"`:  {"FOO", "bar", "QUUX", "lorem ipsum"},
+
+			`JSON="{\"ID\":1,\"Name\":\"Reds\",\"Colors\":[\"Crimson\",\"Red\",\"Ruby\",\"Maroon\"]}"`: {"JSON", `{"ID":1,"Name":"Reds","Colors":["Crimson","Red","Ruby","Maroon"]}`},
+
+			`JSON='{"ID":1,"Name":"Reds","Colors":["Crimson","Red","Ruby","Maroon"]}'`: {"JSON", `{"ID":1,"Name":"Reds","Colors":["Crimson","Red","Ruby","Maroon"]}`},
+		}
+
+		for in, exp := range tests {
+			t.Run(in, func(t *testing.T) {
+				got, err := parseMultienvLine(in)
+				if err != nil {
+					t.Fatalf("unexpected error: %v", err)
+				}
+
+				t.Logf("\n%q\n%q", exp, got)
+
+				if e, g := len(exp), len(got); e != g {
+					t.Fatalf("expecting %d elements, got %d", e, g)
+				}
+
+				for i, e := range exp {
+					if g := got[i]; g != e {
+						t.Errorf("expecting %q at index %d, got %q", e, i, g)
+					}
+				}
+			})
+		}
+	})
+
+	t.Run("error", func(t *testing.T) {
+		tests := map[string]error{
+			"BAD KEY":           errInvalidKeySyntax,
+			"KEY='missingquote": errMisquoted,
+			`KEY="missingquote`: errMisquoted,
+			`KEY="missquoted'`:  errMisquoted,
+			`KEY=a"b`:           errMisquoted,
+			`KEY=value,rem`:     errRemaining,
+		}
+
+		for in, exp := range tests {
+			t.Run(in, func(t *testing.T) {
+				if _, err := parseMultienvLine(in); !errors.Is(err, exp) {
+					t.Fatalf("expecting error %v, got %v", exp, err)
+				}
+			})
+		}
+	})
+}
diff --git a/server/core/runtime/multienv_step_runner_test.go b/server/core/runtime/multienv_step_runner_test.go
@@ -37,6 +37,11 @@ func TestMultiEnvStepRunner_Run(t *testing.T) {
 			ExpErr:  "Invalid environment variable definition: TF_VAR_REPODEFINEDVARIABLE_NO_VALUE",
 			Version: "v1.2.3",
 		},
+		{
+			Command: `echo 'TF_VAR1_MULTILINE="foo\\nbar",TF_VAR2_VALUEWITHCOMMA="one,two",TF_VAR3_CONTROL=true'`,
+			ExpOut:  "Dynamic environment variables added:\nTF_VAR1_MULTILINE\nTF_VAR2_VALUEWITHCOMMA\nTF_VAR3_CONTROL\n",
+			Version: "v1.2.3",
+		},
 	}
 	RegisterMockTestingT(t)
 	tfClient := mocks.NewMockClient()