diff --git a/actions/setup/js/determine_automatic_lockdown.cjs b/actions/setup/js/determine_automatic_lockdown.cjs index 33bdadb729..1a823331bd 100644 --- a/actions/setup/js/determine_automatic_lockdown.cjs +++ b/actions/setup/js/determine_automatic_lockdown.cjs @@ -15,6 +15,9 @@ * For private repositories, lockdown mode is not necessary (false) as there is no risk * of exposing private repository access. * + * Note: This step is NOT generated when tools.github.app is configured. GitHub App tokens + * are already scoped to specific repositories, so automatic lockdown detection is unnecessary. + * * @param {any} github - GitHub API client * @param {any} context - GitHub context * @param {any} core - GitHub Actions core library diff --git a/pkg/workflow/github_mcp_app_token_test.go b/pkg/workflow/github_mcp_app_token_test.go index c368aa71bd..191dbe8fd8 100644 --- a/pkg/workflow/github_mcp_app_token_test.go +++ b/pkg/workflow/github_mcp_app_token_test.go @@ -253,3 +253,55 @@ Test org-wide GitHub MCP app token. assert.Contains(t, lockContent, "owner:", "Should include owner field") assert.Contains(t, lockContent, "app-id:", "Should include app-id field") } + +// TestGitHubMCPAppTokenNoLockdownDetectionStep tests that determine-automatic-lockdown +// step is NOT generated when a GitHub App is configured. +// GitHub App tokens are already scoped to specific repositories, so automatic lockdown +// detection is unnecessary. +func TestGitHubMCPAppTokenNoLockdownDetectionStep(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + markdown := `--- +on: issues +permissions: + contents: read + issues: read +strict: false +tools: + github: + mode: local + app: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + repositories: + - "repo1" + - "repo2" +--- + +# Test Workflow + +Test that determine-automatic-lockdown is not generated when app is configured. +` + + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "test.md") + err := os.WriteFile(testFile, []byte(markdown), 0644) + require.NoError(t, err, "Failed to write test file") + + err = compiler.CompileWorkflow(testFile) + require.NoError(t, err, "Failed to compile workflow") + + lockFile := strings.TrimSuffix(testFile, ".md") + ".lock.yml" + content, err := os.ReadFile(lockFile) + require.NoError(t, err, "Failed to read lock file") + lockContent := string(content) + + // The automatic lockdown detection step must NOT be present when app is configured + assert.NotContains(t, lockContent, "Determine automatic lockdown mode", "determine-automatic-lockdown step should not be generated when app is configured") + assert.NotContains(t, lockContent, "id: determine-automatic-lockdown", "determine-automatic-lockdown step ID should not be present") + assert.NotContains(t, lockContent, "steps.determine-automatic-lockdown.outputs.lockdown", "lockdown step output reference should not be present") + + // App token should still be minted and used + assert.Contains(t, lockContent, "id: github-mcp-app-token", "GitHub App token step should still be generated") + assert.Contains(t, lockContent, "GITHUB_MCP_SERVER_TOKEN: ${{ steps.github-mcp-app-token.outputs.token }}", "App token should be used for MCP server") +} diff --git a/pkg/workflow/mcp_environment.go b/pkg/workflow/mcp_environment.go index 6244e2e420..16d110bb6c 100644 --- a/pkg/workflow/mcp_environment.go +++ b/pkg/workflow/mcp_environment.go @@ -66,13 +66,10 @@ func collectMCPEnvironmentVariables(tools map[string]any, mcpTools []string, wor githubTool := tools["github"] // Check if GitHub App is configured for token minting - hasGitHubApp := false - if workflowData.ParsedTools != nil && workflowData.ParsedTools.GitHub != nil && workflowData.ParsedTools.GitHub.App != nil { - hasGitHubApp = true - } + appConfigured := hasGitHubApp(githubTool) // If GitHub App is configured, use the app token (overrides other tokens) - if hasGitHubApp { + if appConfigured { mcpEnvironmentLog.Print("Using GitHub App token for GitHub MCP server (overrides custom and default tokens)") envVars["GITHUB_MCP_SERVER_TOKEN"] = "${{ steps.github-mcp-app-token.outputs.token }}" } else { @@ -82,10 +79,12 @@ func collectMCPEnvironmentVariables(tools map[string]any, mcpTools []string, wor envVars["GITHUB_MCP_SERVER_TOKEN"] = effectiveToken } - // Add lockdown value if it's determined from step output + // Add lockdown value if it's determined from step output. + // Skip when a GitHub App is configured — in that case, the determine-automatic-lockdown + // step is not generated, so there is no step output to reference. // Security: Pass step output through environment variable to prevent template injection // Convert "true"/"false" to "1"/"0" at the source to avoid shell conversion in templates - if !hasGitHubLockdownExplicitlySet(githubTool) { + if !hasGitHubLockdownExplicitlySet(githubTool) && !appConfigured { envVars["GITHUB_MCP_LOCKDOWN"] = "${{ steps.determine-automatic-lockdown.outputs.lockdown == 'true' && '1' || '0' }}" } } diff --git a/pkg/workflow/mcp_github_config.go b/pkg/workflow/mcp_github_config.go index 5167cfcdff..1e954ccaa4 100644 --- a/pkg/workflow/mcp_github_config.go +++ b/pkg/workflow/mcp_github_config.go @@ -81,6 +81,15 @@ func hasGitHubTool(parsedTools *Tools) bool { return parsedTools.GitHub != nil } +// hasGitHubApp checks if a GitHub App is configured in the (merged) GitHub tool configuration +func hasGitHubApp(githubTool any) bool { + if toolConfig, ok := githubTool.(map[string]any); ok { + _, exists := toolConfig["app"] + return exists + } + return false +} + // getGitHubType extracts the mode from GitHub tool configuration (local or remote) func getGitHubType(githubTool any) string { if toolConfig, ok := githubTool.(map[string]any); ok { @@ -259,8 +268,10 @@ func getGitHubDockerImageVersion(githubTool any) string { // generateGitHubMCPLockdownDetectionStep generates a step to determine automatic lockdown mode // for GitHub MCP server based on repository visibility and token availability. // This step is added when: -// - GitHub tool is enabled AND -// - lockdown field is not explicitly specified in the workflow configuration +// - GitHub tool is enabled AND +// - lockdown field is not explicitly specified in the workflow configuration AND +// - tools.github.app is NOT configured (GitHub App tokens are already repo-scoped, so +// automatic lockdown detection is unnecessary and skipped) // // Lockdown mode is automatically enabled for public repositories when any custom GitHub token // is configured (GH_AW_GITHUB_TOKEN, GH_AW_GITHUB_MCP_SERVER_TOKEN, or custom github-token). @@ -277,6 +288,15 @@ func (c *Compiler) generateGitHubMCPLockdownDetectionStep(yaml *strings.Builder, return } + // Skip automatic lockdown detection when a GitHub App is configured. + // GitHub App tokens are already scoped to specific repositories, so automatic + // lockdown detection is not needed — the token's access is inherently bounded + // by the app installation and the listed repositories. + if hasGitHubApp(githubTool) { + githubConfigLog.Print("GitHub App configured, skipping automatic lockdown determination (app tokens are already repo-scoped)") + return + } + githubConfigLog.Print("Generating automatic lockdown determination step for GitHub MCP server") // Resolve the latest version of actions/github-script diff --git a/pkg/workflow/mcp_renderer.go b/pkg/workflow/mcp_renderer.go index fa16c16f55..79395b84b5 100644 --- a/pkg/workflow/mcp_renderer.go +++ b/pkg/workflow/mcp_renderer.go @@ -126,9 +126,10 @@ func (r *MCPConfigRendererUnified) RenderGitHubMCP(yaml *strings.Builder, github // Get lockdown value - use detected value if lockdown wasn't explicitly set lockdown := getGitHubLockdown(githubTool) - // Check if automatic lockdown determination step will be generated - // The step is always generated when lockdown is not explicitly set - shouldUseStepOutput := !hasGitHubLockdownExplicitlySet(githubTool) + // Check if automatic lockdown determination step will be generated. + // The step is skipped when lockdown is explicitly set, or when a GitHub App is configured + // (app tokens are already repo-scoped, so automatic lockdown detection is not needed). + shouldUseStepOutput := !hasGitHubLockdownExplicitlySet(githubTool) && !hasGitHubApp(githubTool) if shouldUseStepOutput { // Use the detected lockdown value from the step output diff --git a/pkg/workflow/safe_outputs_app.go b/pkg/workflow/safe_outputs_app.go index 3bebf4a6ab..13981e2aea 100644 --- a/pkg/workflow/safe_outputs_app.go +++ b/pkg/workflow/safe_outputs_app.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "sort" - "strings" "github.com/github/gh-aw/pkg/logger" ) @@ -140,14 +139,23 @@ func (c *Compiler) buildGitHubAppTokenMintStep(app *GitHubAppConfig, permissions // Add repositories - behavior depends on configuration: // - If repositories is ["*"], omit the field to allow org-wide access - // - If repositories is specified with values, use those specific repos + // - If repositories is a single value, use inline format + // - If repositories has multiple values, use block scalar format (newline-separated) + // to ensure clarity and proper parsing by actions/create-github-app-token // - If repositories is empty/not specified, default to current repository if len(app.Repositories) == 1 && app.Repositories[0] == "*" { // Org-wide access: omit repositories field entirely safeOutputsAppLog.Print("Using org-wide GitHub App token (repositories: *)") - } else if len(app.Repositories) > 0 { - reposStr := strings.Join(app.Repositories, ",") - steps = append(steps, fmt.Sprintf(" repositories: %s\n", reposStr)) + } else if len(app.Repositories) == 1 { + // Single repository: use inline format for clarity + steps = append(steps, fmt.Sprintf(" repositories: %s\n", app.Repositories[0])) + } else if len(app.Repositories) > 1 { + // Multiple repositories: use block scalar format (newline-separated) + // This format is more readable and avoids potential issues with comma-separated parsing + steps = append(steps, " repositories: |-\n") + for _, repo := range app.Repositories { + steps = append(steps, fmt.Sprintf(" %s\n", repo)) + } } else { // Extract repo name from github.repository (which is "owner/repo") // Using GitHub Actions expression: split(github.repository, '/')[1] diff --git a/pkg/workflow/safe_outputs_app_test.go b/pkg/workflow/safe_outputs_app_test.go index cb481a442b..ef79165c9d 100644 --- a/pkg/workflow/safe_outputs_app_test.go +++ b/pkg/workflow/safe_outputs_app_test.go @@ -177,8 +177,10 @@ Test workflow with app token minting and repository restrictions. // Convert steps to string for easier assertion stepsStr := strings.Join(job.Steps, "") - // Verify repositories are included in the minting step - assert.Contains(t, stepsStr, "repositories: repo1,repo2", "Should include repositories") + // Verify repositories are included in the minting step using block scalar format + assert.Contains(t, stepsStr, "repositories: |-", "Should use block scalar format for multiple repositories") + assert.Contains(t, stepsStr, "repo1", "Should include first repository") + assert.Contains(t, stepsStr, "repo2", "Should include second repository") } // TestSafeOutputsAppWithoutSafeOutputs tests that app without safe outputs doesn't break