diff --git a/pkg/workflow/approve_workflow_run_test.go b/pkg/workflow/approve_workflow_run_test.go index 036527ce9eb..25cf9206bca 100644 --- a/pkg/workflow/approve_workflow_run_test.go +++ b/pkg/workflow/approve_workflow_run_test.go @@ -235,7 +235,7 @@ func TestApproveWorkflowRunAllowedPullRequestsExpressionEmitsJSONArray(t *testin runtimeConfig := strings.ReplaceAll(string(configJSON), wrappedExpression, `""`) var config map[string]map[string]any require.NoError(t, json.Unmarshal([]byte(runtimeConfig), &config)) - assert.Equal(t, "", config["approve_workflow_run"]["allowed_pull_requests"]) + assert.Empty(t, config["approve_workflow_run"]["allowed_pull_requests"]) }) } diff --git a/pkg/workflow/config_helpers.go b/pkg/workflow/config_helpers.go index 3b953a44d4e..0ab1eb01d6e 100644 --- a/pkg/workflow/config_helpers.go +++ b/pkg/workflow/config_helpers.go @@ -128,13 +128,13 @@ func extractStringFromMap(m map[string]any, key string, debugLog *logger.Logger) // Returns an error (indicated by the second return value being true) if the value is "*" (wildcard), // which is not allowed for safe output target repositories. func parseTargetRepoWithValidation(configMap map[string]any) (string, bool) { - targetRepoSlug := extractStringFromMap(configMap, "target-repo", configHelpersLog) - // Validate that target-repo is not "*" - only definite strings are allowed - if targetRepoSlug == "*" { - configHelpersLog.Print("Invalid target-repo: wildcard '*' is not allowed") - return "", true // Return true to indicate validation error + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, configHelpersLog, safeOutputTargetConfigOptions{ + parseTargetRepo: true, + }) + if isInvalid { + return "", true } - return targetRepoSlug, false + return targetConfig.TargetRepoSlug, false } // NOTE: parseExpiresFromConfig and parseRelativeTimeSpec have been moved to time_delta.go diff --git a/pkg/workflow/config_parsing_helpers_test.go b/pkg/workflow/config_parsing_helpers_test.go index dba17faaaaa..85a643c090c 100644 --- a/pkg/workflow/config_parsing_helpers_test.go +++ b/pkg/workflow/config_parsing_helpers_test.go @@ -410,7 +410,8 @@ func TestParsePRReviewCommentsConfigWithHelpers(t *testing.T) { compiler := &Compiler{} outputMap := map[string]any{ "create-pull-request-review-comment": map[string]any{ - "target-repo": "company/codebase", + "target-repo": "company/codebase", + "allowed-repos": []any{"company/codebase", "company/other"}, }, } @@ -422,6 +423,32 @@ func TestParsePRReviewCommentsConfigWithHelpers(t *testing.T) { if result.TargetRepoSlug != "company/codebase" { t.Errorf("expected target-repo 'company/codebase', got %q", result.TargetRepoSlug) } + if len(result.AllowedRepos) != 2 || result.AllowedRepos[0] != "company/codebase" || result.AllowedRepos[1] != "company/other" { + t.Errorf("expected parsed allowed-repos, got %v", result.AllowedRepos) + } +} + +func TestParsePRReviewCommentsConfigPreservesAllowedReposExpression(t *testing.T) { + compiler := &Compiler{} + expr := "${{ inputs['allowed-repos'] }}" + outputMap := map[string]any{ + "create-pull-request-review-comment": map[string]any{ + "target-repo": "${{ inputs.target_repo }}", + "allowed-repos": expr, + }, + } + + result := compiler.parsePullRequestReviewCommentsConfig(outputMap) + if result == nil { + t.Fatal("expected non-nil result") + } + + if result.TargetRepoSlug != "${{ inputs.target_repo }}" { + t.Errorf("expected target-repo expression, got %q", result.TargetRepoSlug) + } + if len(result.AllowedRepos) != 1 || result.AllowedRepos[0] != expr { + t.Errorf("expected allowed-repos expression to be preserved, got %v", result.AllowedRepos) + } } // Test wildcard target-repo is now allowed for all create/close handlers @@ -565,6 +592,101 @@ func TestParseTargetRepoWithValidation(t *testing.T) { } } +func TestParseSafeOutputTargetConfigOptions(t *testing.T) { + t.Run("ignores target repo without option", func(t *testing.T) { + config, isInvalid := parseSafeOutputTargetConfig(map[string]any{ + "target-repo": "owner/repo", + }, nil, safeOutputTargetConfigOptions{}) + + if isInvalid { + t.Fatal("expected config to be valid") + } + if config.TargetRepoSlug != "" { + t.Fatalf("expected target-repo to be ignored, got %q", config.TargetRepoSlug) + } + }) + + t.Run("parses target repo and allowed repos without target", func(t *testing.T) { + config, isInvalid := parseSafeOutputTargetConfig(map[string]any{ + "target": "17", + "target-repo": "owner/repo", + "allowed-repos": []any{"owner/repo", "owner/other"}, + }, nil, safeOutputTargetConfigOptions{ + parseTargetRepo: true, + parseAllowedRepos: true, + }) + + if isInvalid { + t.Fatal("expected config to be valid") + } + if config.Target != "" { + t.Fatalf("expected target to be ignored, got %q", config.Target) + } + if config.TargetRepoSlug != "owner/repo" { + t.Fatalf("expected target-repo owner/repo, got %q", config.TargetRepoSlug) + } + if len(config.AllowedRepos) != 2 || config.AllowedRepos[0] != "owner/repo" || config.AllowedRepos[1] != "owner/other" { + t.Fatalf("expected parsed allowed-repos, got %v", config.AllowedRepos) + } + }) + + t.Run("rejects wildcard target repo by default", func(t *testing.T) { + _, isInvalid := parseSafeOutputTargetConfig(map[string]any{ + "target-repo": "*", + }, nil, safeOutputTargetConfigOptions{ + parseTargetRepo: true, + }) + + if !isInvalid { + t.Fatal("expected wildcard target-repo to be invalid") + } + }) + + t.Run("allows wildcard target repo when configured", func(t *testing.T) { + config, isInvalid := parseSafeOutputTargetConfig(map[string]any{ + "target-repo": "*", + }, nil, safeOutputTargetConfigOptions{ + parseTargetRepo: true, + allowTargetRepoWildcard: true, + }) + + if isInvalid { + t.Fatal("expected wildcard target-repo to be valid") + } + if config.TargetRepoSlug != "*" { + t.Fatalf("expected wildcard target-repo, got %q", config.TargetRepoSlug) + } + }) + + t.Run("parses expression allowed repos only when configured", func(t *testing.T) { + expr := "${{ inputs['allowed-repos'] }}" + baseConfig := map[string]any{ + "allowed-repos": expr, + } + + config, isInvalid := parseSafeOutputTargetConfig(baseConfig, nil, safeOutputTargetConfigOptions{ + parseAllowedRepos: true, + }) + if isInvalid { + t.Fatal("expected config to be valid") + } + if config.AllowedRepos != nil { + t.Fatalf("expected expression allowed-repos to be ignored without expression support, got %v", config.AllowedRepos) + } + + config, isInvalid = parseSafeOutputTargetConfig(baseConfig, nil, safeOutputTargetConfigOptions{ + parseAllowedRepos: true, + allowAllowedReposExpression: true, + }) + if isInvalid { + t.Fatal("expected config to be valid") + } + if len(config.AllowedRepos) != 1 || config.AllowedRepos[0] != expr { + t.Fatalf("expected expression allowed-repos, got %v", config.AllowedRepos) + } + }) +} + func TestParseBoolFromConfig(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/create_code_scanning_alert.go b/pkg/workflow/create_code_scanning_alert.go index 09586f87fa2..3dda00d4407 100644 --- a/pkg/workflow/create_code_scanning_alert.go +++ b/pkg/workflow/create_code_scanning_alert.go @@ -38,14 +38,19 @@ func (c *Compiler) parseCodeScanningAlertsConfig(outputMap map[string]any) *Crea } } - // Parse target-repo - securityReportsConfig.TargetRepoSlug = extractStringFromMap(configMap, "target-repo", createCodeScanningAlertLog) + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, createCodeScanningAlertLog, safeOutputTargetConfigOptions{ + parseTargetRepo: true, + allowTargetRepoWildcard: true, + parseAllowedRepos: true, + }) + if isInvalid { + return nil + } + securityReportsConfig.TargetRepoSlug = targetConfig.TargetRepoSlug if securityReportsConfig.TargetRepoSlug != "" { createCodeScanningAlertLog.Printf("Target repo for code scanning alerts: %s", securityReportsConfig.TargetRepoSlug) } - - // Parse allowed-repos - securityReportsConfig.AllowedRepos = ParseStringArrayFromConfig(configMap, "allowed-repos", createCodeScanningAlertLog) + securityReportsConfig.AllowedRepos = targetConfig.AllowedRepos if len(securityReportsConfig.AllowedRepos) > 0 { createCodeScanningAlertLog.Printf("Allowed repos for cross-repo alerts: %d configured", len(securityReportsConfig.AllowedRepos)) } diff --git a/pkg/workflow/create_pr_review_comment.go b/pkg/workflow/create_pr_review_comment.go index f84a62134ce..f9e1c6f2eb3 100644 --- a/pkg/workflow/create_pr_review_comment.go +++ b/pkg/workflow/create_pr_review_comment.go @@ -39,19 +39,19 @@ func (c *Compiler) parsePullRequestReviewCommentsConfig(outputMap map[string]any } } - // Parse target - if target, exists := configMap["target"]; exists { - if targetStr, ok := target.(string); ok { - prReviewCommentsConfig.Target = targetStr - } - } - - // Parse target-repo using shared helper with validation - targetRepoSlug, isInvalid := parseTargetRepoWithValidation(configMap) + // Parse target config (target, target-repo, allowed-repos) + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, createPRReviewCommentLog, safeOutputTargetConfigOptions{ + parseTarget: true, + parseTargetRepo: true, + parseAllowedRepos: true, + allowAllowedReposExpression: true, + }) if isInvalid { return nil // Invalid configuration, return nil to cause validation error } - prReviewCommentsConfig.TargetRepoSlug = targetRepoSlug + prReviewCommentsConfig.Target = targetConfig.Target + prReviewCommentsConfig.TargetRepoSlug = targetConfig.TargetRepoSlug + prReviewCommentsConfig.AllowedRepos = targetConfig.AllowedRepos if commitId, exists := configMap["commit-id"]; exists { if commitIdStr, ok := commitId.(string); ok { diff --git a/pkg/workflow/dispatch_workflow.go b/pkg/workflow/dispatch_workflow.go index 869fc9886ab..cb2a4184d34 100644 --- a/pkg/workflow/dispatch_workflow.go +++ b/pkg/workflow/dispatch_workflow.go @@ -64,8 +64,17 @@ func (c *Compiler) parseDispatchWorkflowConfig(outputMap map[string]any) *Dispat } // Parse target-repo (optional cross-repo dispatch target) - dispatchWorkflowConfig.TargetRepoSlug = extractStringFromMap(configMap, "target-repo", dispatchWorkflowLog) - dispatchWorkflowConfig.AllowedRepos = ParseStringArrayOrExprFromConfig(configMap, "allowed-repos", dispatchWorkflowLog) + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, dispatchWorkflowLog, safeOutputTargetConfigOptions{ + parseTargetRepo: true, + allowTargetRepoWildcard: true, + parseAllowedRepos: true, + allowAllowedReposExpression: true, + }) + if isInvalid { + return nil + } + dispatchWorkflowConfig.TargetRepoSlug = targetConfig.TargetRepoSlug + dispatchWorkflowConfig.AllowedRepos = targetConfig.AllowedRepos dispatchWorkflowConfig.AllowedRefs = ParseStringArrayOrExprFromConfig(configMap, "allowed-refs", dispatchWorkflowLog) if dispatchWorkflowConfig.AllowedRefs == nil { dispatchWorkflowConfig.AllowedRefs = []string{defaultDispatchWorkflowAllowedRef} diff --git a/pkg/workflow/link_sub_issue.go b/pkg/workflow/link_sub_issue.go index 65573db8fd3..161ff741e69 100644 --- a/pkg/workflow/link_sub_issue.go +++ b/pkg/workflow/link_sub_issue.go @@ -26,13 +26,17 @@ func (c *Compiler) parseLinkSubIssueConfig(outputMap map[string]any) *LinkSubIss linkSubIssueLog.Print("Found link-sub-issue config map") // Parse target config (target-repo) with validation - targetConfig, isInvalid := ParseTargetConfig(configMap) + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, linkSubIssueLog, safeOutputTargetConfigOptions{ + parseTarget: true, + parseTargetRepo: true, + allowTargetRepoWildcard: true, + parseAllowedRepos: true, + allowAllowedReposExpression: true, + }) if isInvalid { - return nil // Invalid configuration (e.g., wildcard target-repo), return nil to cause validation error + return nil // Invalid configuration, return nil to cause validation error } linkSubIssueConfig.SafeOutputTargetConfig = targetConfig - // Override AllowedRepos with expression-aware parsing (supports GitHub Actions expressions) - linkSubIssueConfig.AllowedRepos = ParseStringArrayOrExprFromConfig(configMap, "allowed-repos", linkSubIssueLog) // Parse common base fields with default max of 5 c.parseBaseSafeOutputConfig(configMap, &linkSubIssueConfig.BaseSafeOutputConfig, 5) diff --git a/pkg/workflow/push_to_pull_request_branch.go b/pkg/workflow/push_to_pull_request_branch.go index 78bfb96b5aa..3f664fa1923 100644 --- a/pkg/workflow/push_to_pull_request_branch.go +++ b/pkg/workflow/push_to_pull_request_branch.go @@ -100,13 +100,6 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) } if configMap, ok := configData.(map[string]any); ok { - // Parse target (optional, similar to add-comment) - if target, exists := configMap["target"]; exists { - if targetStr, ok := target.(string); ok { - pushToBranchConfig.Target = targetStr - } - } - // Parse if-no-changes (optional, defaults to "warn") if ifNoChanges, exists := configMap["if-no-changes"]; exists { if ifNoChangesStr, ok := ifNoChanges.(string); ok { @@ -159,7 +152,18 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) } // Parse target-repo for cross-repository push - pushToBranchConfig.TargetRepoSlug = extractStringFromMap(configMap, "target-repo", pushToPullRequestBranchLog) + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, pushToPullRequestBranchLog, safeOutputTargetConfigOptions{ + parseTarget: true, + parseTargetRepo: true, + allowTargetRepoWildcard: true, + parseAllowedRepos: true, + allowAllowedReposExpression: true, + }) + if isInvalid { + return nil + } + pushToBranchConfig.Target = targetConfig.Target + pushToBranchConfig.TargetRepoSlug = targetConfig.TargetRepoSlug pushToBranchConfig.HeadRepoSlug = extractStringFromMap(configMap, "head-repo", pushToPullRequestBranchLog) pushToBranchConfig.HeadGitHubToken = extractStringFromMap(configMap, "head-github-token", pushToPullRequestBranchLog) @@ -179,7 +183,7 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) pushToBranchConfig.BaseBranch = extractStringFromMap(configMap, "base-branch", pushToPullRequestBranchLog) // Parse allowed-repos for cross-repository push (expression-aware) - pushToBranchConfig.AllowedRepos = ParseStringArrayOrExprFromConfig(configMap, "allowed-repos", pushToPullRequestBranchLog) + pushToBranchConfig.AllowedRepos = targetConfig.AllowedRepos // Parse protected-files: supports string enum OR object form {policy, exclude}. exclude := preprocessProtectedFilesField(configMap, pushToPullRequestBranchLog) diff --git a/pkg/workflow/reply_to_pr_review_comment.go b/pkg/workflow/reply_to_pr_review_comment.go index 86e480012f0..ddd6efef234 100644 --- a/pkg/workflow/reply_to_pr_review_comment.go +++ b/pkg/workflow/reply_to_pr_review_comment.go @@ -27,30 +27,16 @@ func (c *Compiler) parseReplyToPullRequestReviewCommentConfig(outputMap map[stri // Parse common base fields with default max of 10 c.parseBaseSafeOutputConfig(configMap, &config.BaseSafeOutputConfig, 10) - // Parse target - if target, exists := configMap["target"]; exists { - if targetStr, ok := target.(string); ok { - config.Target = targetStr - } - } - - // Parse target-repo using shared helper with validation - targetRepoSlug, isInvalid := parseTargetRepoWithValidation(configMap) + // Parse target config (target, target-repo, allowed-repos) + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, replyToPRReviewCommentLog, safeOutputTargetConfigOptions{ + parseTarget: true, + parseTargetRepo: true, + parseAllowedRepos: true, + }) if isInvalid { return nil // Invalid configuration, return nil to cause validation error } - config.TargetRepoSlug = targetRepoSlug - - // Parse allowed-repos - if allowedRepos, exists := configMap["allowed-repos"]; exists { - if repos, ok := allowedRepos.([]any); ok { - for _, repo := range repos { - if repoStr, ok := repo.(string); ok { - config.AllowedRepos = append(config.AllowedRepos, repoStr) - } - } - } - } + config.SafeOutputTargetConfig = targetConfig // Parse footer as templatable bool if err := preprocessBoolFieldAsString(configMap, "footer", replyToPRReviewCommentLog); err != nil { diff --git a/pkg/workflow/safe_outputs_cross_repo_config_test.go b/pkg/workflow/safe_outputs_cross_repo_config_test.go index 51775e39efe..91568c5bbc9 100644 --- a/pkg/workflow/safe_outputs_cross_repo_config_test.go +++ b/pkg/workflow/safe_outputs_cross_repo_config_test.go @@ -149,6 +149,17 @@ func TestCreateCodeScanningAlertConfigTargetRepo(t *testing.T) { expectedToken: "", expectedDriver: "My Scanner", }, + { + name: "wildcard target-repo remains allowed", + configMap: map[string]any{ + "create-code-scanning-alert": map[string]any{ + "target-repo": "*", + }, + }, + expectedRepo: "*", + expectedRepos: nil, + expectedToken: "", + }, { name: "no cross-repo config", configMap: map[string]any{ diff --git a/pkg/workflow/safe_outputs_parser.go b/pkg/workflow/safe_outputs_parser.go index 431540d4ad6..c42a7763312 100644 --- a/pkg/workflow/safe_outputs_parser.go +++ b/pkg/workflow/safe_outputs_parser.go @@ -12,6 +12,14 @@ type SafeOutputTargetConfig struct { AllowedRepos []string `yaml:"allowed-repos,omitempty"` // List of additional repositories that operations can target (additionally to the target-repo) } +type safeOutputTargetConfigOptions struct { + parseTarget bool + parseTargetRepo bool + allowTargetRepoWildcard bool + parseAllowedRepos bool + allowAllowedReposExpression bool +} + // SafeOutputAllowedLabelsConfig contains the shared allowed-labels field for safe output configurations. // Embed this in safe output config structs that restrict which labels may be used. type SafeOutputAllowedLabelsConfig struct { @@ -56,22 +64,45 @@ type ListJobConfig struct { // Returns the parsed SafeOutputTargetConfig and a boolean indicating if there was a validation error. // target-repo accepts "*" (wildcard) to indicate that any repository can be targeted. func ParseTargetConfig(configMap map[string]any) (SafeOutputTargetConfig, bool) { - safeOutputParserLog.Print("Parsing target config from map") + return parseSafeOutputTargetConfig(configMap, safeOutputParserLog, safeOutputTargetConfigOptions{ + parseTarget: true, + parseTargetRepo: true, + allowTargetRepoWildcard: true, + parseAllowedRepos: true, + }) +} + +func parseSafeOutputTargetConfig(configMap map[string]any, debugLog *logger.Logger, opts safeOutputTargetConfigOptions) (SafeOutputTargetConfig, bool) { + if debugLog != nil { + debugLog.Print("Parsing target config from map") + } + config := SafeOutputTargetConfig{} - // Parse target - if target, exists := configMap["target"]; exists { - if targetStr, ok := target.(string); ok { - config.Target = targetStr - safeOutputParserLog.Printf("Target set to: %s", targetStr) + if opts.parseTarget { + config.Target = extractStringFromMap(configMap, "target", debugLog) + if config.Target != "" && debugLog != nil { + debugLog.Printf("Target set to: %s", config.Target) } } - // Parse target-repo; wildcard "*" is allowed and means "any repository" - config.TargetRepoSlug = extractStringFromMap(configMap, "target-repo", safeOutputParserLog) + if opts.parseTargetRepo { + config.TargetRepoSlug = extractStringFromMap(configMap, "target-repo", debugLog) + if config.TargetRepoSlug == "*" && !opts.allowTargetRepoWildcard { + if debugLog != nil { + debugLog.Print("Invalid target-repo: wildcard '*' is not allowed") + } + return SafeOutputTargetConfig{}, true + } + } - // Parse allowed-repos - config.AllowedRepos = ParseStringArrayFromConfig(configMap, "allowed-repos", safeOutputParserLog) + if opts.parseAllowedRepos { + if opts.allowAllowedReposExpression { + config.AllowedRepos = ParseStringArrayOrExprFromConfig(configMap, "allowed-repos", debugLog) + } else { + config.AllowedRepos = ParseStringArrayFromConfig(configMap, "allowed-repos", debugLog) + } + } return config, false } diff --git a/pkg/workflow/submit_pr_review.go b/pkg/workflow/submit_pr_review.go index edc07bff36f..78d4d733606 100644 --- a/pkg/workflow/submit_pr_review.go +++ b/pkg/workflow/submit_pr_review.go @@ -40,19 +40,15 @@ func (c *Compiler) parseSubmitPullRequestReviewConfig(outputMap map[string]any) c.parseBaseSafeOutputConfig(configMap, &config.BaseSafeOutputConfig, 1) // Parse target config (target, target-repo, allowed-repos) - // Uses parseTargetRepoWithValidation to disallow wildcard "*" for target-repo - if target, exists := configMap["target"]; exists { - if targetStr, ok := target.(string); ok { - config.Target = targetStr - } - } - - targetRepoSlug, isInvalid := parseTargetRepoWithValidation(configMap) + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, submitPRReviewLog, safeOutputTargetConfigOptions{ + parseTarget: true, + parseTargetRepo: true, + parseAllowedRepos: true, + }) if isInvalid { return nil // Invalid configuration, return nil to cause validation error } - config.TargetRepoSlug = targetRepoSlug - config.AllowedRepos = ParseStringArrayFromConfig(configMap, "allowed-repos", submitPRReviewLog) + config.SafeOutputTargetConfig = targetConfig // Parse footer configuration (string: "always"/"none"/"if-body", or bool for backward compat) if footer, exists := configMap["footer"]; exists { diff --git a/pkg/workflow/update_project.go b/pkg/workflow/update_project.go index d0541bb64f8..185562e1b1f 100644 --- a/pkg/workflow/update_project.go +++ b/pkg/workflow/update_project.go @@ -51,14 +51,15 @@ func (c *Compiler) parseUpdateProjectConfig(outputMap map[string]any) *UpdatePro } // Parse target-repo for cross-repo content resolution (no wildcard allowed) - targetRepoSlug, isInvalid := parseTargetRepoWithValidation(configMap) + targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, updateProjectLog, safeOutputTargetConfigOptions{ + parseTargetRepo: true, + parseAllowedRepos: true, + }) if isInvalid { return nil } - updateProjectConfig.TargetRepoSlug = targetRepoSlug - - // Parse allowed-repos for cross-repo content resolution - updateProjectConfig.AllowedRepos = ParseStringArrayFromConfig(configMap, "allowed-repos", updateProjectLog) + updateProjectConfig.TargetRepoSlug = targetConfig.TargetRepoSlug + updateProjectConfig.AllowedRepos = targetConfig.AllowedRepos // Parse views if specified updateProjectConfig.Views = parseProjectViews(configMap, updateProjectLog)