Skip to content

Commit 4ba1c79

Browse files
Copilotpelikhan
andauthored
fix: filter GitHub App-only permissions from job-level permissions block in lock file
vulnerability-alerts and other GitHub App-only scopes are not valid GitHub Actions workflow permissions. When declared in the frontmatter, they were incorrectly copied verbatim into the compiled job-level permissions: block, causing GitHub Actions to reject the workflow with a parse error at queue time. Fix: add filterJobLevelPermissions() that routes the raw permissions YAML through the Permissions struct before job-level rendering. RenderToYAML() already skips App-only scopes (lines 278-283 of permissions_operations.go), so the fix is simply ensuring the raw string goes through that path before being used in buildMainJob. The permission-vulnerability-alerts: read entry in create-github-app-token step inputs remains correct and unchanged." Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7798db99-bfd8-4247-bf93-e15d02594865
1 parent f3b623f commit 4ba1c79

4 files changed

Lines changed: 171 additions & 1 deletion

File tree

pkg/workflow/compiler_main_job.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,12 @@ func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) (
251251
// Set up permissions for the agent job
252252
// In dev/script mode, automatically add contents: read if the actions folder checkout is needed
253253
// In release mode, use the permissions as specified by the user (no automatic augmentation)
254-
permissions := data.Permissions
254+
//
255+
// GitHub App-only permissions (e.g., vulnerability-alerts) must be filtered out before
256+
// rendering to the job-level permissions block. These scopes are not valid GitHub Actions
257+
// workflow permissions and cause a parse error when queued. They are handled separately
258+
// when minting GitHub App installation access tokens (as permission-* inputs).
259+
permissions := filterJobLevelPermissions(data.Permissions)
255260
needsContentsRead := (c.actionMode.IsDev() || c.actionMode.IsScript()) && len(c.generateCheckoutActionsFolder(data)) > 0
256261
if needsContentsRead {
257262
if permissions == "" {

pkg/workflow/github_mcp_app_token_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"strings"
99
"testing"
1010

11+
goyaml "github.com/goccy/go-yaml"
1112
"github.com/stretchr/testify/assert"
1213
"github.com/stretchr/testify/require"
1314
)
@@ -359,4 +360,28 @@ Test that permission-vulnerability-alerts is emitted in the App token minting st
359360
assert.Contains(t, lockContent, "permission-security-events: read", "Should also include security-events read permission in App token")
360361
// Verify the token minting step is present
361362
assert.Contains(t, lockContent, "id: github-mcp-app-token", "GitHub App token step should be generated")
363+
// Verify that vulnerability-alerts does NOT appear in any job-level permissions block.
364+
// It is a GitHub App-only permission and not a valid GitHub Actions workflow permission;
365+
// GitHub Actions rejects workflows that declare it at the job level.
366+
var workflow map[string]any
367+
require.NoError(t, goyaml.Unmarshal(content, &workflow), "Lock file should be valid YAML")
368+
jobs, ok := workflow["jobs"].(map[string]any)
369+
require.True(t, ok, "Should have jobs section")
370+
for jobName, jobConfig := range jobs {
371+
jobMap, ok := jobConfig.(map[string]any)
372+
if !ok {
373+
continue
374+
}
375+
perms, hasPerms := jobMap["permissions"]
376+
if !hasPerms {
377+
continue
378+
}
379+
permsMap, ok := perms.(map[string]any)
380+
if !ok {
381+
continue
382+
}
383+
if _, found := permsMap["vulnerability-alerts"]; found {
384+
t.Errorf("Job %q should not have vulnerability-alerts in job-level permissions block (it is a GitHub App-only permission)", jobName)
385+
}
386+
}
362387
}

pkg/workflow/permissions_operations.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,52 @@ func SortPermissionScopes(s []PermissionScope) {
1818
})
1919
}
2020

21+
// filterJobLevelPermissions takes a raw permissions YAML string (as stored in WorkflowData.Permissions)
22+
// and returns a version suitable for use in a GitHub Actions job-level permissions block.
23+
//
24+
// GitHub App-only permission scopes (e.g., vulnerability-alerts, members, administration) are not
25+
// valid GitHub Actions workflow permissions and cause a parse error when GitHub Actions tries to
26+
// queue the workflow. Those scopes must only appear as permission-* inputs when minting GitHub App
27+
// installation access tokens via actions/create-github-app-token, not in the job-level block.
28+
//
29+
// RenderToYAML already skips App-only scopes; this function converts the raw YAML string through
30+
// the Permissions struct so that filtering is applied before job-level rendering.
31+
// The returned string uses 2-space indentation so that the caller's subsequent
32+
// indentYAMLLines(" ") call adds 4 spaces, producing the correct 6-space job-level
33+
// indentation in the final YAML (matching the renderJob format).
34+
//
35+
// If the input YAML is malformed or contains only App-only scopes, an empty string is returned
36+
// so the caller omits the permissions block entirely rather than emitting invalid YAML.
37+
func filterJobLevelPermissions(rawPermissionsYAML string) string {
38+
if rawPermissionsYAML == "" {
39+
return ""
40+
}
41+
42+
filtered := NewPermissionsParser(rawPermissionsYAML).ToPermissions()
43+
rendered := filtered.RenderToYAML()
44+
if rendered == "" {
45+
return ""
46+
}
47+
48+
// RenderToYAML hard-codes 6-space indentation for permission values so that shorthand
49+
// callers that embed the output directly into a job block get the right alignment:
50+
// permissions: ← first line, 4 spaces added by renderJob's fmt.Fprintf
51+
// contents: read ← 6 spaces from RenderToYAML → total 10 would be wrong
52+
// Here we normalise back to 2-space indentation. The caller will then run
53+
// indentYAMLLines(" "), adding 4 spaces to lines 1+, yielding 6 spaces total.
54+
const renderYAMLIndent = 6 // spaces used by RenderToYAML for permission value lines
55+
const targetIndent = 2 // spaces we want here so indentYAMLLines(" ") gives 6
56+
prefix := strings.Repeat(" ", renderYAMLIndent)
57+
replacement := strings.Repeat(" ", targetIndent)
58+
lines := strings.Split(rendered, "\n")
59+
for i := 1; i < len(lines); i++ {
60+
if strings.HasPrefix(lines[i], prefix) {
61+
lines[i] = replacement + lines[i][renderYAMLIndent:]
62+
}
63+
}
64+
return strings.Join(lines, "\n")
65+
}
66+
2167
// Set sets a permission for a specific scope
2268
func (p *Permissions) Set(scope PermissionScope, level PermissionLevel) {
2369
permissionsOpsLog.Printf("Setting permission: scope=%s, level=%s", scope, level)

pkg/workflow/permissions_operations_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package workflow
44

55
import (
6+
"strings"
67
"testing"
78
)
89

@@ -602,3 +603,96 @@ func TestPermissions_AllRead(t *testing.T) {
602603
})
603604
}
604605
}
606+
607+
func TestFilterJobLevelPermissions(t *testing.T) {
608+
tests := []struct {
609+
name string
610+
input string
611+
expectEmpty bool
612+
contains []string
613+
excludes []string
614+
}{
615+
{
616+
name: "empty input returns empty",
617+
input: "",
618+
expectEmpty: true,
619+
contains: []string{},
620+
excludes: []string{},
621+
},
622+
{
623+
name: "standard permissions are preserved",
624+
input: "permissions:\n contents: read\n issues: write",
625+
contains: []string{
626+
"permissions:",
627+
" contents: read",
628+
" issues: write",
629+
},
630+
excludes: []string{},
631+
},
632+
{
633+
name: "vulnerability-alerts is filtered out",
634+
input: "permissions:\n contents: read\n pull-requests: read\n security-events: read\n vulnerability-alerts: read",
635+
contains: []string{
636+
"permissions:",
637+
" contents: read",
638+
" pull-requests: read",
639+
" security-events: read",
640+
},
641+
excludes: []string{"vulnerability-alerts"},
642+
},
643+
{
644+
name: "multiple GitHub App-only scopes are filtered out",
645+
input: "permissions:\n contents: read\n issues: write\n administration: read\n members: read\n vulnerability-alerts: read",
646+
contains: []string{
647+
"permissions:",
648+
" contents: read",
649+
" issues: write",
650+
},
651+
excludes: []string{"administration", "members", "vulnerability-alerts"},
652+
},
653+
{
654+
name: "only GitHub App-only scopes returns empty string",
655+
input: "permissions:\n vulnerability-alerts: read\n members: read",
656+
expectEmpty: true,
657+
contains: []string{},
658+
excludes: []string{"vulnerability-alerts", "members"},
659+
},
660+
{
661+
name: "shorthand read-all is preserved unchanged",
662+
input: "permissions: read-all",
663+
contains: []string{"permissions: read-all"},
664+
excludes: []string{},
665+
},
666+
{
667+
name: "shorthand write-all is preserved unchanged",
668+
input: "permissions: write-all",
669+
contains: []string{"permissions: write-all"},
670+
excludes: []string{},
671+
},
672+
{
673+
name: "shorthand none is preserved unchanged",
674+
input: "permissions: none",
675+
contains: []string{"permissions: none"},
676+
excludes: []string{},
677+
},
678+
}
679+
680+
for _, tt := range tests {
681+
t.Run(tt.name, func(t *testing.T) {
682+
result := filterJobLevelPermissions(tt.input)
683+
if tt.expectEmpty && result != "" {
684+
t.Errorf("filterJobLevelPermissions() should return empty string, but got:\n%q", result)
685+
}
686+
for _, expected := range tt.contains {
687+
if !strings.Contains(result, expected) {
688+
t.Errorf("filterJobLevelPermissions() should contain %q, but got:\n%q", expected, result)
689+
}
690+
}
691+
for _, excluded := range tt.excludes {
692+
if strings.Contains(result, excluded) {
693+
t.Errorf("filterJobLevelPermissions() should NOT contain %q, but got:\n%q", excluded, result)
694+
}
695+
}
696+
})
697+
}
698+
}

0 commit comments

Comments
 (0)