Skip to content

Commit e64db0e

Browse files
Copilotpelikhan
Copilot
and
pelikhan
authored
fix: support github-app checkout token minting in upload job and fix tests
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8a1bd4b1-943a-420a-95a3-69ef317b4ef5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent 415e6e4 commit e64db0e

2 files changed

Lines changed: 117 additions & 49 deletions

File tree

‎pkg/workflow/compiler_safe_outputs_job_test.go‎

Lines changed: 71 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -774,25 +774,30 @@ func TestCallWorkflowOnly_UsesHandlerManagerStep(t *testing.T) {
774774
// TestCreateCodeScanningAlertUploadJob verifies that when create-code-scanning-alert is configured,
775775
// a dedicated upload_code_scanning_sarif job is created (separate from safe_outputs) and that
776776
// the safe_outputs job:
777-
// - exports sarif_file and checkout_token outputs for the upload job
777+
// - exports sarif_file output for the upload job
778778
// - uploads the SARIF file as a GitHub Actions artifact so the upload job
779779
// (which runs in a fresh workspace) can download it
780+
//
781+
// Token handling: the upload job computes tokens directly (static PAT or minted GitHub App token)
782+
// rather than reading from safe_outputs job outputs, because GitHub Actions masks secret references
783+
// in job outputs — "Skip output 'x' since it may contain secret".
780784
func TestCreateCodeScanningAlertUploadJob(t *testing.T) {
781785
tests := []struct {
782786
name string
783787
config *CreateCodeScanningAlertsConfig
788+
checkoutConfigs []*CheckoutConfig
784789
expectUploadJob bool
785-
expectCustomToken string
786-
expectTokenFromOutputs bool // expect needs.safe_outputs.outputs.checkout_token
790+
expectTokenInSteps string // expected token expression in upload job steps
791+
expectAppTokenMintStep bool // expect a GitHub App token minting step in upload job
787792
safeOutputsGitHubToken string
788793
}{
789794
{
790-
name: "default config creates separate upload job using checkout_token from outputs",
795+
name: "default config creates separate upload job with static token computed directly",
791796
config: &CreateCodeScanningAlertsConfig{
792797
BaseSafeOutputConfig: BaseSafeOutputConfig{},
793798
},
794-
expectUploadJob: true,
795-
expectTokenFromOutputs: true,
799+
expectUploadJob: true,
800+
expectTokenInSteps: "${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}",
796801
},
797802
{
798803
name: "custom per-config github-token is used in upload step token",
@@ -801,20 +806,48 @@ func TestCreateCodeScanningAlertUploadJob(t *testing.T) {
801806
GitHubToken: "${{ secrets.GHAS_TOKEN }}",
802807
},
803808
},
804-
expectUploadJob: true,
805-
expectCustomToken: "${{ secrets.GHAS_TOKEN }}",
806-
expectTokenFromOutputs: true,
809+
expectUploadJob: true,
810+
expectTokenInSteps: "${{ secrets.GHAS_TOKEN }}",
807811
},
808812
{
809813
name: "safe-outputs-level github-token is used in upload step token",
810814
config: &CreateCodeScanningAlertsConfig{
811815
BaseSafeOutputConfig: BaseSafeOutputConfig{},
812816
},
813817
expectUploadJob: true,
814-
expectCustomToken: "${{ secrets.SO_TOKEN }}",
815-
expectTokenFromOutputs: true,
818+
expectTokenInSteps: "${{ secrets.SO_TOKEN }}",
816819
safeOutputsGitHubToken: "${{ secrets.SO_TOKEN }}",
817820
},
821+
{
822+
name: "checkout with github-app mints a fresh app token in the upload job",
823+
config: &CreateCodeScanningAlertsConfig{
824+
BaseSafeOutputConfig: BaseSafeOutputConfig{},
825+
},
826+
checkoutConfigs: []*CheckoutConfig{
827+
{
828+
GitHubApp: &GitHubAppConfig{
829+
AppID: "${{ vars.APP_ID }}",
830+
PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}",
831+
},
832+
},
833+
},
834+
expectUploadJob: true,
835+
expectTokenInSteps: "${{ steps.checkout-restore-app-token.outputs.token }}",
836+
expectAppTokenMintStep: true,
837+
},
838+
{
839+
name: "checkout with github-token PAT uses that PAT directly in upload job",
840+
config: &CreateCodeScanningAlertsConfig{
841+
BaseSafeOutputConfig: BaseSafeOutputConfig{},
842+
},
843+
checkoutConfigs: []*CheckoutConfig{
844+
{
845+
GitHubToken: "${{ secrets.MY_CHECKOUT_PAT }}",
846+
},
847+
},
848+
expectUploadJob: true,
849+
expectTokenInSteps: "${{ secrets.MY_CHECKOUT_PAT }}",
850+
},
818851
{
819852
name: "staged mode does not create upload job",
820853
config: &CreateCodeScanningAlertsConfig{
@@ -837,9 +870,10 @@ func TestCreateCodeScanningAlertUploadJob(t *testing.T) {
837870
CreateCodeScanningAlerts: tt.config,
838871
GitHubToken: tt.safeOutputsGitHubToken,
839872
},
873+
CheckoutConfigs: tt.checkoutConfigs,
840874
}
841875

842-
// 1. Verify safe_outputs job exports sarif_file and checkout_token, and uploads artifact
876+
// 1. Verify safe_outputs job exports sarif_file and uploads the artifact
843877
safeOutputsJob, _, err := compiler.buildConsolidatedSafeOutputsJob(workflowData, string(constants.AgentJobName), "test-workflow.md")
844878
require.NoError(t, err, "safe_outputs job should build without error")
845879
require.NotNil(t, safeOutputsJob, "safe_outputs job should be generated")
@@ -853,9 +887,10 @@ func TestCreateCodeScanningAlertUploadJob(t *testing.T) {
853887
assert.Contains(t, safeOutputsJob.Outputs["sarif_file"], "steps.process_safe_outputs.outputs.sarif_file",
854888
"sarif_file output must reference process_safe_outputs step")
855889

856-
// safe_outputs must export checkout_token for the upload job to use
857-
assert.Contains(t, safeOutputsJob.Outputs, "checkout_token",
858-
"safe_outputs job must export checkout_token output for the upload job")
890+
// safe_outputs must NOT export checkout_token — GitHub Actions masks secret
891+
// references in job outputs, making them arrive empty in downstream jobs.
892+
assert.NotContains(t, safeOutputsJob.Outputs, "checkout_token",
893+
"safe_outputs job must NOT export checkout_token (secret refs are masked in job outputs)")
859894

860895
// safe_outputs must upload the SARIF file as an artifact so the upload job
861896
// (running in a fresh workspace) can download it
@@ -888,6 +923,11 @@ func TestCreateCodeScanningAlertUploadJob(t *testing.T) {
888923

889924
uploadSteps := strings.Join(uploadJob.Steps, "")
890925

926+
// The upload job must NOT use needs.safe_outputs.outputs.checkout_token — it
927+
// would arrive empty because GitHub Actions masks secret refs in job outputs.
928+
assert.NotContains(t, uploadSteps, "needs.safe_outputs.outputs.checkout_token",
929+
"Upload job must NOT read checkout_token from safe_outputs outputs (would be masked)")
930+
891931
// Restore checkout step must be present in the upload job
892932
assert.Contains(t, uploadSteps, "Restore checkout to triggering commit",
893933
"Upload job must restore workspace to triggering commit")
@@ -898,9 +938,17 @@ func TestCreateCodeScanningAlertUploadJob(t *testing.T) {
898938
assert.NotContains(t, uploadSteps, "git checkout ${{ github.sha }}",
899939
"Must use actions/checkout, not a raw git command")
900940

901-
// The restore checkout step must always use the checkout_token from safe_outputs outputs
902-
assert.Contains(t, uploadSteps, "needs.safe_outputs.outputs.checkout_token",
903-
"Restore checkout step must use checkout_token from safe_outputs outputs")
941+
if tt.expectAppTokenMintStep {
942+
// GitHub App checkout: a token minting step must appear before the restore checkout
943+
assert.Contains(t, uploadSteps, "checkout-restore-app-token",
944+
"Upload job must mint a GitHub App token before restoring checkout")
945+
mintPos := strings.Index(uploadSteps, "checkout-restore-app-token")
946+
restoreCheckoutPos := strings.Index(uploadSteps, "Restore checkout to triggering commit")
947+
require.NotEqual(t, -1, mintPos, "App token minting step must be present in upload job steps")
948+
require.NotEqual(t, -1, restoreCheckoutPos, "Restore checkout step must be present in upload job steps")
949+
assert.Less(t, mintPos, restoreCheckoutPos,
950+
"App token minting step must appear before the restore checkout step")
951+
}
904952

905953
// Download SARIF artifact step must be present in the upload job
906954
assert.Contains(t, uploadSteps, "Download SARIF artifact",
@@ -946,16 +994,13 @@ func TestCreateCodeScanningAlertUploadJob(t *testing.T) {
946994
assert.Less(t, downloadPos, uploadPos,
947995
"SARIF download must appear before SARIF upload in the job steps")
948996

949-
if tt.expectCustomToken != "" {
950-
assert.Contains(t, uploadSteps, tt.expectCustomToken,
951-
"Upload SARIF token must use the per-config or safe-outputs github-token")
952-
}
953-
if tt.expectTokenFromOutputs {
954-
assert.Contains(t, uploadSteps, "needs.safe_outputs.outputs.checkout_token",
955-
"Upload job should use checkout_token from safe_outputs outputs")
997+
// Verify the expected token expression appears in the upload job steps
998+
if tt.expectTokenInSteps != "" {
999+
assert.Contains(t, uploadSteps, tt.expectTokenInSteps,
1000+
"Upload job must use the expected token in its steps")
9561001
}
9571002
} else {
958-
// staged: safe_outputs should NOT export sarif_file or checkout_token
1003+
// staged: safe_outputs should NOT export sarif_file
9591004
assert.NotContains(t, safeOutputsJob.Outputs, "sarif_file",
9601005
"staged mode: safe_outputs must not export sarif_file")
9611006
assert.NotContains(t, safeOutputsJob.Outputs, "checkout_token",

‎pkg/workflow/create_code_scanning_alert.go‎

Lines changed: 46 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package workflow
33
import (
44
"fmt"
55
"path"
6+
"strings"
67

78
"github.com/github/gh-aw/pkg/constants"
89
"github.com/github/gh-aw/pkg/logger"
@@ -79,22 +80,41 @@ func (c *Compiler) parseCodeScanningAlertsConfig(outputMap map[string]any) *Crea
7980
func (c *Compiler) buildCodeScanningUploadJob(data *WorkflowData) (*Job, error) {
8081
createCodeScanningAlertLog.Print("Building upload_code_scanning_sarif job")
8182

82-
// Compute the restore token directly in this job rather than reading it from the
83-
// safe_outputs job's outputs. GitHub Actions masks job outputs that contain secret
84-
// references (e.g. "${{ secrets.GITHUB_TOKEN }}"), so passing the token through
85-
// safe_outputs.outputs.checkout_token would result in an empty string here.
86-
// Since computeStaticCheckoutToken returns a plain static secret-reference expression
87-
// (e.g. "${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}") it is safe to
88-
// evaluate directly in any job, including this one.
83+
// Compute the effective token for checkout/upload in this job.
84+
// We cannot pass tokens through job outputs (GitHub Actions masks secret references).
85+
// We must either compute the static token directly or mint a fresh GitHub App token.
8986
checkoutMgr := NewCheckoutManager(data.CheckoutConfigs)
90-
restoreToken := computeStaticCheckoutToken(data.SafeOutputs, checkoutMgr)
87+
88+
var restoreToken string
89+
var tokenMintSteps []string
90+
91+
// Check if the default checkout uses GitHub App auth. If so, mint a fresh token
92+
// in this job — activation/safe_outputs app tokens have expired by this point.
93+
defaultOverride := checkoutMgr.GetDefaultCheckoutOverride()
94+
if defaultOverride != nil && defaultOverride.githubApp != nil {
95+
permissions := NewPermissionsContentsReadSecurityEventsWrite()
96+
for _, step := range c.buildGitHubAppTokenMintStep(defaultOverride.githubApp, permissions, "") {
97+
tokenMintSteps = append(tokenMintSteps,
98+
strings.ReplaceAll(step, "id: safe-outputs-app-token", "id: checkout-restore-app-token"))
99+
}
100+
//nolint:gosec // G101: False positive - this is a GitHub Actions expression template, not a hardcoded credential
101+
restoreToken = "${{ steps.checkout-restore-app-token.outputs.token }}"
102+
} else {
103+
// No GitHub App configured for checkout — compute a static secret reference
104+
// directly. This is safe because secret references are evaluated in the job's own
105+
// context (not through job outputs which would be masked by GitHub Actions).
106+
restoreToken = computeStaticCheckoutToken(data.SafeOutputs, checkoutMgr)
107+
}
91108

92109
// Artifact prefix for workflow_call context (so the download name matches the upload name).
93110
agentArtifactPrefix := artifactPrefixExprForDownstreamJob(data)
94111

95112
var steps []string
96113

97-
// Step 1: Restore workspace to the triggering commit.
114+
// Prepend any token minting steps (needed when checkout uses GitHub App auth).
115+
steps = append(steps, tokenMintSteps...)
116+
117+
// Step: Restore workspace to the triggering commit.
98118
// The safe_outputs job may have checked out a different branch (e.g., the base branch for
99119
// a PR) which would leave HEAD pointing at a different commit. The SARIF upload action
100120
// requires HEAD to match the commit being scanned, otherwise it fails with "commit not found".
@@ -106,7 +126,7 @@ func (c *Compiler) buildCodeScanningUploadJob(data *WorkflowData) (*Job, error)
106126
steps = append(steps, " persist-credentials: false\n")
107127
steps = append(steps, " fetch-depth: 1\n")
108128

109-
// Step 2: Download the SARIF artifact produced by safe_outputs.
129+
// Step: Download the SARIF artifact produced by safe_outputs.
110130
// The SARIF file was written to the safe_outputs job workspace and uploaded as an artifact.
111131
// This job runs in a fresh workspace so we must download the artifact before uploading
112132
// to GitHub Code Scanning.
@@ -120,13 +140,14 @@ func (c *Compiler) buildCodeScanningUploadJob(data *WorkflowData) (*Job, error)
120140
// The local SARIF file path after the artifact download completes.
121141
localSarifPath := path.Join(constants.SarifArtifactDownloadPath, constants.SarifFileName)
122142

123-
// Step 3: Upload SARIF file to GitHub Code Scanning.
143+
// Step: Upload SARIF file to GitHub Code Scanning.
124144
steps = append(steps, " - name: Upload SARIF to GitHub Code Scanning\n")
125145
steps = append(steps, fmt.Sprintf(" id: %s\n", constants.UploadCodeScanningJobName))
126146
steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("github/codeql-action/upload-sarif")))
127147
steps = append(steps, " with:\n")
128148
// NOTE: github/codeql-action/upload-sarif uses 'token' as the input name, not 'github-token'
129-
c.addUploadSARIFToken(&steps, data, data.SafeOutputs.CreateCodeScanningAlerts.GitHubToken)
149+
// Pass restoreToken as the fallback so GitHub App-minted tokens flow through consistently.
150+
c.addUploadSARIFToken(&steps, data, data.SafeOutputs.CreateCodeScanningAlerts.GitHubToken, restoreToken)
130151
// sarif_file now references the locally-downloaded artifact, not the path from safe_outputs
131152
steps = append(steps, fmt.Sprintf(" sarif_file: %s\n", localSarifPath))
132153
// ref and sha pin the upload to the exact triggering commit regardless of local git state
@@ -158,9 +179,14 @@ func (c *Compiler) buildCodeScanningUploadJob(data *WorkflowData) (*Job, error)
158179
// addUploadSARIFToken adds the 'token' input for github/codeql-action/upload-sarif.
159180
// This action uses 'token' as the input name (not 'github-token' like other GitHub Actions).
160181
// This runs inside the upload_code_scanning_sarif job (a separate job from safe_outputs), so
161-
// the token must be computed directly in this job from static secret references.
162-
// Uses precedence: config token > safe-outputs global github-token > default fallback
163-
func (c *Compiler) addUploadSARIFToken(steps *[]string, data *WorkflowData, configToken string) {
182+
// the token must be computed directly in this job from static secret references or a freshly
183+
// minted GitHub App token.
184+
//
185+
// Token precedence:
186+
// 1. Per-config github-token (configToken)
187+
// 2. Safe-outputs level github-token
188+
// 3. fallbackToken (either computeStaticCheckoutToken result or a minted app token)
189+
func (c *Compiler) addUploadSARIFToken(steps *[]string, data *WorkflowData, configToken string, fallbackToken string) {
164190
var safeOutputsToken string
165191
if data.SafeOutputs != nil {
166192
safeOutputsToken = data.SafeOutputs.GitHubToken
@@ -185,12 +211,9 @@ func (c *Compiler) addUploadSARIFToken(steps *[]string, data *WorkflowData, conf
185211
return
186212
}
187213

188-
// No per-config or safe-outputs token — compute the static checkout token directly.
189-
// This is the same logic as computeStaticCheckoutToken, which returns a plain static
190-
// secret-reference expression. We cannot pass it through job outputs because GitHub Actions
191-
// masks output values that contain secret references.
192-
checkoutMgr := NewCheckoutManager(data.CheckoutConfigs)
193-
defaultToken := computeStaticCheckoutToken(data.SafeOutputs, checkoutMgr)
194-
createCodeScanningAlertLog.Printf("Using computed static checkout token for SARIF upload token")
195-
*steps = append(*steps, fmt.Sprintf(" token: %s\n", defaultToken))
214+
// No per-config or safe-outputs token — use the fallback token (static secret reference
215+
// or minted GitHub App token) computed by the caller. This avoids the GitHub Actions
216+
// behaviour of masking secret references when they are passed through job outputs.
217+
createCodeScanningAlertLog.Printf("Using fallback token for SARIF upload token")
218+
*steps = append(*steps, fmt.Sprintf(" token: %s\n", fallbackToken))
196219
}

0 commit comments

Comments
 (0)