Repository navigation
Fix mention allowlist token selection during safe-output ingestion - #66571
Conversation
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>
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>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - 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. |
There was a problem hiding this comment.
Addressed in 344905c: mention/team resolution now runs only in the trusted safe_outputs job. Agent-job ingestion performs no privileged mention lookups and receives no mention-specific credentials.
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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The mention-ingestion credential split is headed the right way, but the new app-token permission derivation is still wrong for PR-only comment workflows: the ingestion pre-scan fetches explicit comment targets through the Issues API, while the minted token can now omit issues: read. That breaks target-author allowlisting and silently re-escapes valid mentions in one of the narrower supported configurations.
### Blocking theme
The new token should be scoped to the endpoints ingestion actually calls, not mirrored from the enabled write targets on add-comment. Right now a workflow with add-comment.pull-requests: true and add-comment.issues: false can compile a mention-ingestion token that is too weak for github.rest.issues.get(...), so the feature regresses precisely the PR-only case it is supposed to preserve.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 114.9 AIC · ⌖ 5.52 AIC · ⊞ 19.6K
Comment /review to run again
| permissions := NewPermissions() | ||
| if data.SafeOutputs.AddComments != nil { | ||
| commentPermissions := buildAddCommentPermissions(data.SafeOutputs.AddComments) | ||
| for _, scope := range []PermissionScope{PermissionIssues, PermissionPullRequests} { |
There was a problem hiding this comment.
This permission derivation is too narrow for the lookup the ingestion step actually performs, so PR-only comment workflows can still lose allowed mentions.
💡 Why this blocks the change
The new token minting logic only copies issues/pull-requests scopes that happen to be enabled on add-comment, but the ingestion pre-scan always resolves explicit add_comment.item_number targets through github.rest.issues.get(...) before sanitization. A workflow that intentionally allows only pull-request comments (pull-requests: true, issues: false) will therefore mint an ingestion token without issues: read, causing the target-author lookup to fail and valid @author mentions in PR comments to be escaped.
A safer fix is to derive ingestion permissions from the endpoints this step calls, not from the handler toggles. At minimum, the mention-ingestion token needs issues: read whenever add-comment is enabled, because the pre-scan unconditionally uses the Issues API for explicit comment targets.
There was a problem hiding this comment.
Addressed in 344905c: the safe-output job now retains issues: read for explicit comment-target author lookups when comments are PR-only, with regression coverage for that configuration.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Mention-app overrides can currently mint a write-capable token despite the read-only ingestion contract.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds dedicated mention-resolution credentials so ingestion can preserve allowed team mentions without exposing downstream write credentials.
Changes:
- Adds mention-specific PAT and GitHub App configuration.
- Generates post-agent ingestion tokens with scoped permissions and fallback behavior.
- Adds validation, tests, schema, and documentation updates.
| File | Description |
|---|---|
pkg/workflow/security_architecture_formal_test.go |
Updates credential-isolation assertions. |
pkg/workflow/safe_outputs_step_token_validation.go |
Validates mention token step references. |
pkg/workflow/safe_outputs_step_token_validation_test.go |
Tests same-job mention tokens. |
pkg/workflow/safe_outputs_messages_config.go |
Parses mention credentials. |
pkg/workflow/safe_outputs_mentions_test.go |
Tests parsing and credential isolation. |
pkg/workflow/safe_outputs_config_types.go |
Adds mention credential fields. |
pkg/workflow/compiler_yaml_step_lifecycle.go |
Selects and mints ingestion credentials. |
pkg/workflow/agentic_output_test.go |
Covers token selection, fallback, and ordering. |
pkg/parser/schemas/main_workflow_schema.json |
Adds schema fields. |
docs/src/content/docs/specs/safe-outputs-specification.md |
Defines ingestion credential requirements. |
docs/src/content/docs/reference/safe-outputs.md |
Documents configuration and behavior. |
docs/src/content/docs/reference/frontmatter-full.md |
Updates generated frontmatter reference. |
| steps := c.buildGitHubAppTokenMintStepForJob("agent", app, permissions, fallbackRepo, | ||
| inferSingleCheckoutRepositoryForGitHubAppOwner(data), | ||
| "Generate GitHub App token for output ingestion", stepID) |
There was a problem hiding this comment.
Addressed in 344905c: mention GitHub App permission overrides are validated as read-only; only read and none are accepted, and write-level overrides fail compilation.
🏗️ Design Decision Gate: ADR RequiredThis PR triggers ADR enforcement and no Architecture Decision Record was found. Why enforcement applies
ADR search results
Action taken I drafted an ADR from the PR evidence and committed it to this branch:
Inferred decision summary:
Next step for the author: review
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design to the mention-credential isolation fix. The change is well-scoped: it introduces dedicated safe-outputs.mentions.github-token/github-app credentials, reuses existing app-token-minting helpers (buildGitHubAppTokenMintStepForJob, resolveGitHubAppOwner, combineGitHubIfExpressions) rather than duplicating logic, and ships extensive regression coverage for ordering, fallback chains (app → PAT → default token), wildcard/owner resolution, and credential isolation (TestOutputCollectionGitHubAppToken, TestOutputCollectionGitHubAppOwnerAndWildcard, TestOutputCollectionMentionCredentialsIndependent, TestFormal_P10_WriteTokenIsolatedToSafeOutput).
No blocking issues found. A few minor/non-blocking observations below.
📋 Key Themes & Highlights
Key Themes
- Root cause correctly addressed, not just the symptom (
/diagnosing-bugs): the fix mints a dedicated, properly-scoped (read-only +members:read) ingestion token rather than broadening the default Actions token or reusing the write-scopedsafe-outputs.github-token, which would have reintroduced the isolation problem the spec (AR1) guards against. - Consistent with existing conventions (
/codebase-design): theif: always()rewrite ingenerateOutputCollectionGitHubTokenmirrors the existing pattern incompiler_safe_outputs_job.go(strings.Cut(step, " if: ")), so it's not a novel risk, just a repeated idiom. - Credential isolation is enforced at multiple layers:
buildMentionsHandlerConfigwhitelists only non-credential fields for the runtime handler config,json:"-"tags keepGitHubToken/GitHubAppout of any JSON marshaling, andTestParseMentionsCredentialsexplicitly asserts secret values never leak into serialized config. - Good regression-test design:
TestSameJobStepTokenMissingInAgentIngestionFailsandTestSameJobStepTokenMentionsOnlyMintedInAgentCompilesexercise the exact same-job step-token-ordering bug class the step-token validator is meant to catch, with mention-specific error-field naming (safe-outputs.mentions.github-token) for clearer diagnostics.
Minor Observations (non-blocking)
generateOutputCollectionGitHubTokenderives read permissions for comment-author lookups only fromAddComments(buildAddCommentPermissions). If a future handler besidesadd-commentalso needs ingestion-time author/collaborator lookups, this list would need to grow in lockstep — worth a short code comment noting that the permission set is deliberately tied to today'sresolve_mentions.cjsconsumers, to prevent silent permission drift if new handlers are added later.- The bot-flagged spec line (docs/specs/safe-outputs-specification.md) about privileged token injection appears to refer to pre-existing text rather than this PR's new bullet list — the new "Ingestion credential requirements" section already explicitly documents that
safe-outputs.github-tokenis not inherited, which resolves that concern for new readers.
Positive Highlights
- ✅ Minting step reuses
inferSingleCheckoutRepositoryForGitHubAppOwnerandhasWorkflowCallTriggerinstead of reimplementing owner/fallback-repo derivation. - ✅
assert.Nil(t, app.Permissions, "minting must not mutate the configured app")is a nice defensive test guarding against accidental shared-state mutation. - ✅ Formal security test (
TestFormal_P10_WriteTokenIsolatedToSafeOutput) was updated to also assert the ingestion token never requestssafe_outputs-job write permissions, closing a gap the new feature could have reopened.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 120.8 AIC · ⌖ 15.7 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
Impeccable Skills Review
Applied modes: harden, audit (bug-fix/credential-isolation change).
Summary
This PR adds dedicated safe-outputs.mentions.github-token / safe-outputs.mentions.github-app credentials for mention-allowlist resolution during output ingestion, decoupling it from the default Actions token and from write credentials.
Findings
No high-signal, blocking issues found tied to changed lines. Notable positive security properties verified:
- Mention credentials are correctly isolated from write credentials (
safe-outputs.github-token/github-app) — verified viaTestAgenticOutputCollectionWithGitHubAppandTestFormal_P10_WriteTokenIsolatedToSafeOutput. - Mention app token minting runs in the
agentjob only afterStop MCP Gateway/agent execution completes (asserted order in tests), consistent with the "trusted post-agent step" design goal. always()is correctly combined with any existingignore-if-missingcondition rather than overwriting it.- Mention credentials are excluded from JSON-serialized/agent-visible config (
json:"-"tags) and frombuildMentionsHandlerConfigoutput — verified byTestParseMentionsCredentials. - Literal (non-expression)
mentions.github-tokenvalues are rejected by existing schema validation. - Same-job step-token validation (
collectSafeOutputStepTokenIDs/validateSafeOutputStepTokenReferences) was correctly extended to covermentions.github-tokenwith an accurate field name in the error (safe-outputs.mentions.github-token). - Owner/repository fallback reuses the same
needs.activation.outputs.target_repo_namepattern already used bysafe_outputs/conclusionjobs, and theagentjob already depends onactivation, so no new wiring risk. - Ran
go build ./...and the full set of new/modified tests (TestOutputCollection*,TestSameJobStepToken*,TestFormal_P10_WriteTokenIsolatedToSafeOutput,TestParseMentionsCredentials,TestAgenticOutputCollection*) — all pass.
A pre-existing GHAS comment flagged documentation wording about token privilege; this PR's actual code path only uses mentions.github-token/mentions.github-app, not the global write token, so the implementation is not affected — likely a stale/doc-only concern outside the scope of actionable code changes.
No inline comments added since no blocking or high-confidence issues were found on changed lines.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 170.4 AIC · ⌖ 13.3 AIC · ⊞ 8.1K
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: b8df84b
|
…mention-token Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
Team members allowed by
safe-outputs.mentions.allowed-teamscould have their mentions escaped during ingestion because that step used the default Actions token without access to team membership. Downstream handlers could not restore notifications after the body had already been sanitized.Add dedicated ingestion credentials under
safe-outputs.mentions.github-tokenandsafe-outputs.mentions.github-app. Ingestion never inherits globalsafe-outputs.github-token,safe-outputs.github-app, per-handler credentials, orGH_AW_GITHUB_TOKEN. Without mention-specific credentials it uses the default Actions token. Omitting the mention-specific app skips ingestion token minting without disabling mention filtering or changing downstream write credentials.A mention-specific app token is minted in the agent job after agent execution and MCP gateway shutdown, immediately before Ingest agent output. It takes precedence over the mention PAT, requests comment-author read scopes and
members: readfor configured teams, and preserves explicit app permission overrides. Withignore-if-missing: true, missing app credentials fall back to the mention PAT, thenGITHUB_TOKENonly. Installation owner/repository selection, wildcard scoping, relay repository selection, and missing-key guards reuse existing compiler helpers; minting and owner resolution match ingestion'salways()behavior.Mention credentials are excluded from agent-visible validation and handler configuration. Write app keys remain absent from the agent job. Same-job mention-token references require an earlier producer only in the agent job, with mention-specific diagnostics. Updated schema, generated reference, normative specification, and secret-safe logging describe the configuration; regression coverage checks selection, scoping, fallback, ordering, and credential isolation.
Fixes: #50282
Validation
PATH="/opt/homebrew/bin:$PATH" TMPDIR=/private/tmp go test ./pkg/workflow ./pkg/parser -count=1.make check-workflow-drift: all 324 compiled workflows remain in sync.fmt.Fprintfcalls in the lifecycle compiler and slice indexing in step-token validation. Standard Go lint passed.vite@8.3.2. No runtime JavaScript was changed.