diff --git a/pkg/workflow/activation_github_token_test.go b/pkg/workflow/activation_github_token_test.go index 2d8b769f919..ff5d968e7fd 100644 --- a/pkg/workflow/activation_github_token_test.go +++ b/pkg/workflow/activation_github_token_test.go @@ -166,9 +166,12 @@ func TestActivationGitHubApp(t *testing.T) { require.NotNil(t, job) stepsStr := strings.Join(job.Steps, "") - assert.Contains(t, stepsStr, "if: ${{ secrets.GH_AW_APP_ID != '' && secrets.GH_AW_APP_PRIVATE_KEY != '' }}") - assert.NotContains(t, stepsStr, "GH_AW_APP_CLIENT_ID:") - assert.NotContains(t, stepsStr, "GH_AW_APP_PRIVATE_KEY:") + // Both credentials use secrets.* so ignore-if-missing should guard on + // step-local env aliases instead of secrets.* directly. + assert.NotContains(t, stepsStr, "if: ${{ secrets.") + assert.Contains(t, stepsStr, "GH_AW_IGNORE_IF_MISSING_APP_ID: ${{ secrets.GH_AW_APP_ID }}") + assert.Contains(t, stepsStr, "GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY: ${{ secrets.GH_AW_APP_PRIVATE_KEY }}") + assert.Contains(t, stepsStr, "if: ${{ env.GH_AW_IGNORE_IF_MISSING_APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}") assert.Contains(t, stepsStr, "github-token: ${{ steps.activation-app-token.outputs.token || secrets.GITHUB_TOKEN }}") }) diff --git a/pkg/workflow/compiler_activation_daily_aic.go b/pkg/workflow/compiler_activation_daily_aic.go index c9315b6c633..99534aa05a5 100644 --- a/pkg/workflow/compiler_activation_daily_aic.go +++ b/pkg/workflow/compiler_activation_daily_aic.go @@ -25,7 +25,13 @@ func (c *Compiler) buildDailyAICAppTokenMintStep(app *GitHubAppConfig) []string steps = append(steps, " - name: Generate GitHub App token for daily AIC guardrail\n") steps = append(steps, fmt.Sprintf(" id: %s\n", dailyAICAppTokenStepID)) if app.shouldIgnoreMissingKey() { - steps = append(steps, fmt.Sprintf(" if: %s && %s\n", maxDailyAICreditsConfiguredIfExpr, buildIgnoreIfMissingCondition(app))) + guard := buildIgnoreIfMissingCondition(app) + steps = appendStepEnvAssignments(steps, guard.EnvAssignments) + if condition := combineGitHubIfExpressions(maxDailyAICreditsConfiguredIfExpr, guard.Condition); condition != "" { + steps = append(steps, fmt.Sprintf(" if: %s\n", condition)) + } else { + steps = append(steps, fmt.Sprintf(" if: %s\n", maxDailyAICreditsConfiguredIfExpr)) + } } else { steps = append(steps, fmt.Sprintf(" if: %s\n", maxDailyAICreditsConfiguredIfExpr)) } diff --git a/pkg/workflow/compiler_pre_activation_job.go b/pkg/workflow/compiler_pre_activation_job.go index 4aaddfc1fd0..174d45b5ad7 100644 --- a/pkg/workflow/compiler_pre_activation_job.go +++ b/pkg/workflow/compiler_pre_activation_job.go @@ -753,7 +753,11 @@ func (c *Compiler) buildPreActivationAppTokenMintStep(app *GitHubAppConfig) []st steps = append(steps, " - name: Generate GitHub App token for skip-if checks\n") steps = append(steps, fmt.Sprintf(" id: %s\n", tokenStepID)) if app.shouldIgnoreMissingKey() { - steps = append(steps, fmt.Sprintf(" if: %s\n", buildIgnoreIfMissingCondition(app))) + guard := buildIgnoreIfMissingCondition(app) + steps = appendStepEnvAssignments(steps, guard.EnvAssignments) + if guard.Condition != "" { + steps = append(steps, fmt.Sprintf(" if: %s\n", guard.Condition)) + } } steps = append(steps, fmt.Sprintf(" uses: %s\n", getActionPin("actions/create-github-app-token"))) steps = append(steps, " with:\n") diff --git a/pkg/workflow/safe_outputs_app_config.go b/pkg/workflow/safe_outputs_app_config.go index d77b1e6ff21..aafcb1da2fe 100644 --- a/pkg/workflow/safe_outputs_app_config.go +++ b/pkg/workflow/safe_outputs_app_config.go @@ -144,14 +144,168 @@ func buildGitHubExpressionNonEmptyCheck(value string) ConditionNode { return BuildNotEquals(BuildStringLiteral(strings.TrimSpace(githubExpressionWhitespaceReplacer.Replace(trimmed))), BuildStringLiteral("")) } +// ifInvalidContextNames lists the GitHub Actions expression contexts that are not +// available in step-level 'if:' conditions. GitHub Actions allows matrix in this +// position, but still rejects secrets and jobs references. +// See: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#contexts +var ifInvalidContextNames = map[string]struct{}{ + "jobs": {}, + "secrets": {}, +} + +const ( + ignoreIfMissingAppIDEnvVar = "GH_AW_IGNORE_IF_MISSING_APP_ID" + ignoreIfMissingPrivateKeyEnvVar = "GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY" +) + +type stepEnvAssignment struct { + Name string + Value string +} + +type ignoreIfMissingGuard struct { + Condition string + EnvAssignments []stepEnvAssignment +} + +// isGitHubExpressionIdentifierChar reports whether a byte can appear in a GitHub +// Actions expression identifier token (ASCII letter, digit, or underscore). +func isGitHubExpressionIdentifierChar(ch byte) bool { + return (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '_' +} + +// isGitHubExpressionIdentifierStart reports whether inner[i] begins an identifier +// token rather than landing in the middle of one. +func isGitHubExpressionIdentifierStart(inner string, i int) bool { + if i >= len(inner) { + return false + } + return isGitHubExpressionIdentifierChar(inner[i]) && (i == 0 || !isGitHubExpressionIdentifierChar(inner[i-1])) +} + +// consumeSingleQuotedGitHubExpressionString skips over a single-quoted GitHub +// expression string literal, honoring doubled single quotes as escapes. It returns +// the first byte position after the closing quote, or len(inner) if unterminated. +func consumeSingleQuotedGitHubExpressionString(inner string, start int) int { + i := start + 1 + for i < len(inner) { + if inner[i] != '\'' { + i++ + continue + } + if i+1 < len(inner) && inner[i+1] == '\'' { + i += 2 + continue + } + return i + 1 + } + return i +} + +// containsInvalidIfContextReference returns true when the inner expression body +// contains a jobs or secrets context token anywhere outside single-quoted string +// literals, including bracket notation such as secrets['TOKEN']. +func containsInvalidIfContextReference(inner string) bool { + for i := 0; i < len(inner); { + if inner[i] == '\'' { + i = consumeSingleQuotedGitHubExpressionString(inner, i) + continue + } + + if !isGitHubExpressionIdentifierStart(inner, i) { + i++ + continue + } + + start := i + for i < len(inner) && isGitHubExpressionIdentifierChar(inner[i]) { + i++ + } + name := inner[start:i] + if _, ok := ifInvalidContextNames[name]; !ok { + continue + } + + j := i + for j < len(inner) && (inner[j] == ' ' || inner[j] == '\t' || inner[j] == '\n' || inner[j] == '\r') { + j++ + } + if j < len(inner) && (inner[j] == '.' || inner[j] == '[') { + return true + } + } + return false +} + +// combineGitHubIfExpressions accepts either wrapped `${{ ... }}` conditions or raw +// inner expression fragments and normalizes them into one wrapped if-expression. +func combineGitHubIfExpressions(expressions ...string) string { + var parts []string + for _, expression := range expressions { + trimmed := strings.TrimSpace(expression) + if trimmed == "" { + continue + } + if inner, ok := extractWrappedGitHubExpression(trimmed); ok { + parts = append(parts, inner) + continue + } + parts = append(parts, trimmed) + } + if len(parts) == 0 { + return "" + } + return wrapGitHubExpression(strings.Join(parts, " && ")) +} + +func appendStepEnvAssignments(steps []string, assignments []stepEnvAssignment) []string { + if len(assignments) == 0 { + return steps + } + steps = append(steps, " env:\n") + for _, assignment := range assignments { + steps = append(steps, fmt.Sprintf(" %s: %s\n", assignment.Name, assignment.Value)) + } + return steps +} + // buildIgnoreIfMissingCondition returns a GitHub Actions if-expression that requires -// both GitHub App credential inputs to be non-empty. -func buildIgnoreIfMissingCondition(app *GitHubAppConfig) string { - condition := BuildAnd( - buildGitHubExpressionNonEmptyCheck(app.AppID), - buildGitHubExpressionNonEmptyCheck(app.PrivateKey), - ) - return wrapGitHubExpression(RenderCondition(condition)) +// all GitHub App credential inputs that can be checked in an if: condition to be non-empty. +// Values referencing the secrets or jobs contexts are routed through step-local env +// aliases so the guard can still check them through the supported env context. +func buildIgnoreIfMissingCondition(app *GitHubAppConfig) ignoreIfMissingGuard { + var checks []ConditionNode + guard := ignoreIfMissingGuard{} + for _, credential := range []struct { + value string + envName string + }{ + {value: app.AppID, envName: ignoreIfMissingAppIDEnvVar}, + {value: app.PrivateKey, envName: ignoreIfMissingPrivateKeyEnvVar}, + } { + trimmed := strings.TrimSpace(credential.value) + if inner, ok := extractWrappedGitHubExpression(trimmed); ok { + if containsInvalidIfContextReference(inner) { + safeOutputsAppLog.Printf("Rewriting %q in ignore-if-missing condition through env.%s: context not valid in if: expressions", inner, credential.envName) + guard.EnvAssignments = append(guard.EnvAssignments, stepEnvAssignment{ + Name: credential.envName, + Value: trimmed, + }) + checks = append(checks, BuildNotEquals(&ExpressionNode{Expression: "env." + credential.envName}, BuildStringLiteral(""))) + continue + } + } + checks = append(checks, buildGitHubExpressionNonEmptyCheck(credential.value)) + } + if len(checks) == 0 { + return guard + } + condition := checks[0] + for i := 1; i < len(checks); i++ { + condition = BuildAnd(condition, checks[i]) + } + guard.Condition = wrapGitHubExpression(RenderCondition(condition)) + return guard } // ======================================== @@ -226,7 +380,11 @@ func (c *Compiler) buildGitHubAppTokenMintStepWithMeta(app *GitHubAppConfig, per steps = append(steps, fmt.Sprintf(" - name: %s\n", stepName)) steps = append(steps, fmt.Sprintf(" id: %s\n", stepID)) if app.shouldIgnoreMissingKey() { - steps = append(steps, fmt.Sprintf(" if: %s\n", buildIgnoreIfMissingCondition(app))) + guard := buildIgnoreIfMissingCondition(app) + steps = appendStepEnvAssignments(steps, guard.EnvAssignments) + if guard.Condition != "" { + steps = append(steps, fmt.Sprintf(" if: %s\n", guard.Condition)) + } } steps = append(steps, fmt.Sprintf(" uses: %s\n", getActionPin("actions/create-github-app-token"))) steps = append(steps, " with:\n") @@ -486,7 +644,11 @@ func (c *Compiler) buildActivationAppTokenMintStep(app *GitHubAppConfig, permiss steps = append(steps, " - name: Generate GitHub App token for activation\n") steps = append(steps, " id: activation-app-token\n") if app.shouldIgnoreMissingKey() { - steps = append(steps, fmt.Sprintf(" if: %s\n", buildIgnoreIfMissingCondition(app))) + guard := buildIgnoreIfMissingCondition(app) + steps = appendStepEnvAssignments(steps, guard.EnvAssignments) + if guard.Condition != "" { + steps = append(steps, fmt.Sprintf(" if: %s\n", guard.Condition)) + } } steps = append(steps, fmt.Sprintf(" uses: %s\n", getActionPin("actions/create-github-app-token"))) steps = append(steps, " with:\n") diff --git a/pkg/workflow/safe_outputs_app_test.go b/pkg/workflow/safe_outputs_app_test.go index 9683f752e10..88d6b22efdd 100644 --- a/pkg/workflow/safe_outputs_app_test.go +++ b/pkg/workflow/safe_outputs_app_test.go @@ -117,9 +117,12 @@ safe-outputs: require.NotNil(t, job, "Job should not be nil") stepsStr := strings.Join(job.Steps, "") - assert.Contains(t, stepsStr, "if: ${{ secrets.GH_AW_APP_ID != '' && secrets.GH_AW_APP_PRIVATE_KEY != '' }}") - assert.NotContains(t, stepsStr, "GH_AW_APP_CLIENT_ID:") - assert.NotContains(t, stepsStr, "GH_AW_APP_PRIVATE_KEY:") + // Both credentials use secrets.* context, so the ignore-if-missing guard must + // check step-local env aliases instead of emitting secrets.* directly in if:. + assert.NotContains(t, stepsStr, "if: ${{ secrets.") + assert.Contains(t, stepsStr, "GH_AW_IGNORE_IF_MISSING_APP_ID: ${{ secrets.GH_AW_APP_ID }}") + assert.Contains(t, stepsStr, "GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY: ${{ secrets.GH_AW_APP_PRIVATE_KEY }}") + assert.Contains(t, stepsStr, "if: ${{ env.GH_AW_IGNORE_IF_MISSING_APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}") assert.Contains(t, stepsStr, "github-token: ${{ steps.safe-outputs-app-token.outputs.token || secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}") } @@ -140,19 +143,77 @@ func TestBuildIgnoreIfMissingCondition(t *testing.T) { name string appID string privateKey string - expected string + expected ignoreIfMissingGuard }{ { - name: "wrapped expressions", + name: "both secrets - route through env aliases", appID: "${{ secrets.GH_AW_APP_ID }}", privateKey: "${{ secrets.GH_AW_APP_PRIVATE_KEY }}", - expected: "${{ secrets.GH_AW_APP_ID != '' && secrets.GH_AW_APP_PRIVATE_KEY != '' }}", + expected: ignoreIfMissingGuard{ + Condition: "${{ env.GH_AW_IGNORE_IF_MISSING_APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}", + EnvAssignments: []stepEnvAssignment{ + {Name: ignoreIfMissingAppIDEnvVar, Value: "${{ secrets.GH_AW_APP_ID }}"}, + {Name: ignoreIfMissingPrivateKeyEnvVar, Value: "${{ secrets.GH_AW_APP_PRIVATE_KEY }}"}, + }, + }, + }, + { + name: "vars client-id + secrets private-key", + appID: "${{ vars.APP_CLIENT_ID }}", + privateKey: "${{ secrets.APP_PRIVATE_KEY }}", + expected: ignoreIfMissingGuard{ + Condition: "${{ vars.APP_CLIENT_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}", + EnvAssignments: []stepEnvAssignment{ + {Name: ignoreIfMissingPrivateKeyEnvVar, Value: "${{ secrets.APP_PRIVATE_KEY }}"}, + }, + }, + }, + { + name: "both vars", + appID: "${{ vars.APP_CLIENT_ID }}", + privateKey: "${{ vars.APP_PRIVATE_KEY }}", + expected: ignoreIfMissingGuard{ + Condition: "${{ vars.APP_CLIENT_ID != '' && vars.APP_PRIVATE_KEY != '' }}", + }, }, { name: "literal values", appID: " id value ", privateKey: "key'value", - expected: "${{ 'id value' != '' && 'key''value' != '' }}", + expected: ignoreIfMissingGuard{ + Condition: "${{ 'id value' != '' && 'key''value' != '' }}", + }, + }, + { + name: "matrix remains valid in step if", + appID: "${{ matrix.app_id }}", + privateKey: "${{ matrix.key }}", + expected: ignoreIfMissingGuard{ + Condition: "${{ matrix.app_id != '' && matrix.key != '' }}", + }, + }, + { + name: "jobs context with bracket syntax routes through env aliases", + appID: "${{ jobs.build.outputs['app-id'] }}", + privateKey: "${{ jobs.build.outputs['private-key'] }}", + expected: ignoreIfMissingGuard{ + Condition: "${{ env.GH_AW_IGNORE_IF_MISSING_APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}", + EnvAssignments: []stepEnvAssignment{ + {Name: ignoreIfMissingAppIDEnvVar, Value: "${{ jobs.build.outputs['app-id'] }}"}, + {Name: ignoreIfMissingPrivateKeyEnvVar, Value: "${{ jobs.build.outputs['private-key'] }}"}, + }, + }, + }, + { + name: "composite expression with secrets routes through env alias", + appID: "${{ vars.APP_ID || secrets.FALLBACK_ID }}", + privateKey: "${{ vars.APP_KEY }}", + expected: ignoreIfMissingGuard{ + Condition: "${{ env.GH_AW_IGNORE_IF_MISSING_APP_ID != '' && vars.APP_KEY != '' }}", + EnvAssignments: []stepEnvAssignment{ + {Name: ignoreIfMissingAppIDEnvVar, Value: "${{ vars.APP_ID || secrets.FALLBACK_ID }}"}, + }, + }, }, } diff --git a/pkg/workflow/skip_if_match_test.go b/pkg/workflow/skip_if_match_test.go index eacef972dd1..79c5d6fe336 100644 --- a/pkg/workflow/skip_if_match_test.go +++ b/pkg/workflow/skip_if_match_test.go @@ -485,14 +485,19 @@ engine: claude } lockContentStr := string(lockContent) - if !strings.Contains(lockContentStr, "if: ${{ secrets.WORKFLOW_APP_ID != '' && secrets.WORKFLOW_APP_PRIVATE_KEY != '' }}") { - t.Error("Expected guard to check app secrets directly when ignore-if-missing is enabled") + // Both credentials use secrets.* so ignore-if-missing should guard on + // step-local env aliases instead of secrets.* directly. + if strings.Contains(lockContentStr, "if: ${{ secrets.") { + t.Error("Expected no guard using secrets context: secrets is not valid in if: expressions") } - if strings.Contains(lockContentStr, "GH_AW_APP_CLIENT_ID:") { - t.Error("Did not expect step-local GH_AW_APP_CLIENT_ID env in mint step guard") + if !strings.Contains(lockContentStr, "GH_AW_IGNORE_IF_MISSING_APP_ID: ${{ secrets.WORKFLOW_APP_ID }}") { + t.Error("Expected step-local env alias for app ID in mint step guard") } - if strings.Contains(lockContentStr, "GH_AW_APP_PRIVATE_KEY:") { - t.Error("Did not expect step-local GH_AW_APP_PRIVATE_KEY env in mint step guard") + if !strings.Contains(lockContentStr, "GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY: ${{ secrets.WORKFLOW_APP_PRIVATE_KEY }}") { + t.Error("Expected step-local env alias for private key in mint step guard") + } + if !strings.Contains(lockContentStr, "if: ${{ env.GH_AW_IGNORE_IF_MISSING_APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}") { + t.Error("Expected skip-if-match app token mint step to use env-based ignore-if-missing guard") } if !strings.Contains(lockContentStr, "github-token: ${{ steps.pre-activation-app-token.outputs.token || secrets.GITHUB_TOKEN }}") { t.Error("Expected skip-if-match to fall back to GITHUB_TOKEN when app minting is skipped")