From 5528d3d042cef33c96ab793f964097c3f550de94 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Wed, 7 Oct 2026 05:47:50 -0700 Subject: [PATCH 1/6] fix: use safe-output token when ingesting agent output Resolve mention allowlists with the configured global token before sanitization. Cover credential selection and same-job token validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../content/docs/reference/safe-outputs.md | 2 + pkg/workflow/agentic_output_test.go | 59 ++++++++++++++++++- pkg/workflow/compiler_yaml_step_lifecycle.go | 5 ++ ...safe_outputs_step_token_validation_test.go | 38 +++++++++++- 4 files changed, 100 insertions(+), 4 deletions(-) diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 55599c6bc6b..90952ac3660 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -2123,6 +2123,8 @@ safe-outputs: **`allowed-teams`** lets organizations allow all members of specific GitHub teams to be mentioned without listing individual usernames. Team members are fetched from the GitHub API at runtime using `GET /orgs/{org}/teams/{team_slug}/members`. Bot accounts within the team are excluded. Use `org/team-slug` for cross-org teams or just `team-slug` to resolve against the current repository's organization. +Set `safe-outputs.github-token` to a token that can read team membership. The global token is also used by **Ingest agent output** to resolve allowed mentions before sanitization; a per-output `github-token` override does not apply to ingestion. + > [!IMPORTANT] > `allowed-teams` requires the workflow token to have `read:org` scope. The default `GITHUB_TOKEN` does **not** include this scope. Use one of the following: > - A **classic PAT** with the `read:org` scope stored as a repository secret diff --git a/pkg/workflow/agentic_output_test.go b/pkg/workflow/agentic_output_test.go index 03f41474056..ba98b5cebcf 100644 --- a/pkg/workflow/agentic_output_test.go +++ b/pkg/workflow/agentic_output_test.go @@ -12,8 +12,57 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestOutputCollectionGitHubToken(t *testing.T) { + for _, tt := range []struct { + name string + safeOutputs *SafeOutputsConfig + token string + }{ + {name: "no safe outputs"}, + {name: "default token", safeOutputs: &SafeOutputsConfig{}}, + { + name: "step token with fallback", + safeOutputs: &SafeOutputsConfig{GitHubToken: "${{ steps.mint.outputs.token || secrets.MENTIONS_PAT || secrets.GITHUB_TOKEN }}"}, + token: "${{ steps.mint.outputs.token || secrets.MENTIONS_PAT || secrets.GITHUB_TOKEN }}", + }, + { + name: "per-handler token is not used", + safeOutputs: &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{GitHubToken: "${{ secrets.COMMENT_PAT }}"}}, + }, + }, + { + name: "global token takes precedence over per-handler token", + safeOutputs: &SafeOutputsConfig{ + GitHubToken: "${{ secrets.MENTIONS_PAT }}", + AddComments: &AddCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{GitHubToken: "${{ secrets.COMMENT_PAT }}"}}, + }, + token: "${{ secrets.MENTIONS_PAT }}", + }, + { + name: "safe outputs app token is not available in agent job", + safeOutputs: &SafeOutputsConfig{ + GitHubApp: &GitHubAppConfig{AppID: "${{ vars.APP_ID }}", PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}"}, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + var yaml strings.Builder + require.NoError(t, NewCompiler().generateOutputCollectionStep(&yaml, &WorkflowData{SafeOutputs: tt.safeOutputs})) + if tt.token == "" { + assert.NotContains(t, yaml.String(), "github-token:") + } else { + assert.Contains(t, yaml.String(), " with:\n github-token: "+tt.token+"\n script: |\n") + } + assert.Contains(t, yaml.String(), "setupGlobals(core, github, context, exec, io, getOctokit);") + }) + } +} + func TestAgenticOutputCollection(t *testing.T) { // Create temporary directory for test files tmpDir := testutil.TempDir(t, "agentic-output-test") @@ -31,6 +80,9 @@ tools: engine: claude strict: false safe-outputs: + github-token: ${{ secrets.MENTIONS_PAT }} + mentions: + allowed-teams: [my-org/my-team] add-labels: allowed: ["bug", "enhancement"] --- @@ -86,9 +138,10 @@ This workflow tests the agentic output collection functionality. t.Error("runner.tool_cache must not be interpolated directly in the shell script") } - if !strings.Contains(lockContent, "- name: Ingest agent output") { - t.Error("Expected 'Ingest agent output' step to be in generated workflow") - } + require.Contains(t, lockContent, "- name: Ingest agent output\n") + ingestStep := strings.SplitN(strings.SplitN(lockContent, "- name: Ingest agent output\n", 2)[1], "\n - ", 2)[0] + assert.Contains(t, ingestStep, "github-token: ${{ secrets.MENTIONS_PAT }}") + assert.Contains(t, extractJobSection(lockContent, "safe_outputs"), "github-token: ${{ secrets.MENTIONS_PAT }}") // Upload Safe Outputs and Upload sanitized agent output are now merged into the // unified 'agent' artifact — individual upload steps no longer exist. diff --git a/pkg/workflow/compiler_yaml_step_lifecycle.go b/pkg/workflow/compiler_yaml_step_lifecycle.go index be10cf2bf19..8408aa1f730 100644 --- a/pkg/workflow/compiler_yaml_step_lifecycle.go +++ b/pkg/workflow/compiler_yaml_step_lifecycle.go @@ -352,6 +352,11 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor } yaml.WriteString(" with:\n") + // Resolve mention allowlists with the configured token before sanitization. + // Safe-output app tokens are minted in a different job and are unavailable here. + if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { + yaml.WriteString(" github-token: " + resolveSafeOutputGitHubToken(data.SafeOutputs.GitHubToken) + "\n") + } yaml.WriteString(" script: |\n") // Load script from external file using require() diff --git a/pkg/workflow/safe_outputs_step_token_validation_test.go b/pkg/workflow/safe_outputs_step_token_validation_test.go index 8608e95801d..835d084338e 100644 --- a/pkg/workflow/safe_outputs_step_token_validation_test.go +++ b/pkg/workflow/safe_outputs_step_token_validation_test.go @@ -65,7 +65,7 @@ jobs: require.NoError(t, err) lockYAML := string(lockContent) - for _, jobName := range []string{"safe_outputs", "conclusion"} { + for _, jobName := range []string{"agent", "safe_outputs", "conclusion"} { section := extractJobSection(lockYAML, jobName) require.NotEmpty(t, section, "expected %s job section", jobName) assert.Contains(t, section, "id: octosts") @@ -104,6 +104,42 @@ safe-outputs: assert.Contains(t, err.Error(), "pre-steps:") } +func TestSameJobStepTokenMissingInAgentIngestionFails(t *testing.T) { + workflowFile := writeStepTokenWorkflow(t, "missing-agent", `--- +on: + workflow_dispatch: +permissions: + contents: read + id-token: write +engine: claude +strict: false +safe-outputs: + add-comment: + mentions: + allowed-teams: [my-org/my-team] + github-token: ${{ steps.octosts.outputs.token || secrets.GITHUB_TOKEN }} +jobs: + safe_outputs: + pre-steps: + - name: Mint token (safe outputs) + id: octosts + uses: `+stsMintStep+` + conclusion: + pre-steps: + - name: Mint token (conclusion) + id: octosts + uses: `+stsMintStep+` +--- + +# Missing agent ingestion token +`) + + err := NewCompiler().CompileWorkflow(workflowFile) + require.Error(t, err) + assert.Contains(t, err.Error(), `job "agent" has no step with id "octosts"`) + assert.Contains(t, err.Error(), "pre-steps:") +} + // TestSameJobStepTokenMintedAfterConsumerFails verifies that safe-outputs.steps, which run // after the safe_outputs checkout and git credential steps, are reported as too late for a // token consumed by those steps. From edafc2724283c949b61face47a0899a15a954326 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Wed, 7 Oct 2026 05:52:41 -0700 Subject: [PATCH 2/6] docs: specify ingestion credentials and log token selection Document mention allowlist credential requirements and add secret-safe compiler debug logs for the selected ingestion token source. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/src/content/docs/specs/safe-outputs-specification.md | 8 ++++++++ pkg/workflow/compiler_yaml_step_lifecycle.go | 3 +++ 2 files changed, 11 insertions(+) diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 414174dcc65..c9e41aaec55 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -2134,6 +2134,14 @@ Requirements: - Neutralize unauthorized: `@user` becomes `@ user` (add space) - Preserve mentions in code blocks +**Ingestion credential requirements**: + +- When `safe-outputs.github-token` is configured, **Ingest agent output** MUST use that global token to resolve mention allowlists, including `mentions.allowed-teams`, before sanitization. +- Allowed team members' raw `@login` mentions MUST be preserved during ingestion so downstream handlers can notify those users. +- Without a global `safe-outputs.github-token`, ingestion MUST retain the default GitHub Actions token. Per-output token overrides and GitHub App tokens minted in the `safe_outputs` job MUST NOT be used by ingestion in the `agent` job. +- A global token referencing `steps..outputs.*` MUST be produced by an earlier step in the `agent` job as well as in every other consuming job. +- Compiler debug logging SHOULD identify the ingestion token source without logging credential values or token expressions. + **Transformation T6: Markdown Safety** Requirements: diff --git a/pkg/workflow/compiler_yaml_step_lifecycle.go b/pkg/workflow/compiler_yaml_step_lifecycle.go index 8408aa1f730..c8d04d07b84 100644 --- a/pkg/workflow/compiler_yaml_step_lifecycle.go +++ b/pkg/workflow/compiler_yaml_step_lifecycle.go @@ -355,7 +355,10 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor // Resolve mention allowlists with the configured token before sanitization. // Safe-output app tokens are minted in a different job and are unavailable here. if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { + compilerYamlStepLifecycleLog.Print("Ingest agent output uses safe-outputs.github-token for mention allowlist resolution") yaml.WriteString(" github-token: " + resolveSafeOutputGitHubToken(data.SafeOutputs.GitHubToken) + "\n") + } else { + compilerYamlStepLifecycleLog.Print("Ingest agent output uses the default GitHub Actions token for mention allowlist resolution") } yaml.WriteString(" script: |\n") From a10225f5e465d70dbd3815e8965bfeeacab8e54c Mon Sep 17 00:00:00 2001 From: pelikhan Date: Wed, 7 Oct 2026 06:09:50 -0700 Subject: [PATCH 3/6] fix: mint safe-output app tokens for agent output ingestion Mint a dedicated installation token after agent execution and gateway shutdown, request ingestion read scopes, and preserve ignore-if-missing fallback semantics. Cover credential scope, ordering, isolation, and relay workflows, and update the specification. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../content/docs/reference/safe-outputs.md | 2 +- .../docs/specs/safe-outputs-specification.md | 11 +- pkg/workflow/agentic_output_test.go | 112 +++++++++++++++++- pkg/workflow/compiler_yaml_step_lifecycle.go | 61 +++++++++- .../security_architecture_formal_test.go | 26 ++-- 5 files changed, 191 insertions(+), 21 deletions(-) diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 90952ac3660..c6be4436a51 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -2123,7 +2123,7 @@ safe-outputs: **`allowed-teams`** lets organizations allow all members of specific GitHub teams to be mentioned without listing individual usernames. Team members are fetched from the GitHub API at runtime using `GET /orgs/{org}/teams/{team_slug}/members`. Bot accounts within the team are excluded. Use `org/team-slug` for cross-org teams or just `team-slug` to resolve against the current repository's organization. -Set `safe-outputs.github-token` to a token that can read team membership. The global token is also used by **Ingest agent output** to resolve allowed mentions before sanitization; a per-output `github-token` override does not apply to ingestion. +Configure `safe-outputs.github-token` or `safe-outputs.github-app` with access to team membership. **Ingest agent output** resolves allowed mentions before sanitization using the global token or a dedicated app token minted in the `agent` job after agent execution. The app token takes precedence and requests `members: read` when `allowed-teams` is configured. With `ignore-if-missing: true`, missing app credentials fall back to the global token, then `GH_AW_GITHUB_TOKEN`, then `GITHUB_TOKEN`. Per-output token overrides do not apply to ingestion. > [!IMPORTANT] > `allowed-teams` requires the workflow token to have `read:org` scope. The default `GITHUB_TOKEN` does **not** include this scope. Use one of the following: diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index c9e41aaec55..aaad43e502a 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -319,7 +319,7 @@ The Safe Outputs MCP Gateway implements defense-in-depth through strict architec **Requirement AR1: Agent Isolation** -Agents MUST execute without GitHub write permissions. Only read-level tokens SHALL be accessible to agent processes. Write-capable tokens MUST reside exclusively in safe output job contexts. +Agents MUST execute without GitHub write permissions. Only read-level tokens SHALL be accessible to agent processes. Write-capable tokens MUST remain confined to trusted safe-output processing steps, including post-agent ingestion, and MUST NOT be accessible during agent execution. **Verification**: @@ -396,7 +396,7 @@ Safe Output Processors MAY target external APIs. Credentials for each external s - **Method**: Manual security audit and code review - **Tool**: Security review of workflow structure and GitHub Actions architecture -- **Criteria**: No GITHUB_TOKEN or credentials are accessible from agent job context; tokens only exist in safe output job contexts +- **Criteria**: Safe-output credentials are inaccessible to agent processes; post-agent ingestion credentials are supplied only to trusted steps after agent execution and gateway shutdown - **Manual Check**: Audit all communication channels (artifacts, environment variables, network, filesystem) to confirm no credential leakage **Formal Definition**: @@ -2136,9 +2136,12 @@ Requirements: **Ingestion credential requirements**: -- When `safe-outputs.github-token` is configured, **Ingest agent output** MUST use that global token to resolve mention allowlists, including `mentions.allowed-teams`, before sanitization. +- When `safe-outputs.github-app` is configured, the compiler MUST mint a dedicated installation token in the `agent` job immediately before **Ingest agent output**, after agent execution. The token MUST use the configured installation owner, repositories, and explicit permission overrides, with read permissions for ingestion's repository and comment-author lookups and `members: read` when `mentions.allowed-teams` is configured. +- Ingestion app credentials MUST be confined to trusted post-agent token-minting step inputs and MUST NOT be added to agent execution environments. The ingestion token MUST NOT inherit the safe-output handlers' write permissions; configured app permission overrides remain explicit author choices. +- Token minting and installation-owner resolution MUST run even after an earlier step fails, matching ingestion's `always()` behavior. With `ignore-if-missing: true`, missing credentials MUST skip minting and ingestion MUST fall back to `safe-outputs.github-token`, then `GH_AW_GITHUB_TOKEN`, then `GITHUB_TOKEN`. +- The same-job app token MUST take precedence over `safe-outputs.github-token`. Without an app, a configured global token MUST be used to resolve mention allowlists, including `mentions.allowed-teams`, before sanitization. - Allowed team members' raw `@login` mentions MUST be preserved during ingestion so downstream handlers can notify those users. -- Without a global `safe-outputs.github-token`, ingestion MUST retain the default GitHub Actions token. Per-output token overrides and GitHub App tokens minted in the `safe_outputs` job MUST NOT be used by ingestion in the `agent` job. +- Without a global app or token, ingestion MUST retain the default GitHub Actions token. Per-output token overrides and GitHub App tokens minted in the `safe_outputs` job MUST NOT be used by ingestion in the `agent` job. - A global token referencing `steps..outputs.*` MUST be produced by an earlier step in the `agent` job as well as in every other consuming job. - Compiler debug logging SHOULD identify the ingestion token source without logging credential values or token expressions. diff --git a/pkg/workflow/agentic_output_test.go b/pkg/workflow/agentic_output_test.go index ba98b5cebcf..c6711e3e49c 100644 --- a/pkg/workflow/agentic_output_test.go +++ b/pkg/workflow/agentic_output_test.go @@ -44,10 +44,11 @@ func TestOutputCollectionGitHubToken(t *testing.T) { token: "${{ secrets.MENTIONS_PAT }}", }, { - name: "safe outputs app token is not available in agent job", + name: "safe outputs app token is minted in agent job", safeOutputs: &SafeOutputsConfig{ GitHubApp: &GitHubAppConfig{AppID: "${{ vars.APP_ID }}", PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}"}, }, + token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token }}", }, } { t.Run(tt.name, func(t *testing.T) { @@ -63,6 +64,115 @@ func TestOutputCollectionGitHubToken(t *testing.T) { } } +func TestOutputCollectionGitHubAppToken(t *testing.T) { + for _, tt := range []struct { + name string + ignore bool + pat string + token string + }{ + {name: "app overrides PAT", pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token }}"}, + {name: "missing credentials fall back to PAT", ignore: true, pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token || secrets.PAT }}"}, + {name: "missing credentials fall back to defaults", ignore: true, token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token || secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}"}, + } { + t.Run(tt.name, func(t *testing.T) { + app := &GitHubAppConfig{ + AppID: "${{ vars.APP_ID }}", PrivateKey: "${{ secrets.APP_KEY }}", + Owner: "my-org", Repositories: []string{"my-repo", "another-repo"}, IgnoreIfMissing: tt.ignore, + } + data := &WorkflowData{SafeOutputs: &SafeOutputsConfig{ + GitHubApp: app, GitHubToken: tt.pat, AddComments: &AddCommentsConfig{}, + Mentions: &MentionsConfig{AllowedTeams: []string{"my-org/my-team"}}, + }} + var yaml strings.Builder + require.NoError(t, NewCompiler().generateOutputCollectionStep(&yaml, data)) + output := yaml.String() + assert.Contains(t, output, "owner: my-org\n") + assert.Contains(t, output, "repositories: |-\n my-repo\n another-repo\n") + assert.NotContains(t, output, "permission-contents:") + assert.Contains(t, output, "permission-issues: read\n") + assert.Contains(t, output, "permission-pull-requests: read\n") + assert.Contains(t, output, "permission-members: read\n") + assert.NotContains(t, output, ": write\n") + assert.Contains(t, output, "github-token: "+tt.token+"\n") + assert.NotContains(t, output, "steps.safe-outputs-app-token.outputs.token") + assert.Less(t, strings.Index(output, "id: safe-outputs-ingestion-app-token\n"), strings.Index(output, "- name: Ingest agent output\n")) + if tt.ignore { + assert.Contains(t, output, "if: ${{ always() && vars.APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}") + assert.Contains(t, output, "GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY: ${{ secrets.APP_KEY }}") + assert.NotContains(t, output, "if: ${{ secrets.") + } else { + assert.Contains(t, output, "- name: Generate GitHub App token for output ingestion\n if: always()\n") + } + assert.Nil(t, app.Permissions, "minting must not mutate the configured app") + }) + } +} + +func TestOutputCollectionGitHubAppOwnerAndWildcard(t *testing.T) { + for _, wildcard := range []bool{false, true} { + t.Run(map[bool]string{false: "repository-scoped", true: "installation-wide"}[wildcard], func(t *testing.T) { + app := &GitHubAppConfig{AppID: "${{ vars.APP_ID }}", PrivateKey: "${{ secrets.APP_KEY }}"} + if wildcard { + app.Repositories = []string{"*"} + } + data := &WorkflowData{ + On: "on:\n workflow_call:\n", + SafeOutputs: &SafeOutputsConfig{GitHubApp: app}, + } + compiler := NewCompiler() + var yaml strings.Builder + require.NoError(t, compiler.generateOutputCollectionStep(&yaml, data)) + output := yaml.String() + assert.Contains(t, output, "- name: Derive GitHub App owner for output ingestion\n if: always()\n") + assert.Contains(t, output, "GH_AW_TARGET_REPOSITORY: ${{ needs.activation.outputs.target_repo }}") + assert.Contains(t, output, "owner: ${{ steps.safe-outputs-ingestion-app-token-owner.outputs.owner }}") + assert.NotContains(t, output, "permission-members:") + if wildcard { + assert.NotContains(t, output, "repositories:") + assert.True(t, compiler.wildcardAppTokenSteps[appTokenStepKey{ + jobName: "agent", id: "safe-outputs-ingestion-app-token", clientID: app.AppID, privateKey: app.PrivateKey, + }]) + } else { + assert.Contains(t, output, "repositories: ${{ needs.activation.outputs.target_repo_name }}") + } + }) + } +} + +func TestAgenticOutputCollectionWithGitHubApp(t *testing.T) { + workflowFile := writeStepTokenWorkflow(t, "ingestion-app", `--- +on: workflow_dispatch +engine: claude +strict: false +safe-outputs: + github-token: ${{ secrets.PAT }} + github-app: + client-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_KEY }} + owner: my-org + repositories: [my-repo] + permissions: + members: read + mentions: + allowed-teams: [my-org/my-team] + add-comment: +--- +# Ingestion app token +`) + require.NoError(t, NewCompiler().CompileWorkflow(workflowFile)) + content, err := os.ReadFile(strings.TrimSuffix(workflowFile, ".md") + ".lock.yml") + require.NoError(t, err) + agent := extractJobSection(string(content), "agent") + assert.Contains(t, agent, "id: safe-outputs-ingestion-app-token\n") + assert.Contains(t, agent, "github-token: ${{ steps.safe-outputs-ingestion-app-token.outputs.token }}") + assert.NotContains(t, agent, "steps.safe-outputs-app-token.outputs.token") + assert.Less(t, strings.Index(agent, "id: agentic_execution\n"), strings.Index(agent, "id: safe-outputs-ingestion-app-token\n")) + safeOutputs := extractJobSection(string(content), "safe_outputs") + assert.Contains(t, safeOutputs, "github-token: ${{ steps.safe-outputs-app-token.outputs.token }}") + assert.NotContains(t, safeOutputs, "steps.safe-outputs-ingestion-app-token.outputs.token") +} + func TestAgenticOutputCollection(t *testing.T) { // Create temporary directory for test files tmpDir := testutil.TempDir(t, "agentic-output-test") diff --git a/pkg/workflow/compiler_yaml_step_lifecycle.go b/pkg/workflow/compiler_yaml_step_lifecycle.go index c8d04d07b84..5e39099de23 100644 --- a/pkg/workflow/compiler_yaml_step_lifecycle.go +++ b/pkg/workflow/compiler_yaml_step_lifecycle.go @@ -265,6 +265,56 @@ func (c *Compiler) generateCreateAwInfo(yaml *strings.Builder, data *WorkflowDat yaml.WriteString(" await main(core, context);\n") } +func (c *Compiler) generateOutputCollectionGitHubToken(yaml *strings.Builder, data *WorkflowData) string { + if data.SafeOutputs == nil { + return "" + } + config := data.SafeOutputs + app := config.GitHubApp + if app == nil { + return config.GitHubToken + } + + permissions := NewPermissions() + if config.AddComments != nil { + commentPermissions := buildAddCommentPermissions(config.AddComments) + for _, scope := range []PermissionScope{PermissionIssues, PermissionPullRequests} { + if _, ok := commentPermissions.Get(scope); ok { + permissions.Set(scope, PermissionRead) + } + } + } + if config.Mentions != nil && len(config.Mentions.AllowedTeams) > 0 { + permissions.Set(PermissionMembers, PermissionRead) + } + const stepID = "safe-outputs-ingestion-app-token" + fallbackRepo := "" + if hasWorkflowCallTrigger(data.On) { + fallbackRepo = "${{ needs.activation.outputs.target_repo_name }}" + } + steps := c.buildGitHubAppTokenMintStepForJob("agent", app, permissions, fallbackRepo, + inferSingleCheckoutRepositoryForGitHubAppOwner(data), + "Generate GitHub App token for output ingestion", stepID) + for _, step := range collapseYAMLLinesIntoSteps(steps) { + // Ingestion runs even after agent failure; token minting and owner resolution must too. + if prefix, condition, found := strings.Cut(step, " if: "); found { + existing, rest, _ := strings.Cut(condition, "\n") + step = prefix + " if: " + combineGitHubIfExpressions("always()", existing) + "\n" + rest + } else { + firstLine, rest, _ := strings.Cut(step, "\n") + step = firstLine + "\n if: always()\n" + rest + } + yaml.WriteString(step) + } + token := "${{ steps." + stepID + ".outputs.token }}" + if app.shouldIgnoreMissingKey() { + compilerYamlStepLifecycleLog.Print("Ingest agent output uses a same-job safe-outputs.github-app token with a configured/default token fallback") + return combineTokenExpressions(token, resolveSafeOutputGitHubToken(config.GitHubToken)) + } + compilerYamlStepLifecycleLog.Print("Ingest agent output uses a same-job safe-outputs.github-app token for mention allowlist resolution") + return token +} + func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *WorkflowData) error { //nolint:largefunc // Existing artifact collection keeps related output paths and ordering together. // Copy the raw safe-output NDJSON to a /tmp/gh-aw/ path so it can be included in the // unified agent artifact together with all other /tmp/gh-aw/ outputs. @@ -289,6 +339,7 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor yaml.WriteString(" fi\n") } + githubToken := c.generateOutputCollectionGitHubToken(yaml, data) yaml.WriteString(" - name: Ingest agent output\n") yaml.WriteString(" id: collect_output\n") yaml.WriteString(" if: always()\n") @@ -352,11 +403,11 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor } yaml.WriteString(" with:\n") - // Resolve mention allowlists with the configured token before sanitization. - // Safe-output app tokens are minted in a different job and are unavailable here. - if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { - compilerYamlStepLifecycleLog.Print("Ingest agent output uses safe-outputs.github-token for mention allowlist resolution") - yaml.WriteString(" github-token: " + resolveSafeOutputGitHubToken(data.SafeOutputs.GitHubToken) + "\n") + if githubToken != "" { + if data.SafeOutputs.GitHubApp == nil { + compilerYamlStepLifecycleLog.Print("Ingest agent output uses safe-outputs.github-token for mention allowlist resolution") + } + yaml.WriteString(" github-token: " + githubToken + "\n") } else { compilerYamlStepLifecycleLog.Print("Ingest agent output uses the default GitHub Actions token for mention allowlist resolution") } diff --git a/pkg/workflow/security_architecture_formal_test.go b/pkg/workflow/security_architecture_formal_test.go index 6bcbd58f112..b607032e515 100644 --- a/pkg/workflow/security_architecture_formal_test.go +++ b/pkg/workflow/security_architecture_formal_test.go @@ -416,12 +416,9 @@ Simulate a wildcard network violation. // TestFormal_P10_WriteTokenIsolatedToSafeOutput (P10 TokenIsolation) // -// Spec Section 5: write tokens must be absent from the agent job's environment -// and present only in the safe_outputs job. -// -// Compiles a real workflow with a safe-outputs github-app configuration and -// inspects the produced YAML to verify that the private key appears only in -// the safe_outputs mint step inputs (under with:) and not in the agent job. +// Spec Section 5: write credentials must not be available during agent execution. +// The ingestion app key may appear in a trusted post-agent mint step, not in the +// execution environment; that token must not request write permissions by default. func TestFormal_P10_WriteTokenIsolatedToSafeOutput(t *testing.T) { md := `--- name: token-isolation-test @@ -438,7 +435,7 @@ safe-outputs: # Mission -Token isolation test: verify the private key is restricted to the safe_outputs job. +Token isolation test: verify the private key is unavailable during agent execution. ` tmpDir := t.TempDir() mdPath := filepath.Join(tmpDir, "workflow.md") @@ -462,9 +459,18 @@ Token isolation test: verify the private key is restricted to the safe_outputs j safeOutputsSection, hasSafeOutputs := sections["safe_outputs"] require.True(t, hasSafeOutputs, "compiled YAML must contain a safe_outputs job") - // The agent job must not carry the private key material in any form. - assert.NotContains(t, agentSection, "APP_PRIVATE_KEY", - "agent job must not carry the private key material — token isolation requires it stays in safe_outputs") + mintStart := strings.Index(agentSection, " - name: Generate GitHub App token for output ingestion\n") + require.GreaterOrEqual(t, mintStart, 0) + executionStart := strings.Index(agentSection, " id: agentic_execution\n") + require.GreaterOrEqual(t, executionStart, 0) + assert.Less(t, executionStart, mintStart) + require.Contains(t, agentSection, " - name: Stop MCP Gateway\n") + assert.Less(t, strings.Index(agentSection, " - name: Stop MCP Gateway\n"), mintStart) + assert.NotContains(t, agentSection[:mintStart], "APP_PRIVATE_KEY", + "the ingestion app key must not be present before agent execution finishes") + assert.Contains(t, agentSection[mintStart:], "private-key: ${{ secrets.APP_PRIVATE_KEY }}") + assert.NotContains(t, agentSection, "permission-issues: write", + "the ingestion token must not request the safe_outputs job's write permissions") // The safe_outputs job must hold the private key in its mint step inputs (with: block). assert.Contains(t, safeOutputsSection, "private-key: ${{ secrets.APP_PRIVATE_KEY }}", From 72fd72d172288636367b60f37212904beb409cac Mon Sep 17 00:00:00 2001 From: pelikhan Date: Wed, 7 Oct 2026 06:38:42 -0700 Subject: [PATCH 4/6] Separate mention filtering credentials from safe-output write tokens Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../docs/reference/frontmatter-full.md | 204 ++++++++++++++++++ .../content/docs/reference/safe-outputs.md | 11 +- .../docs/specs/safe-outputs-specification.md | 15 +- pkg/parser/schemas/main_workflow_schema.json | 8 + pkg/workflow/agentic_output_test.go | 108 ++++++++-- pkg/workflow/compiler_yaml_step_lifecycle.go | 26 ++- pkg/workflow/safe_outputs_config_types.go | 3 + pkg/workflow/safe_outputs_mentions_test.go | 23 ++ pkg/workflow/safe_outputs_messages_config.go | 4 + .../safe_outputs_step_token_validation.go | 14 +- ...safe_outputs_step_token_validation_test.go | 46 +++- .../security_architecture_formal_test.go | 18 +- 12 files changed, 430 insertions(+), 50 deletions(-) diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index abd33e732ef..3b30b0e934e 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -22160,6 +22160,210 @@ safe-outputs: # Format 2: Advanced configuration for @mention filtering with fine-grained # control mentions: + # Dedicated token for resolving mention allowlists during ingestion. Does not + # inherit safe-outputs.github-token or affect write operations. + # (optional) + github-token: "${{ secrets.GITHUB_TOKEN }}" + + # Dedicated GitHub App for resolving mention allowlists during ingestion. Minted + # after agent execution; takes precedence over mentions.github-token. Does not + # inherit safe-outputs.github-app. + # (optional) + github-app: + # Deprecated alias for client-id. GitHub App ID/client ID (e.g., '${{ vars.APP_ID + # }}'). + # (optional) + app-id: "example-value" + + # GitHub App client ID (e.g., '${{ vars.APP_ID }}'). Required to mint a GitHub App + # token. + # (optional) + client-id: "example-value" + + # GitHub App private key (e.g., '${{ secrets.APP_PRIVATE_KEY }}'). Required to + # mint a GitHub App token. + # (optional) + private-key: "example-value" + + # If true, skip token minting when client-id/private-key resolve to empty strings + # at runtime. Defaults to false. + # (optional) + ignore-if-missing: true + + # Optional owner of the GitHub App installation (defaults to current repository + # owner if not specified) + # (optional) + owner: "example-value" + + # Optional list of repositories to grant access to (defaults to current repository + # if not specified) + # (optional) + repositories: [] + # Array of strings + + # Optional extra GitHub App-only permissions to merge into the minted token. Takes + # effect for tools.github.github-app and safe-outputs.github-app; ignored in + # on.github-app and the top-level github-app fallback. Use to add GitHub App-only + # scopes (e.g. members, organization-administration) not expressible via standard + # handler declarations. + # (optional) + permissions: + # Permission level for repository administration (read/none; "write" is rejected + # by the compiler). GitHub App-only permission for repository administration. + # (optional) + administration: "read" + + # Permission level for Codespaces (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + codespaces: "read" + + # Permission level for Codespaces lifecycle administration (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + codespaces-lifecycle-admin: "read" + + # Permission level for Codespaces metadata (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + codespaces-metadata: "read" + + # Permission level for user email addresses (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + email-addresses: "read" + + # Permission level for repository environments (read/none; "write" is rejected by + # the compiler). GitHub App-only permission. + # (optional) + environments: "read" + + # Permission level for git signing (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + git-signing: "read" + + # Permission level for organization members (read/none; "write" is rejected by the + # compiler). Required for org team membership API calls. + # (optional) + members: "read" + + # Permission level for organization administration (read/none; "write" is rejected + # by the compiler). GitHub App-only permission. + # (optional) + organization-administration: "read" + + # Permission level for organization announcement banners (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-announcement-banners: "read" + + # Permission level for organization Codespaces (read/none; "write" is rejected by + # the compiler). GitHub App-only permission. + # (optional) + organization-codespaces: "read" + + # Permission level for organization Copilot (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + organization-copilot: "read" + + # Permission level for organization custom org roles (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-custom-org-roles: "read" + + # Permission level for organization custom properties (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-custom-properties: "read" + + # Permission level for organization custom repository roles (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-custom-repository-roles: "read" + + # Permission level for organization events (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + organization-events: "read" + + # Permission level for organization webhooks (read/none; "write" is rejected by + # the compiler). GitHub App-only permission. + # (optional) + organization-hooks: "read" + + # Permission level for organization members management (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-members: "read" + + # Permission level for organization packages (read/none; "write" is rejected by + # the compiler). GitHub App-only permission. + # (optional) + organization-packages: "read" + + # Permission level for organization personal access token requests (read/none; + # "write" is rejected by the compiler). GitHub App-only permission. + # (optional) + organization-personal-access-token-requests: "read" + + # Permission level for organization personal access tokens (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-personal-access-tokens: "read" + + # Permission level for organization plan (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + organization-plan: "read" + + # Permission level for organization self-hosted runners (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-self-hosted-runners: "read" + + # Permission level for organization user blocking (read/none; "write" is rejected + # by the compiler). GitHub App-only permission. + # (optional) + organization-user-blocking: "read" + + # Permission level for repository custom properties (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + repository-custom-properties: "read" + + # Permission level for secret scanning alerts (read/none). Forwarded as + # permission-secret-scanning-alerts input for actions/create-github-app-token. + # (optional) + secret-scanning-alerts: "read" + + # Permission level for repository webhooks (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + repository-hooks: "read" + + # Permission level for single file access (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + single-file: "read" + + # Permission level for team discussions (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + team-discussions: "read" + + # Permission level for Dependabot vulnerability alerts (read/none; "write" is + # rejected by the compiler). Also available as a GITHUB_TOKEN scope. When used + # with a GitHub App, forwarded as permission-vulnerability-alerts input. + # (optional) + vulnerability-alerts: "read" + + # Permission level for GitHub Actions workflow files (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + workflows: "read" + # Allow mentions of repository collaborators (users with repository access, # excluding bots). Default: true # (optional) diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index c6be4436a51..80841f4d0d2 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -2123,7 +2123,16 @@ safe-outputs: **`allowed-teams`** lets organizations allow all members of specific GitHub teams to be mentioned without listing individual usernames. Team members are fetched from the GitHub API at runtime using `GET /orgs/{org}/teams/{team_slug}/members`. Bot accounts within the team are excluded. Use `org/team-slug` for cross-org teams or just `team-slug` to resolve against the current repository's organization. -Configure `safe-outputs.github-token` or `safe-outputs.github-app` with access to team membership. **Ingest agent output** resolves allowed mentions before sanitization using the global token or a dedicated app token minted in the `agent` job after agent execution. The app token takes precedence and requests `members: read` when `allowed-teams` is configured. With `ignore-if-missing: true`, missing app credentials fall back to the global token, then `GH_AW_GITHUB_TOKEN`, then `GITHUB_TOKEN`. Per-output token overrides do not apply to ingestion. +Configure `safe-outputs.mentions.github-token` or `safe-outputs.mentions.github-app` with read access to team membership. **Ingest agent output** resolves allowed mentions before sanitization using these dedicated credentials. It does not inherit `safe-outputs.github-token`, `safe-outputs.github-app`, or `GH_AW_GITHUB_TOKEN`; without mention-specific credentials it uses the default Actions token. + +```yaml wrap +safe-outputs: + mentions: + github-token: ${{ secrets.MENTIONS_READ_PAT }} + allowed-teams: [my-org/my-team] +``` + +Alternatively, configure `safe-outputs.mentions.github-app` with the standard `client-id`, `private-key`, `owner`, `repositories`, and `permissions` fields. Its token is minted after agent execution and gateway shutdown, takes precedence over `mentions.github-token`, and requests `members: read` when `allowed-teams` is configured. With `ignore-if-missing: true`, missing app credentials fall back to `mentions.github-token`, then `GITHUB_TOKEN`. Omitting the mention-specific app disables ingestion app-token minting without disabling mention filtering. Downstream write credentials remain unchanged. > [!IMPORTANT] > `allowed-teams` requires the workflow token to have `read:org` scope. The default `GITHUB_TOKEN` does **not** include this scope. Use one of the following: diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index aaad43e502a..85a88c447d8 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -319,7 +319,7 @@ The Safe Outputs MCP Gateway implements defense-in-depth through strict architec **Requirement AR1: Agent Isolation** -Agents MUST execute without GitHub write permissions. Only read-level tokens SHALL be accessible to agent processes. Write-capable tokens MUST remain confined to trusted safe-output processing steps, including post-agent ingestion, and MUST NOT be accessible during agent execution. +Agents MUST execute without GitHub write permissions. Only read-level tokens SHALL be accessible to agent processes. Safe-output write credentials MUST remain confined to downstream safe-output processing jobs. Dedicated mention-resolution credentials MUST be supplied only to trusted post-agent ingestion steps and MUST NOT be accessible during agent execution. **Verification**: @@ -2136,13 +2136,16 @@ Requirements: **Ingestion credential requirements**: -- When `safe-outputs.github-app` is configured, the compiler MUST mint a dedicated installation token in the `agent` job immediately before **Ingest agent output**, after agent execution. The token MUST use the configured installation owner, repositories, and explicit permission overrides, with read permissions for ingestion's repository and comment-author lookups and `members: read` when `mentions.allowed-teams` is configured. +- Ingestion MUST use only the credentials configured under `safe-outputs.mentions.github-token` or `safe-outputs.mentions.github-app`. It MUST NOT inherit `safe-outputs.github-token`, `safe-outputs.github-app`, per-output credentials, or `GH_AW_GITHUB_TOKEN`. +- When `safe-outputs.mentions.github-app` is configured, the compiler MUST mint a dedicated installation token in the `agent` job immediately before **Ingest agent output**, after agent execution and gateway shutdown. The token MUST use the configured installation owner, repositories, and explicit permission overrides, with read permissions for ingestion's comment-author lookups and `members: read` when `mentions.allowed-teams` is configured. +- Without a mention-specific app, ingestion MUST NOT mint an app token or resolve its installation owner; it MUST use `safe-outputs.mentions.github-token` when configured, otherwise the default GitHub Actions token. Mention credentials MUST NOT change downstream safe-output write credentials. - Ingestion app credentials MUST be confined to trusted post-agent token-minting step inputs and MUST NOT be added to agent execution environments. The ingestion token MUST NOT inherit the safe-output handlers' write permissions; configured app permission overrides remain explicit author choices. -- Token minting and installation-owner resolution MUST run even after an earlier step fails, matching ingestion's `always()` behavior. With `ignore-if-missing: true`, missing credentials MUST skip minting and ingestion MUST fall back to `safe-outputs.github-token`, then `GH_AW_GITHUB_TOKEN`, then `GITHUB_TOKEN`. -- The same-job app token MUST take precedence over `safe-outputs.github-token`. Without an app, a configured global token MUST be used to resolve mention allowlists, including `mentions.allowed-teams`, before sanitization. +- Token minting and installation-owner resolution MUST run even after an earlier step fails, matching ingestion's `always()` behavior. With `ignore-if-missing: true`, missing credentials MUST skip minting and ingestion MUST fall back to `safe-outputs.mentions.github-token`, then `GITHUB_TOKEN`. +- The same-job mention app token MUST take precedence over `safe-outputs.mentions.github-token`. - Allowed team members' raw `@login` mentions MUST be preserved during ingestion so downstream handlers can notify those users. -- Without a global app or token, ingestion MUST retain the default GitHub Actions token. Per-output token overrides and GitHub App tokens minted in the `safe_outputs` job MUST NOT be used by ingestion in the `agent` job. -- A global token referencing `steps..outputs.*` MUST be produced by an earlier step in the `agent` job as well as in every other consuming job. +- Without mention-specific credentials, ingestion MUST retain the default GitHub Actions token, regardless of configured write credentials. +- A mention token referencing `steps..outputs.*` MUST be produced by an earlier step in the `agent` job; it MUST NOT require minting in the safe-output processing jobs. +- Mention credentials MUST NOT be serialized into agent-visible validation or handler configuration. - Compiler debug logging SHOULD identify the ingestion token source without logging credential values or token expressions. **Transformation T6: Markdown Safety** diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 7ffa91cb82d..ee19817fe11 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -12438,6 +12438,14 @@ "type": "object", "description": "Advanced configuration for @mention filtering with fine-grained control", "properties": { + "github-token": { + "$ref": "#/$defs/github_token_same_job", + "description": "Dedicated token for resolving mention allowlists during ingestion. Does not inherit safe-outputs.github-token or affect write operations." + }, + "github-app": { + "$ref": "#/$defs/github_app", + "description": "Dedicated GitHub App for resolving mention allowlists during ingestion. Minted after agent execution; takes precedence over mentions.github-token. Does not inherit safe-outputs.github-app." + }, "allowed-collaborators": { "type": "boolean", "description": "Allow mentions of repository collaborators (users with repository access, excluding bots). Default: true", diff --git a/pkg/workflow/agentic_output_test.go b/pkg/workflow/agentic_output_test.go index c6711e3e49c..669348caa18 100644 --- a/pkg/workflow/agentic_output_test.go +++ b/pkg/workflow/agentic_output_test.go @@ -26,7 +26,7 @@ func TestOutputCollectionGitHubToken(t *testing.T) { {name: "default token", safeOutputs: &SafeOutputsConfig{}}, { name: "step token with fallback", - safeOutputs: &SafeOutputsConfig{GitHubToken: "${{ steps.mint.outputs.token || secrets.MENTIONS_PAT || secrets.GITHUB_TOKEN }}"}, + safeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{GitHubToken: "${{ steps.mint.outputs.token || secrets.MENTIONS_PAT || secrets.GITHUB_TOKEN }}"}}, token: "${{ steps.mint.outputs.token || secrets.MENTIONS_PAT || secrets.GITHUB_TOKEN }}", }, { @@ -36,19 +36,17 @@ func TestOutputCollectionGitHubToken(t *testing.T) { }, }, { - name: "global token takes precedence over per-handler token", + name: "global token is not used", safeOutputs: &SafeOutputsConfig{ GitHubToken: "${{ secrets.MENTIONS_PAT }}", AddComments: &AddCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{GitHubToken: "${{ secrets.COMMENT_PAT }}"}}, }, - token: "${{ secrets.MENTIONS_PAT }}", }, { - name: "safe outputs app token is minted in agent job", + name: "safe outputs app is not used", safeOutputs: &SafeOutputsConfig{ GitHubApp: &GitHubAppConfig{AppID: "${{ vars.APP_ID }}", PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}"}, }, - token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token }}", }, } { t.Run(tt.name, func(t *testing.T) { @@ -73,7 +71,7 @@ func TestOutputCollectionGitHubAppToken(t *testing.T) { }{ {name: "app overrides PAT", pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token }}"}, {name: "missing credentials fall back to PAT", ignore: true, pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token || secrets.PAT }}"}, - {name: "missing credentials fall back to defaults", ignore: true, token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token || secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}"}, + {name: "missing credentials fall back to default Actions token", ignore: true, token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token || secrets.GITHUB_TOKEN }}"}, } { t.Run(tt.name, func(t *testing.T) { app := &GitHubAppConfig{ @@ -81,8 +79,8 @@ func TestOutputCollectionGitHubAppToken(t *testing.T) { Owner: "my-org", Repositories: []string{"my-repo", "another-repo"}, IgnoreIfMissing: tt.ignore, } data := &WorkflowData{SafeOutputs: &SafeOutputsConfig{ - GitHubApp: app, GitHubToken: tt.pat, AddComments: &AddCommentsConfig{}, - Mentions: &MentionsConfig{AllowedTeams: []string{"my-org/my-team"}}, + GitHubToken: "${{ secrets.WRITE_PAT }}", AddComments: &AddCommentsConfig{}, + Mentions: &MentionsConfig{GitHubApp: app, GitHubToken: tt.pat, AllowedTeams: []string{"my-org/my-team"}}, }} var yaml strings.Builder require.NoError(t, NewCompiler().generateOutputCollectionStep(&yaml, data)) @@ -96,6 +94,7 @@ func TestOutputCollectionGitHubAppToken(t *testing.T) { assert.NotContains(t, output, ": write\n") assert.Contains(t, output, "github-token: "+tt.token+"\n") assert.NotContains(t, output, "steps.safe-outputs-app-token.outputs.token") + assert.NotContains(t, output, "secrets.WRITE_PAT") assert.Less(t, strings.Index(output, "id: safe-outputs-ingestion-app-token\n"), strings.Index(output, "- name: Ingest agent output\n")) if tt.ignore { assert.Contains(t, output, "if: ${{ always() && vars.APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}") @@ -118,7 +117,7 @@ func TestOutputCollectionGitHubAppOwnerAndWildcard(t *testing.T) { } data := &WorkflowData{ On: "on:\n workflow_call:\n", - SafeOutputs: &SafeOutputsConfig{GitHubApp: app}, + SafeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{GitHubApp: app}}, } compiler := NewCompiler() var yaml strings.Builder @@ -140,6 +139,71 @@ func TestOutputCollectionGitHubAppOwnerAndWildcard(t *testing.T) { } } +func TestOutputCollectionMentionCredentialsIndependent(t *testing.T) { + for _, tt := range []struct { + name string + pat string + }{ + {name: "default token"}, + {name: "dedicated mention PAT", pat: "${{ secrets.MENTIONS_PAT }}"}, + } { + t.Run(tt.name, func(t *testing.T) { + content := `--- +on: workflow_dispatch +engine: claude +safe-outputs: + github-token: ${{ secrets.WRITE_PAT }} + github-app: + client-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_KEY }} + mentions: + allowed-teams: [my-org/my-team] +` + if tt.pat != "" { + content += " github-token: " + tt.pat + "\n" + } + content += " add-comment:\n---\n# Independent mention credentials\n" + file := writeStepTokenWorkflow(t, "mention-credentials", content) + compiler := NewCompiler() + data, err := compiler.ParseWorkflowFile(file) + require.NoError(t, err) + require.Equal(t, tt.pat, data.SafeOutputs.Mentions.GitHubToken) + require.NoError(t, compiler.CompileWorkflow(file)) + lock, err := os.ReadFile(strings.TrimSuffix(file, ".md") + ".lock.yml") + require.NoError(t, err) + agent := extractJobSection(string(lock), "agent") + require.Contains(t, agent, "- name: Ingest agent output\n") + ingest := strings.SplitN(strings.SplitN(agent, "- name: Ingest agent output\n", 2)[1], "\n - ", 2)[0] + assert.NotContains(t, agent, "safe-outputs-ingestion-app-token") + assert.NotContains(t, ingest, "secrets.WRITE_PAT") + if tt.pat == "" { + assert.NotContains(t, ingest, "github-token:") + } else { + assert.Contains(t, ingest, "github-token: "+tt.pat) + } + assert.Equal(t, []string{"my-org/my-team"}, data.SafeOutputs.Mentions.AllowedTeams) + assert.Nil(t, data.SafeOutputs.Mentions.Enabled, "mention filtering policy must remain unchanged") + assert.Contains(t, ingest, "collect_ndjson_output.cjs") + assert.Contains(t, extractJobSection(string(lock), "safe_outputs"), "id: safe-outputs-app-token\n") + }) + } +} + +func TestOutputCollectionMentionTokenRejectsLiteral(t *testing.T) { + file := writeStepTokenWorkflow(t, "invalid-mention-token", `--- +on: workflow_dispatch +safe-outputs: + add-comment: + mentions: + github-token: "literal-token" +--- +# Invalid mention credential +`) + _, err := NewCompiler().ParseWorkflowFile(file) + require.Error(t, err) + assert.Contains(t, err.Error(), "github-token") +} + func TestAgenticOutputCollectionWithGitHubApp(t *testing.T) { workflowFile := writeStepTokenWorkflow(t, "ingestion-app", `--- on: workflow_dispatch @@ -149,12 +213,20 @@ safe-outputs: github-token: ${{ secrets.PAT }} github-app: client-id: ${{ vars.APP_ID }} - private-key: ${{ secrets.APP_KEY }} + private-key: ${{ secrets.WRITE_APP_KEY }} owner: my-org repositories: [my-repo] permissions: members: read mentions: + github-token: ${{ secrets.MENTIONS_PAT }} + github-app: + client-id: ${{ vars.MENTIONS_APP_ID }} + private-key: ${{ secrets.MENTIONS_APP_KEY }} + owner: my-org + repositories: [my-repo] + permissions: + members: read allowed-teams: [my-org/my-team] add-comment: --- @@ -167,10 +239,19 @@ safe-outputs: assert.Contains(t, agent, "id: safe-outputs-ingestion-app-token\n") assert.Contains(t, agent, "github-token: ${{ steps.safe-outputs-ingestion-app-token.outputs.token }}") assert.NotContains(t, agent, "steps.safe-outputs-app-token.outputs.token") - assert.Less(t, strings.Index(agent, "id: agentic_execution\n"), strings.Index(agent, "id: safe-outputs-ingestion-app-token\n")) + assert.NotContains(t, agent, "secrets.WRITE_APP_KEY") + assert.Contains(t, agent, "private-key: ${{ secrets.MENTIONS_APP_KEY }}") + mintStart := strings.Index(agent, " - name: Generate GitHub App token for output ingestion\n") + require.GreaterOrEqual(t, mintStart, 0) + require.Contains(t, agent, "id: agentic_execution\n") + require.Contains(t, agent, " - name: Stop MCP Gateway\n") + assert.Less(t, strings.Index(agent, "id: agentic_execution\n"), mintStart) + assert.Less(t, strings.Index(agent, " - name: Stop MCP Gateway\n"), mintStart) + assert.NotContains(t, agent[:mintStart], "secrets.MENTIONS_APP_KEY") safeOutputs := extractJobSection(string(content), "safe_outputs") assert.Contains(t, safeOutputs, "github-token: ${{ steps.safe-outputs-app-token.outputs.token }}") assert.NotContains(t, safeOutputs, "steps.safe-outputs-ingestion-app-token.outputs.token") + assert.NotContains(t, safeOutputs, "secrets.MENTIONS_APP_KEY") } func TestAgenticOutputCollection(t *testing.T) { @@ -190,8 +271,9 @@ tools: engine: claude strict: false safe-outputs: - github-token: ${{ secrets.MENTIONS_PAT }} + github-token: ${{ secrets.WRITE_PAT }} mentions: + github-token: ${{ secrets.MENTIONS_PAT }} allowed-teams: [my-org/my-team] add-labels: allowed: ["bug", "enhancement"] @@ -251,7 +333,7 @@ This workflow tests the agentic output collection functionality. require.Contains(t, lockContent, "- name: Ingest agent output\n") ingestStep := strings.SplitN(strings.SplitN(lockContent, "- name: Ingest agent output\n", 2)[1], "\n - ", 2)[0] assert.Contains(t, ingestStep, "github-token: ${{ secrets.MENTIONS_PAT }}") - assert.Contains(t, extractJobSection(lockContent, "safe_outputs"), "github-token: ${{ secrets.MENTIONS_PAT }}") + assert.Contains(t, extractJobSection(lockContent, "safe_outputs"), "github-token: ${{ secrets.WRITE_PAT }}") // Upload Safe Outputs and Upload sanitized agent output are now merged into the // unified 'agent' artifact — individual upload steps no longer exist. diff --git a/pkg/workflow/compiler_yaml_step_lifecycle.go b/pkg/workflow/compiler_yaml_step_lifecycle.go index 5e39099de23..7aabd3ca2d4 100644 --- a/pkg/workflow/compiler_yaml_step_lifecycle.go +++ b/pkg/workflow/compiler_yaml_step_lifecycle.go @@ -266,25 +266,28 @@ func (c *Compiler) generateCreateAwInfo(yaml *strings.Builder, data *WorkflowDat } func (c *Compiler) generateOutputCollectionGitHubToken(yaml *strings.Builder, data *WorkflowData) string { - if data.SafeOutputs == nil { + if data.SafeOutputs == nil || data.SafeOutputs.Mentions == nil { return "" } - config := data.SafeOutputs + config := data.SafeOutputs.Mentions app := config.GitHubApp if app == nil { + if config.GitHubToken != "" { + compilerYamlStepLifecycleLog.Print("Ingest agent output uses safe-outputs.mentions.github-token for mention allowlist resolution") + } return config.GitHubToken } permissions := NewPermissions() - if config.AddComments != nil { - commentPermissions := buildAddCommentPermissions(config.AddComments) + if data.SafeOutputs.AddComments != nil { + commentPermissions := buildAddCommentPermissions(data.SafeOutputs.AddComments) for _, scope := range []PermissionScope{PermissionIssues, PermissionPullRequests} { if _, ok := commentPermissions.Get(scope); ok { permissions.Set(scope, PermissionRead) } } } - if config.Mentions != nil && len(config.Mentions.AllowedTeams) > 0 { + if len(config.AllowedTeams) > 0 { permissions.Set(PermissionMembers, PermissionRead) } const stepID = "safe-outputs-ingestion-app-token" @@ -308,10 +311,14 @@ func (c *Compiler) generateOutputCollectionGitHubToken(yaml *strings.Builder, da } token := "${{ steps." + stepID + ".outputs.token }}" if app.shouldIgnoreMissingKey() { - compilerYamlStepLifecycleLog.Print("Ingest agent output uses a same-job safe-outputs.github-app token with a configured/default token fallback") - return combineTokenExpressions(token, resolveSafeOutputGitHubToken(config.GitHubToken)) + compilerYamlStepLifecycleLog.Print("Ingest agent output uses a same-job safe-outputs.mentions.github-app token with a mention-specific/default token fallback") + fallback := config.GitHubToken + if fallback == "" { + fallback = "${{ secrets.GITHUB_TOKEN }}" + } + return combineTokenExpressions(token, fallback) } - compilerYamlStepLifecycleLog.Print("Ingest agent output uses a same-job safe-outputs.github-app token for mention allowlist resolution") + compilerYamlStepLifecycleLog.Print("Ingest agent output uses a same-job safe-outputs.mentions.github-app token for mention allowlist resolution") return token } @@ -404,9 +411,6 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor yaml.WriteString(" with:\n") if githubToken != "" { - if data.SafeOutputs.GitHubApp == nil { - compilerYamlStepLifecycleLog.Print("Ingest agent output uses safe-outputs.github-token for mention allowlist resolution") - } yaml.WriteString(" github-token: " + githubToken + "\n") } else { compilerYamlStepLifecycleLog.Print("Ingest agent output uses the default GitHub Actions token for mention allowlist resolution") diff --git a/pkg/workflow/safe_outputs_config_types.go b/pkg/workflow/safe_outputs_config_types.go index fa65ba7a6b4..85f109df00c 100644 --- a/pkg/workflow/safe_outputs_config_types.go +++ b/pkg/workflow/safe_outputs_config_types.go @@ -179,6 +179,9 @@ type MentionsConfig struct { // AllowContext determines if mentions from event context are allowed (default: true) AllowContext *bool `yaml:"allow-context,omitempty" json:"allowContext,omitempty"` + GitHubToken string `yaml:"github-token,omitempty" json:"-"` + GitHubApp *GitHubAppConfig `yaml:"github-app,omitempty" json:"-"` + // Allowed is a list of user/bot names always allowed (bots not allowed by default) Allowed []string `yaml:"allowed,omitempty" json:"allowed,omitempty"` diff --git a/pkg/workflow/safe_outputs_mentions_test.go b/pkg/workflow/safe_outputs_mentions_test.go index 49664a69bae..b61a8be0325 100644 --- a/pkg/workflow/safe_outputs_mentions_test.go +++ b/pkg/workflow/safe_outputs_mentions_test.go @@ -50,6 +50,29 @@ func TestParseMentionsConfig_Boolean(t *testing.T) { } } +func TestParseMentionsCredentials(t *testing.T) { + config := parseMentionsConfig(map[string]any{ + "github-token": "${{ secrets.MENTIONS_PAT }}", + "github-app": map[string]any{ + "client-id": "${{ vars.MENTIONS_APP_ID }}", "private-key": "${{ secrets.MENTIONS_APP_KEY }}", + "ignore-if-missing": true, "permissions": map[string]any{"members": "read"}, + }, + }) + require.Equal(t, "${{ secrets.MENTIONS_PAT }}", config.GitHubToken) + require.NotNil(t, config.GitHubApp) + require.Equal(t, "${{ vars.MENTIONS_APP_ID }}", config.GitHubApp.AppID) + require.Equal(t, "${{ secrets.MENTIONS_APP_KEY }}", config.GitHubApp.PrivateKey) + require.True(t, config.GitHubApp.IgnoreIfMissing) + require.Equal(t, map[string]string{"members": "read"}, config.GitHubApp.Permissions) + runtimeConfig := buildMentionsHandlerConfig(config) + require.NotContains(t, runtimeConfig, "github-token") + require.NotContains(t, runtimeConfig, "github-app") + content, err := json.Marshal(config) + require.NoError(t, err) + require.NotContains(t, string(content), "MENTIONS_PAT") + require.NotContains(t, string(content), "MENTIONS_APP_KEY") +} + func TestParseMentionsConfig_Object(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/safe_outputs_messages_config.go b/pkg/workflow/safe_outputs_messages_config.go index e0c66c12ef8..aa0e0f4135d 100644 --- a/pkg/workflow/safe_outputs_messages_config.go +++ b/pkg/workflow/safe_outputs_messages_config.go @@ -78,6 +78,10 @@ func parseMentionsConfig(mentions any) *MentionsConfig { // Handle object configuration if mentionsMap, ok := mentions.(map[string]any); ok { + config.GitHubToken = extractStringFromMap(mentionsMap, "github-token", nil) + if app, ok := mentionsMap["github-app"].(map[string]any); ok { + config.GitHubApp = parseAppConfig(app) + } // Parse allowed-collaborators (preferred) with fallback to deprecated allow-team-members if allowedCollaborators, exists := mentionsMap["allowed-collaborators"]; exists { if val, ok := allowedCollaborators.(bool); ok { diff --git a/pkg/workflow/safe_outputs_step_token_validation.go b/pkg/workflow/safe_outputs_step_token_validation.go index 1e5cd456163..3705b9a6a62 100644 --- a/pkg/workflow/safe_outputs_step_token_validation.go +++ b/pkg/workflow/safe_outputs_step_token_validation.go @@ -17,7 +17,7 @@ var stepOutputReferencePattern = regexp.MustCompile(`steps\.([A-Za-z_][A-Za-z0-9 // collectSafeOutputStepTokenIDs returns the set of step ids referenced by // safe-outputs `github-token` expressions of the form `${{ steps..outputs. }}`, -// covering both the global token and per-output overrides. +// covering the global token, mention credentials, and per-output overrides. func collectSafeOutputStepTokenIDs(config *SafeOutputsConfig) map[string]struct{} { ids := make(map[string]struct{}) if config == nil { @@ -31,6 +31,9 @@ func collectSafeOutputStepTokenIDs(config *SafeOutputsConfig) map[string]struct{ } collect(config.GitHubToken) + if config.Mentions != nil { + collect(config.Mentions.GitHubToken) + } for _, handler := range safeOutputHandlers { if handler.StructField == "" { continue @@ -120,10 +123,15 @@ func (c *Compiler) validateSafeOutputStepTokenReferences(data *WorkflowData) err if declareIdx >= 0 && declareIdx < consumeIdx { continue } + field := "safe-outputs.github-token" + if jobName == string(constants.AgentJobName) && data.SafeOutputs.Mentions != nil && + strings.Contains(data.SafeOutputs.Mentions.GitHubToken, "steps."+stepID+".outputs.") { + field = "safe-outputs.mentions.github-token" + } if declareIdx < 0 { safeOutputsStepTokenValidationLog.Printf("Job %q consumes steps.%s.outputs.* without declaring the step", jobName, stepID) return NewValidationError( - "safe-outputs.github-token", + field, fmt.Sprintf("${{ steps.%s.outputs.* }}", stepID), fmt.Sprintf("job %q has no step with id %q; step outputs are only available inside the job that produced them, so this token would be empty at runtime and requires the minting step to run in that job", jobName, stepID), fmt.Sprintf("Add the token-minting step to job %q:\n\n%s", jobName, stepMintingHint(jobName, stepID)), @@ -131,7 +139,7 @@ func (c *Compiler) validateSafeOutputStepTokenReferences(data *WorkflowData) err } safeOutputsStepTokenValidationLog.Printf("Job %q consumes steps.%s.outputs.* before the step that declares it", jobName, stepID) return NewValidationError( - "safe-outputs.github-token", + field, fmt.Sprintf("${{ steps.%s.outputs.* }}", stepID), fmt.Sprintf("job %q runs the step with id %q after the first step that consumes the token, so the token would be empty at runtime and requires the minting step to run before its first consumer", jobName, stepID), fmt.Sprintf("Move the token-minting step earlier in job %q, for example using its pre-steps:\n\n%s", jobName, stepMintingHint(jobName, stepID)), diff --git a/pkg/workflow/safe_outputs_step_token_validation_test.go b/pkg/workflow/safe_outputs_step_token_validation_test.go index 835d084338e..e858be4fbde 100644 --- a/pkg/workflow/safe_outputs_step_token_validation_test.go +++ b/pkg/workflow/safe_outputs_step_token_validation_test.go @@ -65,7 +65,7 @@ jobs: require.NoError(t, err) lockYAML := string(lockContent) - for _, jobName := range []string{"agent", "safe_outputs", "conclusion"} { + for _, jobName := range []string{"safe_outputs", "conclusion"} { section := extractJobSection(lockYAML, jobName) require.NotEmpty(t, section, "expected %s job section", jobName) assert.Contains(t, section, "id: octosts") @@ -117,7 +117,7 @@ safe-outputs: add-comment: mentions: allowed-teams: [my-org/my-team] - github-token: ${{ steps.octosts.outputs.token || secrets.GITHUB_TOKEN }} + github-token: ${{ steps.octosts.outputs.token || secrets.GITHUB_TOKEN }} jobs: safe_outputs: pre-steps: @@ -137,9 +137,45 @@ jobs: err := NewCompiler().CompileWorkflow(workflowFile) require.Error(t, err) assert.Contains(t, err.Error(), `job "agent" has no step with id "octosts"`) + assert.Contains(t, err.Error(), "safe-outputs.mentions.github-token") assert.Contains(t, err.Error(), "pre-steps:") } +func TestSameJobStepTokenMentionsOnlyMintedInAgentCompiles(t *testing.T) { + workflowFile := writeStepTokenWorkflow(t, "mentions-only", `--- +on: + workflow_dispatch: +permissions: + contents: read + id-token: write +engine: claude +strict: false +pre-steps: + - name: Mint mention token + id: mention_mint + uses: `+stsMintStep+` +safe-outputs: + add-comment: + mentions: + allowed-teams: [my-org/my-team] + github-token: ${{ steps.mention_mint.outputs.token }} +--- + +# Mention-only same-job token +`) + + require.NoError(t, NewCompiler().CompileWorkflow(workflowFile)) + lockContent, err := os.ReadFile(filepath.Join(filepath.Dir(workflowFile), "mentions-only.lock.yml")) + require.NoError(t, err) + lockYAML := string(lockContent) + agent := extractJobSection(lockYAML, "agent") + assert.Contains(t, agent, "github-token: ${{ steps.mention_mint.outputs.token }}") + assert.Less(t, jobStepIDDeclarationIndex(agent, "mention_mint"), jobStepOutputConsumptionIndex(agent, "mention_mint")) + for _, jobName := range []string{"safe_outputs", "conclusion"} { + assert.NotContains(t, extractJobSection(lockYAML, jobName), "steps.mention_mint.outputs.token") + } +} + // TestSameJobStepTokenMintedAfterConsumerFails verifies that safe-outputs.steps, which run // after the safe_outputs checkout and git credential steps, are reported as too late for a // token consumed by those steps. @@ -184,6 +220,9 @@ jobs: func TestCollectSafeOutputStepTokenIDs(t *testing.T) { config := &SafeOutputsConfig{ GitHubToken: "${{ steps.global_mint.outputs.token || secrets.GITHUB_TOKEN }}", + Mentions: &MentionsConfig{ + GitHubToken: "${{ steps.mention_mint.outputs.token }}", + }, CreateIssues: &CreateIssuesConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{ GitHubToken: "${{ steps.issue_mint.outputs.token }}", @@ -197,7 +236,8 @@ func TestCollectSafeOutputStepTokenIDs(t *testing.T) { } ids := collectSafeOutputStepTokenIDs(config) - assert.Len(t, ids, 2) + assert.Len(t, ids, 3) + assert.Contains(t, ids, "mention_mint") assert.Contains(t, ids, "global_mint") assert.Contains(t, ids, "issue_mint") diff --git a/pkg/workflow/security_architecture_formal_test.go b/pkg/workflow/security_architecture_formal_test.go index b607032e515..3ccf17a9814 100644 --- a/pkg/workflow/security_architecture_formal_test.go +++ b/pkg/workflow/security_architecture_formal_test.go @@ -416,9 +416,8 @@ Simulate a wildcard network violation. // TestFormal_P10_WriteTokenIsolatedToSafeOutput (P10 TokenIsolation) // -// Spec Section 5: write credentials must not be available during agent execution. -// The ingestion app key may appear in a trusted post-agent mint step, not in the -// execution environment; that token must not request write permissions by default. +// Spec Section 5: write tokens and app keys must be absent from the agent job. +// Mention filtering credentials are configured separately from write credentials. func TestFormal_P10_WriteTokenIsolatedToSafeOutput(t *testing.T) { md := `--- name: token-isolation-test @@ -459,16 +458,9 @@ Token isolation test: verify the private key is unavailable during agent executi safeOutputsSection, hasSafeOutputs := sections["safe_outputs"] require.True(t, hasSafeOutputs, "compiled YAML must contain a safe_outputs job") - mintStart := strings.Index(agentSection, " - name: Generate GitHub App token for output ingestion\n") - require.GreaterOrEqual(t, mintStart, 0) - executionStart := strings.Index(agentSection, " id: agentic_execution\n") - require.GreaterOrEqual(t, executionStart, 0) - assert.Less(t, executionStart, mintStart) - require.Contains(t, agentSection, " - name: Stop MCP Gateway\n") - assert.Less(t, strings.Index(agentSection, " - name: Stop MCP Gateway\n"), mintStart) - assert.NotContains(t, agentSection[:mintStart], "APP_PRIVATE_KEY", - "the ingestion app key must not be present before agent execution finishes") - assert.Contains(t, agentSection[mintStart:], "private-key: ${{ secrets.APP_PRIVATE_KEY }}") + assert.NotContains(t, agentSection, "APP_PRIVATE_KEY", + "the safe-output write app key must not enter the agent job") + assert.NotContains(t, agentSection, "safe-outputs-ingestion-app-token") assert.NotContains(t, agentSection, "permission-issues: write", "the ingestion token must not request the safe_outputs job's write permissions") From b8df84b83780b2e33b023e1d33feca8a19c19748 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 7 Oct 2026 14:29:44 +0000 Subject: [PATCH 5/6] docs(adr): add draft ADR-66571 for dedicated mention ingestion credentials --- ...dedicated-mention-ingestion-credentials.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/adr/66571-dedicated-mention-ingestion-credentials.md diff --git a/docs/adr/66571-dedicated-mention-ingestion-credentials.md b/docs/adr/66571-dedicated-mention-ingestion-credentials.md new file mode 100644 index 00000000000..352ca938e30 --- /dev/null +++ b/docs/adr/66571-dedicated-mention-ingestion-credentials.md @@ -0,0 +1,50 @@ +# ADR-66571: Use Dedicated, Non-Inheriting Credentials for Mention Allowlist Resolution During Ingestion + +**Date**: 2026-11-19 +**Status**: Draft +**Deciders**: pelikhan [TODO: verify full decider list] + +--- + +### Context + +`safe-outputs.mentions.allowed-teams` lets a workflow preserve `@login` mentions for members of specific teams, but the **Ingest agent output** step — the step that sanitizes agent output before any safe-output handler runs — executed with the default GitHub Actions token, which cannot read organization team membership. Allowed team members therefore had their mentions escaped during ingestion, and downstream handlers could not restore the notification because the body was already sanitized (#50282). Mention resolution happens inside the `agent` job, which by the repository's security architecture must never hold safe-output *write* credentials, so the fix could not simply reuse `safe-outputs.github-token`, `safe-outputs.github-app`, per-handler tokens, or `GH_AW_GITHUB_TOKEN`. Any new credential also had to survive ingestion's `always()` semantics and remain invisible to the agent process itself. + +### Decision + +We will introduce dedicated ingestion credentials under `safe-outputs.mentions.github-token` and `safe-outputs.mentions.github-app` that are used **only** for mention allowlist resolution and that never inherit from any other safe-output credential. When a mention-specific app is configured, the compiler mints its installation token in the `agent` job after agent execution and MCP gateway shutdown, immediately before **Ingest agent output**; that token takes precedence over the mention PAT, requests comment-author read scopes plus `members: read` when `allowed-teams` is set, and honours explicit permission overrides. Without mention-specific credentials, ingestion keeps the default Actions token and no app token is minted. The primary driver is least privilege: read-only team-membership access must be grantable without widening what the agent job or the agent process can do. + +### Alternatives Considered + +#### Alternative 1: Let ingestion inherit the existing safe-output write credentials + +Reuse `safe-outputs.github-token` / `safe-outputs.github-app` (or `GH_AW_GITHUB_TOKEN`) for the membership lookup. This was the smallest change and required no new schema surface. It was rejected because it would place write-scoped safe-output credentials inside the `agent` job, violating the formal security property that agents execute without GitHub write permissions and that write credentials stay confined to downstream safe-output jobs (`security_architecture_formal_test.go`). + +#### Alternative 2: Defer mention restoration to the downstream safe-output handlers + +Keep ingestion token-less and have each handler job — which already holds appropriate credentials — re-expand mentions for allowed teams. Rejected because ingestion escapes mentions before handlers see the payload, so the original `@login` is no longer distinguishable from author-supplied text; restoring it would require carrying unsanitized content past the sanitization boundary, re-introducing the injection risk ingestion exists to prevent. + +#### Alternative 3: Mint the mention token at job start, alongside other tokens + +Resolve the installation token at the beginning of the `agent` job together with existing token minting. Rejected because the credential would then be present in the environment for the entire agent execution and MCP gateway lifetime; minting it after gateway shutdown keeps the exposure window to the ingestion step only. + +### Consequences + +#### Positive +- Mentions of allowed team members survive ingestion, so downstream handlers can notify those users — the behaviour #50282 asks for. +- Membership read access is granted with a narrowly scoped, separately configured credential; the agent job gains no write capability and downstream write credentials are unchanged. +- Mention credentials are excluded from agent-visible validation and handler configuration, and app private keys stay out of the pre-ingestion portion of the agent job — properties now asserted by regression tests. + +#### Negative +- Adds a third credential concept (`mentions.github-token` / `mentions.github-app`) with its own precedence and fallback chain (app → PAT → `GITHUB_TOKEN`), increasing the configuration surface users must understand and the compiler must keep consistent. +- Users who expect credentials to cascade from `safe-outputs.github-token` will see mentions silently escaped until they configure the mention-specific token; the non-inheritance rule is deliberate but is a latent surprise. +- Token minting inside the `agent` job adds compiler-emitted steps and ordering constraints (`always()`, same-job producer validation) to an already intricate step-lifecycle generator. + +#### Neutral +- `MentionsConfig` gains `GitHubToken` and `GitHubApp`; the workflow JSON schema, generated frontmatter reference, safe-outputs reference, and the normative safe-outputs specification were updated in the same change. +- Installation owner/repository selection, wildcard scoping, relay repository selection, and missing-key guards reuse existing compiler helpers rather than introducing a parallel resolution path. +- Secret-safe diagnostic logging now reports which token class ingestion selected (mention app, mention PAT, or default Actions token). + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 344905c6ad4873263d25c056b1e44fd88b1488a2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:34:23 +0000 Subject: [PATCH 6/6] Defer mention resolution to trusted safe-output job Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- actions/setup/js/add_comment.cjs | 5 +- actions/setup/js/collect_ndjson_output.cjs | 69 +++------------ .../setup/js/collect_ndjson_output.test.cjs | 31 +++---- .../js/resolve_mentions_from_payload.cjs | 17 ++++ .../js/resolve_mentions_from_payload.test.cjs | 29 +++++- .../setup/js/safe_output_handler_manager.cjs | 6 +- .../setup/js/safe_output_type_validator.cjs | 7 ++ actions/setup/js/sanitize_content.cjs | 9 +- actions/setup/js/sanitize_content.test.cjs | 5 ++ ...dedicated-mention-ingestion-credentials.md | 22 ++--- .../docs/reference/frontmatter-full.md | 20 +++-- .../content/docs/reference/safe-outputs.md | 8 +- .../docs/specs/safe-outputs-specification.md | 30 +++---- pkg/parser/schemas/main_workflow_schema.json | 6 +- pkg/workflow/agentic_output_test.go | 88 +++++++++---------- pkg/workflow/compiler_safe_outputs_steps.go | 40 +++++++++ pkg/workflow/compiler_yaml_step_lifecycle.go | 63 ------------- .../github_app_permissions_validation.go | 25 ++++++ .../github_app_permissions_validation_test.go | 30 +++++++ .../permissions_compiler_validator.go | 3 + pkg/workflow/safe_outputs_config_types.go | 8 +- pkg/workflow/safe_outputs_permissions.go | 33 +++++++ pkg/workflow/safe_outputs_permissions_test.go | 25 +++++- .../safe_outputs_step_token_validation.go | 2 +- ...safe_outputs_step_token_validation_test.go | 31 +++---- 25 files changed, 352 insertions(+), 260 deletions(-) diff --git a/actions/setup/js/add_comment.cjs b/actions/setup/js/add_comment.cjs index c5454510815..633ae302394 100644 --- a/actions/setup/js/add_comment.cjs +++ b/actions/setup/js/add_comment.cjs @@ -25,7 +25,7 @@ const { createDiscussionComment, resolveTopLevelDiscussionCommentId } = require( const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { ERR_NOT_FOUND } = require("./error_codes.cjs"); const { isPayloadUserBot } = require("./resolve_mentions.cjs"); -const { resolveMentionsForItem } = require("./resolve_mentions_from_payload.cjs"); +const { getMentionsGithubClient, resolveMentionsForItem } = require("./resolve_mentions_from_payload.cjs"); const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); const { generateHistoryUrl } = require("./generate_history_link.cjs"); const { resolveInvocationContext } = require("./invocation_context_helpers.cjs"); @@ -753,7 +753,8 @@ async function main(config = {}) { if (itemTargetResult.number != null || hasExplicitCommentId) { // Explicit item_number/issue_number: fetch the issue/PR to get its author try { - const { data: issueData } = await githubClient.rest.issues.get({ + const mentionsGithubClient = getMentionsGithubClient(githubClient); + const { data: issueData } = await mentionsGithubClient.rest.issues.get({ owner: repoParts.owner, repo: repoParts.repo, issue_number: itemNumber, diff --git a/actions/setup/js/collect_ndjson_output.cjs b/actions/setup/js/collect_ndjson_output.cjs index 31c1b949c06..6edd617c141 100644 --- a/actions/setup/js/collect_ndjson_output.cjs +++ b/actions/setup/js/collect_ndjson_output.cjs @@ -5,19 +5,17 @@ const { getErrorMessage } = require("./error_helpers.cjs"); const { repairJson, sanitizePrototypePollution } = require("./json_repair_helpers.cjs"); const { AGENT_OUTPUT_FILENAME, TMP_GH_AW_PATH } = require("./constants.cjs"); const { ERR_API, ERR_PARSE } = require("./error_codes.cjs"); -const { isPayloadUserBot } = require("./resolve_mentions.cjs"); const { parseIntTemplatable } = require("./templatable.cjs"); -const { getDefaultTargetRepo, parseAllowedRepos, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { isProbingNoopMessage } = require("./intent_probe.cjs"); const { buildEmptyOutputOutcome } = require("./empty_output_outcome.cjs"); +const MENTION_AWARE_OUTPUT_TYPES = new Set(["add_comment", "close_discussion", "create_discussion", "create_issue", "create_pull_request", "create_pull_request_review_comment", "reply_to_pull_request_review_comment"]); + async function main() { try { const fs = require("fs"); const { sanitizeContent } = require("./sanitize_content.cjs"); const { validateItem, getMaxAllowedForType, getMinRequiredForType, hasValidationConfig, MAX_BODY_LENGTH: maxBodyLength, resetValidationConfigCache } = require("./safe_output_type_validator.cjs"); - const { resolveAllowedMentionsFromPayload } = require("./resolve_mentions_from_payload.cjs"); - // Load validation config from file and set it in environment for the validator to read const validationConfigPath = process.env.GH_AW_VALIDATION_CONFIG_PATH || `${process.env.RUNNER_TEMP}/gh-aw/safeoutputs/validation.json`; /** @type {any} */ @@ -38,8 +36,10 @@ async function main() { const mentionsConfig = validationConfig?.mentions || null; const maxMentions = parseIntTemplatable(mentionsConfig?.max, 50); - // Resolve mentions for each output's destination before sanitizing it. + // Mention filtering happens in the trusted safe_outputs job. Preserve mentions + // in these output types until their destination and allowlist can be resolved. let allowedMentions = []; + let deferMentionFiltering = false; // maxBotMentions is populated after safeOutputsConfig is read below /** @type {number | undefined} */ @@ -68,7 +68,7 @@ async function main() { error: `Line ${lineNum}: ${fieldName} must be a string`, }; } - normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen }); + normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen, deferMentions: deferMentionFiltering }); break; case "boolean": if (typeof value !== "boolean") { @@ -99,11 +99,11 @@ async function main() { error: `Line ${lineNum}: ${fieldName} must be one of: ${inputSchema.options.join(", ")}`, }; } - normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen }); + normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen, deferMentions: deferMentionFiltering }); break; default: if (typeof value === "string") { - normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen }); + normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen, deferMentions: deferMentionFiltering }); } break; } @@ -224,46 +224,6 @@ async function main() { // indentation/pretty-printing, parsing will fail. const lines = outputContent.trim().split("\n"); - function resolveMentionRepo(item, itemType) { - const typeConfig = expectedOutputTypes[itemType]; - const defaultTargetRepo = getDefaultTargetRepo(typeConfig && typeof typeConfig === "object" ? typeConfig : undefined); - const allowedRepos = parseAllowedRepos(typeConfig?.allowed_repos ?? safeOutputsConfig?.allowed_repos); - return resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, "mention"); - } - - // Pre-scan: collect target issue authors from add_comment items with explicit item_number - // so they are included when sanitizing the corresponding comment. - const targetIssueAuthors = new Map(); - for (const line of lines) { - const trimmedLine = line.trim(); - if (!trimmedLine) continue; - try { - const preview = JSON.parse(trimmedLine); - const previewType = (preview?.type || "").replace(/-/g, "_"); - if (previewType === "add_comment" && preview.item_number != null && typeof preview.item_number === "number") { - const repoResult = resolveMentionRepo(preview, "add_comment"); - if (!repoResult.success) { - core.info(`[MENTIONS] Skipping target issue author lookup: ${repoResult.error}`); - continue; - } - try { - const { data: issueData } = await github.rest.issues.get({ - owner: repoResult.repoParts.owner, - repo: repoResult.repoParts.repo, - issue_number: preview.item_number, - }); - if (issueData.user?.login && !isPayloadUserBot(issueData.user)) { - targetIssueAuthors.set(`${repoResult.repo.toLowerCase()}#${preview.item_number}`, issueData.user.login); - } - } catch (fetchErr) { - core.info(`[MENTIONS] Could not fetch issue #${preview.item_number} author for mention allowlist: ${getErrorMessage(fetchErr)}`); - } - } - } catch { - // Ignore parse errors - main loop will report them - } - } - const parsedItems = []; const errors = collectionErrors; for (let i = 0; i < lines.length; i++) { @@ -286,6 +246,7 @@ async function main() { core.info(`[INGESTION] Line ${i + 1}: Original type='${originalType}', Normalized type='${itemType}'`); // Update item.type to normalized value item.type = itemType; + deferMentionFiltering = MENTION_AWARE_OUTPUT_TYPES.has(itemType); if (!expectedOutputTypes[itemType]) { core.warning(`[INGESTION] Line ${i + 1}: Type '${itemType}' not found in expected types: ${JSON.stringify(Object.keys(expectedOutputTypes))}`); errors.push(`Line ${i + 1}: Unexpected output type '${itemType}'. Expected one of: ${Object.keys(expectedOutputTypes).join(", ")}`); @@ -295,17 +256,6 @@ async function main() { core.info(`[INGESTION] Line ${i + 1}: Ignoring probing noop message (does not count against the noop budget): ${JSON.stringify(item.message)}`); continue; } - const repoResult = resolveMentionRepo(item, itemType); - allowedMentions = repoResult.success - ? await resolveAllowedMentionsFromPayload( - context, - github, - core, - mentionsConfig, - itemType === "add_comment" ? [targetIssueAuthors.get(`${repoResult.repo.toLowerCase()}#${item.item_number}`)].filter(Boolean) : undefined, - repoResult.repoParts - ) - : []; const typeCount = parsedItems.filter(existing => existing.type === itemType).length; const maxAllowed = getMaxAllowedForType(itemType, expectedOutputTypes); if (typeCount >= maxAllowed) { @@ -333,6 +283,7 @@ async function main() { allowedAliases: allowedMentions, maxMentions, maxBotMentions, + deferMentions: deferMentionFiltering, normalizeIssueClosingKeywords, dataEnabled: typeConfig !== null && typeof typeConfig === "object" && typeConfig.data_enabled === true, dataSchema: typeConfig !== null && typeof typeConfig === "object" ? typeConfig.data_schema : undefined, diff --git a/actions/setup/js/collect_ndjson_output.test.cjs b/actions/setup/js/collect_ndjson_output.test.cjs index a69872dcad2..c8d5f89f4d9 100644 --- a/actions/setup/js/collect_ndjson_output.test.cjs +++ b/actions/setup/js/collect_ndjson_output.test.cjs @@ -1391,7 +1391,7 @@ describe("collect_ndjson_output.cjs", () => { parsedOutput = JSON.parse(outputCall[1]); expect(parsedOutput.items[0].body).toBe("GitHub URLs: https://github.com/repo, https://api.github.com/users, https://githubusercontent.com/file. External: (example.com/redacted)"); }), - it("should handle @mentions neutralization", async () => { + it("should defer mention filtering for trusted output processing", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt", ndjsonContent = '{"type": "create_issue", "title": "@mention Test", "body": "Hey @username and @org/team, check this out! But preserve email@domain.com"}'; (fs.writeFileSync(testFile, ndjsonContent), (process.env.GH_AW_SAFE_OUTPUTS = testFile)); @@ -1400,9 +1400,10 @@ describe("collect_ndjson_output.cjs", () => { (fs.mkdirSync("/tmp/gh-aw/safeoutputs", { recursive: !0 }), fs.writeFileSync(configPath, __config), await eval(`(async () => { ${collectScript}; await main(); })()`)); const outputCall = mockCore.setOutput.mock.calls.find(call => "output" === call[0]), parsedOutput = JSON.parse(outputCall[1]); - expect(parsedOutput.items[0].body).toBe("Hey `@username` and `@org/team`, check this out! But preserve email@domain.com"); + expect(parsedOutput.items[0].body).toBe("Hey @username and @org/team, check this out! But preserve email@domain.com"); + expect(global.github.rest.repos.listCollaborators).not.toHaveBeenCalled(); }), - it("checks collaborators in each comment's target repository, never the workflow repository", async () => { + it("does not query collaborators during untrusted ingestion", async () => { global.context.payload.issue = { user: { login: "alice", type: "User" } }; const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; fs.writeFileSync(testFile, [JSON.stringify({ type: "add_comment", repo: "target-org/first", body: "Hello @alice" }), JSON.stringify({ type: "add_comment", repo: "target-org/second", body: "Hello @alice" })].join("\n")); @@ -1411,11 +1412,11 @@ describe("collect_ndjson_output.cjs", () => { await eval(`(async () => { ${collectScript}; await main(); })()`); - expect(global.github.rest.repos.listCollaborators).toHaveBeenCalledWith(expect.objectContaining({ owner: "target-org", repo: "first" })); - expect(global.github.rest.repos.listCollaborators).toHaveBeenCalledWith(expect.objectContaining({ owner: "target-org", repo: "second" })); - expect(global.github.rest.repos.listCollaborators).not.toHaveBeenCalledWith(expect.objectContaining({ owner: "test-owner", repo: "test-repo" })); + expect(global.github.rest.repos.listCollaborators).not.toHaveBeenCalled(); + const parsed = JSON.parse(mockCore.setOutput.mock.calls.find(call => call[0] === "output")[1]); + expect(parsed.items.map(item => item.body)).toEqual(["Hello @alice", "Hello @alice"]); }), - it("keeps target issue authors scoped to their own repository and issue", async () => { + it("does not query target issue authors during untrusted ingestion", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; fs.writeFileSync( testFile, @@ -1433,10 +1434,10 @@ describe("collect_ndjson_output.cjs", () => { await eval(`(async () => { ${collectScript}; await main(); })()`); const parsed = JSON.parse(mockCore.setOutput.mock.calls.find(call => call[0] === "output")[1]); - expect(parsed.items.map(item => item.body)).toEqual(["Hello @first-author", "Hello `@first-author`"]); - expect(global.github.rest.issues.get).toHaveBeenCalledWith(expect.objectContaining({ owner: "target-org", repo: "first", issue_number: 7 })); + expect(parsed.items.map(item => item.body)).toEqual(["Hello @first-author", "Hello @first-author"]); + expect(global.github.rest.issues.get).not.toHaveBeenCalled(); }), - it("looks up explicit issue authors in a configured target-repo", async () => { + it("does not look up explicit issue authors during untrusted ingestion", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; fs.writeFileSync(testFile, JSON.stringify({ type: "add_comment", item_number: 7, body: "Hello @target-author" })); process.env.GH_AW_SAFE_OUTPUTS = testFile; @@ -1445,11 +1446,11 @@ describe("collect_ndjson_output.cjs", () => { await eval(`(async () => { ${collectScript}; await main(); })()`); - expect(global.github.rest.issues.get).toHaveBeenCalledWith(expect.objectContaining({ owner: "target-org", repo: "target-repo", issue_number: 7 })); + expect(global.github.rest.issues.get).not.toHaveBeenCalled(); const parsed = JSON.parse(mockCore.setOutput.mock.calls.find(call => call[0] === "output")[1]); expect(parsed.items[0].body).toBe("Hello @target-author"); }), - it("does not query either repository for a disallowed per-item override", async () => { + it("does not query repositories for per-item overrides during untrusted ingestion", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; fs.writeFileSync(testFile, JSON.stringify({ type: "add_comment", repo: "unauthorized/repo", body: "Hello @alice" })); process.env.GH_AW_SAFE_OUTPUTS = testFile; @@ -1460,7 +1461,7 @@ describe("collect_ndjson_output.cjs", () => { expect(global.github.rest.repos.listCollaborators).not.toHaveBeenCalled(); const parsed = JSON.parse(mockCore.setOutput.mock.calls.find(call => call[0] === "output")[1]); - expect(parsed.items[0].body).toBe("Hello `@alice`"); + expect(parsed.items[0].body).toBe("Hello @alice"); }), it("should preserve allowed aliases after max when no more than max occur", async () => { const allowed = Array.from({ length: 60 }, (_, i) => `user${i}`); @@ -1481,7 +1482,7 @@ describe("collect_ndjson_output.cjs", () => { const parsedOutput = JSON.parse(outputCall[1]); expect(parsedOutput.items[0].body).toBe("Thanks @user57, @user58, and @user59"); }), - it("should apply the mention limit across all fields in one item", async () => { + it("should defer mention limits across all fields to trusted output processing", async () => { const validationPath = "/tmp/gh-aw/safeoutputs/validation.json"; const validationConfig = JSON.parse(fs.readFileSync(validationPath, "utf8")); validationConfig.mentions = { allowContext: false, allowed: ["user1", "user2", "user3", "user4"], max: 3 }; @@ -1497,7 +1498,7 @@ describe("collect_ndjson_output.cjs", () => { const outputCall = mockCore.setOutput.mock.calls.find(call => call[0] === "output"); const parsedOutput = JSON.parse(outputCall[1]); expect(parsedOutput.items[0].title).toBe("@user1 @user2"); - expect(parsedOutput.items[0].body).toBe("@user3 `@user4` @user1"); + expect(parsedOutput.items[0].body).toBe("@user3 @user4 @user1"); }), it("should neutralize bot trigger phrases", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt", diff --git a/actions/setup/js/resolve_mentions_from_payload.cjs b/actions/setup/js/resolve_mentions_from_payload.cjs index d2f057ce8e4..f19321f040f 100644 --- a/actions/setup/js/resolve_mentions_from_payload.cjs +++ b/actions/setup/js/resolve_mentions_from_payload.cjs @@ -9,6 +9,21 @@ const { resolveMentionsLazily, isPayloadUserBot } = require("./resolve_mentions. const { getErrorMessage } = require("./error_helpers.cjs"); const { parseRepoSlug } = require("./repo_helpers.cjs"); +/** + * Use the mention-specific token for allowlist lookups instead of inheriting a + * downstream safe-output write token. + * @param {any} fallback + * @returns {any} + */ +function getMentionsGithubClient(fallback) { + const token = process.env.GH_AW_MENTIONS_GITHUB_TOKEN; + const globalState = /** @type {any} */ global; + if (token && typeof globalState.getOctokit === "function") { + return globalState.getOctokit(token); + } + return fallback; +} + /** * Push a non-bot user's login to the array if present. * @param {string[]} users - Target array @@ -182,6 +197,7 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions if (!context || !github || !core) { return []; } + github = getMentionsGithubClient(github); // If mentions is explicitly set to false, return empty array (all mentions escaped) if (mentionsConfig === false || mentionsConfig?.enabled === false) { @@ -298,6 +314,7 @@ async function resolveDefaultMentions(context, github, core, mentionsConfig, def } module.exports = { + getMentionsGithubClient, resolveAllowedMentionsFromPayload, resolveMentionsForItem, resolveDefaultMentions, diff --git a/actions/setup/js/resolve_mentions_from_payload.test.cjs b/actions/setup/js/resolve_mentions_from_payload.test.cjs index cb808376cfb..959434b063d 100644 --- a/actions/setup/js/resolve_mentions_from_payload.test.cjs +++ b/actions/setup/js/resolve_mentions_from_payload.test.cjs @@ -18,7 +18,7 @@ vi.mock("./error_helpers.cjs", () => ({ getErrorMessage: vi.fn(err => (err instanceof Error ? err.message : String(err))), })); -const { resolveAllowedMentionsFromPayload, extractKnownAuthorsFromPayload, fetchTeamMembers, pushNonBotUser, pushNonBotAssignees } = await import("./resolve_mentions_from_payload.cjs"); +const { getMentionsGithubClient, resolveAllowedMentionsFromPayload, extractKnownAuthorsFromPayload, fetchTeamMembers, pushNonBotUser, pushNonBotAssignees } = await import("./resolve_mentions_from_payload.cjs"); /** @returns {{ info: ReturnType, warning: ReturnType, error: ReturnType }} */ function makeMockCore() { @@ -30,6 +30,33 @@ function makeMockGithub() { return {}; } +describe("getMentionsGithubClient", () => { + it("uses the configured mention token instead of the handler client", () => { + const fallback = makeMockGithub(); + const mentionClient = makeMockGithub(); + const originalToken = process.env.GH_AW_MENTIONS_GITHUB_TOKEN; + const originalGetOctokit = global.getOctokit; + process.env.GH_AW_MENTIONS_GITHUB_TOKEN = "mention-token"; + global.getOctokit = vi.fn(token => (token === "mention-token" ? mentionClient : fallback)); + + try { + expect(getMentionsGithubClient(fallback)).toBe(mentionClient); + expect(global.getOctokit).toHaveBeenCalledWith("mention-token"); + } finally { + if (originalToken === undefined) { + delete process.env.GH_AW_MENTIONS_GITHUB_TOKEN; + } else { + process.env.GH_AW_MENTIONS_GITHUB_TOKEN = originalToken; + } + if (originalGetOctokit === undefined) { + delete global.getOctokit; + } else { + global.getOctokit = originalGetOctokit; + } + } + }); +}); + describe("pushNonBotUser", () => { it("pushes a regular user login", () => { const users = /** @type {string[]} */ []; diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 622902ec9b5..1106e5f09d0 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -411,8 +411,8 @@ async function loadHandlers(config, prReviewBufferRegistry, resolvedAllowedMenti } // Pass the mentions policy to handlers; aliases are resolved for each destination. - if (handlerConfig.mentions == null && config.mentions != null) { - handlerConfig.mentions = config.mentions; + if (MENTION_HANDLER_TYPES.has(type) && handlerConfig.mentions == null) { + handlerConfig.mentions = config.mentions ?? {}; } // Inject shared PR review buffer registry into handlers that need it if (PR_REVIEW_HANDLER_TYPES.has(type)) { @@ -426,7 +426,7 @@ async function loadHandlers(config, prReviewBufferRegistry, resolvedAllowedMenti if (handlerConfig[GITHUB_TOKEN_CONFIG_KEY] && typeof globalState.getOctokit === "function") { handlerGithubClient = globalState.getOctokit(handlerConfig[GITHUB_TOKEN_CONFIG_KEY]); } - if (MENTION_HANDLER_TYPES.has(type) && handlerConfig.mentions != null && handlerConfig.allowedMentionAliases == null) { + if (MENTION_HANDLER_TYPES.has(type) && handlerConfig.allowedMentionAliases == null) { if (Array.isArray(resolvedAllowedMentionAliases)) { handlerConfig.allowedMentionAliases = resolvedAllowedMentionAliases; } else { diff --git a/actions/setup/js/safe_output_type_validator.cjs b/actions/setup/js/safe_output_type_validator.cjs index 5e629f2283f..f41a89fb855 100644 --- a/actions/setup/js/safe_output_type_validator.cjs +++ b/actions/setup/js/safe_output_type_validator.cjs @@ -34,6 +34,7 @@ const ISSUE_INTENT_RATIONALE_MAX_LENGTH = 280; * maxMentions?: number, * allowedAliasesSeen?: Set, * maxBotMentions?: number, + * deferMentions?: boolean, * normalizeIssueClosingKeywords?: boolean, * dataEnabled?: boolean, * dataSchema?: any @@ -91,6 +92,7 @@ function normalizeIssueIntentRationale(rationale, options) { maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }).trim(); // sanitizeContent appends "\n[Content truncated due to length]" when it truncates, // so clamp again to guarantee the GitHub API hard limit. @@ -119,6 +121,7 @@ function validateIssueIntentLabels(value, lineNum, itemType, fieldName, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }); if (!name) { return { isValid: false, error: `Line ${lineNum}: ${itemType} ${fieldName}[${i}] must be a non-empty string` }; @@ -154,6 +157,7 @@ function validateIssueIntentLabels(value, lineNum, itemType, fieldName, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }); if (!name) { return { @@ -560,6 +564,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }); } return { isValid: true, normalizedValue: normalizedResult }; @@ -584,6 +589,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }); } if (options?.normalizeIssueClosingKeywords && fieldName === "body" && NORMALIZE_CLOSER_BODY_TYPES.has(itemType)) { @@ -664,6 +670,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }) : item ); diff --git a/actions/setup/js/sanitize_content.cjs b/actions/setup/js/sanitize_content.cjs index ddf832a66f6..727a21c5ddf 100644 --- a/actions/setup/js/sanitize_content.cjs +++ b/actions/setup/js/sanitize_content.cjs @@ -44,6 +44,7 @@ const RUNTIME_TO_MENTION_ALIAS_MAP = { * @property {number} [maxMentions] - Maximum number of unique allowed aliases to preserve * @property {Set} [allowedAliasesSeen] - Allowed aliases already preserved in this output item * @property {number} [maxBotMentions] - Maximum bot trigger references before filtering (default: 10) + * @property {boolean} [deferMentions] - Preserve mentions for filtering in the trusted safe-outputs job */ /** @@ -62,6 +63,7 @@ function sanitizeContent(content, maxLengthOrOptions) { let maxMentions; /** @type {number | undefined} */ let maxBotMentions; + let deferMentions = false; /** @type {Set | undefined} */ let allowedAliasesSeen; @@ -74,11 +76,12 @@ function sanitizeContent(content, maxLengthOrOptions) { allowedAliasesLowercase = expandAllowedAliases(normalizedAllowedAliases); maxMentions = maxLengthOrOptions.maxMentions; maxBotMentions = maxLengthOrOptions.maxBotMentions; + deferMentions = maxLengthOrOptions.deferMentions === true; allowedAliasesSeen = maxLengthOrOptions.allowedAliasesSeen; } // If no allowed aliases specified, use core sanitization (which neutralizes all mentions) - if (allowedAliasesLowercase.length === 0) { + if (allowedAliasesLowercase.length === 0 && !deferMentions) { return sanitizeContentCore(content, maxLength, maxBotMentions); } @@ -132,7 +135,9 @@ function sanitizeContent(content, maxLengthOrOptions) { // Neutralize mentions after truncation so the length boundary cannot split an // inserted code-span delimiter and reactivate a mention. - sanitized = neutralizeMentions(sanitized, allowedAliasesLowercase, maxMentions, allowedAliasesSeen); + if (!deferMentions) { + sanitized = neutralizeMentions(sanitized, allowedAliasesLowercase, maxMentions, allowedAliasesSeen); + } // Neutralize GitHub references if restrictions are configured sanitized = neutralizeGitHubReferences(sanitized, allowedGitHubRefs); diff --git a/actions/setup/js/sanitize_content.test.cjs b/actions/setup/js/sanitize_content.test.cjs index 8f9100432ec..67fea4f985e 100644 --- a/actions/setup/js/sanitize_content.test.cjs +++ b/actions/setup/js/sanitize_content.test.cjs @@ -314,6 +314,11 @@ describe("sanitize_content.cjs", () => { expect(result).toBe("Hello `@user`"); }); + it("should defer mention neutralization when requested", () => { + const result = sanitizeContent("Hello @user", { deferMentions: true }); + expect(result).toBe("Hello @user"); + }); + it("should not neutralize org/team mentions in allowedAliases", () => { const result = sanitizeContent("Hello @myorg/myteam", { allowedAliases: ["myorg/myteam"] }); expect(result).toBe("Hello @myorg/myteam"); diff --git a/docs/adr/66571-dedicated-mention-ingestion-credentials.md b/docs/adr/66571-dedicated-mention-ingestion-credentials.md index 352ca938e30..e7251125396 100644 --- a/docs/adr/66571-dedicated-mention-ingestion-credentials.md +++ b/docs/adr/66571-dedicated-mention-ingestion-credentials.md @@ -1,4 +1,4 @@ -# ADR-66571: Use Dedicated, Non-Inheriting Credentials for Mention Allowlist Resolution During Ingestion +# ADR-66571: Resolve Mention Allowlists in the Trusted Safe-Output Job **Date**: 2026-11-19 **Status**: Draft @@ -8,11 +8,11 @@ ### Context -`safe-outputs.mentions.allowed-teams` lets a workflow preserve `@login` mentions for members of specific teams, but the **Ingest agent output** step — the step that sanitizes agent output before any safe-output handler runs — executed with the default GitHub Actions token, which cannot read organization team membership. Allowed team members therefore had their mentions escaped during ingestion, and downstream handlers could not restore the notification because the body was already sanitized (#50282). Mention resolution happens inside the `agent` job, which by the repository's security architecture must never hold safe-output *write* credentials, so the fix could not simply reuse `safe-outputs.github-token`, `safe-outputs.github-app`, per-handler tokens, or `GH_AW_GITHUB_TOKEN`. Any new credential also had to survive ingestion's `always()` semantics and remain invisible to the agent process itself. +`safe-outputs.mentions.allowed-teams` lets a workflow preserve `@login` mentions for members of specific teams, but **Ingest agent output** runs in the untrusted `agent` job. Supplying a lookup token there would expose it to agent-controlled files and scripts, even if the token were read-only. The trusted `safe_outputs` job already re-sanitizes mention-aware content before publication, so mention candidates can remain intact through ingestion and be checked after the agent artifact is transferred (#50282). ### Decision -We will introduce dedicated ingestion credentials under `safe-outputs.mentions.github-token` and `safe-outputs.mentions.github-app` that are used **only** for mention allowlist resolution and that never inherit from any other safe-output credential. When a mention-specific app is configured, the compiler mints its installation token in the `agent` job after agent execution and MCP gateway shutdown, immediately before **Ingest agent output**; that token takes precedence over the mention PAT, requests comment-author read scopes plus `members: read` when `allowed-teams` is set, and honours explicit permission overrides. Without mention-specific credentials, ingestion keeps the default Actions token and no app token is minted. The primary driver is least privilege: read-only team-membership access must be grantable without widening what the agent job or the agent process can do. +Mention/team allowlist resolution will be deferred to the trusted `safe_outputs` job. Ingestion will preserve mention candidates without performing directory, collaborator, or comment-author lookups and will not receive mention-specific credentials. The processor will use `safe-outputs.mentions.github-app`, then `safe-outputs.mentions.github-token`, then `github.token`; these credentials never inherit from global or per-handler safe-output credentials. A dedicated app token is minted in `safe_outputs` before **Process Safe Outputs**, with `issues: read` when `add-comment` is enabled and `members: read` when `allowed-teams` is configured. Permission overrides are restricted to `read` and `none`. ### Alternatives Considered @@ -20,30 +20,26 @@ We will introduce dedicated ingestion credentials under `safe-outputs.mentions.g Reuse `safe-outputs.github-token` / `safe-outputs.github-app` (or `GH_AW_GITHUB_TOKEN`) for the membership lookup. This was the smallest change and required no new schema surface. It was rejected because it would place write-scoped safe-output credentials inside the `agent` job, violating the formal security property that agents execute without GitHub write permissions and that write credentials stay confined to downstream safe-output jobs (`security_architecture_formal_test.go`). -#### Alternative 2: Defer mention restoration to the downstream safe-output handlers +#### Alternative 2: Reuse the default Actions token for mention lookups in the agent job -Keep ingestion token-less and have each handler job — which already holds appropriate credentials — re-expand mentions for allowed teams. Rejected because ingestion escapes mentions before handlers see the payload, so the original `@login` is no longer distinguishable from author-supplied text; restoring it would require carrying unsanitized content past the sanitization boundary, re-introducing the injection risk ingestion exists to prevent. - -#### Alternative 3: Mint the mention token at job start, alongside other tokens - -Resolve the installation token at the beginning of the `agent` job together with existing token minting. Rejected because the credential would then be present in the environment for the entire agent execution and MCP gateway lifetime; minting it after gateway shutdown keeps the exposure window to the ingestion step only. +Keep ingestion token-less but perform directory lookups with the job's default Actions token. Rejected because the default token cannot access organization team membership, and the lookup would still execute in the untrusted job boundary. ### Consequences #### Positive - Mentions of allowed team members survive ingestion, so downstream handlers can notify those users — the behaviour #50282 asks for. - Membership read access is granted with a narrowly scoped, separately configured credential; the agent job gains no write capability and downstream write credentials are unchanged. -- Mention credentials are excluded from agent-visible validation and handler configuration, and app private keys stay out of the pre-ingestion portion of the agent job — properties now asserted by regression tests. +- Mention credentials and mention lookups stay out of the agent job; the trusted handler sanitizes mention-aware content before publication. #### Negative - Adds a third credential concept (`mentions.github-token` / `mentions.github-app`) with its own precedence and fallback chain (app → PAT → `GITHUB_TOKEN`), increasing the configuration surface users must understand and the compiler must keep consistent. - Users who expect credentials to cascade from `safe-outputs.github-token` will see mentions silently escaped until they configure the mention-specific token; the non-inheritance rule is deliberate but is a latent surprise. -- Token minting inside the `agent` job adds compiler-emitted steps and ordering constraints (`always()`, same-job producer validation) to an already intricate step-lifecycle generator. +- Deferring mention filtering means mention-aware messages cross the artifact boundary before final mention sanitization; the safe-output handler must keep its sanitization step before every write. #### Neutral - `MentionsConfig` gains `GitHubToken` and `GitHubApp`; the workflow JSON schema, generated frontmatter reference, safe-outputs reference, and the normative safe-outputs specification were updated in the same change. -- Installation owner/repository selection, wildcard scoping, relay repository selection, and missing-key guards reuse existing compiler helpers rather than introducing a parallel resolution path. -- Secret-safe diagnostic logging now reports which token class ingestion selected (mention app, mention PAT, or default Actions token). +- Installation owner/repository selection, wildcard scoping, relay repository selection, and missing-key guards reuse existing compiler helpers in the trusted job. +- Same-job token references for mention credentials are validated against the `safe_outputs` job. --- diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index 3b30b0e934e..02d3002b5b6 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -22160,14 +22160,15 @@ safe-outputs: # Format 2: Advanced configuration for @mention filtering with fine-grained # control mentions: - # Dedicated token for resolving mention allowlists during ingestion. Does not - # inherit safe-outputs.github-token or affect write operations. + # Dedicated token for resolving mention allowlists in the trusted safe_outputs + # job. It is not exposed to agent-job ingestion and does not inherit other + # safe-output credentials or affect write operations. # (optional) github-token: "${{ secrets.GITHUB_TOKEN }}" - # Dedicated GitHub App for resolving mention allowlists during ingestion. Minted - # after agent execution; takes precedence over mentions.github-token. Does not - # inherit safe-outputs.github-app. + # Dedicated GitHub App for resolving mention allowlists in the trusted + # safe_outputs job. Its token is read-only, takes precedence over + # mentions.github-token, and does not inherit safe-outputs.github-app. # (optional) github-app: # Deprecated alias for client-id. GitHub App ID/client ID (e.g., '${{ vars.APP_ID @@ -22382,10 +22383,11 @@ safe-outputs: # List of team slugs whose members are always allowed to be mentioned. Accepts # 'team-slug' (resolved against the current org) or 'org/team-slug' format. Team - # members are fetched from the GitHub API at runtime; bots are excluded. - # IMPORTANT: requires read:org scope — not available with the default - # GITHUB_TOKEN. Use a classic PAT with read:org, a fine-grained PAT with - # Members:Read, or a GitHub App with the Members:Read permission. Without the + # members are resolved in the trusted safe_outputs job; bots are excluded. + # Requires the mention-resolution token to have read:org scope, which the default + # github.token does not provide. Configure safe-outputs.mentions.github-token with + # a classic PAT or fine-grained PAT with Members:Read, or + # safe-outputs.mentions.github-app with the Members:Read permission. Without the # required scope, team lookups fail with a warning and those members are skipped. # (optional) allowed-teams: [] diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 80841f4d0d2..e23f0bfbfbb 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -2097,7 +2097,7 @@ Accepts a literal integer or a GitHub Actions expression string (e.g., `${{ inpu By default, `@mentions` in AI-generated content are escaped with backticks unless the mentioned user is a verified collaborator or inferred from the event context (issue/PR author, assignees, etc.). Use `mentions:` to control this behavior: -Collaborator checks use the repository receiving each safe output. With `target-repo`, the workflow repository's collaborators are not used as a fallback when the token cannot read the target repository. The agent job sanitizes output first, so its token needs read access to the target repository to preserve collaborator mentions; a later handler token cannot restore escaped mentions. +Collaborator checks use the repository receiving each safe output. With `target-repo`, the workflow repository's collaborators are not used as a fallback when the token cannot read the target repository. The agent job preserves mention candidates without performing lookups; the trusted `safe_outputs` job filters them before publication. ```yaml wrap safe-outputs: @@ -2123,7 +2123,7 @@ safe-outputs: **`allowed-teams`** lets organizations allow all members of specific GitHub teams to be mentioned without listing individual usernames. Team members are fetched from the GitHub API at runtime using `GET /orgs/{org}/teams/{team_slug}/members`. Bot accounts within the team are excluded. Use `org/team-slug` for cross-org teams or just `team-slug` to resolve against the current repository's organization. -Configure `safe-outputs.mentions.github-token` or `safe-outputs.mentions.github-app` with read access to team membership. **Ingest agent output** resolves allowed mentions before sanitization using these dedicated credentials. It does not inherit `safe-outputs.github-token`, `safe-outputs.github-app`, or `GH_AW_GITHUB_TOKEN`; without mention-specific credentials it uses the default Actions token. +Configure `safe-outputs.mentions.github-token` or `safe-outputs.mentions.github-app` with read access to team membership. Mention and team lookups run in the trusted `safe_outputs` job, after the agent artifact is transferred. The token selection is mention app, mention token, then `github.token`; it never inherits `safe-outputs.github-token`, `safe-outputs.github-app`, per-output credentials, or `GH_AW_GITHUB_TOKEN`. ```yaml wrap safe-outputs: @@ -2132,10 +2132,10 @@ safe-outputs: allowed-teams: [my-org/my-team] ``` -Alternatively, configure `safe-outputs.mentions.github-app` with the standard `client-id`, `private-key`, `owner`, `repositories`, and `permissions` fields. Its token is minted after agent execution and gateway shutdown, takes precedence over `mentions.github-token`, and requests `members: read` when `allowed-teams` is configured. With `ignore-if-missing: true`, missing app credentials fall back to `mentions.github-token`, then `GITHUB_TOKEN`. Omitting the mention-specific app disables ingestion app-token minting without disabling mention filtering. Downstream write credentials remain unchanged. +Alternatively, configure `safe-outputs.mentions.github-app` with the standard `client-id`, `private-key`, `owner`, `repositories`, and `permissions` fields. Its token is minted in the trusted `safe_outputs` job before **Process Safe Outputs**, takes precedence over `mentions.github-token`, and requests `issues: read` when `add-comment` is enabled and `members: read` when `allowed-teams` is configured. Permission overrides may be `read` or `none`; `write` and other values are rejected. With `ignore-if-missing: true`, missing app credentials fall back to `mentions.github-token`, then `github.token`. Omitting the mention-specific app does not disable mention filtering. Downstream write credentials remain unchanged. > [!IMPORTANT] -> `allowed-teams` requires the workflow token to have `read:org` scope. The default `GITHUB_TOKEN` does **not** include this scope. Use one of the following: +> `allowed-teams` requires the mention-resolution token to have `read:org` scope. The default `github.token` does **not** include this scope. Use one of the following: > - A **classic PAT** with the `read:org` scope stored as a repository secret > - A **fine-grained PAT** with the "Members" repository permission (read) > - A **GitHub App** installation token with the "Members" permission (read) diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 85a88c447d8..b84e5e87aba 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -319,7 +319,7 @@ The Safe Outputs MCP Gateway implements defense-in-depth through strict architec **Requirement AR1: Agent Isolation** -Agents MUST execute without GitHub write permissions. Only read-level tokens SHALL be accessible to agent processes. Safe-output write credentials MUST remain confined to downstream safe-output processing jobs. Dedicated mention-resolution credentials MUST be supplied only to trusted post-agent ingestion steps and MUST NOT be accessible during agent execution. +Agents MUST execute without GitHub write permissions. Only read-level tokens SHALL be accessible to agent processes. Safe-output write credentials MUST remain confined to downstream safe-output processing jobs. Mention/team allowlist resolution MUST be deferred to the trusted `safe_outputs` job; ingestion in the `agent` job MUST perform no privileged mention or team lookups and MUST NOT receive mention-specific credentials. **Verification**: @@ -396,14 +396,14 @@ Safe Output Processors MAY target external APIs. Credentials for each external s - **Method**: Manual security audit and code review - **Tool**: Security review of workflow structure and GitHub Actions architecture -- **Criteria**: Safe-output credentials are inaccessible to agent processes; post-agent ingestion credentials are supplied only to trusted steps after agent execution and gateway shutdown +- **Criteria**: Safe-output and mention-resolution credentials are inaccessible to agent processes; mention resolution occurs only in trusted safe-output processing after the agent artifact has been transferred - **Manual Check**: Audit all communication channels (artifacts, environment variables, network, filesystem) to confirm no credential leakage **Formal Definition**: ``` ∀ t ∈ [agent_start, agent_end]: - accessible_credentials(agent_context, t) ∩ safe_output_credentials = ∅ + accessible_credentials(agent_context, t) ∩ (safe_output_credentials ∪ mention_resolution_credentials) = ∅ ``` **Requirement AR5: Steering Issue Provenance** @@ -2129,24 +2129,22 @@ Requirements: Requirements: -- Detect @mentions: `@[a-zA-Z0-9_-]+` -- Check against allowed-aliases list -- Neutralize unauthorized: `@user` becomes `@ user` (add space) +- Ingestion MUST preserve mentions for mention-aware output types without performing allowlist lookups. +- The trusted safe-output processor MUST detect @mentions: `@[a-zA-Z0-9_-]+` +- Check each mention against the resolved allowed-aliases list +- Neutralize unauthorized mentions before publication - Preserve mentions in code blocks **Ingestion credential requirements**: -- Ingestion MUST use only the credentials configured under `safe-outputs.mentions.github-token` or `safe-outputs.mentions.github-app`. It MUST NOT inherit `safe-outputs.github-token`, `safe-outputs.github-app`, per-output credentials, or `GH_AW_GITHUB_TOKEN`. -- When `safe-outputs.mentions.github-app` is configured, the compiler MUST mint a dedicated installation token in the `agent` job immediately before **Ingest agent output**, after agent execution and gateway shutdown. The token MUST use the configured installation owner, repositories, and explicit permission overrides, with read permissions for ingestion's comment-author lookups and `members: read` when `mentions.allowed-teams` is configured. -- Without a mention-specific app, ingestion MUST NOT mint an app token or resolve its installation owner; it MUST use `safe-outputs.mentions.github-token` when configured, otherwise the default GitHub Actions token. Mention credentials MUST NOT change downstream safe-output write credentials. -- Ingestion app credentials MUST be confined to trusted post-agent token-minting step inputs and MUST NOT be added to agent execution environments. The ingestion token MUST NOT inherit the safe-output handlers' write permissions; configured app permission overrides remain explicit author choices. -- Token minting and installation-owner resolution MUST run even after an earlier step fails, matching ingestion's `always()` behavior. With `ignore-if-missing: true`, missing credentials MUST skip minting and ingestion MUST fall back to `safe-outputs.mentions.github-token`, then `GITHUB_TOKEN`. -- The same-job mention app token MUST take precedence over `safe-outputs.mentions.github-token`. -- Allowed team members' raw `@login` mentions MUST be preserved during ingestion so downstream handlers can notify those users. -- Without mention-specific credentials, ingestion MUST retain the default GitHub Actions token, regardless of configured write credentials. -- A mention token referencing `steps..outputs.*` MUST be produced by an earlier step in the `agent` job; it MUST NOT require minting in the safe-output processing jobs. +- Ingestion MUST use only the default GitHub Actions context and MUST NOT resolve collaborators, comment authors, or team membership. It MUST preserve mention candidates for the trusted safe-output processor to filter before any write operation. +- Mention resolution MUST run in the trusted `safe_outputs` job. Its lookup token MUST come only from `safe-outputs.mentions.github-app`, then `safe-outputs.mentions.github-token`, then `github.token`; it MUST NOT inherit `safe-outputs.github-token`, `safe-outputs.github-app`, per-output credentials, or `GH_AW_GITHUB_TOKEN`. +- When `safe-outputs.mentions.github-app` is configured, the compiler MUST mint its dedicated installation token in `safe_outputs` before **Process Safe Outputs**. The token MUST use the configured installation owner and repository scope, request `issues: read` whenever `add-comment` is enabled, and request `members: read` when `mentions.allowed-teams` is configured. Explicit permission overrides MAY select only `read` or `none`; the compiler MUST reject `write` and other levels. +- With `ignore-if-missing: true`, missing app credentials MUST skip minting and mention resolution MUST fall back to `safe-outputs.mentions.github-token`, then `github.token`. Mention credentials MUST NOT change downstream safe-output write credentials. +- Allowed team members' raw `@login` mentions MUST remain unmodified during untrusted ingestion and MUST be filtered in the trusted safe-output processor before publication. +- A mention token referencing `steps..outputs.*` MUST be produced by an earlier step in the `safe_outputs` job. - Mention credentials MUST NOT be serialized into agent-visible validation or handler configuration. -- Compiler debug logging SHOULD identify the ingestion token source without logging credential values or token expressions. +- Compiler debug logging SHOULD identify the mention-resolution token source without logging credential values or token expressions. **Transformation T6: Markdown Safety** diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index ee19817fe11..f98de608c6e 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -12440,11 +12440,11 @@ "properties": { "github-token": { "$ref": "#/$defs/github_token_same_job", - "description": "Dedicated token for resolving mention allowlists during ingestion. Does not inherit safe-outputs.github-token or affect write operations." + "description": "Dedicated token for resolving mention allowlists in the trusted safe_outputs job. It is not exposed to agent-job ingestion and does not inherit other safe-output credentials or affect write operations." }, "github-app": { "$ref": "#/$defs/github_app", - "description": "Dedicated GitHub App for resolving mention allowlists during ingestion. Minted after agent execution; takes precedence over mentions.github-token. Does not inherit safe-outputs.github-app." + "description": "Dedicated GitHub App for resolving mention allowlists in the trusted safe_outputs job. Its token is read-only, takes precedence over mentions.github-token, and does not inherit safe-outputs.github-app." }, "allowed-collaborators": { "type": "boolean", @@ -12472,7 +12472,7 @@ }, "allowed-teams": { "type": "array", - "description": "List of team slugs whose members are always allowed to be mentioned. Accepts 'team-slug' (resolved against the current org) or 'org/team-slug' format. Team members are fetched from the GitHub API at runtime; bots are excluded. IMPORTANT: requires read:org scope \u2014 not available with the default GITHUB_TOKEN. Use a classic PAT with read:org, a fine-grained PAT with Members:Read, or a GitHub App with the Members:Read permission. Without the required scope, team lookups fail with a warning and those members are skipped.", + "description": "List of team slugs whose members are always allowed to be mentioned. Accepts 'team-slug' (resolved against the current org) or 'org/team-slug' format. Team members are resolved in the trusted safe_outputs job; bots are excluded. Requires the mention-resolution token to have read:org scope, which the default github.token does not provide. Configure safe-outputs.mentions.github-token with a classic PAT or fine-grained PAT with Members:Read, or safe-outputs.mentions.github-app with the Members:Read permission. Without the required scope, team lookups fail with a warning and those members are skipped.", "items": { "type": "string", "minLength": 1 diff --git a/pkg/workflow/agentic_output_test.go b/pkg/workflow/agentic_output_test.go index 669348caa18..40ae749f268 100644 --- a/pkg/workflow/agentic_output_test.go +++ b/pkg/workflow/agentic_output_test.go @@ -16,18 +16,16 @@ import ( "github.com/stretchr/testify/require" ) -func TestOutputCollectionGitHubToken(t *testing.T) { +func TestOutputCollectionDoesNotUseMentionCredentials(t *testing.T) { for _, tt := range []struct { name string safeOutputs *SafeOutputsConfig - token string }{ {name: "no safe outputs"}, {name: "default token", safeOutputs: &SafeOutputsConfig{}}, { - name: "step token with fallback", - safeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{GitHubToken: "${{ steps.mint.outputs.token || secrets.MENTIONS_PAT || secrets.GITHUB_TOKEN }}"}}, - token: "${{ steps.mint.outputs.token || secrets.MENTIONS_PAT || secrets.GITHUB_TOKEN }}", + name: "mention token is deferred to trusted job", + safeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{GitHubToken: "${{ secrets.MENTIONS_PAT }}"}}, }, { name: "per-handler token is not used", @@ -52,11 +50,9 @@ func TestOutputCollectionGitHubToken(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var yaml strings.Builder require.NoError(t, NewCompiler().generateOutputCollectionStep(&yaml, &WorkflowData{SafeOutputs: tt.safeOutputs})) - if tt.token == "" { - assert.NotContains(t, yaml.String(), "github-token:") - } else { - assert.Contains(t, yaml.String(), " with:\n github-token: "+tt.token+"\n script: |\n") - } + assert.NotContains(t, yaml.String(), "github-token:") + assert.NotContains(t, yaml.String(), "MENTIONS_PAT") + assert.NotContains(t, yaml.String(), "safe-outputs-ingestion-app-token") assert.Contains(t, yaml.String(), "setupGlobals(core, github, context, exec, io, getOctokit);") }) } @@ -69,9 +65,9 @@ func TestOutputCollectionGitHubAppToken(t *testing.T) { pat string token string }{ - {name: "app overrides PAT", pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token }}"}, - {name: "missing credentials fall back to PAT", ignore: true, pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token || secrets.PAT }}"}, - {name: "missing credentials fall back to default Actions token", ignore: true, token: "${{ steps.safe-outputs-ingestion-app-token.outputs.token || secrets.GITHUB_TOKEN }}"}, + {name: "app overrides PAT", pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-mentions-app-token.outputs.token }}"}, + {name: "missing credentials fall back to PAT", ignore: true, pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-mentions-app-token.outputs.token || secrets.PAT }}"}, + {name: "missing credentials fall back to default Actions token", ignore: true, token: "${{ steps.safe-outputs-mentions-app-token.outputs.token || github.token }}"}, } { t.Run(tt.name, func(t *testing.T) { app := &GitHubAppConfig{ @@ -82,9 +78,8 @@ func TestOutputCollectionGitHubAppToken(t *testing.T) { GitHubToken: "${{ secrets.WRITE_PAT }}", AddComments: &AddCommentsConfig{}, Mentions: &MentionsConfig{GitHubApp: app, GitHubToken: tt.pat, AllowedTeams: []string{"my-org/my-team"}}, }} - var yaml strings.Builder - require.NoError(t, NewCompiler().generateOutputCollectionStep(&yaml, data)) - output := yaml.String() + compiler := NewCompiler() + output := strings.Join(compiler.addAppTokenMintingSteps(data), "") assert.Contains(t, output, "owner: my-org\n") assert.Contains(t, output, "repositories: |-\n my-repo\n another-repo\n") assert.NotContains(t, output, "permission-contents:") @@ -92,17 +87,17 @@ func TestOutputCollectionGitHubAppToken(t *testing.T) { assert.Contains(t, output, "permission-pull-requests: read\n") assert.Contains(t, output, "permission-members: read\n") assert.NotContains(t, output, ": write\n") - assert.Contains(t, output, "github-token: "+tt.token+"\n") assert.NotContains(t, output, "steps.safe-outputs-app-token.outputs.token") assert.NotContains(t, output, "secrets.WRITE_PAT") - assert.Less(t, strings.Index(output, "id: safe-outputs-ingestion-app-token\n"), strings.Index(output, "- name: Ingest agent output\n")) + assert.Contains(t, output, "id: safe-outputs-mentions-app-token\n") if tt.ignore { - assert.Contains(t, output, "if: ${{ always() && vars.APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}") + assert.Contains(t, output, "if: ${{ vars.APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}") assert.Contains(t, output, "GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY: ${{ secrets.APP_KEY }}") assert.NotContains(t, output, "if: ${{ secrets.") } else { - assert.Contains(t, output, "- name: Generate GitHub App token for output ingestion\n if: always()\n") + assert.Contains(t, output, "- name: Generate GitHub App token for mention resolution\n") } + assert.Equal(t, tt.token, safeOutputMentionsGitHubToken(&WorkflowData{SafeOutputs: data.SafeOutputs})) assert.Nil(t, app.Permissions, "minting must not mutate the configured app") }) } @@ -120,17 +115,15 @@ func TestOutputCollectionGitHubAppOwnerAndWildcard(t *testing.T) { SafeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{GitHubApp: app}}, } compiler := NewCompiler() - var yaml strings.Builder - require.NoError(t, compiler.generateOutputCollectionStep(&yaml, data)) - output := yaml.String() - assert.Contains(t, output, "- name: Derive GitHub App owner for output ingestion\n if: always()\n") + output := strings.Join(compiler.addAppTokenMintingSteps(data), "") + assert.Contains(t, output, "- name: Derive GitHub App owner for mention resolution\n") assert.Contains(t, output, "GH_AW_TARGET_REPOSITORY: ${{ needs.activation.outputs.target_repo }}") - assert.Contains(t, output, "owner: ${{ steps.safe-outputs-ingestion-app-token-owner.outputs.owner }}") + assert.Contains(t, output, "owner: ${{ steps.safe-outputs-mentions-app-token-owner.outputs.owner }}") assert.NotContains(t, output, "permission-members:") if wildcard { assert.NotContains(t, output, "repositories:") assert.True(t, compiler.wildcardAppTokenSteps[appTokenStepKey{ - jobName: "agent", id: "safe-outputs-ingestion-app-token", clientID: app.AppID, privateKey: app.PrivateKey, + jobName: "safe_outputs", id: "safe-outputs-mentions-app-token", clientID: app.AppID, privateKey: app.PrivateKey, }]) } else { assert.Contains(t, output, "repositories: ${{ needs.activation.outputs.target_repo_name }}") @@ -175,16 +168,19 @@ safe-outputs: require.Contains(t, agent, "- name: Ingest agent output\n") ingest := strings.SplitN(strings.SplitN(agent, "- name: Ingest agent output\n", 2)[1], "\n - ", 2)[0] assert.NotContains(t, agent, "safe-outputs-ingestion-app-token") + assert.NotContains(t, agent, "MENTIONS_PAT") assert.NotContains(t, ingest, "secrets.WRITE_PAT") - if tt.pat == "" { - assert.NotContains(t, ingest, "github-token:") - } else { - assert.Contains(t, ingest, "github-token: "+tt.pat) - } + assert.NotContains(t, ingest, "github-token:") assert.Equal(t, []string{"my-org/my-team"}, data.SafeOutputs.Mentions.AllowedTeams) assert.Nil(t, data.SafeOutputs.Mentions.Enabled, "mention filtering policy must remain unchanged") assert.Contains(t, ingest, "collect_ndjson_output.cjs") - assert.Contains(t, extractJobSection(string(lock), "safe_outputs"), "id: safe-outputs-app-token\n") + safeOutputs := extractJobSection(string(lock), "safe_outputs") + assert.Contains(t, safeOutputs, "id: safe-outputs-app-token\n") + if tt.pat == "" { + assert.Contains(t, safeOutputs, "GH_AW_MENTIONS_GITHUB_TOKEN: ${{ github.token }}") + } else { + assert.Contains(t, safeOutputs, "GH_AW_MENTIONS_GITHUB_TOKEN: "+tt.pat) + } }) } } @@ -236,22 +232,23 @@ safe-outputs: content, err := os.ReadFile(strings.TrimSuffix(workflowFile, ".md") + ".lock.yml") require.NoError(t, err) agent := extractJobSection(string(content), "agent") - assert.Contains(t, agent, "id: safe-outputs-ingestion-app-token\n") - assert.Contains(t, agent, "github-token: ${{ steps.safe-outputs-ingestion-app-token.outputs.token }}") + assert.NotContains(t, agent, "safe-outputs-ingestion-app-token") + assert.NotContains(t, agent, "MENTIONS_APP_KEY") assert.NotContains(t, agent, "steps.safe-outputs-app-token.outputs.token") assert.NotContains(t, agent, "secrets.WRITE_APP_KEY") - assert.Contains(t, agent, "private-key: ${{ secrets.MENTIONS_APP_KEY }}") - mintStart := strings.Index(agent, " - name: Generate GitHub App token for output ingestion\n") - require.GreaterOrEqual(t, mintStart, 0) - require.Contains(t, agent, "id: agentic_execution\n") - require.Contains(t, agent, " - name: Stop MCP Gateway\n") - assert.Less(t, strings.Index(agent, "id: agentic_execution\n"), mintStart) - assert.Less(t, strings.Index(agent, " - name: Stop MCP Gateway\n"), mintStart) - assert.NotContains(t, agent[:mintStart], "secrets.MENTIONS_APP_KEY") + assert.NotContains(t, agent, "private-key: ${{ secrets.MENTIONS_APP_KEY }}") safeOutputs := extractJobSection(string(content), "safe_outputs") + assert.Contains(t, safeOutputs, "id: safe-outputs-mentions-app-token\n") + assert.Contains(t, safeOutputs, "GH_AW_MENTIONS_GITHUB_TOKEN: ${{ steps.safe-outputs-mentions-app-token.outputs.token }}") + assert.Contains(t, safeOutputs, "private-key: ${{ secrets.MENTIONS_APP_KEY }}") + assert.Contains(t, safeOutputs, "permission-issues: read\n") + assert.Contains(t, safeOutputs, "permission-pull-requests: read\n") + mintStart := strings.Index(safeOutputs, " - name: Generate GitHub App token for mention resolution\n") + require.GreaterOrEqual(t, mintStart, 0) + require.Contains(t, safeOutputs, "- name: Process Safe Outputs\n") + assert.Less(t, mintStart, strings.Index(safeOutputs, "- name: Process Safe Outputs\n")) assert.Contains(t, safeOutputs, "github-token: ${{ steps.safe-outputs-app-token.outputs.token }}") assert.NotContains(t, safeOutputs, "steps.safe-outputs-ingestion-app-token.outputs.token") - assert.NotContains(t, safeOutputs, "secrets.MENTIONS_APP_KEY") } func TestAgenticOutputCollection(t *testing.T) { @@ -332,8 +329,9 @@ This workflow tests the agentic output collection functionality. require.Contains(t, lockContent, "- name: Ingest agent output\n") ingestStep := strings.SplitN(strings.SplitN(lockContent, "- name: Ingest agent output\n", 2)[1], "\n - ", 2)[0] - assert.Contains(t, ingestStep, "github-token: ${{ secrets.MENTIONS_PAT }}") - assert.Contains(t, extractJobSection(lockContent, "safe_outputs"), "github-token: ${{ secrets.WRITE_PAT }}") + assert.NotContains(t, ingestStep, "github-token:") + assert.NotContains(t, ingestStep, "MENTIONS_PAT") + assert.Contains(t, extractJobSection(lockContent, "safe_outputs"), "GH_AW_MENTIONS_GITHUB_TOKEN: ${{ secrets.MENTIONS_PAT }}") // Upload Safe Outputs and Upload sanitized agent output are now merged into the // unified 'agent' artifact — individual upload steps no longer exist. diff --git a/pkg/workflow/compiler_safe_outputs_steps.go b/pkg/workflow/compiler_safe_outputs_steps.go index 994fe847f3b..ba2a69d761c 100644 --- a/pkg/workflow/compiler_safe_outputs_steps.go +++ b/pkg/workflow/compiler_safe_outputs_steps.go @@ -233,6 +233,22 @@ func (c *Compiler) addAppTokenMintingSteps(data *WorkflowData) []string { } } + if mentions := data.SafeOutputs.Mentions; mentions != nil && mentions.GitHubApp != nil { + fallbackRepo := "" + if hasWorkflowCallTrigger(data.On) { + fallbackRepo = "${{ needs.activation.outputs.target_repo_name }}" + } + steps = append(steps, c.buildGitHubAppTokenMintStepForJob( + "safe_outputs", + mentions.GitHubApp, + buildMentionResolutionPermissions(data.SafeOutputs), + fallbackRepo, + inferSingleCheckoutRepositoryForGitHubAppOwner(data), + "Generate GitHub App token for mention resolution", + "safe-outputs-mentions-app-token", + )...) + } + return steps } @@ -368,6 +384,7 @@ func buildCustomScriptFilesStep(scripts map[string]*SafeScriptConfig) ([]string, func (c *Compiler) addSafeOutputCoreEnvVars(steps *[]string, data *WorkflowData) error { *steps = append(*steps, " GH_AW_AGENT_OUTPUT: ${{ steps.setup-agent-output-env.outputs.GH_AW_AGENT_OUTPUT }}\n") *steps = append(*steps, " GH_AW_COMMENT_ID: ${{ needs.activation.outputs.comment_id }}\n") + *steps = append(*steps, " GH_AW_MENTIONS_GITHUB_TOKEN: "+safeOutputMentionsGitHubToken(data)+"\n") // Add allowed domains configuration for URL sanitization in safe output handlers. // Without this, sanitizeContent() in safe_output_handler_manager.cjs only allows @@ -430,6 +447,29 @@ func (c *Compiler) addSafeOutputCoreEnvVars(steps *[]string, data *WorkflowData) return nil } +func safeOutputMentionsGitHubToken(data *WorkflowData) string { + if data == nil || data.SafeOutputs == nil || data.SafeOutputs.Mentions == nil { + return "${{ github.token }}" + } + mentions := data.SafeOutputs.Mentions + if mentions.GitHubApp == nil { + if mentions.GitHubToken != "" { + return mentions.GitHubToken + } + return "${{ github.token }}" + } + + token := "${{ steps.safe-outputs-mentions-app-token.outputs.token }}" + if !mentions.GitHubApp.shouldIgnoreMissingKey() { + return token + } + fallback := mentions.GitHubToken + if fallback == "" { + fallback = "${{ github.token }}" + } + return combineTokenExpressions(token, fallback) +} + // addCITriggerTokenEnvVar appends the GH_AW_CI_TRIGGER_TOKEN env var used to push an // empty commit after code changes to trigger CI events, working around the GITHUB_TOKEN // limitation where events don't trigger other workflows. The env var is only emitted diff --git a/pkg/workflow/compiler_yaml_step_lifecycle.go b/pkg/workflow/compiler_yaml_step_lifecycle.go index 7aabd3ca2d4..be10cf2bf19 100644 --- a/pkg/workflow/compiler_yaml_step_lifecycle.go +++ b/pkg/workflow/compiler_yaml_step_lifecycle.go @@ -265,63 +265,6 @@ func (c *Compiler) generateCreateAwInfo(yaml *strings.Builder, data *WorkflowDat yaml.WriteString(" await main(core, context);\n") } -func (c *Compiler) generateOutputCollectionGitHubToken(yaml *strings.Builder, data *WorkflowData) string { - if data.SafeOutputs == nil || data.SafeOutputs.Mentions == nil { - return "" - } - config := data.SafeOutputs.Mentions - app := config.GitHubApp - if app == nil { - if config.GitHubToken != "" { - compilerYamlStepLifecycleLog.Print("Ingest agent output uses safe-outputs.mentions.github-token for mention allowlist resolution") - } - return config.GitHubToken - } - - permissions := NewPermissions() - if data.SafeOutputs.AddComments != nil { - commentPermissions := buildAddCommentPermissions(data.SafeOutputs.AddComments) - for _, scope := range []PermissionScope{PermissionIssues, PermissionPullRequests} { - if _, ok := commentPermissions.Get(scope); ok { - permissions.Set(scope, PermissionRead) - } - } - } - if len(config.AllowedTeams) > 0 { - permissions.Set(PermissionMembers, PermissionRead) - } - const stepID = "safe-outputs-ingestion-app-token" - fallbackRepo := "" - if hasWorkflowCallTrigger(data.On) { - fallbackRepo = "${{ needs.activation.outputs.target_repo_name }}" - } - steps := c.buildGitHubAppTokenMintStepForJob("agent", app, permissions, fallbackRepo, - inferSingleCheckoutRepositoryForGitHubAppOwner(data), - "Generate GitHub App token for output ingestion", stepID) - for _, step := range collapseYAMLLinesIntoSteps(steps) { - // Ingestion runs even after agent failure; token minting and owner resolution must too. - if prefix, condition, found := strings.Cut(step, " if: "); found { - existing, rest, _ := strings.Cut(condition, "\n") - step = prefix + " if: " + combineGitHubIfExpressions("always()", existing) + "\n" + rest - } else { - firstLine, rest, _ := strings.Cut(step, "\n") - step = firstLine + "\n if: always()\n" + rest - } - yaml.WriteString(step) - } - token := "${{ steps." + stepID + ".outputs.token }}" - if app.shouldIgnoreMissingKey() { - compilerYamlStepLifecycleLog.Print("Ingest agent output uses a same-job safe-outputs.mentions.github-app token with a mention-specific/default token fallback") - fallback := config.GitHubToken - if fallback == "" { - fallback = "${{ secrets.GITHUB_TOKEN }}" - } - return combineTokenExpressions(token, fallback) - } - compilerYamlStepLifecycleLog.Print("Ingest agent output uses a same-job safe-outputs.mentions.github-app token for mention allowlist resolution") - return token -} - func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *WorkflowData) error { //nolint:largefunc // Existing artifact collection keeps related output paths and ordering together. // Copy the raw safe-output NDJSON to a /tmp/gh-aw/ path so it can be included in the // unified agent artifact together with all other /tmp/gh-aw/ outputs. @@ -346,7 +289,6 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor yaml.WriteString(" fi\n") } - githubToken := c.generateOutputCollectionGitHubToken(yaml, data) yaml.WriteString(" - name: Ingest agent output\n") yaml.WriteString(" id: collect_output\n") yaml.WriteString(" if: always()\n") @@ -410,11 +352,6 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor } yaml.WriteString(" with:\n") - if githubToken != "" { - yaml.WriteString(" github-token: " + githubToken + "\n") - } else { - compilerYamlStepLifecycleLog.Print("Ingest agent output uses the default GitHub Actions token for mention allowlist resolution") - } yaml.WriteString(" script: |\n") // Load script from external file using require() diff --git a/pkg/workflow/github_app_permissions_validation.go b/pkg/workflow/github_app_permissions_validation.go index a288f3e3e59..dc8684f10c5 100644 --- a/pkg/workflow/github_app_permissions_validation.go +++ b/pkg/workflow/github_app_permissions_validation.go @@ -175,6 +175,31 @@ func validateGitHubMCPAppPermissionsNoWrite(workflowData *WorkflowData) error { return errors.New(strings.Join(lines, "\n")) } +func validateMentionGitHubAppPermissionsReadOnly(workflowData *WorkflowData) error { + if workflowData == nil || workflowData.SafeOutputs == nil || + workflowData.SafeOutputs.Mentions == nil || workflowData.SafeOutputs.Mentions.GitHubApp == nil { + return nil + } + + var invalidScopes []string + for scope, level := range workflowData.SafeOutputs.Mentions.GitHubApp.Permissions { + normalized := strings.ToLower(strings.TrimSpace(level)) + if normalized != string(PermissionRead) && normalized != string(PermissionNone) { + invalidScopes = append(invalidScopes, scope+" (level: "+level+")") + } + } + if len(invalidScopes) == 0 { + return nil + } + sort.Strings(invalidScopes) + return NewValidationError( + "safe-outputs.mentions.github-app.permissions", + strings.Join(invalidScopes, ", "), + "mention-resolution GitHub App permissions must be read-only; each level must be \"read\" or \"none\"", + "Change each permission level to \"read\" or \"none\". Write permissions are not allowed for mention resolution.", + ) +} + // warnGitHubAppPermissionsUnsupportedContexts emits a warning when // github-app.permissions is set in contexts that do not support it. // The permissions field takes effect for tools.github.github-app and diff --git a/pkg/workflow/github_app_permissions_validation_test.go b/pkg/workflow/github_app_permissions_validation_test.go index 82f6f9f6eb1..0fcda11b82e 100644 --- a/pkg/workflow/github_app_permissions_validation_test.go +++ b/pkg/workflow/github_app_permissions_validation_test.go @@ -210,6 +210,36 @@ func TestValidateGitHubAppOnlyPermissions(t *testing.T) { } } +func TestValidateMentionGitHubAppPermissionsReadOnly(t *testing.T) { + for _, tc := range []struct { + name string + permissions map[string]string + wantError bool + }{ + {name: "read and none are accepted", permissions: map[string]string{"members": "read", "issues": "none"}}, + {name: "write is rejected", permissions: map[string]string{"members": "write"}, wantError: true}, + {name: "unknown levels are rejected", permissions: map[string]string{"issues": "admin"}, wantError: true}, + } { + t.Run(tc.name, func(t *testing.T) { + err := validateMentionGitHubAppPermissionsReadOnly(&WorkflowData{ + SafeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{ + GitHubApp: &GitHubAppConfig{Permissions: tc.permissions}, + }}, + }) + if tc.wantError { + if err == nil { + t.Fatal("expected invalid mention app permissions to fail") + } + if !strings.Contains(err.Error(), "safe-outputs.mentions.github-app.permissions") { + t.Fatalf("expected mention-specific diagnostic, got: %v", err) + } + } else if err != nil { + t.Fatalf("expected read-only permissions to pass: %v", err) + } + }) + } +} + func TestIsGitHubAppOnlyScope(t *testing.T) { tests := []struct { scope PermissionScope diff --git a/pkg/workflow/permissions_compiler_validator.go b/pkg/workflow/permissions_compiler_validator.go index 343adf228d3..a679a4c6a97 100644 --- a/pkg/workflow/permissions_compiler_validator.go +++ b/pkg/workflow/permissions_compiler_validator.go @@ -99,6 +99,9 @@ func (c *Compiler) validatePermissions(workflowData *WorkflowData, markdownPath if err := validateGitHubMCPAppPermissionsNoWrite(workflowData); err != nil { return nil, formatCompilerError(markdownPath, "error", err.Error(), err) } + if err := validateMentionGitHubAppPermissionsReadOnly(workflowData); err != nil { + return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + } // Warn when github-app.permissions is set in contexts that don't support it warnGitHubAppPermissionsUnsupportedContexts(workflowData) diff --git a/pkg/workflow/safe_outputs_config_types.go b/pkg/workflow/safe_outputs_config_types.go index 85f109df00c..aac3f1c9459 100644 --- a/pkg/workflow/safe_outputs_config_types.go +++ b/pkg/workflow/safe_outputs_config_types.go @@ -187,10 +187,10 @@ type MentionsConfig struct { // AllowedTeams is a list of team slugs whose members are always allowed to be mentioned. // Accepts "team-slug" (resolved against the current org) or "org/team-slug" format. - // Requires the workflow token to have read:org scope (a fine-grained PAT, classic PAT with - // read:org, or a GitHub App with the Members:Read permission). The default GITHUB_TOKEN - // does not include read:org and will produce a 403/404 warning; team members will be skipped - // but the workflow will not fail. + // Team membership is resolved in the trusted safe_outputs job and requires the mention + // resolution token to have read:org scope (a fine-grained PAT, classic PAT with read:org, + // or a GitHub App with the Members:Read permission). The default github.token does not + // include read:org; team members will be skipped with a warning if no suitable token is set. AllowedTeams []string `yaml:"allowed-teams,omitempty" json:"allowedTeams,omitempty"` // Max is the maximum number of mentions per message (default: 50). Supports integer or GitHub Actions expression. diff --git a/pkg/workflow/safe_outputs_permissions.go b/pkg/workflow/safe_outputs_permissions.go index 53fdecfaf2c..13c498457d6 100644 --- a/pkg/workflow/safe_outputs_permissions.go +++ b/pkg/workflow/safe_outputs_permissions.go @@ -156,6 +156,15 @@ func computePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig, excludePer permissions.Merge(handlerPermissions) } + if safeOutputs.AddComments != nil && addCommentTargetsEnabled(safeOutputs.AddComments) { + // Mention resolution fetches explicit comment targets through the Issues API, + // including when add-comment is configured for pull requests only. Add this + // only when the selected safe-output token does not already have Issues access. + if _, hasIssuesPermission := permissions.Get(PermissionIssues); !hasIssuesPermission { + permissions.Set(PermissionIssues, PermissionRead) + } + } + if dispatchRepositoryPermissions := computeDispatchRepositoryPermissions(safeOutputs, excludePerHandlerApps); dispatchRepositoryPermissions != nil { permissions.Merge(dispatchRepositoryPermissions) } @@ -201,6 +210,30 @@ func computePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig, excludePer return permissions } +func buildMentionResolutionPermissions(safeOutputs *SafeOutputsConfig) *Permissions { + permissions := NewPermissions() + if safeOutputs == nil { + return permissions + } + if safeOutputs.AddComments != nil && addCommentTargetsEnabled(safeOutputs.AddComments) { + // The pre-handler author lookup uses issues.get for both issue and PR targets. + permissions.Set(PermissionIssues, PermissionRead) + if safeOutputs.AddComments.PullRequests == nil || *safeOutputs.AddComments.PullRequests { + permissions.Set(PermissionPullRequests, PermissionRead) + } + } + if safeOutputs.Mentions != nil && len(safeOutputs.Mentions.AllowedTeams) > 0 { + permissions.Set(PermissionMembers, PermissionRead) + } + return permissions +} + +func addCommentTargetsEnabled(config *AddCommentsConfig) bool { + return config != nil && + ((config.Issues == nil || *config.Issues) || + (config.PullRequests == nil || *config.PullRequests)) +} + func computeDispatchRepositoryPermissions(safeOutputs *SafeOutputsConfig, excludePerToolApps bool) *Permissions { if safeOutputs == nil || safeOutputs.DispatchRepository == nil || len(safeOutputs.DispatchRepository.Tools) == 0 { return nil diff --git a/pkg/workflow/safe_outputs_permissions_test.go b/pkg/workflow/safe_outputs_permissions_test.go index fd327a477e3..e42af7d23bb 100644 --- a/pkg/workflow/safe_outputs_permissions_test.go +++ b/pkg/workflow/safe_outputs_permissions_test.go @@ -122,7 +122,7 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { }, }, { - name: "add-comment with issues:false - no issues permission and no discussions by default", + name: "add-comment with issues:false - read permission for PR target author lookup", safeOutputs: &SafeOutputsConfig{ AddComments: &AddCommentsConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, @@ -130,6 +130,7 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { }, }, expected: map[PermissionScope]PermissionLevel{ + PermissionIssues: PermissionRead, PermissionPullRequests: PermissionWrite, }, }, @@ -636,7 +637,7 @@ func TestComputePermissionsForSafeOutputsExcludesPerHandlerAppsFromGlobalAppToke perms := computePermissionsForSafeOutputs(safeOutputs, true) require.NotNil(t, perms) assert.Equal(t, PermissionWrite, perms.permissions[PermissionActions]) - assert.NotContains(t, perms.permissions, PermissionIssues) + assert.Equal(t, PermissionRead, perms.permissions[PermissionIssues]) } func TestComputePermissionsForSafeOutputsDispatchRepositoryAppSplit(t *testing.T) { @@ -699,11 +700,29 @@ Test workflow. perms := computePermissionsForSafeOutputs(workflowData.SafeOutputs, true) require.NotNil(t, perms) - assert.NotContains(t, perms.permissions, PermissionIssues) + assert.Equal(t, PermissionRead, perms.permissions[PermissionIssues]) assert.NotContains(t, perms.permissions, PermissionPullRequests) assert.Equal(t, PermissionWrite, perms.permissions[PermissionContents]) } +func TestMentionResolutionPermissionsIncludeIssuesReadForPullRequestOnlyComments(t *testing.T) { + issues := false + pullRequests := true + safeOutputs := &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{Issues: &issues, PullRequests: &pullRequests}, + Mentions: &MentionsConfig{AllowedTeams: []string{"my-org/my-team"}}, + } + + jobPermissions := computePermissionsForSafeOutputs(safeOutputs, true) + require.Equal(t, PermissionRead, jobPermissions.permissions[PermissionIssues]) + assert.Equal(t, PermissionWrite, jobPermissions.permissions[PermissionPullRequests]) + + mentionTokenPermissions := buildMentionResolutionPermissions(safeOutputs) + assert.Equal(t, PermissionRead, mentionTokenPermissions.permissions[PermissionIssues]) + assert.Equal(t, PermissionRead, mentionTokenPermissions.permissions[PermissionPullRequests]) + assert.Equal(t, PermissionRead, mentionTokenPermissions.permissions[PermissionMembers]) +} + func TestBuildPreambleTokenStepsExcludesParsedPerHandlerApps(t *testing.T) { compiler := NewCompiler(WithVersion("1.0.0")) tmpDir := t.TempDir() diff --git a/pkg/workflow/safe_outputs_step_token_validation.go b/pkg/workflow/safe_outputs_step_token_validation.go index 3705b9a6a62..6be7f42aa3f 100644 --- a/pkg/workflow/safe_outputs_step_token_validation.go +++ b/pkg/workflow/safe_outputs_step_token_validation.go @@ -124,7 +124,7 @@ func (c *Compiler) validateSafeOutputStepTokenReferences(data *WorkflowData) err continue } field := "safe-outputs.github-token" - if jobName == string(constants.AgentJobName) && data.SafeOutputs.Mentions != nil && + if data.SafeOutputs.Mentions != nil && strings.Contains(data.SafeOutputs.Mentions.GitHubToken, "steps."+stepID+".outputs.") { field = "safe-outputs.mentions.github-token" } diff --git a/pkg/workflow/safe_outputs_step_token_validation_test.go b/pkg/workflow/safe_outputs_step_token_validation_test.go index e858be4fbde..c3e90bc27a9 100644 --- a/pkg/workflow/safe_outputs_step_token_validation_test.go +++ b/pkg/workflow/safe_outputs_step_token_validation_test.go @@ -104,7 +104,7 @@ safe-outputs: assert.Contains(t, err.Error(), "pre-steps:") } -func TestSameJobStepTokenMissingInAgentIngestionFails(t *testing.T) { +func TestSameJobStepTokenMissingInSafeOutputMentionResolutionFails(t *testing.T) { workflowFile := writeStepTokenWorkflow(t, "missing-agent", `--- on: workflow_dispatch: @@ -119,11 +119,6 @@ safe-outputs: allowed-teams: [my-org/my-team] github-token: ${{ steps.octosts.outputs.token || secrets.GITHUB_TOKEN }} jobs: - safe_outputs: - pre-steps: - - name: Mint token (safe outputs) - id: octosts - uses: `+stsMintStep+` conclusion: pre-steps: - name: Mint token (conclusion) @@ -131,12 +126,12 @@ jobs: uses: `+stsMintStep+` --- -# Missing agent ingestion token +# Missing safe-output mention-resolution token `) err := NewCompiler().CompileWorkflow(workflowFile) require.Error(t, err) - assert.Contains(t, err.Error(), `job "agent" has no step with id "octosts"`) + assert.Contains(t, err.Error(), `job "safe_outputs" has no step with id "octosts"`) assert.Contains(t, err.Error(), "safe-outputs.mentions.github-token") assert.Contains(t, err.Error(), "pre-steps:") } @@ -150,15 +145,17 @@ permissions: id-token: write engine: claude strict: false -pre-steps: - - name: Mint mention token - id: mention_mint - uses: `+stsMintStep+` safe-outputs: add-comment: mentions: allowed-teams: [my-org/my-team] github-token: ${{ steps.mention_mint.outputs.token }} +jobs: + safe_outputs: + pre-steps: + - name: Mint mention token + id: mention_mint + uses: `+stsMintStep+` --- # Mention-only same-job token @@ -169,11 +166,11 @@ safe-outputs: require.NoError(t, err) lockYAML := string(lockContent) agent := extractJobSection(lockYAML, "agent") - assert.Contains(t, agent, "github-token: ${{ steps.mention_mint.outputs.token }}") - assert.Less(t, jobStepIDDeclarationIndex(agent, "mention_mint"), jobStepOutputConsumptionIndex(agent, "mention_mint")) - for _, jobName := range []string{"safe_outputs", "conclusion"} { - assert.NotContains(t, extractJobSection(lockYAML, jobName), "steps.mention_mint.outputs.token") - } + assert.NotContains(t, agent, "steps.mention_mint.outputs.token") + safeOutputs := extractJobSection(lockYAML, "safe_outputs") + assert.Contains(t, safeOutputs, "GH_AW_MENTIONS_GITHUB_TOKEN: ${{ steps.mention_mint.outputs.token }}") + assert.Less(t, jobStepIDDeclarationIndex(safeOutputs, "mention_mint"), jobStepOutputConsumptionIndex(safeOutputs, "mention_mint")) + assert.NotContains(t, extractJobSection(lockYAML, "conclusion"), "steps.mention_mint.outputs.token") } // TestSameJobStepTokenMintedAfterConsumerFails verifies that safe-outputs.steps, which run