From 1538603107ac12fef03015f56b7ff4df5b3f11a4 Mon Sep 17 00:00:00 2001 From: anirudhwarrier <12178754+anirudhwarrier@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:56:09 +0400 Subject: [PATCH 1/3] Harden workflow-name and owner-label validation against injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The --owner-label flag had no effective validation: `validate:"omitempty"` on a plain string is a no-op, so it accepted arbitrary bytes including quotes, newlines and shell metacharacters. Add an owner_label validator (letters, numbers, spaces, dots, dashes, underscores; must start alphanumeric; max 64) and apply it on both `cre account link-key` and `cre workflow deploy`, trimming surrounding whitespace first. Also close the interactive bypass: Execute runs after ValidateInputs, so a label supplied at the ui.Input prompt never reached the validator at all. Add a WithValidate option to ui.Input (only InputForm supported one) for inline feedback, and re-check the value after the prompt returns. Workflow names already enforced ^[a-zA-Z0-9_-]+$, but only deploy, pause, delete and activate checked it. Validate the name once at settings load so hash, get and every future consumer inherit it, and tighten simulate's tag. Enforced only when the setting is non-empty, so commands that run without a workflow.yaml are unaffected; anything ever deployed already passed the same regex. Validate the workflow dirs declared by remote template manifests too. These were never checked, yet each becomes a path segment during scaffolding and the workflow name substituted into workflow.yaml — where rendering is a naive strings.NewReplacer into a double-quoted YAML scalar, so a quote or newline could inject arbitrary keys. The check runs before the project directory is created, so a bad manifest writes nothing. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/account/link_key/link_key.go | 32 +++-- cmd/account/link_key/link_key_test.go | 46 +++++++ cmd/creinit/creinit.go | 20 +++ cmd/creinit/creinit_test.go | 91 ++++++++++++ cmd/workflow/deploy/deploy.go | 5 +- cmd/workflow/deploy/deploy_test.go | 24 ++++ cmd/workflow/simulate/simulate.go | 2 +- internal/settings/workflow_settings.go | 8 ++ internal/settings/workflow_settings_test.go | 59 ++++++++ internal/validation/validation.go | 2 + internal/validation/workflow.go | 33 +++++ internal/validation/workflow_test.go | 145 ++++++++++++++++++++ 12 files changed, 454 insertions(+), 13 deletions(-) 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..4df32ff8 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -17,6 +17,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 +38,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}", 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..dff98751 100644 --- a/internal/validation/workflow_test.go +++ b/internal/validation/workflow_test.go @@ -1,6 +1,7 @@ package validation import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -267,6 +268,150 @@ 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) + } + }) + } +} + +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") From ffd1fd578acaa7e5ed2a9f2fa58890a82be64b04 Mon Sep 17 00:00:00 2001 From: anirudhwarrier <12178754+anirudhwarrier@users.noreply.github.com> Date: Thu, 6 Aug 2026 18:29:30 +0400 Subject: [PATCH 2/3] add test --- internal/validation/validation.go | 22 ++++++++- internal/validation/workflow_test.go | 71 ++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/internal/validation/validation.go b/internal/validation/validation.go index 4df32ff8..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" @@ -123,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 }, ) @@ -176,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 { @@ -188,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_test.go b/internal/validation/workflow_test.go index dff98751..787eafa7 100644 --- a/internal/validation/workflow_test.go +++ b/internal/validation/workflow_test.go @@ -1,10 +1,13 @@ package validation import ( + "strconv" "strings" "testing" + "unicode" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestValidateWorkflowOwner(t *testing.T) { @@ -391,6 +394,74 @@ func TestValidateOwnerLabel(t *testing.T) { } } +// 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‮b"}}, + {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") From fc2368be7cbd98dadfc401c5f747bf4e6fa0fb24 Mon Sep 17 00:00:00 2001 From: Tarcisio Ferraz Date: Thu, 6 Aug 2026 12:04:17 -0300 Subject: [PATCH 3/3] fix lint --- internal/validation/workflow_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/validation/workflow_test.go b/internal/validation/workflow_test.go index 787eafa7..c011609a 100644 --- a/internal/validation/workflow_test.go +++ b/internal/validation/workflow_test.go @@ -415,7 +415,7 @@ func TestValidationMessagesEscapeControlCharacters(t *testing.T) { {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‮b"}}, + {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"}}, }