From 86b58ac89b4cfacf87de74bda8f637e7fea23d52 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 13 Jun 2026 21:00:51 -0400 Subject: [PATCH] fix: route untrusted workflow inputs through env to prevent shell injection Generated workflows interpolated workflow_dispatch inputs and pull_request event fields directly into run: shell bodies. GitHub substitutes ${{ ... }} into the script text before the shell parses it, so a value containing a single quote, backtick, or $(...) breaks out of its argument and executes as shell. This is both a correctness bug for legitimate values carrying shell metacharacters and an injection vector in the external-update receiver. Bind each untrusted value to a step-level env: variable and reference it as a quoted shell variable, matching the pattern the hotfix workflow already uses. Sites converted: - external.go: source_repo, deploy_name, environment, sha, version, artifacts - promote.go: mode input echoed in the validate step - pr_preview.go: pull_request base/head SHAs in detect-changes and plan steps Signed-off-by: Joshua Temple --- e2e/harness/multi_repo_scenario.go | 19 +++++ .../multi-repo/external-state-promotion.yaml | 12 ++++ internal/generate/external.go | 28 ++++++-- internal/generate/external_test.go | 61 ++++++++++++++++ internal/generate/pr_preview.go | 16 ++++- internal/generate/pr_preview_test.go | 33 ++++++++- internal/generate/promote.go | 8 ++- internal/generate/promote_test.go | 72 +++++++++++++++++++ 8 files changed, 235 insertions(+), 14 deletions(-) diff --git a/e2e/harness/multi_repo_scenario.go b/e2e/harness/multi_repo_scenario.go index ac8abb92..011591d5 100644 --- a/e2e/harness/multi_repo_scenario.go +++ b/e2e/harness/multi_repo_scenario.go @@ -495,6 +495,25 @@ func (r *MultiRepoRunner) assertState(ctx context.Context, repoName string, expe envName, deployName, expectedVersion, got) } } + + // Artifacts are a string->string map. Asserting them end-to-end proves + // the dispatched --artifacts JSON survived the receiver's run: shell + // verbatim, which is the contract that breaks if the value is + // interpolated into the script text instead of routed through env:. + if expectedArtifacts, ok := deployMap["artifacts"].(map[string]interface{}); ok { + actualArtifacts := mapAt(actual, "artifacts") + for k, v := range expectedArtifacts { + want, ok := v.(string) + if !ok { + continue + } + want = r.interpolate(want) + if got := stringAt(actualArtifacts, k); got != want { + return fmt.Errorf("%s.external.%s.artifacts.%s: expected %q, got %q", + envName, deployName, k, want, got) + } + } + } } } diff --git a/e2e/scenarios/multi-repo/external-state-promotion.yaml b/e2e/scenarios/multi-repo/external-state-promotion.yaml index f24184cb..0d48fbc2 100644 --- a/e2e/scenarios/multi-repo/external-state-promotion.yaml +++ b/e2e/scenarios/multi-repo/external-state-promotion.yaml @@ -86,6 +86,13 @@ steps: environment: dev sha: "${infra.head_sha}" version: "v1.1.0-rc.0" + # Shell metacharacters in the artifacts JSON: a single quote, a $(...) + # command substitution, and a backtick. If the receiver interpolated this + # input into its run: script text instead of routing it through env:, the + # single quote would break out of the argument and $(...)/`...` would + # execute. Routing through env: lets the raw JSON reach the verb intact, + # so the recorded artifact value must equal the dispatched value exactly. + artifacts: "{\"image_tag\":\"cdk-it's-$(whoami)-`id`\"}" # Step 3: Update K8s in satellite - name: update-k8s @@ -118,5 +125,10 @@ expect: external: cdk: version: "v1.1.0-rc.0" + # The artifacts payload dispatched in step 2 must round-trip through + # the receiver's env-routed run: body byte-for-byte, including the + # single quote, $(...), and backtick. + artifacts: + image_tag: "cdk-it's-$(whoami)-`id`" k8s: version: "v1.1.0-rc.1" diff --git a/internal/generate/external.go b/internal/generate/external.go index bcb877be..356b1a55 100644 --- a/internal/generate/external.go +++ b/internal/generate/external.go @@ -138,18 +138,32 @@ func (g *ExternalUpdateGenerator) writeJob(sb *strings.Builder) { writeGitConfigSteps(sb, g.config, " ") sb.WriteString("\n") - // Run external update + // Run external update. + // + // Every workflow_dispatch input is untrusted: GitHub expands ${{ ... }} into + // the run: script text before the shell parses it, so a value carrying a single + // quote, backtick, or $(...) would break out of its argument and execute as + // shell. Bind each input to a step-level env: variable and reference it as a + // quoted shell variable so the value reaches the verb as a single inert + // argument regardless of its contents. sb.WriteString(" - name: Update External State\n") + sb.WriteString(" env:\n") + sb.WriteString(" SOURCE_REPO: ${{ inputs.source_repo }}\n") + sb.WriteString(" DEPLOY_NAME: ${{ inputs.deploy_name }}\n") + sb.WriteString(" ENVIRONMENT: ${{ inputs.environment }}\n") + sb.WriteString(" SHA: ${{ inputs.sha }}\n") + sb.WriteString(" VERSION: ${{ inputs.version }}\n") + sb.WriteString(" ARTIFACTS: ${{ inputs.artifacts }}\n") sb.WriteString(" run: |\n") sb.WriteString(" cascade external update \\\n") fmt.Fprintf(sb, " --config %s \\\n", g.getManifestFilePath()) fmt.Fprintf(sb, " --manifest-key %s \\\n", g.getManifestKey()) - sb.WriteString(" --source-repo \"${{ inputs.source_repo }}\" \\\n") - sb.WriteString(" --deploy-name \"${{ inputs.deploy_name }}\" \\\n") - sb.WriteString(" --environment \"${{ inputs.environment }}\" \\\n") - sb.WriteString(" --sha \"${{ inputs.sha }}\" \\\n") - sb.WriteString(" --version \"${{ inputs.version }}\" \\\n") - sb.WriteString(" --artifacts '${{ inputs.artifacts }}'\n") + sb.WriteString(" --source-repo \"$SOURCE_REPO\" \\\n") + sb.WriteString(" --deploy-name \"$DEPLOY_NAME\" \\\n") + sb.WriteString(" --environment \"$ENVIRONMENT\" \\\n") + sb.WriteString(" --sha \"$SHA\" \\\n") + sb.WriteString(" --version \"$VERSION\" \\\n") + sb.WriteString(" --artifacts \"$ARTIFACTS\"\n") } // writeConcurrency emits a top-level concurrency: block on the external-update diff --git a/internal/generate/external_test.go b/internal/generate/external_test.go index ead1791a..06b2cacd 100644 --- a/internal/generate/external_test.go +++ b/internal/generate/external_test.go @@ -568,6 +568,67 @@ func TestExternalUpdateGenerator_HasConcurrencyBlock(t *testing.T) { assert.Contains(t, content, "cancel-in-progress: false", "external-update default must queue, not cancel") } +// TestExternalUpdateGenerator_InputsAreNotInterpolatedIntoRun asserts that the +// generated "Update External State" step never substitutes workflow_dispatch +// inputs directly into the shell script text. GitHub Actions expands ${{ ... }} +// into the run: body before the shell parses it, so a value carrying a single +// quote, backtick, or $(...) would break out of its argument and execute as +// shell. Every untrusted input must instead be bound to a step-level env: var +// and referenced as a quoted shell variable. +func TestExternalUpdateGenerator_InputsAreNotInterpolatedIntoRun(t *testing.T) { + cfg := &config.TrunkConfig{ + TrunkBranch: "master", + Environments: []string{"dev", "test", "prod"}, + External: []config.ExternalRepoConfig{ + { + Repo: "example/cdk-infra", + Ref: "main", + Deploys: []config.ExternalDeployConfig{ + {Name: "cdk", Workflow: "example/cdk-infra/.github/workflows/deploy.yaml"}, + }, + }, + }, + } + + gen := NewExternalUpdateGenerator(cfg, "/tmp") + content, err := gen.Generate() + require.NoError(t, err) + + runBody := stepRunBody(t, content, "Update External State") + + untrusted := []string{ + "${{ inputs.source_repo }}", + "${{ inputs.deploy_name }}", + "${{ inputs.environment }}", + "${{ inputs.sha }}", + "${{ inputs.version }}", + "${{ inputs.artifacts }}", + } + for _, expr := range untrusted { + assert.NotContainsf(t, runBody, expr, + "untrusted input %q must not be interpolated into the run: body; bind it to env: and reference the shell variable", expr) + } + // No ${{ inputs.* }} expansion of any kind should remain in the run body. + assert.NotContains(t, runBody, "${{ inputs.", + "no workflow_dispatch input may be interpolated into the run: shell body") + + // The values must instead flow through a step-level env: mapping... + assert.Contains(t, content, "SOURCE_REPO: ${{ inputs.source_repo }}") + assert.Contains(t, content, "DEPLOY_NAME: ${{ inputs.deploy_name }}") + assert.Contains(t, content, "ENVIRONMENT: ${{ inputs.environment }}") + assert.Contains(t, content, "SHA: ${{ inputs.sha }}") + assert.Contains(t, content, "VERSION: ${{ inputs.version }}") + assert.Contains(t, content, "ARTIFACTS: ${{ inputs.artifacts }}") + + // ...and be consumed as quoted shell variables in the verb invocation. + assert.Contains(t, runBody, "--source-repo \"$SOURCE_REPO\"") + assert.Contains(t, runBody, "--deploy-name \"$DEPLOY_NAME\"") + assert.Contains(t, runBody, "--environment \"$ENVIRONMENT\"") + assert.Contains(t, runBody, "--sha \"$SHA\"") + assert.Contains(t, runBody, "--version \"$VERSION\"") + assert.Contains(t, runBody, "--artifacts \"$ARTIFACTS\"") +} + // TestExternalUpdateGenerator_ConcurrencyOverride asserts that a manifest-level // concurrency config is forwarded to the generated external-update workflow. func TestExternalUpdateGenerator_ConcurrencyOverride(t *testing.T) { diff --git a/internal/generate/pr_preview.go b/internal/generate/pr_preview.go index a228e458..c14ed0ac 100644 --- a/internal/generate/pr_preview.go +++ b/internal/generate/pr_preview.go @@ -141,11 +141,17 @@ func (g *PRPreviewGenerator) writeValidateStep(sb *strings.Builder) { // read-only and only reports which builds/deploys this merge would trigger. func (g *PRPreviewGenerator) writeDetectStep(sb *strings.Builder) { sb.WriteString(" - name: Detect changes\n") + // pull_request event fields are attacker-influenceable on a fork PR. GitHub + // expands ${{ ... }} into the run: script before the shell parses it, so route + // the SHAs through env: and reference them as quoted shell variables. + sb.WriteString(" env:\n") + sb.WriteString(" BASE_SHA: ${{ github.event.pull_request.base.sha }}\n") + sb.WriteString(" HEAD_SHA: ${{ github.event.pull_request.head.sha }}\n") sb.WriteString(" run: |\n") sb.WriteString(" cascade detect-changes \\\n") fmt.Fprintf(sb, " --config %s \\\n", g.config.GetManifestFile()) - sb.WriteString(" --base-sha \"${{ github.event.pull_request.base.sha }}\" \\\n") - sb.WriteString(" --head-sha \"${{ github.event.pull_request.head.sha }}\" \\\n") + sb.WriteString(" --base-sha \"$BASE_SHA\" \\\n") + sb.WriteString(" --head-sha \"$HEAD_SHA\" \\\n") sb.WriteString(" > cascade-changes.json\n") sb.WriteString("\n") } @@ -156,12 +162,16 @@ func (g *PRPreviewGenerator) writeDetectStep(sb *strings.Builder) { // be cut, and which builds/deploys this merge would run. func (g *PRPreviewGenerator) writeDeployDryRunStep(sb *strings.Builder) { sb.WriteString(" - name: Compute plan (dry-run)\n") + // pull_request.head.sha is attacker-influenceable on a fork PR; route it + // through env: rather than interpolating it into the run: script. + sb.WriteString(" env:\n") + sb.WriteString(" HEAD_SHA: ${{ github.event.pull_request.head.sha }}\n") sb.WriteString(" run: |\n") sb.WriteString(" # READ-ONLY: --dry-run is enforced and never sourced from an input,\n") sb.WriteString(" # so no release is cut, no state is written, and no deploy is triggered.\n") sb.WriteString(" cascade --dry-run orchestrate setup \\\n") fmt.Fprintf(sb, " --config %s \\\n", g.config.GetManifestFile()) - sb.WriteString(" --sha \"${{ github.event.pull_request.head.sha }}\" \\\n") + sb.WriteString(" --sha \"$HEAD_SHA\" \\\n") sb.WriteString(" > cascade-plan.json\n") sb.WriteString("\n") } diff --git a/internal/generate/pr_preview_test.go b/internal/generate/pr_preview_test.go index ca6a63f9..3192a2cb 100644 --- a/internal/generate/pr_preview_test.go +++ b/internal/generate/pr_preview_test.go @@ -23,6 +23,32 @@ func prPreviewConfig(comment bool) *config.TrunkConfig { } } +// TestPRPreviewGenerator_PRFieldsAreNotInterpolatedIntoRun asserts that the +// pull_request event fields the preview workflow consumes (head/base SHAs) are +// not substituted directly into a run: shell body. Although these are SHAs, the +// values originate from the pull_request event, which is attacker-influenceable +// on a fork PR, so they are routed through env: like every other untrusted input. +func TestPRPreviewGenerator_PRFieldsAreNotInterpolatedIntoRun(t *testing.T) { + gen := NewPRPreviewGenerator(prPreviewConfig(false), "") + content, err := gen.Generate() + require.NoError(t, err) + + detectBody := stepRunBody(t, content, "Detect changes") + assert.NotContains(t, detectBody, "${{ github.event.", + "detect-changes run: body must not interpolate pull_request event fields") + assert.Contains(t, detectBody, "--base-sha \"$BASE_SHA\"") + assert.Contains(t, detectBody, "--head-sha \"$HEAD_SHA\"") + + planBody := stepRunBody(t, content, "Compute plan (dry-run)") + assert.NotContains(t, planBody, "${{ github.event.", + "plan dry-run run: body must not interpolate pull_request event fields") + assert.Contains(t, planBody, "--sha \"$HEAD_SHA\"") + + // Values bound via step env:. + assert.Contains(t, content, "BASE_SHA: ${{ github.event.pull_request.base.sha }}") + assert.Contains(t, content, "HEAD_SHA: ${{ github.event.pull_request.head.sha }}") +} + func TestPRPreviewGenerator_Disabled(t *testing.T) { // nil pr_preview gen := NewPRPreviewGenerator(&config.TrunkConfig{TrunkBranch: "main"}, "") @@ -59,9 +85,10 @@ func TestPRPreviewGenerator_Enabled(t *testing.T) { assert.Contains(t, content, "cascade detect-changes") assert.Contains(t, content, "cascade --dry-run orchestrate setup") - // Change detection runs against the PR diff (base..head). - assert.Contains(t, content, "--base-sha \"${{ github.event.pull_request.base.sha }}\"") - assert.Contains(t, content, "--head-sha \"${{ github.event.pull_request.head.sha }}\"") + // Change detection runs against the PR diff (base..head). The SHAs are bound + // to env: and passed as quoted shell variables, never interpolated into run:. + assert.Contains(t, content, "--base-sha \"$BASE_SHA\"") + assert.Contains(t, content, "--head-sha \"$HEAD_SHA\"") // Plan written to the step summary. assert.Contains(t, content, "$GITHUB_STEP_SUMMARY") diff --git a/internal/generate/promote.go b/internal/generate/promote.go index eac27df7..d032f545 100644 --- a/internal/generate/promote.go +++ b/internal/generate/promote.go @@ -713,9 +713,15 @@ func (g *PromoteGenerator) writePromoteJob(sb *strings.Builder) { fmt.Fprintf(sb, " token: %s\n", g.getReleaseTokenRef()) fmt.Fprintf(sb, " version: %s\n", g.config.GetCLIVersion()) sb.WriteString(" - name: Validate Promotion\n") + // The mode input is untrusted workflow_dispatch data. GitHub expands ${{ ... }} + // into the run: script before the shell runs it, so a mode value carrying shell + // metacharacters would break out of the echo. Bind it to env: and print the + // quoted shell variable instead. + sb.WriteString(" env:\n") + sb.WriteString(" MODE: ${{ github.event.inputs.mode }}\n") sb.WriteString(" run: |\n") sb.WriteString(" echo \"Promotion validated by preflight job\"\n") - sb.WriteString(" echo \"Mode: ${{ github.event.inputs.mode }}\"\n") + sb.WriteString(" echo \"Mode: $MODE\"\n") sb.WriteString(" echo \"Source: ${{ needs.preflight.outputs.source_env }}\"\n") sb.WriteString(" echo \"Final Env: ${{ needs.preflight.outputs.target_env }}\"\n") sb.WriteString(" echo \"::notice::Promotion validation completed successfully\"\n\n") diff --git a/internal/generate/promote_test.go b/internal/generate/promote_test.go index 455fb74b..98b10251 100644 --- a/internal/generate/promote_test.go +++ b/internal/generate/promote_test.go @@ -29,6 +29,78 @@ func concurrencyGroupLine(t *testing.T, content string) string { return "" } +// stepRunBody extracts the run: script body of the step whose "- name: " +// header matches stepName, from a generated workflow. It returns only the shell +// lines under that step's "run: |" block, stopping at the next step or key at the +// same or shallower indentation. This lets injection tests assert on what the +// shell actually sees, without false matches from a sibling env: mapping (which +// is the safe place for ${{ ... }} expansions) elsewhere in the same step. +func stepRunBody(t *testing.T, content, stepName string) string { + t.Helper() + lines := strings.Split(content, "\n") + + stepIdx := -1 + for i, line := range lines { + if strings.Contains(line, "- name: "+stepName) { + stepIdx = i + break + } + } + require.GreaterOrEqual(t, stepIdx, 0, "step %q not found in workflow", stepName) + + runIdx := -1 + for i := stepIdx + 1; i < len(lines); i++ { + trimmed := strings.TrimSpace(lines[i]) + // Stop if we hit the next step before finding this step's run: block. + if strings.HasPrefix(trimmed, "- name: ") { + break + } + if trimmed == "run: |" || trimmed == "run: |-" { + runIdx = i + break + } + } + require.GreaterOrEqual(t, runIdx, 0, "step %q has no block run: body", stepName) + + runIndent := len(lines[runIdx]) - len(strings.TrimLeft(lines[runIdx], " ")) + var body []string + for i := runIdx + 1; i < len(lines); i++ { + line := lines[i] + if strings.TrimSpace(line) == "" { + body = append(body, line) + continue + } + indent := len(line) - len(strings.TrimLeft(line, " ")) + if indent <= runIndent { + break + } + body = append(body, line) + } + return strings.Join(body, "\n") +} + +// TestPromoteGenerator_ModeInputNotInterpolatedIntoRun asserts that the +// workflow_dispatch "mode" input is not echoed into a run: shell body via +// ${{ github.event.inputs.mode }}. A mode value containing shell metacharacters +// would otherwise break out of the echo. It must be bound to env: and printed as +// a quoted shell variable. +func TestPromoteGenerator_ModeInputNotInterpolatedIntoRun(t *testing.T) { + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev", "prod"}, + } + + gen := NewPromoteGenerator(cfg, "") + content, err := gen.Generate() + require.NoError(t, err) + + body := stepRunBody(t, content, "Validate Promotion") + assert.NotContains(t, body, "${{ github.event.inputs.mode }}", + "the mode input must not be interpolated into the Validate Promotion run: body") + assert.Contains(t, content, "MODE: ${{ github.event.inputs.mode }}") + assert.Contains(t, body, "echo \"Mode: $MODE\"") +} + func TestPromoteGenerator_Generate(t *testing.T) { tests := []struct { name string