From 4574c7d792c1efc99f0974b0a95d4b851fdf7cc9 Mon Sep 17 00:00:00 2001 From: Jeremy Ruppel Date: Mon, 15 Jun 2026 11:34:47 -0400 Subject: [PATCH] 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
Implementation details - 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
--- coderd/templatebuilder/compose.go | 120 ++++++++- .../templatebuilder/compose_internal_test.go | 234 +++++++++++++++++- coderd/templatebuilder/compose_test.go | 56 +++++ 3 files changed, 399 insertions(+), 11 deletions(-) diff --git a/coderd/templatebuilder/compose.go b/coderd/templatebuilder/compose.go index 381bb8772f..b56ab5e7e6 100644 --- a/coderd/templatebuilder/compose.go +++ b/coderd/templatebuilder/compose.go @@ -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, diff --git a/coderd/templatebuilder/compose_internal_test.go b/coderd/templatebuilder/compose_internal_test.go index 53edf70957..9d56286572 100644 --- a/coderd/templatebuilder/compose_internal_test.go +++ b/coderd/templatebuilder/compose_internal_test.go @@ -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") + }) } diff --git a/coderd/templatebuilder/compose_test.go b/coderd/templatebuilder/compose_test.go index 175c0e78cb..04690d4157 100644 --- a/coderd/templatebuilder/compose_test.go +++ b/coderd/templatebuilder/compose_test.go @@ -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.