feat: validate module variable keys and values (#26354)

Validates caller-supplied module variable keys and values in the template builder compose endpoint before template rendering. Previously, `mergeModuleVariables` accepted any caller-supplied key and value without validation, allowing unknown keys, computed/sensitive variable overrides, and malformed HCL literals (including injection payloads) to pass through to rendered output. Now `mergeModuleVariables` rejects unknown keys (those not in the manifest's non-computed, non-sensitive variables) and type-checks values: strings must be quoted HCL literals without interpolation markers or unescaped newlines, numbers must be strict numeric literals, and bools must be exactly `true` or `false`. The literal `null` is accepted for any type. Closes https://linear.app/codercom/issue/DEVEX-278 <details> <summary>Implementation details</summary> - Changed `mergeModuleVariables` signature from `map[string]string` to `(map[string]string, error)` to surface validation failures - Added `validateVariableValue`, `validateStringValue`, `validateNumberValue`, `validateBoolValue` in `compose.go` - String validation rejects: unquoted values, HCL interpolation (`${`, `%{`), unescaped newlines/quotes, trailing backslashes (which would escape the closing delimiter), and values exceeding 4096 bytes - Errors wrap the module ID and variable name for clear diagnostics (e.g. `module "code-server": variable "port": invalid number value`) - Tests cover key validation, type validation, injection attempts, and full Compose flow integration > Generated with the help of [Coder Agents](https://coder.com) by @jeremyruppel </details>
2026-09-24 15:04:27 +08:00 · 2026-06-15 11:34:47 -04:00
parent 3cde346cbb
commit 4574c7d792
3 changed files with 399 additions and 11 deletions
@@ -4,11 +4,19 @@ import (
 	"archive/tar"
 	"bytes"
 	"encoding/json"
+	"regexp"
+	"strings"
 	"time"

 	"golang.org/x/xerrors"
 )

+// maxStringValueLen is the maximum byte length for a string variable value.
+const maxStringValueLen = 4096
+
+// numberPattern matches valid HCL number literals (integers and decimals).
+var numberPattern = regexp.MustCompile(`^-?[0-9]+(\.[0-9]+)?$`)
+
 // ComposeRequest describes which base template and modules to render.
 type ComposeRequest struct {
 	BaseTemplateID string
@@ -147,7 +155,10 @@ func renderModules(
 			return nil, xerrors.Errorf("module template FS for %q: %w", cm.ID, err)
 		}

-		vars := mergeModuleVariables(manifest, cm.Variables)
+		vars, err := mergeModuleVariables(manifest, cm.Variables)
+		if err != nil {
+			return nil, xerrors.Errorf("module %q: %w", cm.ID, err)
+		}
 		modCtx := ModuleRenderContext{
 			RegistryBase:      registryURL,
 			PinnedVersion:     manifest.PinnedVersion,
@@ -170,9 +181,31 @@ func renderModules(

 // mergeModuleVariables builds the final Variables map for a module template.
 // It starts with manifest defaults for all non-computed, non-sensitive
-// variables, then overlays caller-supplied values. This ensures every
-// variable referenced in the template has a value.
-func mergeModuleVariables(manifest ModuleManifest, callerVars map[string]string) map[string]string {
+// variables, then overlays caller-supplied values. Caller-supplied keys
+// are validated against the manifest and values are checked for type
+// correctness before being accepted.
+func mergeModuleVariables(manifest ModuleManifest, callerVars map[string]string) (map[string]string, error) {
+	// Build lookup structures for the manifest variables.
+	allowedVars := make(map[string]ModuleVariable, len(manifest.Variables))
+	for _, v := range manifest.Variables {
+		if v.Computed || v.Sensitive {
+			continue
+		}
+		allowedVars[v.Name] = v
+	}
+
+	// Validate caller-supplied keys and values before merging.
+	for k, val := range callerVars {
+		v, ok := allowedVars[k]
+		if !ok {
+			return nil, xerrors.Errorf("unknown variable %q", k)
+		}
+		if err := validateVariableValue(v, val); err != nil {
+			return nil, xerrors.Errorf("variable %q: %w", k, err)
+		}
+	}
+
+	// Build merged map from manifest defaults.
 	merged := make(map[string]string, len(manifest.Variables))
 	for _, v := range manifest.Variables {
 		if v.Computed || v.Sensitive {
@@ -191,10 +224,87 @@ func mergeModuleVariables(manifest ModuleManifest, callerVars map[string]string)
 		// Required variables without defaults are left out so that
 		// missingkey=error surfaces the omission at render time.
 	}
+
+	// Overlay validated caller values.
 	for k, val := range callerVars {
 		merged[k] = val
 	}
-	return merged
+	return merged, nil
+}
+
+// validateVariableValue checks that value is a valid HCL literal for the
+// variable's declared type. The literal "null" is accepted for any type.
+func validateVariableValue(v ModuleVariable, value string) error {
+	if value == "null" {
+		return nil
+	}
+	switch v.Type {
+	case "string":
+		return validateStringValue(value)
+	case "number":
+		return validateNumberValue(value)
+	case "bool":
+		return validateBoolValue(value)
+	default:
+		return xerrors.Errorf("unsupported variable type %q", v.Type)
+	}
+}
+
+// validateStringValue checks that value is a valid quoted HCL string literal.
+// It must start and end with '"', contain no unescaped newlines or quotes,
+// and must not contain HCL interpolation/directive markers.
+func validateStringValue(value string) error {
+	if len(value) > maxStringValueLen {
+		return xerrors.Errorf("value exceeds maximum length of %d bytes", maxStringValueLen)
+	}
+	if len(value) < 2 || value[0] != '"' || value[len(value)-1] != '"' {
+		return xerrors.New("must be a quoted string (e.g. \"value\")")
+	}
+
+	inner := value[1 : len(value)-1]
+
+	if strings.Contains(inner, "${") || strings.Contains(inner, "%{") {
+		return xerrors.New("must not contain HCL interpolation or directive sequences")
+	}
+
+	// Walk the inner content to reject unescaped newlines and quotes.
+	for i := 0; i < len(inner); i++ {
+		ch := inner[i]
+		if ch == '\\' {
+			i++
+			if i >= len(inner) {
+				// Trailing backslash with no character to escape.
+				// In HCL this would escape the closing quote delimiter,
+				// producing an unterminated string.
+				return xerrors.New("must not end with a trailing backslash")
+			}
+			continue
+		}
+		if ch == '"' {
+			return xerrors.New("must not contain unescaped quotes")
+		}
+		if ch == '\n' || ch == '\r' {
+			return xerrors.New("must not contain unescaped newlines")
+		}
+	}
+
+	return nil
+}
+
+// validateNumberValue checks that value is a valid HCL number literal.
+func validateNumberValue(value string) error {
+	if !numberPattern.MatchString(value) {
+		return xerrors.Errorf("invalid number value %q, must be a numeric literal (e.g. 42, 3.14)", value)
+	}
+	return nil
+}
+
+// validateBoolValue checks that value is exactly "true" or "false".
+func validateBoolValue(value string) error {
+	if value != "true" && value != "false" {
+		return xerrors.Errorf("invalid bool value %q, must be true or false", value)
+	}
+	return nil
 }

 // isSimpleJSONValue returns true if raw is a valid JSON string, number,
@@ -2,6 +2,7 @@ package templatebuilder

 import (
 	"encoding/json"
+	"strings"
 	"testing"

 	"github.com/stretchr/testify/require"
@@ -56,43 +57,264 @@ func TestMergeModuleVariables(t *testing.T) {

 	t.Run("DefaultsApplied", func(t *testing.T) {
 		t.Parallel()
-		merged := mergeModuleVariables(manifest, nil)
+		merged, err := mergeModuleVariables(manifest, nil)
+		require.NoError(t, err)
 		require.Equal(t, "13337", merged["port"])
 		require.Equal(t, "false", merged["enabled"])
 	})

 	t.Run("ComputedAndSensitiveSkipped", func(t *testing.T) {
 		t.Parallel()
-		merged := mergeModuleVariables(manifest, nil)
+		merged, err := mergeModuleVariables(manifest, nil)
+		require.NoError(t, err)
 		require.NotContains(t, merged, "agent_id")
 		require.NotContains(t, merged, "api_key")
 	})

 	t.Run("NonRequiredWithoutDefaultGetsNull", func(t *testing.T) {
 		t.Parallel()
-		merged := mergeModuleVariables(manifest, nil)
+		merged, err := mergeModuleVariables(manifest, nil)
+		require.NoError(t, err)
 		require.Equal(t, "null", merged["optional_no_default"])
 	})

 	t.Run("RequiredWithoutDefaultOmitted", func(t *testing.T) {
 		t.Parallel()
-		merged := mergeModuleVariables(manifest, nil)
+		merged, err := mergeModuleVariables(manifest, nil)
+		require.NoError(t, err)
 		require.NotContains(t, merged, "required_no_default")
 	})

 	t.Run("CallerOverridesDefault", func(t *testing.T) {
 		t.Parallel()
-		merged := mergeModuleVariables(manifest, map[string]string{
+		merged, err := mergeModuleVariables(manifest, map[string]string{
 			"port": "9999",
 		})
+		require.NoError(t, err)
 		require.Equal(t, "9999", merged["port"])
 	})

 	t.Run("CallerProvidesRequired", func(t *testing.T) {
 		t.Parallel()
-		merged := mergeModuleVariables(manifest, map[string]string{
+		merged, err := mergeModuleVariables(manifest, map[string]string{
 			"required_no_default": `"value"`,
 		})
+		require.NoError(t, err)
 		require.Equal(t, `"value"`, merged["required_no_default"])
 	})
+
+	t.Run("UnknownKeyRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := mergeModuleVariables(manifest, map[string]string{
+			"nonexistent": `"val"`,
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `unknown variable "nonexistent"`)
+	})
+
+	t.Run("ComputedKeyRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := mergeModuleVariables(manifest, map[string]string{
+			"agent_id": `"injected"`,
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `unknown variable "agent_id"`)
+	})
+
+	t.Run("SensitiveKeyRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := mergeModuleVariables(manifest, map[string]string{
+			"api_key": `"secret"`,
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `unknown variable "api_key"`)
+	})
+
+	t.Run("InvalidNumberValueRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := mergeModuleVariables(manifest, map[string]string{
+			"port": "abc",
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `variable "port"`)
+		require.Contains(t, err.Error(), "invalid number value")
+	})
+
+	t.Run("InvalidBoolValueRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := mergeModuleVariables(manifest, map[string]string{
+			"enabled": "yes",
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `variable "enabled"`)
+		require.Contains(t, err.Error(), "invalid bool value")
+	})
+
+	t.Run("InvalidStringValueRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := mergeModuleVariables(manifest, map[string]string{
+			"optional_no_default": "unquoted",
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `variable "optional_no_default"`)
+		require.Contains(t, err.Error(), "quoted string")
+	})
+
+	t.Run("NullAcceptedForAnyType", func(t *testing.T) {
+		t.Parallel()
+		merged, err := mergeModuleVariables(manifest, map[string]string{
+			"port":                "null",
+			"enabled":             "null",
+			"optional_no_default": "null",
+		})
+		require.NoError(t, err)
+		require.Equal(t, "null", merged["port"])
+		require.Equal(t, "null", merged["enabled"])
+		require.Equal(t, "null", merged["optional_no_default"])
+	})
+
+	t.Run("EmptyCallerVarsNoError", func(t *testing.T) {
+		t.Parallel()
+		merged, err := mergeModuleVariables(manifest, map[string]string{})
+		require.NoError(t, err)
+		require.Equal(t, "13337", merged["port"])
+	})
+}
+
+func TestValidateStringValue(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		value   string
+		wantErr string
+	}{
+		{"ValidEmpty", `""`, ""},
+		{"ValidSimple", `"hello"`, ""},
+		{"ValidPath", `"/home/coder"`, ""},
+		{"ValidURL", `"https://github.com/coder/coder"`, ""},
+		{"ValidLiteralBackslashN", `"line\\nbreak"`, ""},
+		{"ValidEscapedQuote", `"say \"hi\""`, ""},
+		{"ValidEscapedBackslash", `"path\\to\\file"`, ""},
+
+		{"RejectedUnquoted", "hello", "quoted string"},
+		{"RejectedMissingOpenQuote", `hello"`, "quoted string"},
+		{"RejectedMissingCloseQuote", `"hello`, "quoted string"},
+		{"RejectedEmpty", "", "quoted string"},
+		{"RejectedSingleChar", `"`, "quoted string"},
+		{"RejectedUnescapedNewline", "\"line\nbreak\"", "unescaped newlines"},
+		{"RejectedCarriageReturn", "\"line\rbreak\"", "unescaped newlines"},
+		{"RejectedUnescapedQuote", `"say "hi""`, "unescaped quotes"},
+		{"RejectedHCLInterpolation", `"${var.foo}"`, "interpolation"},
+		{"RejectedHCLDirective", `"%{if true}yes%{endif}"`, "interpolation"},
+		{"RejectedOverlong", `"` + strings.Repeat("a", maxStringValueLen) + `"`, "maximum length"},
+		{"RejectedTrailingBackslash", `"test\"`, "trailing backslash"},
+		{"RejectedTrailingBackslashOnly", `"\"`, "trailing backslash"},
+		{"ValidEvenTrailingBackslashes", `"test\\\\"`, ""},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			err := validateStringValue(tc.value)
+			if tc.wantErr == "" {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.wantErr)
+			}
+		})
+	}
+}
+
+func TestValidateNumberValue(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		value   string
+		wantErr bool
+	}{
+		{"Zero", "0", false},
+		{"Positive", "42", false},
+		{"Negative", "-1", false},
+		{"Decimal", "3.14", false},
+		{"NegativeDecimal", "-0.5", false},
+
+		{"Scientific", "1e10", true},
+		{"Hex", "0x1F", true},
+		{"Underscore", "1_000", true},
+		{"Expression", "1 + 1", true},
+		{"Empty", "", true},
+		{"Letters", "abc", true},
+		{"TrailingDot", "1.", true},
+		{"LeadingDot", ".5", true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			err := validateNumberValue(tc.value)
+			if tc.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestValidateBoolValue(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		value   string
+		wantErr bool
+	}{
+		{"True", "true", false},
+		{"False", "false", false},
+
+		{"UpperTrue", "True", true},
+		{"UpperFALSE", "FALSE", true},
+		{"QuotedTrue", `"true"`, true},
+		{"One", "1", true},
+		{"Yes", "yes", true},
+		{"Empty", "", true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			err := validateBoolValue(tc.value)
+			if tc.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestValidateVariableValue(t *testing.T) {
+	t.Parallel()
+
+	t.Run("NullAcceptedForAllTypes", func(t *testing.T) {
+		t.Parallel()
+		for _, typ := range []string{"string", "number", "bool"} {
+			t.Run(typ, func(t *testing.T) {
+				t.Parallel()
+				v := ModuleVariable{Name: "test", Type: typ}
+				require.NoError(t, validateVariableValue(v, "null"))
+			})
+		}
+	})
+
+	t.Run("UnsupportedTypeRejected", func(t *testing.T) {
+		t.Parallel()
+		v := ModuleVariable{Name: "test", Type: "list"}
+		err := validateVariableValue(v, `"val"`)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "unsupported variable type")
+	})
 }
@@ -169,6 +169,62 @@ func TestCompose(t *testing.T) {
 		require.Contains(t, err.Error(), `unknown module "nonexistent-module"`)
 	})

+	t.Run("UnknownVariableKeyRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := templatebuilder.Compose(templatebuilder.ComposeRequest{
+			BaseTemplateID: "docker",
+			RegistryURL:    "https://registry.coder.com",
+			Modules: []templatebuilder.ComposeModule{
+				{
+					ID: "code-server",
+					Variables: map[string]string{
+						"nonexistent_var": `"value"`,
+					},
+				},
+			},
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `module "code-server"`)
+		require.Contains(t, err.Error(), `unknown variable "nonexistent_var"`)
+	})
+
+	t.Run("InvalidVariableValueRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := templatebuilder.Compose(templatebuilder.ComposeRequest{
+			BaseTemplateID: "docker",
+			RegistryURL:    "https://registry.coder.com",
+			Modules: []templatebuilder.ComposeModule{
+				{
+					ID: "code-server",
+					Variables: map[string]string{
+						"port": "not-a-number",
+					},
+				},
+			},
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `module "code-server"`)
+		require.Contains(t, err.Error(), `variable "port"`)
+	})
+
+	t.Run("HCLInjectionRejected", func(t *testing.T) {
+		t.Parallel()
+		_, err := templatebuilder.Compose(templatebuilder.ComposeRequest{
+			BaseTemplateID: "docker",
+			RegistryURL:    "https://registry.coder.com",
+			Modules: []templatebuilder.ComposeModule{
+				{
+					ID: "code-server",
+					Variables: map[string]string{
+						"folder": `"${var.evil}"`,
+					},
+				},
+			},
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "interpolation")
+	})
+
 	t.Run("MissingRequiredVariable", func(t *testing.T) {
 		t.Parallel()
 		// git-clone has a required "url" variable with no default.