Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions actions/setup/js/determine_automatic_lockdown.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
52 changes: 52 additions & 0 deletions pkg/workflow/github_mcp_app_token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
13 changes: 6 additions & 7 deletions pkg/workflow/mcp_environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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' }}"
}
}
Expand Down
24 changes: 22 additions & 2 deletions pkg/workflow/mcp_github_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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).
Expand All @@ -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
Expand Down
7 changes: 4 additions & 3 deletions pkg/workflow/mcp_renderer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 13 additions & 5 deletions pkg/workflow/safe_outputs_app.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"encoding/json"
"fmt"
"sort"
"strings"

"github.com/github/gh-aw/pkg/logger"
)
Expand Down Expand Up @@ -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]
Expand Down
6 changes: 4 additions & 2 deletions pkg/workflow/safe_outputs_app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading