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
386 changes: 274 additions & 112 deletions pkg/parser/schemas/main_workflow_schema.json

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions pkg/workflow/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,15 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
return formatCompilerError(markdownPath, "error", err.Error(), err)
}

// Validate tools.github.github-app.permissions does not use "write"
log.Printf("Validating GitHub MCP app permissions (no write)")
if err := validateGitHubMCPAppPermissionsNoWrite(workflowData); err != nil {
return formatCompilerError(markdownPath, "error", err.Error(), err)
}

// Warn when github-app.permissions is set in contexts that don't support it
warnGitHubAppPermissionsUnsupportedContexts(workflowData)

// Validate agent file exists if specified in engine config
log.Printf("Validating agent file if specified")
if err := c.validateAgentFile(workflowData, markdownPath); err != nil {
Expand Down
22 changes: 22 additions & 0 deletions pkg/workflow/compiler_github_mcp_steps.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package workflow

import (
"fmt"
"os"
"strings"

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/constants"
)

Expand Down Expand Up @@ -102,6 +104,26 @@ func (c *Compiler) generateGitHubMCPAppTokenMintingStep(yaml *strings.Builder, d
permissions = NewPermissions()
}

// Apply extra permissions from github-app.permissions (nested wins over job-level)
if len(app.Permissions) > 0 {
githubConfigLog.Printf("Applying %d extra permissions from github-app.permissions", len(app.Permissions))
for key, val := range app.Permissions {
scope := convertStringToPermissionScope(key)
if scope == "" {
msg := fmt.Sprintf("Unknown permission scope %q in tools.github.github-app.permissions. Valid scopes include: members, organization-administration, team-discussions, organization-members, administration, etc.", key)
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg))
continue
}
level := strings.ToLower(strings.TrimSpace(val))
if level != string(PermissionRead) && level != string(PermissionNone) {
msg := fmt.Sprintf("Unknown permission level %q for scope %q in tools.github.github-app.permissions. Valid levels are: read, none.", val, key)
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg))
continue
}
permissions.Set(scope, PermissionLevel(level))
}
}

// Generate the token minting step using the existing helper from safe_outputs_app.go
steps := c.buildGitHubAppTokenMintStep(app, permissions, "")

Expand Down
99 changes: 99 additions & 0 deletions pkg/workflow/github_app_permissions_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ package workflow

import (
"errors"
"fmt"
"os"
"sort"
"strings"

"github.com/github/gh-aw/pkg/console"
)

var githubAppPermissionsLog = newValidationLogger("github_app_permissions")
Expand Down Expand Up @@ -109,6 +113,101 @@ func formatWriteOnAppScopesError(scopes []PermissionScope) error {
return errors.New(strings.Join(lines, "\n"))
}

// validateGitHubMCPAppPermissionsNoWrite validates that every scope in
// tools.github.github-app.permissions is set to "read" or "none" (after trimming/lowercasing).
// The schema allows "write" so that editors can offer it as a completion, but
// the compiler must reject it because GitHub App-only scopes have no write-level
// semantics in this context — write operations must go through safe-outputs.
// Any other unrecognised level is also rejected here.
func validateGitHubMCPAppPermissionsNoWrite(workflowData *WorkflowData) error {
if workflowData.ParsedTools == nil ||
workflowData.ParsedTools.GitHub == nil ||
workflowData.ParsedTools.GitHub.GitHubApp == nil {
return nil
}
app := workflowData.ParsedTools.GitHub.GitHubApp
if len(app.Permissions) == 0 {
return nil
}

var invalidScopes []string
var writeScopes []string
for scope, level := range app.Permissions {
normalized := strings.ToLower(strings.TrimSpace(level))
switch normalized {
case string(PermissionRead), string(PermissionNone):
// valid
default:
invalidScopes = append(invalidScopes, scope+" (level: "+level+")")
if normalized == string(PermissionWrite) {
writeScopes = append(writeScopes, scope)
}
}
}
if len(invalidScopes) == 0 {
return nil
}
sort.Strings(invalidScopes)
sort.Strings(writeScopes)

var lines []string
lines = append(lines, "Invalid permission levels in tools.github.github-app.permissions.")
lines = append(lines, "Each permission level must be exactly \"read\" or \"none\".")
if len(writeScopes) > 0 {
lines = append(lines, "")
lines = append(lines, `"write" is not allowed: write operations must be performed via safe-outputs.`)
lines = append(lines, "")
lines = append(lines, "The following scopes were declared with \"write\" access:")
lines = append(lines, "")
for _, s := range writeScopes {
lines = append(lines, " - "+s)
}
}
lines = append(lines, "")
lines = append(lines, "The following scopes have invalid permission levels:")
lines = append(lines, "")
for _, s := range invalidScopes {
lines = append(lines, " - "+s)
}
lines = append(lines, "")
lines = append(lines, "Change the permission level to \"read\" for read-only access, or \"none\" to disable the scope.")
return errors.New(strings.Join(lines, "\n"))
}

// warnGitHubAppPermissionsUnsupportedContexts emits a warning when
// tools.github.github-app.permissions is set in contexts that do not support it.
// The permissions field only takes effect for the GitHub MCP token minting step;
// it is silently ignored if set on safe-outputs.github-app, on.github-app, or the
// top-level github-app fallback.
func warnGitHubAppPermissionsUnsupportedContexts(workflowData *WorkflowData) {
type context struct {
label string
app *GitHubAppConfig
}
unsupported := []context{
{"safe-outputs.github-app", safeOutputsGitHubApp(workflowData)},
{"on.github-app", workflowData.ActivationGitHubApp},
{"github-app (top-level fallback)", workflowData.TopLevelGitHubApp},
}
for _, ctx := range unsupported {
if ctx.app != nil && len(ctx.app.Permissions) > 0 {
msg := fmt.Sprintf(
"The 'permissions' field under '%s' has no effect. "+
"Extra GitHub App permissions only apply to tools.github.github-app.",
ctx.label,
)
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg))
}
}
}

func safeOutputsGitHubApp(workflowData *WorkflowData) *GitHubAppConfig {
if workflowData.SafeOutputs == nil {
return nil
}
return workflowData.SafeOutputs.GitHubApp
}

func hasGitHubAppConfigured(workflowData *WorkflowData) bool {
// Check tools.github.github-app
if workflowData.ParsedTools != nil &&
Expand Down
144 changes: 144 additions & 0 deletions pkg/workflow/github_mcp_app_token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -385,3 +385,147 @@ Test that permission-vulnerability-alerts is emitted in the App token minting st
}
}
}

// TestGitHubMCPAppTokenWithExtraPermissions tests that extra permissions under
// tools.github.github-app.permissions are merged into the minted token (nested wins).
// This allows org-level permissions (e.g. members: read) that are not valid GitHub
// Actions scopes but are supported by GitHub Apps.
func TestGitHubMCPAppTokenWithExtraPermissions(t *testing.T) {
compiler := NewCompilerWithVersion("1.0.0")

markdown := `---
on: issues
permissions:
contents: read
issues: read
strict: false
tools:
github:
mode: local
toolsets: [orgs, users]
github-app:
app-id: ${{ vars.APP_ID }}
private-key: ${{ secrets.APP_PRIVATE_KEY }}
repositories: ["*"]
permissions:
members: read
organization-administration: read
---

# Test Workflow

Test extra org-level permissions in GitHub App token.
`

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)

// Verify the token minting step is present
assert.Contains(t, lockContent, "id: github-mcp-app-token", "GitHub App token step should be generated")

// Verify that job-level permissions are still included
assert.Contains(t, lockContent, "permission-contents: read", "Should include job-level contents permission")
assert.Contains(t, lockContent, "permission-issues: read", "Should include job-level issues permission")

// Verify that the extra org-level permissions from github-app.permissions are included
assert.Contains(t, lockContent, "permission-members: read", "Should include extra members permission from github-app.permissions")
assert.Contains(t, lockContent, "permission-organization-administration: read", "Should include extra organization-administration permission from github-app.permissions")
}

// TestGitHubMCPAppTokenExtraPermissionsOverrideJobLevel tests that extra permissions
// under tools.github.github-app.permissions can suppress a GitHub App-only scope
// that was set at job level by overriding it with 'none' (nested wins).
func TestGitHubMCPAppTokenExtraPermissionsOverrideJobLevel(t *testing.T) {
compiler := NewCompilerWithVersion("1.0.0")

markdown := `---
on: issues
permissions:
contents: read
issues: read
vulnerability-alerts: read
strict: false
tools:
github:
mode: local
github-app:
app-id: ${{ vars.APP_ID }}
private-key: ${{ secrets.APP_PRIVATE_KEY }}
permissions:
vulnerability-alerts: none
---

# Test Workflow

Test that nested permissions override job-level GitHub App-only scopes (nested wins).
`

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 nested permission (none) should win over the job-level permission (read)
assert.Contains(t, lockContent, "permission-vulnerability-alerts: none", "Nested vulnerability-alerts: none should override job-level: read")
assert.NotContains(t, lockContent, "permission-vulnerability-alerts: read", "Job-level vulnerability-alerts: read should be overridden by nested none")

// Other job-level permissions should still be present
assert.Contains(t, lockContent, "permission-contents: read", "Unaffected job-level contents permission should still be present")
assert.Contains(t, lockContent, "permission-issues: read", "Unaffected job-level issues permission should still be present")
}

// TestGitHubMCPAppTokenExtraPermissionsWriteRejected tests that the compiler
// rejects a workflow where tools.github.github-app.permissions contains a "write"
// value, since write access is not allowed for GitHub App-only scopes in this section.
func TestGitHubMCPAppTokenExtraPermissionsWriteRejected(t *testing.T) {
compiler := NewCompilerWithVersion("1.0.0")

markdown := `---
on: issues
permissions:
contents: read
strict: false
tools:
github:
mode: local
github-app:
app-id: ${{ vars.APP_ID }}
private-key: ${{ secrets.APP_PRIVATE_KEY }}
permissions:
members: write
---

# Test Workflow

Test that write is rejected in tools.github.github-app.permissions.
`

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.Error(t, err, "Compiler should reject write in tools.github.github-app.permissions")
assert.Contains(t, err.Error(), "Invalid permission levels in tools.github.github-app.permissions", "Error should mention invalid permission levels")
assert.Contains(t, err.Error(), `"write" is not allowed`, "Error should mention that write is not allowed")
assert.Contains(t, err.Error(), "members", "Error should mention the offending scope")
}
25 changes: 21 additions & 4 deletions pkg/workflow/safe_outputs_app_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ var safeOutputsAppLog = logger.New("workflow:safe_outputs_app")

// GitHubAppConfig holds configuration for GitHub App-based token minting
type GitHubAppConfig struct {
AppID string `yaml:"app-id,omitempty"` // GitHub App ID (e.g., "${{ vars.APP_ID }}")
PrivateKey string `yaml:"private-key,omitempty"` // GitHub App private key (e.g., "${{ secrets.APP_PRIVATE_KEY }}")
Owner string `yaml:"owner,omitempty"` // Optional: owner of the GitHub App installation (defaults to current repository owner)
Repositories []string `yaml:"repositories,omitempty"` // Optional: comma or newline-separated list of repositories to grant access to
AppID string `yaml:"app-id,omitempty"` // GitHub App ID (e.g., "${{ vars.APP_ID }}")
PrivateKey string `yaml:"private-key,omitempty"` // GitHub App private key (e.g., "${{ secrets.APP_PRIVATE_KEY }}")
Owner string `yaml:"owner,omitempty"` // Optional: owner of the GitHub App installation (defaults to current repository owner)
Repositories []string `yaml:"repositories,omitempty"` // Optional: comma or newline-separated list of repositories to grant access to
Permissions map[string]string `yaml:"permissions,omitempty"` // Optional: extra permission-* fields to merge into the minted token (nested wins over job-level)
}

// ========================================
Expand Down Expand Up @@ -65,6 +66,22 @@ func parseAppConfig(appMap map[string]any) *GitHubAppConfig {
}
}

// Parse permissions (optional) - extra permission-* fields to merge into the minted token
if perms, exists := appMap["permissions"]; exists {
if permsMap, ok := perms.(map[string]any); ok {
appConfig.Permissions = make(map[string]string, len(permsMap))
for key, val := range permsMap {
if valStr, ok := val.(string); ok {
appConfig.Permissions[key] = valStr
} else {
safeOutputsAppLog.Printf("Ignoring github-app.permissions[%q]: expected string value, got %T", key, val)
}
}
Comment on lines +75 to +79
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseAppConfig silently drops non-string entries under github-app.permissions (and also ignores a non-object permissions value) without any warning or error. This can lead to confusing behavior where a user configures permissions but they are simply not applied. Consider logging a warning (including the key/type) or returning a validation error when permissions is present but not a map[string]string.

Suggested change
appConfig.Permissions[key] = valStr
}
}
appConfig.Permissions[key] = valStr
} else {
safeOutputsAppLog.Printf("Ignoring github-app.permissions[%q]: expected string value, got %T", key, val)
}
}
} else {
safeOutputsAppLog.Printf("Ignoring github-app.permissions: expected map[string]string-compatible object, got %T", perms)

Copilot uses AI. Check for mistakes.
} else {
safeOutputsAppLog.Printf("Ignoring github-app.permissions: expected object, got %T", perms)
}
}

return appConfig
}

Expand Down
Loading