diff --git a/cmd/account/link_key/link_key.go b/cmd/account/link_key/link_key.go index 47d32b02..342c1ab3 100644 --- a/cmd/account/link_key/link_key.go +++ b/cmd/account/link_key/link_key.go @@ -39,8 +39,7 @@ const ( ) type Inputs struct { - // TODO: Add validation for WorkflowOwnerLabel - WorkflowOwnerLabel string `validate:"omitempty"` + WorkflowOwnerLabel string `validate:"omitempty,owner_label" cli:"--owner-label"` WorkflowOwner string `validate:"required,workflow_owner"` WorkflowRegistryContractAddress string `validate:"required"` NonInteractive bool @@ -138,7 +137,7 @@ func (h *handler) ResolveInputs(v *viper.Viper) (Inputs, error) { return Inputs{ WorkflowOwner: h.settings.Workflow.UserWorkflowSettings.WorkflowOwnerAddress, WorkflowRegistryContractAddress: h.environmentSet.WorkflowRegistryAddress, - WorkflowOwnerLabel: v.GetString("owner-label"), + WorkflowOwnerLabel: strings.TrimSpace(v.GetString("owner-label")), NonInteractive: v.GetBool(settings.Flags.NonInteractive.Name), }, nil } @@ -185,16 +184,21 @@ func (h *handler) Execute(ctx context.Context, in Inputs) error { label, err := ui.Input( title, ui.WithDefaultValue(defaultLabel), + // IsValidOwnerLabel rejects the empty string, so this also enforces + // that a label is required. ui.WithValidate(func(s string) error { - if strings.TrimSpace(s) == "" { - return fmt.Errorf("a label is required") - } - return nil + return validation.IsValidOwnerLabel(strings.TrimSpace(s)) }), ) if err != nil { return err } + // Re-check after the prompt: Execute runs after ValidateInputs, so a + // prompt-supplied label would otherwise never reach the validator. + label = strings.TrimSpace(label) + if err := validation.IsValidOwnerLabel(label); err != nil { + return err + } in.WorkflowOwnerLabel = label } @@ -425,17 +429,25 @@ func (h *handler) checkIfAlreadyLinked() (bool, error) { } // defaultOwnerLabel derives a suggested label from the authenticated user's -// email address (the portion before the "@"). Returns "" if no email is available. +// email address (the portion before the "@"). Returns "" if no email is +// available, or if the derived value is not a valid owner label — an email +// local part may contain characters a label may not, and pre-filling the +// prompt with a value the validator rejects would block submission. func (h *handler) defaultOwnerLabel() string { email, err := h.credentials.GetEmail() if err != nil || email == "" { return "" } + label := email if idx := strings.Index(email, "@"); idx > 0 { - return email[:idx] + label = email[:idx] + } + + if validation.IsValidOwnerLabel(label) != nil { + return "" } - return email + return label } func (h *handler) displayDetails() { diff --git a/cmd/account/link_key/link_key_test.go b/cmd/account/link_key/link_key_test.go index 90829061..611f024c 100644 --- a/cmd/account/link_key/link_key_test.go +++ b/cmd/account/link_key/link_key_test.go @@ -1,12 +1,58 @@ package link_key import ( + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestValidateInputs_OwnerLabel(t *testing.T) { + t.Parallel() + + const ( + validOwner = "0x0000000000000000000000000000000000000001" + validRegistry = "0x0000000000000000000000000000000000000002" + ) + + tests := []struct { + name string + label string + wantError bool + }{ + {name: "valid label", label: "prod owner 1", wantError: false}, + {name: "valid label with dashes", label: "owner-label-1", wantError: false}, + {name: "empty label is deferred to the prompt", label: "", wantError: false}, + {name: "double quote breaks out of yaml scalar", label: `bad"; echo pwned`, wantError: true}, + {name: "command substitution", label: "bad$(id)", wantError: true}, + {name: "newline", label: "a\ninjected: true", wantError: true}, + {name: "leading dash looks like a flag", label: "-rf /", wantError: true}, + {name: "too long", label: strings.Repeat("a", 65), wantError: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + h := &handler{} + err := h.ValidateInputs(Inputs{ + WorkflowOwnerLabel: tt.label, + WorkflowOwner: validOwner, + WorkflowRegistryContractAddress: validRegistry, + }) + + if tt.wantError { + require.Error(t, err, "expected label %q to be rejected", tt.label) + assert.Contains(t, err.Error(), "--owner-label", + "error should name the flag the user passed") + assert.False(t, h.validated, "handler must not be marked validated") + } else { + require.NoError(t, err, "expected label %q to be accepted", tt.label) + } + }) + } +} + func TestNonInteractive_WithoutOwnerLabel_BlocksPrompt(t *testing.T) { t.Parallel() in := Inputs{ diff --git a/cmd/creinit/creinit.go b/cmd/creinit/creinit.go index 45abc595..8c2a3eb8 100644 --- a/cmd/creinit/creinit.go +++ b/cmd/creinit/creinit.go @@ -167,6 +167,19 @@ func (h *handler) ValidateInputs(inputs Inputs) error { return nil } +// validateTemplateWorkflowDirs checks every workflow directory declared by a template manifest. +// The manifest comes from a remote repository, and each dir is used both as a path segment during +// scaffolding and as the workflow name substituted into workflow.yaml, so each must satisfy the +// same rules as a user-supplied workflow name. +func validateTemplateWorkflowDirs(tmpl *templaterepo.TemplateSummary) error { + for _, wf := range tmpl.Workflows { + if err := validation.IsValidWorkflowName(wf.Dir); err != nil { + return fmt.Errorf("template %q declares an invalid workflow directory %q: %w", tmpl.Name, wf.Dir, err) + } + } + return nil +} + func (h *handler) Execute(inputs Inputs) error { if !h.validated { return fmt.Errorf("handler inputs not validated") @@ -338,6 +351,13 @@ func (h *handler) Execute(inputs Inputs) error { return fmt.Errorf("no template selected") } + // Template manifests are fetched from a remote repo, so their declared workflow dirs are + // untrusted input: each becomes a path segment during scaffolding and the workflow name + // substituted into workflow.yaml. Check them all before anything is written to disk. + if err := validateTemplateWorkflowDirs(selectedTemplate); err != nil { + return err + } + // Store for telemetry (flag will be set in RunE) h.selectedTemplateName = selectedTemplate.Name diff --git a/cmd/creinit/creinit_test.go b/cmd/creinit/creinit_test.go index ea33988e..13caad71 100644 --- a/cmd/creinit/creinit_test.go +++ b/cmd/creinit/creinit_test.go @@ -1042,3 +1042,94 @@ func TestInitProjectRootFlagFindsExistingProject(t *testing.T) { GetTemplateFileListGo(), ) } + +// maliciousDirTemplate builds a template whose second declared workflow dir is untrusted. Two +// workflows are declared so the wizard skips the workflow-name step (which would need a TTY), +// and so the unsafe dir is reached through the multi-workflow scaffolding path. +func maliciousDirTemplate(name, dir string) templaterepo.TemplateSummary { + return templaterepo.TemplateSummary{ + TemplateMetadata: templaterepo.TemplateMetadata{ + Kind: "starter-template", + Name: name, + Title: "Malicious", + Description: "Template declaring an unsafe workflow dir", + Language: "go", + Category: "workflow", + Author: "Test", + License: "MIT", + Networks: []string{"ethereum-testnet-sepolia"}, + Workflows: []templaterepo.WorkflowDirEntry{ + {Dir: "safe", Description: "safe"}, + {Dir: dir, Description: "unsafe"}, + }, + }, + Path: "starter-templates/malicious", + Source: templaterepo.RepoSource{Owner: "test", Repo: "templates", Ref: "main"}, + } +} + +func TestTemplateWorkflowDirIsValidated(t *testing.T) { + tests := []struct { + name string + dir string + }{ + {name: "parent traversal", dir: "../escape"}, + {name: "absolute path", dir: "/tmp/escape"}, + {name: "nested path", dir: "a/b"}, + {name: "double quote breaks yaml scalar", dir: `a" injected: true`}, + {name: "newline", dir: "a\ninjected: true"}, + {name: "command substitution", dir: "a$(id)"}, + {name: "empty", dir: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sim := chainsim.NewSimulatedEnvironment(t) + defer sim.Close() + + tempDir := t.TempDir() + restoreCwd, err := testutil.ChangeWorkingDirectory(tempDir) + require.NoError(t, err) + defer restoreCwd() + + const templateName = "malicious-template" + registry := &mockRegistry{ + templates: []templaterepo.TemplateSummary{maliciousDirTemplate(templateName, tt.dir)}, + } + + inputs := Inputs{ + ProjectName: "victimProj", + TemplateName: templateName, + WorkflowName: "", + RpcURLs: map[string]string{"ethereum-testnet-sepolia": "https://rpc.example.com"}, + DeploymentRegistry: "private", + } + + h := newHandlerWithRegistry(sim.NewRuntimeContext(), registry) + require.NoError(t, h.ValidateInputs(inputs)) + + err = h.Execute(inputs) + require.Error(t, err, "template dir %q should be rejected", tt.dir) + require.Contains(t, err.Error(), "invalid workflow directory") + + // Nothing may be written, in or out of the project root: the check runs before + // the project directory is created and before the template is scaffolded. + require.False(t, pathExistsForTest(filepath.Join(tempDir, "victimProj")), + "project root should not be created when the template is rejected") + entries, err := os.ReadDir(tempDir) + require.NoError(t, err) + require.Empty(t, entries, "no files should be written outside the project root") + }) + } +} + +func TestTemplateWorkflowDirsAcceptValidNames(t *testing.T) { + t.Parallel() + + tmpl := testMultiWorkflowTemplate + require.NoError(t, validateTemplateWorkflowDirs(&tmpl), + "the multi-workflow fixture declares valid dirs and must keep loading") + + single := testGoTemplate + require.NoError(t, validateTemplateWorkflowDirs(&single)) +} diff --git a/cmd/workflow/deploy/deploy.go b/cmd/workflow/deploy/deploy.go index 91da1284..79cd584b 100644 --- a/cmd/workflow/deploy/deploy.go +++ b/cmd/workflow/deploy/deploy.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "strings" "github.com/rs/zerolog" "github.com/spf13/cobra" @@ -38,7 +39,7 @@ type Inputs struct { OutputPath string `validate:"omitempty,filepath,ascii,max=97" cli:"--output"` WasmPath string `validate:"omitempty,file,ascii,max=2048" cli:"--wasm"` - OwnerLabel string `validate:"omitempty"` + OwnerLabel string `validate:"omitempty,owner_label" cli:"--owner-label"` SkipConfirmation bool NonInteractive bool // SkipTypeChecks passes --skip-type-checks to cre-compile for TypeScript workflows. @@ -163,7 +164,7 @@ func (h *handler) ResolveInputs(v *viper.Viper) (Inputs, error) { OutputPath: v.GetString("output"), WasmPath: v.GetString("wasm"), - OwnerLabel: v.GetString("owner-label"), + OwnerLabel: strings.TrimSpace(v.GetString("owner-label")), SkipConfirmation: v.GetBool(settings.Flags.SkipConfirmation.Name), NonInteractive: v.GetBool(settings.Flags.NonInteractive.Name), SkipTypeChecks: v.GetBool(cmdcommon.SkipTypeChecksCLIFlag), diff --git a/cmd/workflow/deploy/deploy_test.go b/cmd/workflow/deploy/deploy_test.go index 027b2165..ee08f895 100644 --- a/cmd/workflow/deploy/deploy_test.go +++ b/cmd/workflow/deploy/deploy_test.go @@ -100,6 +100,30 @@ func TestWorkflowDeployCommand(t *testing.T) { wantKey: "Inputs.DonFamily", wantDetail: "DonFamily is a required field", }, + { + name: "Owner Label With Quote Breaks Out Of YAML Scalar", + inputs: Inputs{ + WorkflowName: "valid_workflow", + WorkflowOwner: chainsim.TestAddress, + DonFamily: "test_label", + OwnerLabel: `bad"; echo pwned`, + }, + wantErr: true, + wantKey: "Inputs.OwnerLabel", + wantDetail: `--owner-label must be non-empty, no longer than 64 characters, start with a letter or number, and contain only letters (a-z, A-Z), numbers (0-9), spaces, dots (.), dashes (-), and underscores (_): bad"; echo pwned`, + }, + { + name: "Owner Label With Command Substitution", + inputs: Inputs{ + WorkflowName: "valid_workflow", + WorkflowOwner: chainsim.TestAddress, + DonFamily: "test_label", + OwnerLabel: "bad$(id)", + }, + wantErr: true, + wantKey: "Inputs.OwnerLabel", + wantDetail: "--owner-label must be non-empty, no longer than 64 characters, start with a letter or number, and contain only letters (a-z, A-Z), numbers (0-9), spaces, dots (.), dashes (-), and underscores (_): bad$(id)", + }, { name: "Invalid Binary URL", inputs: Inputs{ diff --git a/cmd/workflow/simulate/simulate.go b/cmd/workflow/simulate/simulate.go index 1ab3db2d..d6c86938 100644 --- a/cmd/workflow/simulate/simulate.go +++ b/cmd/workflow/simulate/simulate.go @@ -54,7 +54,7 @@ type Inputs struct { SecretsPath string `validate:"omitempty,file,ascii,max=97"` EngineLogs bool `validate:"omitempty" cli:"--engine-logs"` Broadcast bool `validate:"-"` - WorkflowName string `validate:"required"` + WorkflowName string `validate:"required,workflow_name"` // Chain-type-specific fields ChainTypeClients map[string]map[uint64]chain.ChainClient `validate:"omitempty"` ChainTypeKeys map[string]interface{} `validate:"-"` diff --git a/internal/settings/workflow_settings.go b/internal/settings/workflow_settings.go index ba5c58a3..81a87ac2 100644 --- a/internal/settings/workflow_settings.go +++ b/internal/settings/workflow_settings.go @@ -13,6 +13,7 @@ import ( "github.com/smartcontractkit/cre-cli/internal/constants" "github.com/smartcontractkit/cre-cli/internal/rpc" + "github.com/smartcontractkit/cre-cli/internal/validation" ) // GetWorkflowPathFromFile reads workflow-path from a workflow.yaml file (same value deploy/simulate get from Settings). @@ -137,6 +138,13 @@ func loadWorkflowSettings(logger *zerolog.Logger, v *viper.Viper, cmd *cobra.Com } workflowSettings.UserWorkflowSettings.WorkflowName = getSetting(WorkflowNameSettingName) + // Validate the name here rather than per-command so every consumer of the setting inherits + // the check. Only enforced when set, so commands that run without a workflow.yaml still work. + if name := workflowSettings.UserWorkflowSettings.WorkflowName; name != "" { + if err := validation.IsValidWorkflowName(name); err != nil { + return WorkflowSettings{}, errors.Wrapf(err, "invalid %s in workflow.yaml for target %q", WorkflowNameSettingName, target) + } + } workflowSettings.WorkflowArtifactSettings.WorkflowPath = getSetting(WorkflowPathSettingName) workflowSettings.WorkflowArtifactSettings.ConfigPath = getSetting(ConfigPathSettingName) workflowSettings.WorkflowArtifactSettings.SecretsPath = getSetting(SecretsPathSettingName) diff --git a/internal/settings/workflow_settings_test.go b/internal/settings/workflow_settings_test.go index 56419d10..033ff19b 100644 --- a/internal/settings/workflow_settings_test.go +++ b/internal/settings/workflow_settings_test.go @@ -3,6 +3,9 @@ package settings import ( "testing" + "github.com/rs/zerolog" + "github.com/spf13/cobra" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -71,3 +74,59 @@ func TestWorkflowPathFromRaw(t *testing.T) { assert.True(t, path == "staging.go" || path == "production.go", "got %q", path) }) } + +func TestLoadWorkflowSettingsValidatesWorkflowName(t *testing.T) { + t.Parallel() + + const target = "staging" + + // A command named "hash" makes ShouldSkipGetOwner return true, so the loader skips owner + // derivation and we can exercise workflow-name validation in isolation. + newViper := func(t *testing.T, setName bool, name string) *viper.Viper { + t.Helper() + v := viper.New() + v.Set(Flags.Target.Name, target) + v.Set(target+".user-workflow.workflow-owner-address", "0x0000000000000000000000000000000000000001") + if setName { + v.Set(target+"."+WorkflowNameSettingName, name) + } + return v + } + + tests := []struct { + name string + setName bool + value string + wantError bool + }{ + {name: "valid name loads", setName: true, value: "my-workflow", wantError: false}, + {name: "absent name still loads", setName: false, wantError: false}, + {name: "empty name still loads", setName: true, value: "", wantError: false}, + {name: "quote injects yaml", setName: true, value: "a\"\ninjected: true", wantError: true}, + {name: "newline", setName: true, value: "a\ninjected: true", wantError: true}, + {name: "path traversal", setName: true, value: "../../escape", wantError: true}, + {name: "space", setName: true, value: "my workflow", wantError: true}, + {name: "command substitution", setName: true, value: "a$(id)", wantError: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + logger := zerolog.Nop() + cmd := &cobra.Command{Use: "hash"} + v := newViper(t, tt.setName, tt.value) + + got, err := loadWorkflowSettings(&logger, v, cmd, "") + + if tt.wantError { + require.Error(t, err, "expected workflow-name %q to be rejected", tt.value) + assert.Contains(t, err.Error(), WorkflowNameSettingName, + "error should name the offending setting") + assert.Contains(t, err.Error(), target, "error should name the target") + } else { + require.NoError(t, err, "expected workflow-name %q to be accepted", tt.value) + assert.Equal(t, tt.value, got.UserWorkflowSettings.WorkflowName) + } + }) + } +} diff --git a/internal/validation/validation.go b/internal/validation/validation.go index 799dbf46..4af115fa 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -4,6 +4,9 @@ import ( "errors" "fmt" "reflect" + "strconv" + "strings" + "unicode" "github.com/go-playground/locales/en" ut "github.com/go-playground/universal-translator" @@ -17,6 +20,7 @@ var customValidators = map[string]validator.Func{ "ecdsa_private_key": isECDSAPrivateKey, "uint8_string_array": isUint8Array, "json": files.IsValidJSON, + "owner_label": isOwnerLabel, "path_read": files.HasReadAccessToPath, "project_name": isProjectName, "wasm": files.IsValidWASM, @@ -37,6 +41,7 @@ var customTranslations = map[string]string{ "http_url": "{0} must be a valid HTTP URL: {1}", "http_url|eq=": "{0} must be empty or a valid HTTP URL: {1}", "json": "{0} must be a valid JSON file: {1}", + "owner_label": "{0} must be non-empty, no longer than 64 characters, start with a letter or number, and contain only letters (a-z, A-Z), numbers (0-9), spaces, dots (.), dashes (-), and underscores (_): {1}", "path_read": "{0} must have read access to path: {1}", "workflow_path_read": "{0} must have read access to path: {1}", "project_name": "{0} must be non-empty, no longer than 64 characters, and contain only letters (a-z, A-Z), numbers (0-9), dashes (-), and underscores (_): {1}", @@ -121,7 +126,7 @@ func (v *Validator) RegisterCustomTranslation(tag, msg string) error { return ut.Add(tag, msg, true) }, func(ut ut.Translator, fe validator.FieldError) string { - t, _ := ut.T(tag, fe.Field(), fmt.Sprintf("%v", fe.Value())) + t, _ := ut.T(tag, fe.Field(), safeErrorValue(fe.Value())) return t }, ) @@ -174,6 +179,21 @@ func (v *Validator) ParseValidationErrors(err error) ValidationErrors { return ves } +// safeErrorValue renders a rejected value for inclusion in a validation message. +// These messages reach the terminal and the logs, and the rejected value is +// attacker-influenced — it can come from a workflow.yaml in a cloned repo, not +// just a flag the user typed. Emitting it verbatim would let control characters +// (ESC sequences, newlines, bidi overrides) rewrite terminal output on the error +// path, so any value containing non-printable runes is quoted and escaped. +// Ordinary values are returned unchanged to keep messages readable. +func safeErrorValue(value interface{}) string { + s := fmt.Sprintf("%v", value) + if strings.ContainsFunc(s, func(r rune) bool { return !unicode.IsPrint(r) }) { + return strconv.Quote(s) + } + return s +} + func registerDefaultTranslations(v *validator.Validate, trans ut.Translator) error { // Register default translations for all built-in tags if err := en_translations.RegisterDefaultTranslations(v, trans); err != nil { @@ -186,7 +206,7 @@ func registerDefaultTranslations(v *validator.Validate, trans ut.Translator) err return ut.Add(tag, message, true) }, func(ut ut.Translator, fe validator.FieldError) string { - t, _ := ut.T(tag, fe.Field(), fmt.Sprintf("%v", fe.Value())) + t, _ := ut.T(tag, fe.Field(), safeErrorValue(fe.Value())) return t }, ); err != nil { diff --git a/internal/validation/workflow.go b/internal/validation/workflow.go index be64c44f..c33ef41a 100644 --- a/internal/validation/workflow.go +++ b/internal/validation/workflow.go @@ -12,11 +12,18 @@ import ( const ( maxWorkflowNameLength = 64 maxProjectNameLength = 64 + maxOwnerLabelLength = 64 ) // ValidNameRegex matches only letters (upper and lower case), numbers, dashes, and underscores var ValidNameRegex = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`) +// ValidOwnerLabelRegex matches human-readable labels: letters, numbers, spaces, dots, dashes and +// underscores, and must start with a letter or number. Quotes, shell metacharacters, path +// separators, control characters and non-ASCII bytes are all excluded so that a label can never +// break out of the YAML scalar it is substituted into, nor be mistaken for a flag or a path. +var ValidOwnerLabelRegex = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9 ._-]*$`) + func isWorkflowOwner(fl validator.FieldLevel) bool { field := fl.Field() if field.Kind() != reflect.String { @@ -47,6 +54,16 @@ func isWorkflowName(fl validator.FieldLevel) bool { return IsValidWorkflowName(workflowName) == nil } +func isOwnerLabel(fl validator.FieldLevel) bool { + field := fl.Field() + if field.Kind() != reflect.String { + panic(fmt.Sprintf("input field name is not a string: %s", fl.FieldName())) + } + + ownerLabel := fl.Field().String() + return IsValidOwnerLabel(ownerLabel) == nil +} + func IsValidProjectName(projectName string) error { if projectName == "" { return fmt.Errorf("project name can't be an empty string") @@ -78,3 +95,19 @@ func IsValidWorkflowName(workflowName string) error { return nil } + +func IsValidOwnerLabel(ownerLabel string) error { + if ownerLabel == "" { + return fmt.Errorf("owner label can't be an empty string") + } + + if len(ownerLabel) > maxOwnerLabelLength { + return fmt.Errorf("owner label is too long, limit is %d characters", maxOwnerLabelLength) + } + + if !ValidOwnerLabelRegex.MatchString(ownerLabel) { + return fmt.Errorf("owner label must start with a letter or number and can only contain letters (a-z, A-Z), numbers (0-9), spaces, dots (.), dashes (-), and underscores (_)") + } + + return nil +} diff --git a/internal/validation/workflow_test.go b/internal/validation/workflow_test.go index a5c6cfda..c011609a 100644 --- a/internal/validation/workflow_test.go +++ b/internal/validation/workflow_test.go @@ -1,9 +1,13 @@ package validation import ( + "strconv" + "strings" "testing" + "unicode" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestValidateWorkflowOwner(t *testing.T) { @@ -267,6 +271,218 @@ func TestValidateProjectName(t *testing.T) { } } +func TestIsValidOwnerLabel(t *testing.T) { + tests := []struct { + name string + input string + wantError bool + }{ + // Accepted: human-readable labels. + {name: "simple", input: "Alice", wantError: false}, + {name: "with spaces", input: "prod owner 1", wantError: false}, + {name: "with dot and underscore", input: "team.alpha_v2", wantError: false}, + {name: "with dash", input: "my-label", wantError: false}, + {name: "digit leading", input: "1st owner", wantError: false}, + {name: "at max length", input: strings.Repeat("a", 64), wantError: false}, + + // Rejected: length and emptiness. + {name: "empty", input: "", wantError: true}, + {name: "over max length", input: strings.Repeat("a", 65), wantError: true}, + + // Rejected: leading character must be alphanumeric, so a label can never be + // mistaken for a flag and cannot carry leading whitespace. + {name: "leading dash looks like a flag", input: "-rf /", wantError: true}, + {name: "leading space", input: " label", wantError: true}, + {name: "leading dot", input: ".label", wantError: true}, + + // Rejected: quotes would break out of the YAML scalar they are substituted into. + {name: "double quote", input: `a"; rm -rf .`, wantError: true}, + {name: "single quote", input: "Alice's team", wantError: true}, + {name: "backtick", input: "a`id`", wantError: true}, + + // Rejected: shell metacharacters. + {name: "command substitution", input: "a$(whoami)", wantError: true}, + {name: "semicolon", input: "a;b", wantError: true}, + {name: "pipe", input: "a|b", wantError: true}, + {name: "ampersand", input: "a&b", wantError: true}, + {name: "parens", input: "prod (us-east)", wantError: true}, + {name: "comma", input: "a,b", wantError: true}, + + // Rejected: path separators and traversal. + {name: "forward slash", input: "a/b", wantError: true}, + {name: "parent traversal", input: "../x", wantError: true}, + {name: "backslash", input: `a\b`, wantError: true}, + + // Rejected: control characters and non-ASCII. + {name: "newline", input: "line1\nline2", wantError: true}, + {name: "carriage return", input: "line1\rline2", wantError: true}, + {name: "tab", input: "a\tb", wantError: true}, + {name: "ansi escape", input: "a\x1b[31m", wantError: true}, + {name: "non-ascii", input: "café", wantError: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := IsValidOwnerLabel(tt.input) + if tt.wantError { + assert.Error(t, err, "expected %q to be rejected", tt.input) + } else { + assert.NoError(t, err, "expected %q to be accepted", tt.input) + } + }) + } +} + +func TestValidateOwnerLabel(t *testing.T) { + validator, err := NewValidator() + assert.NoError(t, err, "Expected no error during validator initialization") + + type OwnerLabelTestStruct struct { + OwnerLabel string `validate:"owner_label"` + } + + const wantDetailPrefix = "OwnerLabel must be non-empty, no longer than 64 characters, start with a letter or number, and contain only letters (a-z, A-Z), numbers (0-9), spaces, dots (.), dashes (-), and underscores (_): " + + tests := []struct { + name string + input OwnerLabelTestStruct + wantError bool + wantErrorKey string + wantErrorDetail string + }{ + { + name: "Valid Owner Label", + input: OwnerLabelTestStruct{OwnerLabel: "prod owner 1"}, + wantError: false, + }, + { + name: "Empty Owner Label", + input: OwnerLabelTestStruct{OwnerLabel: ""}, + wantError: true, + wantErrorKey: "OwnerLabelTestStruct.OwnerLabel", + wantErrorDetail: wantDetailPrefix, + }, + { + name: "Owner Label with injected YAML", + input: OwnerLabelTestStruct{OwnerLabel: `a" injected: true`}, + wantError: true, + wantErrorKey: "OwnerLabelTestStruct.OwnerLabel", + wantErrorDetail: wantDetailPrefix + `a" injected: true`, + }, + { + name: "Owner Label exceeds max length", + input: OwnerLabelTestStruct{OwnerLabel: strings.Repeat("a", 65)}, + wantError: true, + wantErrorKey: "OwnerLabelTestStruct.OwnerLabel", + wantErrorDetail: wantDetailPrefix + strings.Repeat("a", 65), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err = validator.Struct(tt.input) + + if !tt.wantError && err != nil { + assert.NoError(t, err, "test should not result in errors") + } + + if tt.wantError { + assert.Error(t, err, "test should result in error") + AssertErrors(t, err, tt.wantErrorKey, tt.wantErrorDetail, validator) + } + }) + } +} + +// Validation messages echo the rejected value, and that value can come from a +// workflow.yaml in a cloned repo rather than a flag the user typed. Control +// characters must never reach the terminal verbatim on the error path. +func TestValidationMessagesEscapeControlCharacters(t *testing.T) { + t.Parallel() + + validator, err := NewValidator() + assert.NoError(t, err, "Expected no error during validator initialization") + + type S struct { + OwnerLabel string `validate:"omitempty,owner_label"` + WorkflowName string `validate:"omitempty,workflow_name"` + } + + tests := []struct { + name string + input S + }{ + {name: "ansi escape in owner label", input: S{OwnerLabel: "a\x1b[31mRED"}}, + {name: "newline in owner label", input: S{OwnerLabel: "a\ninjected: true"}}, + {name: "carriage return in owner label", input: S{OwnerLabel: "a\rb"}}, + {name: "bidi override in owner label", input: S{OwnerLabel: "a\u202eb"}}, + {name: "ansi escape in workflow name", input: S{WorkflowName: "a\x1b[31mRED"}}, + {name: "newline in workflow name", input: S{WorkflowName: "a\ninjected: true"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := validator.Struct(tt.input) + require.Error(t, err, "value should be rejected") + + errs := validator.ParseValidationErrors(err) + require.NotEmpty(t, errs) + + for _, ve := range errs { + for _, r := range ve.Detail { + assert.True(t, unicode.IsPrint(r), + "message must not contain non-printable rune %q: %s", + r, strconv.Quote(ve.Detail)) + } + } + }) + } +} + +// Values without control characters must render unchanged, so ordinary messages +// stay readable and are not gratuitously quoted. +func TestValidationMessagesLeavePrintableValuesUnquoted(t *testing.T) { + t.Parallel() + + validator, err := NewValidator() + assert.NoError(t, err, "Expected no error during validator initialization") + + type S struct { + OwnerLabel string `validate:"owner_label"` + } + + err = validator.Struct(S{OwnerLabel: `bad"; echo pwned`}) + require.Error(t, err) + + errs := validator.ParseValidationErrors(err) + require.Len(t, errs, 1) + assert.Contains(t, errs[0].Detail, `bad"; echo pwned`, + "printable values should appear verbatim, not escaped") +} + +func TestOwnerLabelPanicOnNonString(t *testing.T) { + validator, err := NewValidator() + assert.NoError(t, err, "Expected no error during validator initialization") + + type InvalidTypeStruct struct { + OwnerLabel int `validate:"owner_label"` + } + + input := InvalidTypeStruct{OwnerLabel: 12345} + + defer func() { + if r := recover(); r != nil { + assert.Contains(t, r, "input field name is not a string", "Expected panic for non-string argument") + } else { + t.Error("Expected panic, but did not get one") + } + }() + + _ = validator.Struct(input) +} + func TestProjectNamePanicOnNonString(t *testing.T) { validator, err := NewValidator() assert.NoError(t, err, "Expected no error during validator initialization")