Skip to content

Harden workflow-name and owner-label validation against injection - #571

Merged
tarcisiozf merged 3 commits into
mainfrom
DEVSVCS-5077/harden-validation
Aug 6, 2026
Merged

Harden workflow-name and owner-label validation against injection#571
tarcisiozf merged 3 commits into
mainfrom
DEVSVCS-5077/harden-validation

Conversation

@anirudhwarrier

Copy link
Copy Markdown
Collaborator

What

Hardens validation of two user-supplied identifiers — the owner address label
and the workflow name — that flow into string-templated YAML, file paths, and
copy-pasteable shell hints.

Ticket: DEVSVCS-5077

Why

--owner-label had no effective validation. validate:"omitempty" on a
plain string is a no-op, so the flag accepted arbitrary bytes — quotes,
newlines, shell metacharacters — on both cre account link-key and
cre workflow deploy. The value is sent to the backend, echoed to the
terminal, and exported via telemetry. There was a standing
// TODO: Add validation for WorkflowOwnerLabel on the field.

The interactive prompt bypassed validation entirely. Execute runs after
ValidateInputs, so a label typed at the ui.Input prompt never reached the
validator at all — only flag-supplied values were ever checked, and those
weren't checked either per the above.

Workflow names were already constrained, but not everywhere. The regex
^[a-zA-Z0-9_-]+$ is tight, but only deploy, pause, delete and
activate carried the workflow_name tag. simulate was required only,
hash had no validate tags, get checked emptiness, and the value read from
workflow.yaml was never validated at load.

Remote template manifests were trusted. cre init uses a template's
declared dir as the workflow name. That value is fetched from a remote repo
and was never validated, yet it becomes a path segment during scaffolding and
is substituted into workflow.yaml.

The sink that makes this exploitable is GenerateFileFromTemplate: rendering
is a naive strings.NewReplacer, not text/template, and the target is a
double-quoted YAML scalar (workflow-name: "{{WorkflowName}}-staging"). A
value containing " or a newline escapes the scalar and injects YAML keys.

Changes

  • New owner_label validator (internal/validation/workflow.go):
    ^[a-zA-Z0-9][a-zA-Z0-9 ._-]*$, max 64. Human-readable — spaces and dots
    allowed — but no quotes, backticks, $, ;, |, /, parens, control
    characters or non-ASCII. Must start alphanumeric, so a label can never be
    read as a flag or carry leading whitespace. Mirrors the existing
    IsValidWorkflowName / isWorkflowName pair.
  • Applied on both entry points: link_key.Inputs and deploy.Inputs, with
    cli:"--owner-label" so errors name the flag. Flag reads are trimmed.
  • Prompt bypass closed: the ui.Input call validates inline, and the
    result is re-checked after the prompt returns.
  • Workflow name validated at settings load — every consumer inherits the
    check, including hash and get. simulate's tag tightened to
    required,workflow_name.
  • Template manifest dirs validated in cre init, before the project
    directory is created and before scaffolding, so a bad manifest writes
    nothing.

Workflow names keep ^[a-zA-Z0-9_-]+$ — the charset was already correct; only
enforcement was missing.

Compatibility

Settings-load validation is enforced only when the value is non-empty, so
commands that run without a workflow.yaml are unaffected. Practical risk is
low: any name that has ever been deployed already passed the same regex via
deploy's validator. Existing e2e fixtures (owner-label-1,
test-owner-label) pass the new label regex unchanged.

Testing

  • go build ./... and go vet ./... clean.
  • New table-driven tests for IsValidOwnerLabel and the owner_label tag
    covering quotes, command substitution, newlines, path traversal, ANSI
    escapes, non-ASCII and length bounds.
  • Real ValidateInputs tests for link_key (the two existing tests were
    tautological — they re-implemented the guard in the assertion).
  • Settings-load tests: an injected name fails to load; valid, empty and absent
    names still load.
  • creinit test asserting a malicious template dir is rejected and the temp
    dir is left completely empty. Verified non-vacuous: with the guard disabled
    all 7 cases fail.
  • Full suite passes

Note for reviewers

This branch was rebased onto #555, which independently added ui.WithValidate
(identical signature — my duplicate was dropped) and an email-derived default
owner label. Combining the two surfaced a bug neither has alone: an email local
part may contain characters a label may not (first+tag@…), so pre-filling the
prompt with an invalid default would block submission. defaultOwnerLabel now
returns "" when the derived value fails validation. Sanitizing the default
instead of dropping it would also be reasonable.

@anirudhwarrier anirudhwarrier self-assigned this Aug 6, 2026
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) <noreply@anthropic.com>
@anirudhwarrier
anirudhwarrier force-pushed the DEVSVCS-5077/harden-validation branch from eb88f67 to 1538603 Compare August 6, 2026 09:15
@anirudhwarrier
anirudhwarrier marked this pull request as ready for review August 6, 2026 09:36
@anirudhwarrier
anirudhwarrier requested a review from a team as a code owner August 6, 2026 09:36
@tarcisiozf
tarcisiozf requested a review from Copilot August 6, 2026 12:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request hardens validation for user-supplied identifiers (owner address label and workflow name) that flow into YAML templating, filesystem paths, and CLI output, reducing injection risk across multiple commands and settings-loading paths.

Changes:

  • Adds an owner_label validator (regex + length) and applies it to link-key and workflow deploy, including trimming flag inputs and closing the prompt-validation bypass.
  • Ensures workflow-name validation is consistently enforced by validating on settings load and tightening simulate’s input tag.
  • Validates remote template manifest workflow directories in cre init before any filesystem writes, with tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/validation/workflow.go Adds owner-label regex/length validation and validator hook.
internal/validation/workflow_test.go Adds table-driven tests for IsValidOwnerLabel and tag behavior.
internal/validation/validation.go Registers owner_label validator and adds translated error message.
internal/settings/workflow_settings.go Validates workflow name during workflow.yaml settings load.
internal/settings/workflow_settings_test.go Adds coverage for settings-load workflow-name validation.
cmd/workflow/simulate/simulate.go Tightens simulate’s workflow-name validation tag.
cmd/workflow/deploy/deploy.go Applies owner_label tag and trims --owner-label input.
cmd/workflow/deploy/deploy_test.go Adds deploy validation test cases for malicious owner labels.
cmd/creinit/creinit.go Validates template workflow dirs from remote manifests before scaffolding.
cmd/creinit/creinit_test.go Tests that malicious template dirs are rejected and write nothing.
cmd/account/link_key/link_key.go Applies owner_label validation, trims flag input, validates prompt input, and drops invalid email-derived defaults.
cmd/account/link_key/link_key_test.go Adds real ValidateInputs tests for owner-label validation behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/validation/validation.go
tarcisiozf
tarcisiozf previously approved these changes Aug 6, 2026
tarcisiozf
tarcisiozf previously approved these changes Aug 6, 2026
@anirudhwarrier
anirudhwarrier added this pull request to the merge queue Aug 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 6, 2026
@tarcisiozf
tarcisiozf added this pull request to the merge queue Aug 6, 2026
Merged via the queue into main with commit 65682fd Aug 6, 2026
22 checks passed
@tarcisiozf
tarcisiozf deleted the DEVSVCS-5077/harden-validation branch August 6, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants