Isolate safe_outputs checkout auth from safe-outputs app/token credentials#43005
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (38 additions in 3 files, none in default business logic directories). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
There was a problem hiding this comment.
Pull request overview
This PR updates the workflow compiler so that safe_outputs push credentials are no longer injected into generated actions/checkout steps, preventing cross-org failures where a safe-outputs token/app token cannot read the workflow repository. It also adjusts tests to assert that the safe_outputs push token remains available for later git credential configuration while being excluded from checkout with.token.
Changes:
- Removed the safe_outputs push-token fallback from default and additional checkout step generation so checkout auth comes only from checkout config (or default
GITHUB_TOKENbehavior). - Updated checkout manager tests to assert push-token isolation (no push token in checkout YAML).
- Added safe_outputs step tests asserting push tokens flow into
GIT_TOKENenv but are not emitted as checkoutwith.token.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_safe_outputs_steps_test.go | Adds assertions that safe_outputs tokens/apps are used for GIT_TOKEN but not emitted as checkout with.token. |
| pkg/workflow/checkout_step_generator.go | Removes push-token fallback injection into checkout steps and documents intended separation between checkout vs push auth. |
| pkg/workflow/checkout_manager_test.go | Reworks tests to validate push-token isolation for both default and additional checkouts. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| // Apply user overrides only when NOT in trial mode to avoid conflicting | ||
| // repository/token values in the same checkout step. Note that safe_outputs push | ||
| // auth is intentionally isolated from checkout auth: this step only emits tokens | ||
| // from checkout config (or trial mode), while push auth is applied later by | ||
| // CheckoutManager.GenerateConfigureGitCredentialsSteps. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (4 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Isolate safe_outputs checkout auth from safe-outputs app/token credentials
This PR correctly fixes a security boundary issue: the safe_outputs push token was leaking into actions/checkout with.token, which could cause cross-org failures and unnecessarily exposes the push token to the checkout phase.
Security assessment: The fix is correct and tightens the security boundary. Checkout now only receives credentials from checkout-specific config, while push auth is separately wired by GenerateConfigureGitCredentialsSteps.
Correctness: The two-phase auth model is sound: (1) checkout uses only its own credentials or the default workflow token; (2) push auth is configured separately after checkout.
Test coverage: Both regressions are well-covered. TestCheckoutPushTokenIsolation now asserts token absence. New compiler test cases assert safe-outputs token/app token appears in GIT_TOKEN but not in checkout with.token.
Code clarity: The inline comment on GenerateDefaultCheckoutStep explicitly documents the intentional decoupling, helping future maintainers avoid re-introducing the bug.
No blocking issues found. The change is minimal, targeted, and well-tested. ✅
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 57.1 AIC · ⌖ 6.45 AIC · ⊞ 4.9K
|
@copilot review changes for side repo and trial modes.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — overall solid fix with two small test-coverage gaps worth addressing.
📋 Key Themes & Highlights
Key Themes
- Missing regression guard for
persist-credentials: The newsafe-outputs github-apptest case (compiler_safe_outputs_steps_test.go:117) verifies the token is absent fromwith.token, but doesn't assertpersist-credentials: trueis still emitted. A future regression breaking push-auth would slip through. - Cross-org failure mode not encoded as a test: The PR description motivates the fix with a specific scenario (checkout token scoped to workflow-org vs push token scoped to target-org), but no sub-test in
TestCheckoutPushTokenIsolationdirectly exercises that combination. - Signature simplification opportunity noted:
generateCheckoutStepLinesnow only useskeepCredentialsForPushforpersist-credentials— the removedpushTokenwas the other consumer. The updated doc comment handles this well.
Positive Highlights
- ✅ Clean deletion of the push-token fallback — net negative LoC, no dead code left behind.
- ✅ Test rename from
TestCheckoutPushTokenFallback→TestCheckoutPushTokenIsolationprecisely captures the new invariant. - ✅ Dual coverage path: unit test in
checkout_manager_test.goand integration-style assertions incompiler_safe_outputs_steps_test.go. - ✅ Updated
GenerateConfigureGitCredentialsStepsreference in the function doc makes the two-phase auth model explicit. - ✅ PR description includes before/after YAML — great for reviewers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 83.6 AIC · ⌖ 7.31 AIC · ⊞ 6.6K
Comment /matt to run again
| }, | ||
| }, | ||
| { | ||
| name: "safe-outputs github-app token is not used by checkout step", |
There was a problem hiding this comment.
[/diagnosing-bugs] This test verifies the app token is absent from with.token, but does not assert that persist-credentials: true is still present — which is the mechanism that keeps credentials available for GenerateConfigureGitCredentialsSteps.
💡 Suggested addition
Add to checkContains:
"persist-credentials: true", // must retain creds for the git-credential step to workWithout this guard, a future regression silently setting persist-credentials: false in the safe_outputs job path would not be caught here, potentially breaking push auth without a failing test.
@copilot please address this.
| const pushToken = "${{ secrets.PUSH_TOKEN }}" | ||
|
|
||
| t.Run("default checkout with no explicit token emits pushToken once", func(t *testing.T) { | ||
| t.Run("default checkout with no explicit token does not emit pushToken", func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test covers the case where no checkout token is set, but there is no sub-test for the actual cross-org failure mode the PR describes: a checkout.github-token scoped to org-A while pushToken is scoped to org-B.
💡 Suggested sub-test
t.Run("default checkout with its own token uses that token, not pushToken", func(t *testing.T) {
cm := NewCheckoutManager([]*CheckoutConfig{
{GitHubToken: "${{ secrets.CHECKOUT_TOKEN }}"},
})
cm.SetKeepCredentialsForPush(true)
cm.SetPushToken(pushToken)
lines := cm.GenerateDefaultCheckoutStep(false, "", getPin)
combined := strings.Join(lines, "")
assert.Contains(t, combined, "token: ${{ secrets.CHECKOUT_TOKEN }}", "explicit checkout token must be used")
assert.NotContains(t, combined, pushToken, "push token must not override checkout token")
})This sub-test directly encodes the cross-org failure scenario as a regression guard.
@copilot please address this.
| func generateCheckoutStepLines(entry *resolvedCheckout, index int, keepCredentialsForPush bool, pushToken string, getActionPin func(string) string) []string { | ||
| // git fetch/push can authenticate after CheckoutManager.GenerateConfigureGitCredentialsSteps | ||
| // rewrites remotes with the resolved push token. | ||
| func generateCheckoutStepLines(entry *resolvedCheckout, index int, keepCredentialsForPush bool, getActionPin func(string) string) []string { |
There was a problem hiding this comment.
[/codebase-design] generateCheckoutStepLines still accepts keepCredentialsForPush bool and uses it only to decide persist-credentials:. The removed pushToken string parameter was its only other consumer. This is fine as-is, but the function signature now partially describes checkout persistence policy, not just step generation — worth noting if this function grows. No change required; just a flag to keep the interface cohesion in mind.
📝 Context
If keepCredentialsForPush ever needs to influence more than persist-credentials, the call-site in GenerateAdditionalCheckoutSteps will need to be audited again. Keeping the comment in the function doc mentioning that push auth is handled downstream by GenerateConfigureGitCredentialsSteps (as done here) is the right mitigation.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…dSharedPRCheckoutSteps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed and confirmed — the existing fix already isolates safe-outputs tokens from checkout steps in both trial mode and side-repo (additional checkout) scenarios:
In both cases the safe-outputs token only reaches Added explicit regression tests in commit
|
|
Great work on the The PR looks ready for review:
This looks ready for a maintainer pass! 🚀
|
Addressed in the latest two commits. Here's what was done: Root cause fix —
Regression tests added
|
|
@copilot please run the Priorities:
Run: https://github.com/github/gh-aw/actions/runs/28628188522
|
… github-app checkout path Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Here's a final status summary: persist-credentials: true regression guard in safe-outputs github-app path — added Cross-org checkout-token-vs-push-token regression tests — already present in
http./.extraheader cleanup coverage — confirmed for both paths:
All |
Triage Result
Rationale: Auth isolation bug causing cross-org checkout failures when safe_outputs applies its push/app token to the workflow-repo checkout. Large diff (65 files, +266/-96) but targeted fix. Currently blocked — expedited human review recommended given auth/security scope.
|
|
@copilot please run Run: https://github.com/github/gh-aw/actions/runs/28632917404
|
…uts-token-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done — |
|
@copilot please run the
|
…uts-token-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Merged the latest |
|
@copilot please run Run: https://github.com/github/gh-aw/actions/runs/28641705440
|
…uts-token-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done — |
safe_outputswas applying its resolved push/auth token to checkout steps, causing the default workflow-repo checkout to usesafe-outputs.github-app/safe-outputs.github-token. In cross-org setups, that token can be scoped to the target org and fail when checking out the workflow repo.Auth flow correction (checkout vs push)
checkout_step_generator.go(both default and additional checkouts).with.tokenis now emitted only from checkout-specific config (checkout.github-token/checkout.github-app) or normal default behavior.CheckoutManager.GenerateConfigureGitCredentialsSteps, keeping push credentials separate from checkout credentials.Regression coverage
GIT_TOKEN)with.token.Code clarity