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
6 changes: 6 additions & 0 deletions pkg/workflow/dangerous_permissions_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func validateDangerousPermissions(workflowData *WorkflowData) error {

// findWritePermissions returns a list of permission scopes that have write access
// Excludes id-token since it's safe (used for OIDC authentication) and doesn't modify repository content
// Excludes metadata since it's a built-in read-only permission
func findWritePermissions(permissions *Permissions) []PermissionScope {
if permissions == nil {
return nil
Expand All @@ -70,6 +71,11 @@ func findWritePermissions(permissions *Permissions) []PermissionScope {
continue
}

// Skip metadata as it's a built-in read-only permission
if scope == PermissionMetadata {
continue
}

level, exists := permissions.Get(scope)
if exists && level == PermissionWrite {
writePerms = append(writePerms, scope)
Expand Down
4 changes: 4 additions & 0 deletions pkg/workflow/permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ func convertStringToPermissionScope(key string) PermissionScope {
return PermissionIdToken
case "issues":
return PermissionIssues
case "metadata":
return PermissionMetadata
case "models":
return PermissionModels
case "packages":
Expand Down Expand Up @@ -93,6 +95,7 @@ const (
PermissionDiscussions PermissionScope = "discussions"
PermissionIdToken PermissionScope = "id-token"
PermissionIssues PermissionScope = "issues"
PermissionMetadata PermissionScope = "metadata"
PermissionModels PermissionScope = "models"
PermissionPackages PermissionScope = "packages"
PermissionPages PermissionScope = "pages"
Expand All @@ -114,6 +117,7 @@ func GetAllPermissionScopes() []PermissionScope {
PermissionDiscussions,
PermissionIdToken,
PermissionIssues,
PermissionMetadata,
PermissionModels,
PermissionPackages,
PermissionPages,
Expand Down
5 changes: 5 additions & 0 deletions pkg/workflow/permissions_operations.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ func (p *Permissions) RenderToYAML() string {
continue
}

// Skip metadata - it's a built-in permission that is always available with read access
if scope == PermissionMetadata {
continue
}

// Add 2 spaces for proper indentation under permissions:
// When rendered in a job, the job renderer adds 4 spaces to the first line only,
// so we need to pre-indent continuation lines with 4 additional spaces
Expand Down
4 changes: 4 additions & 0 deletions pkg/workflow/permissions_operations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ func TestPermissionsMerge(t *testing.T) {
PermissionDeployments: PermissionRead,
PermissionDiscussions: PermissionRead,
PermissionIssues: PermissionRead,
PermissionMetadata: PermissionRead,
PermissionPackages: PermissionRead,
PermissionPages: PermissionRead,
PermissionPullRequests: PermissionRead,
Expand All @@ -381,6 +382,7 @@ func TestPermissionsMerge(t *testing.T) {
PermissionDiscussions: PermissionWrite,
PermissionIdToken: PermissionWrite, // id-token supports write
PermissionIssues: PermissionWrite,
PermissionMetadata: PermissionWrite,
PermissionPackages: PermissionWrite,
PermissionPages: PermissionWrite,
PermissionPullRequests: PermissionWrite,
Expand All @@ -403,6 +405,7 @@ func TestPermissionsMerge(t *testing.T) {
PermissionDeployments: PermissionRead,
PermissionDiscussions: PermissionRead,
PermissionIssues: PermissionRead,
PermissionMetadata: PermissionRead,
PermissionPackages: PermissionRead,
PermissionPages: PermissionRead,
PermissionPullRequests: PermissionRead,
Expand All @@ -427,6 +430,7 @@ func TestPermissionsMerge(t *testing.T) {
PermissionDeployments: PermissionWrite,
PermissionDiscussions: PermissionWrite,
PermissionIdToken: PermissionWrite, // id-token supports write
PermissionMetadata: PermissionWrite,
PermissionPackages: PermissionWrite,
PermissionPages: PermissionWrite,
PermissionPullRequests: PermissionWrite,
Expand Down
76 changes: 76 additions & 0 deletions pkg/workflow/permissions_rendering_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -260,3 +260,79 @@ func TestPermissions_AllReadRenderToYAML(t *testing.T) {
})
}
}

func TestPermissions_MetadataExcluded(t *testing.T) {
tests := []struct {
name string
perms *Permissions
contains []string
notContains []string
}{
{
name: "metadata permission should be excluded from YAML output",
perms: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{
PermissionContents: PermissionRead,
PermissionMetadata: PermissionRead,
PermissionIssues: PermissionWrite,
}),
contains: []string{
"permissions:",
" contents: read",
" issues: write",
},
notContains: []string{
"metadata",
},
},
{
name: "all: read should expand without metadata",
perms: NewPermissionsAllRead(),
contains: []string{
"permissions:",
" contents: read",
" issues: read",
},
notContains: []string{
"metadata",
},
},
{
name: "metadata: write should also be excluded",
perms: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{
PermissionContents: PermissionRead,
PermissionMetadata: PermissionWrite,
}),
contains: []string{
"permissions:",
" contents: read",
},
notContains: []string{
"metadata",
},
},
{
name: "only metadata permission should render empty permissions",
perms: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{
PermissionMetadata: PermissionRead,
}),
contains: []string{},
notContains: []string{"metadata"},
},
Comment on lines +313 to +320
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

This test case has insufficient validation. The test name says "should render empty permissions" but the test only checks that "metadata" is not present - it doesn't verify that the result is actually an empty string. The test should check that the result equals "" to catch the bug where only filtered permissions would result in "permissions:" being rendered (which is invalid YAML). Consider using a structure similar to TestPermissionsRenderToYAML that explicitly checks the expected output value.

Copilot uses AI. Check for mistakes.
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := tt.perms.RenderToYAML()
for _, expected := range tt.contains {
if !strings.Contains(result, expected) {
t.Errorf("RenderToYAML() should contain %q, but got:\n%s", expected, result)
}
}
for _, notExpected := range tt.notContains {
if strings.Contains(result, notExpected) {
t.Errorf("RenderToYAML() should NOT contain %q, but got:\n%s", notExpected, result)
}
}
})
}
}
Loading