bug: change quoting behavior

When reading the #! interpreter line variables in quotes "${FOO}" will
be evaluated and passed as a single argument to the command. Unquoted
variables like ${FOO} may result in multiple argument. For example,
if FOO="Hello World" that will result in two arguments "Hello" and
"World", where "${FOO}" will result in one argument "Hello World"

Author: ibuildthecloud

Diff for: pkg/engine/cmd.go

@@ -123,6 +123,9 @@ func (e *Engine) runCommand(ctx Context, tool types.Tool, input string, toolCate
 	}
 	cmd, stop, err := e.newCommand(ctx.Ctx, extraEnv, tool, input)
 	if err != nil {
+		if toolCategory == NoCategory {
+			return fmt.Sprintf("ERROR: got (%v) while parsing command", err), nil
+		}
 		return "", err
 	}
 	defer stop()
@@ -268,6 +271,12 @@ func (e *Engine) newCommand(ctx context.Context, extraEnv []string, tool types.T
 		})
 	}
 
+	// After we determined the interpreter we again interpret the args by env vars
+	args, err = replaceVariablesForInterpreter(interpreter, envMap)
+	if err != nil {
+		return nil, nil, err
+	}
+
 	if runtime.GOOS == "windows" && (args[0] == "/bin/bash" || args[0] == "/bin/sh") {
 		args[0] = path.Base(args[0])
 	}
@@ -314,3 +323,88 @@ func (e *Engine) newCommand(ctx context.Context, extraEnv []string, tool types.T
 	cmd.Env = compressEnv(envvars)
 	return cmd, stop, nil
 }
+
+func replaceVariablesForInterpreter(interpreter string, envMap map[string]string) ([]string, error) {
+	var parts []string
+	for i, part := range splitByQuotes(interpreter) {
+		if i%2 == 0 {
+			part = os.Expand(part, func(s string) string {
+				return envMap[s]
+			})
+			// We protect newly resolved env vars from getting replaced when we do the second Expand
+			// after shlex. Yeah, crazy. I'm guessing this isn't secure, but just trying to avoid a foot gun.
+			part = os.Expand(part, func(s string) string {
+				return "${__" + s + "}"
+			})
+		}
+		parts = append(parts, part)
+	}
+
+	parts, err := shlex.Split(strings.Join(parts, ""))
+	if err != nil {
+		return nil, err
+	}
+
+	for i, part := range parts {
+		parts[i] = os.Expand(part, func(s string) string {
+			if strings.HasPrefix(s, "__") {
+				return "${" + s[2:] + "}"
+			} else {
+				return envMap[s]
+			}
+		})
+	}
+
+	return parts, nil
+}
+
+// splitByQuotes will split a string by parsing matching double quotes (with \ as the escape character).
+// The return value conforms to the following properties
+//  1. s == strings.Join(result, "")
+//  2. Even indexes are strings that were not in quotes.
+//  3. Odd indexes are strings that were quoted.
+//
+// Example: s = `In a "quoted string" quotes can be escaped with \"`
+//
+//	result = [`In a `, `"quoted string"`, ` quotes can be escaped with \"`]
+func splitByQuotes(s string) (result []string) {
+	var (
+		buf               strings.Builder
+		inEscape, inQuote bool
+	)
+
+	for _, c := range s {
+		if inEscape {
+			buf.WriteRune(c)
+			inEscape = false
+			continue
+		}
+
+		switch c {
+		case '"':
+			if inQuote {
+				buf.WriteRune(c)
+			}
+			result = append(result, buf.String())
+			buf.Reset()
+			if !inQuote {
+				buf.WriteRune(c)
+			}
+			inQuote = !inQuote
+		case '\\':
+			inEscape = true
+			buf.WriteRune(c)
+		default:
+			buf.WriteRune(c)
+		}
+	}
+
+	if buf.Len() > 0 {
+		if inQuote {
+			result = append(result, "")
+		}
+		result = append(result, buf.String())
+	}
+
+	return
+}

Diff for: pkg/engine/cmd_test.go

@@ -0,0 +1,135 @@
+// File: cmd_test.go
+package engine
+
+import "testing"
+
+func TestSplitByQuotes(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected []string
+	}{
+		{
+			name:     "NoQuotes",
+			input:    "Hello World",
+			expected: []string{"Hello World"},
+		},
+		{
+			name:     "ValidQuote",
+			input:    `"Hello" "World"`,
+			expected: []string{``, `"Hello"`, ` `, `"World"`},
+		},
+		{
+			name:     "ValidQuoteWithEscape",
+			input:    `"Hello\" World"`,
+			expected: []string{``, `"Hello\" World"`},
+		},
+		{
+			name:     "Nothing",
+			input:    "",
+			expected: []string{},
+		},
+		{
+			name:     "SpaceInsideQuote",
+			input:    `"Hello World"`,
+			expected: []string{``, `"Hello World"`},
+		},
+		{
+			name:     "SingleChar",
+			input:    "H",
+			expected: []string{"H"},
+		},
+		{
+			name:     "SingleQuote",
+			input:    `"Hello`,
+			expected: []string{``, ``, `"Hello`},
+		},
+		{
+			name:     "ThreeQuotes",
+			input:    `Test "Hello "World" End\"`,
+			expected: []string{`Test `, `"Hello "`, `World`, ``, `" End\"`},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := splitByQuotes(tt.input)
+			if !equal(got, tt.expected) {
+				t.Errorf("splitByQuotes() = %v, want %v", got, tt.expected)
+			}
+		})
+	}
+}
+
+// Helper function to assert equality of two string slices.
+func equal(a, b []string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i, v := range a {
+		if v != b[i] {
+			return false
+		}
+	}
+	return true
+}
+
+// Testing for replaceVariablesForInterpreter
+func TestReplaceVariablesForInterpreter(t *testing.T) {
+	tests := []struct {
+		name        string
+		interpreter string
+		envMap      map[string]string
+		expected    []string
+		shouldFail  bool
+	}{
+		{
+			name:        "No quotes",
+			interpreter: "/bin/bash -c ${COMMAND} tail",
+			envMap:      map[string]string{"COMMAND": "echo Hello!"},
+			expected:    []string{"/bin/bash", "-c", "echo", "Hello!", "tail"},
+		},
+		{
+			name:        "Quotes Variables",
+			interpreter: `/bin/bash -c "${COMMAND}" tail`,
+			envMap:      map[string]string{"COMMAND": "Hello, World!"},
+			expected:    []string{"/bin/bash", "-c", "Hello, World!", "tail"},
+		},
+		{
+			name:        "Double escape",
+			interpreter: `/bin/bash -c "${COMMAND}" ${TWO} tail`,
+			envMap: map[string]string{
+				"COMMAND": "Hello, World!",
+				"TWO":     "${COMMAND}",
+			},
+			expected: []string{"/bin/bash", "-c", "Hello, World!", "${COMMAND}", "tail"},
+		},
+		{
+			name:        "aws cli issue",
+			interpreter: "aws ${ARGS}",
+			envMap: map[string]string{
+				"ARGS": `ec2 describe-instances --region us-east-1 --query 'Reservations[*].Instances[*].{Instance:InstanceId,State:State.Name}'`,
+			},
+			expected: []string{
+				`aws`,
+				`ec2`,
+				`describe-instances`,
+				`--region`, `us-east-1`,
+				`--query`, `Reservations[*].Instances[*].{Instance:InstanceId,State:State.Name}`,
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := replaceVariablesForInterpreter(tt.interpreter, tt.envMap)
+			if (err != nil) != tt.shouldFail {
+				t.Errorf("replaceVariablesForInterpreter() error = %v, want %v", err, tt.shouldFail)
+				return
+			}
+			if !equal(got, tt.expected) {
+				t.Errorf("replaceVariablesForInterpreter() = %v, want %v", got, tt.expected)
+			}
+		})
+	}
+}