From bb18da218ddb8af5cff7f0b19c32869cc0b3b84d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leandro=20L=C3=B3pez?= Date: Fri, 6 Oct 2023 13:05:19 -0300 Subject: [PATCH] fix(multienv): allow commas and quoted values (#3542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Make code more Go-idiomatic While at it makes it more readable. Signed-off-by: Leandro López (inkel) * 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) * Add regression test for multienv output with comma in values See #2765 for an issue report. Signed-off-by: Leandro López (inkel) * Use parseMultienvLine for parsing multienv steps output Signed-off-by: Leandro López (inkel) * 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) --------- Signed-off-by: Leandro López (inkel) Co-authored-by: PePe Amengual --- server/core/runtime/multienv_step_runner.go | 138 +++++++++++++++--- .../multienv_step_runner_internal_test.go | 85 +++++++++++ .../core/runtime/multienv_step_runner_test.go | 5 + 3 files changed, 208 insertions(+), 20 deletions(-) create mode 100644 server/core/runtime/multienv_step_runner_internal_test.go diff --git a/server/core/runtime/multienv_step_runner.go b/server/core/runtime/multienv_step_runner.go index be4ac7ee43..515eb66896 100644 --- 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 new file mode 100644 index 0000000000..40eb65aacd --- /dev/null +++ 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 index 171f026117..f7d6b1132f 100644 --- 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()