Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 500
Allow cross-repo safe-outputs.dispatch-workflow allowlists#39080
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
42ab0338f2994a5079acbffaca536f1dc3e7d61b05a3ef28dFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # ADR-39080: Accept `allowed-repos` for cross-repo `dispatch-workflow` | ||
| **Date**: 2026-06-13 | ||
| **Status**: Draft | ||
| ## Context | ||
| The `safe-outputs.dispatch-workflow` feature lets an agent dispatch other workflows, including in a different repository when `target-repo` is set. The JavaScript handler already enforced an allowlist at runtime (error `E004`), expecting an `allowed_repos` config value, but the workflow frontmatter schema and the Go compiler never accepted an `allowed-repos` key. As a result, an author who wrote a valid cross-repo configuration with `allowed-repos` failed at compile time, leaving no supported way to configure cross-repository dispatch end to end. Other cross-repo safe outputs (e.g. `create-code-scanning-alert`, `push-to-pull-request-branch`) already follow a consistent pattern of declaring an `allowed-repos` allowlist in frontmatter that is parsed by the compiler and emitted into the handler config. | ||
| ## Decision | ||
| We will add `allowed-repos` to the `dispatch-workflow` safe-output as a first-class frontmatter field, wired through the full pipeline so cross-repo dispatch is configurable end to end: | ||
| - Add `allowed-repos` to the `dispatch-workflow` JSON schema, accepting both an array of `owner/repo` slugs (supporting wildcards such as `org/*`) and a GitHub Actions expression resolving to a comma-separated list. | ||
| - Parse `allowed-repos` into `DispatchWorkflowConfig.AllowedRepos` via the shared `ParseStringArrayFromConfig` helper. | ||
| - Emit it into the runtime handler config as `allowed_repos` using `AddTemplatableStringSlice`, matching the JS handler contract and preserving templated allowlists the same way other cross-repo safe outputs do. | ||
| We chose to mirror the existing cross-repo safe-output convention rather than invent a new mechanism, so configuration and templating behave identically across all safe outputs. The runtime error message is also tightened to point authors at the `safe-outputs.dispatch-workflow` section instead of an internal config key name. | ||
| ## Alternatives Considered | ||
| ### Alternative 1: Keep runtime-only enforcement, document the limitation | ||
| Leave the schema and compiler unchanged and require users to rely on the runtime check, documenting that cross-repo dispatch could not be statically configured. Rejected because it leaves a valid feature unreachable through supported configuration — any `allowed-repos` config fails to compile — and contradicts the runtime handler that already expects the allowlist. | ||
| ### Alternative 2: Introduce a new, dispatch-specific allowlist key/shape | ||
| Add a bespoke field (e.g. `dispatch-allowlist`) with its own parsing and emission logic specific to `dispatch-workflow`. Rejected because it would diverge from the established `allowed-repos` convention used by other cross-repo safe outputs, duplicating templating logic and increasing cognitive load for authors who already know the existing pattern. | ||
| ## Consequences | ||
| ### Positive | ||
| - Cross-repository `dispatch-workflow` can now be configured end to end (schema → compiler → handler) without compile-time failures. | ||
| - Behavior is consistent with other cross-repo safe outputs, including templated/expression-based allowlists, reducing surprise for authors. | ||
| - Regression tests cover schema acceptance, config parsing, and handler-config emission for the new field. | ||
| ### Negative | ||
| - Adds another surface (schema + parsing + emission) that must stay in sync with the JS handler's `allowed_repos` contract; a future change to one side risks drift. | ||
| - Broadens the configuration surface of a security-sensitive feature (cross-repo dispatch), so the allowlist semantics and wildcard matching must be reviewed carefully. | ||
| ### Neutral | ||
| - The frontmatter key is `allowed-repos` while the emitted handler key is `allowed_repos`; the naming difference follows the existing convention but must be remembered when tracing config through the pipeline. | ||
| - No change to default behavior: omitting `allowed-repos` still blocks cross-repo dispatch via the existing `E004` error. | ||
| --- | ||
| *This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27468597143) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -11,6 +11,84 @@ import ( | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
| // TestDispatchWorkflowConfigTargetRepo verifies that dispatch-workflow correctly parses | ||
| // target-repo and allowed-repos fields. | ||
| func TestDispatchWorkflowConfigTargetRepo(t *testing.T) { | ||
| compiler := NewCompiler() | ||
| tests := []struct { | ||
| name string | ||
| configMap map[string]any | ||
| expectedRepo string | ||
| expectedRepos []string | ||
| expectedToken string | ||
| }{ | ||
| { | ||
| name: "target-repo and allowed-repos configured", | ||
| configMap: map[string]any{ | ||
| "dispatch-workflow": map[string]any{ | ||
| "workflows": []any{"worker"}, | ||
| "target-repo": "githubnext/gh-aw-side-repo", | ||
| "allowed-repos": []any{"githubnext/gh-aw-side-repo"}, | ||
| "github-token": "${{ secrets.TEMP_USER_PAT }}", | ||
| }, | ||
| }, | ||
| expectedRepo: "githubnext/gh-aw-side-repo", | ||
| expectedRepos: []string{"githubnext/gh-aw-side-repo"}, | ||
| expectedToken: "${{ secrets.TEMP_USER_PAT }}", | ||
| }, | ||
| { | ||
| name: "multiple allowed repos", | ||
| configMap: map[string]any{ | ||
| "dispatch-workflow": map[string]any{ | ||
| "workflows": []any{"worker"}, | ||
| "target-repo": "org/primary-repo", | ||
| "allowed-repos": []any{"org/primary-repo", "org/secondary-repo"}, | ||
| }, | ||
| }, | ||
| expectedRepo: "org/primary-repo", | ||
| expectedRepos: []string{"org/primary-repo", "org/secondary-repo"}, | ||
| expectedToken: "", | ||
| }, | ||
Comment on lines
+19
to
+52
| ||
| { | ||
| name: "allowed-repos as GitHub Actions expression", | ||
| configMap: map[string]any{ | ||
| "dispatch-workflow": map[string]any{ | ||
| "workflows": []any{"worker"}, | ||
| "target-repo": "${{ inputs.target_repo }}", | ||
| "allowed-repos": "${{ inputs['allowed-repos'] }}", | ||
| }, | ||
| }, | ||
| expectedRepo: "${{ inputs.target_repo }}", | ||
| expectedRepos: []string{"${{ inputs['allowed-repos'] }}"}, | ||
| expectedToken: "", | ||
| }, | ||
| { | ||
| name: "no cross-repo config", | ||
| configMap: map[string]any{ | ||
| "dispatch-workflow": map[string]any{ | ||
| "workflows": []any{"worker"}, | ||
| "max": 2, | ||
| }, | ||
| }, | ||
| expectedRepo: "", | ||
| expectedRepos: nil, | ||
| expectedToken: "", | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| cfg := compiler.parseDispatchWorkflowConfig(tt.configMap) | ||
| require.NotNil(t, cfg, "config should not be nil") | ||
| assert.Equal(t, tt.expectedRepo, cfg.TargetRepoSlug, "TargetRepoSlug mismatch") | ||
| assert.Equal(t, tt.expectedRepos, cfg.AllowedRepos, "AllowedRepos mismatch") | ||
| assert.Equal(t, tt.expectedToken, cfg.GitHubToken, "GitHubToken mismatch") | ||
| }) | ||
| } | ||
| } | ||
| // TestCreateCodeScanningAlertConfigTargetRepo verifies that create-code-scanning-alert | ||
| // correctly parses target-repo and allowed-repos fields. | ||
| func TestCreateCodeScanningAlertConfigTargetRepo(t *testing.T) { | ||
| @@ -420,6 +498,42 @@ func TestPushToPullRequestBranchCrossRepoInHandlerConfig(t *testing.T) { | ||
| assert.Contains(t, allowedRepos, "githubnext/gh-aw-side-repo", "allowed_repos should contain the repo") | ||
| } | ||
| // TestDispatchWorkflowCrossRepoInHandlerConfig verifies that target-repo, allowed-repos, | ||
| // and github-token are included in the handler manager config JSON for dispatch-workflow. | ||
| func TestDispatchWorkflowCrossRepoInHandlerConfig(t *testing.T) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The handler-config test only exercises a literal-array 💡 Suggested parallel testfuncTestDispatchWorkflowCrossRepoInHandlerConfigExpression(t*testing.T) {
compiler:=NewCompiler()
workflowData:=&WorkflowData{
Name: "Test",
SafeOutputs: &SafeOutputsConfig{
DispatchWorkflow: &DispatchWorkflowConfig{
Workflows: []string{"worker"},
TargetRepoSlug: "${{ inputs.target-repo }}",
AllowedRepos: []string{"${{ inputs.allowed-repos }}"},
},
},
}
varsteps []stringcompiler.addHandlerManagerConfigEnvVar(&steps, workflowData)
handlerConfig:=extractHandlerConfig(t, strings.Join(steps, ""))
dispatchWorkflow, ok:=handlerConfig["dispatch_workflow"]
require.True(t, ok)
// AddTemplatableStringSlice emits a JSON string, not an array, for an expressionallowedRepos:=dispatchWorkflow["allowed_repos"]
assert.Equal(t, "${{ inputs.allowed-repos }}", allowedRepos)
} | ||
| compiler := NewCompiler() | ||
| workflowData := &WorkflowData{ | ||
| Name: "Test", | ||
| SafeOutputs: &SafeOutputsConfig{ | ||
| DispatchWorkflow: &DispatchWorkflowConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{ | ||
| GitHubToken: "${{ secrets.TEMP_USER_PAT }}", | ||
| }, | ||
| Workflows: []string{"worker"}, | ||
| TargetRepoSlug: "githubnext/gh-aw-side-repo", | ||
| AllowedRepos: []string{"githubnext/gh-aw-side-repo"}, | ||
| }, | ||
| }, | ||
| } | ||
| var steps []string | ||
| compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) | ||
| require.NotEmpty(t, steps) | ||
| handlerConfig := extractHandlerConfig(t, strings.Join(steps, "")) | ||
| dispatchWorkflow, ok := handlerConfig["dispatch_workflow"] | ||
| require.True(t, ok, "dispatch_workflow config should be present") | ||
| assert.Equal(t, "${{ secrets.TEMP_USER_PAT }}", dispatchWorkflow["github-token"], "github-token should be in handler config") | ||
| assert.Equal(t, "githubnext/gh-aw-side-repo", dispatchWorkflow["target-repo"], "target-repo should be in handler config") | ||
| allowedRepos, ok := dispatchWorkflow["allowed_repos"] | ||
| require.True(t, ok, "allowed_repos should be present") | ||
| assert.Contains(t, allowedRepos, "githubnext/gh-aw-side-repo", "allowed_repos should contain the repo") | ||
| } | ||
| // TestHandlerManagerStepPerOutputTokenInHandlerConfig verifies that per-output tokens | ||
| // (e.g., add-comment.github-token) are wired into the handler config JSON (GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG) | ||
| // but NOT used as the step-level with.github-token. The step-level token follows the same | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] Missing test case for the template-expression form of
allowed-repos— this would have caught the parse-function mismatch above.💡 Suggested addition to the test table
Add a fourth case to
TestDispatchWorkflowConfigTargetRepo:{ name: "allowed-repos as GitHub Actions expression", configMap: map[string]any{ "dispatch-workflow": map[string]any{ "workflows": []any{"worker"}, "target-repo": "${{ inputs.target-repo }}", "allowed-repos": "${{ inputs.allowed-repos }}", }, }, expectedRepo: "${{ inputs.target-repo }}", // With ParseStringArrayOrExprFromConfig the expression is wrapped:expectedRepos: []string{"${{ inputs.allowed-repos }}"}, },With
ParseStringArrayFromConfig(the current code)expectedReposwill benil, making the test fail and immediately surfacing the wrong function choice.