Exclude metadata permission from generated workflow YAML#13757
Exclude metadata permission from generated workflow YAML#13757
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Investigate https://github.com/github/gh-aw/actions/runs/21681755294/job/62517904763?pr=13757 and fix test |
|
@copilot Investigate https://github.com/github/gh-aw/actions/runs/21681755294/job/62517904763?pr=13757 and fix test |
The metadata permission is a built-in read-only permission and should not be considered a dangerous permission. Updated findWritePermissions to skip metadata (similar to how it skips id-token). Fixes test failure in TestFindWritePermissions/write-all_shorthand. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in f91bc60. The test was failing because the dangerous permissions validation wasn't excluding the new
Fixed in f91bc60. The test failure was in |
There was a problem hiding this comment.
Pull request overview
This pull request adds the metadata permission scope to the permissions system and ensures it's excluded from generated YAML output, as it's a built-in GitHub Actions permission that should not be explicitly declared.
Changes:
- Added
PermissionMetadataconstant and integrated it into the permission scope system (parsing, enumeration, merging) - Modified YAML rendering to skip metadata permission (similar to organization-projects)
- Updated dangerous permissions validation to exclude metadata from write permission checks
- Added comprehensive tests for metadata exclusion and updated merge tests to include metadata in expected results
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/permissions.go | Added PermissionMetadata constant, updated convertStringToPermissionScope() and GetAllPermissionScopes() to include metadata |
| pkg/workflow/permissions_operations.go | Added logic to skip metadata during YAML rendering in RenderToYAML() |
| pkg/workflow/dangerous_permissions_validation.go | Updated findWritePermissions() to exclude metadata from dangerous permission checks |
| pkg/workflow/permissions_rendering_test.go | Added TestPermissions_MetadataExcluded with 4 test cases to verify metadata exclusion |
| pkg/workflow/permissions_operations_test.go | Updated merge tests to include metadata in expected permission maps |
Comments suppressed due to low confidence (1)
pkg/workflow/permissions_operations.go:262
- The RenderToYAML function has a logic bug when only filtered permissions (metadata or organization-projects) are present. The empty check on line 224 happens before filtering, so if allPerms contains only metadata and/or organization-projects, it will pass the empty check but then all entries will be filtered out in the loop (lines 241-259), resulting in just "permissions:" being rendered with no actual permission entries. This produces invalid YAML.
The fix should check if any permissions remain after filtering. One approach is to count the actual lines added, and if only the "permissions:" header was added, return an empty string instead.
var lines []string
lines = append(lines, "permissions:")
for _, scopeStr := range scopes {
scope := PermissionScope(scopeStr)
level := allPerms[scope]
// Skip organization-projects - it's only valid for GitHub App tokens, not workflow permissions
if scope == PermissionOrganizationProj {
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
// to get 6 total spaces (4 from job + 2 for being under permissions)
lines = append(lines, fmt.Sprintf(" %s: %s", scope, level))
}
return strings.Join(lines, "\n")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| name: "only metadata permission should render empty permissions", | ||
| perms: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ | ||
| PermissionMetadata: PermissionRead, | ||
| }), | ||
| contains: []string{}, | ||
| notContains: []string{"metadata"}, | ||
| }, |
There was a problem hiding this comment.
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.
Implementation: Exclude
metadata: readfrom Generated PermissionsBackground: The
metadata: readpermission is a built-in GitHub Actions permission that is always available and should not be explicitly declared in workflow YAML files.Changes Made:
PermissionMetadataconstant to permission scopes inpkg/workflow/permissions.goGetAllPermissionScopes()to include the new metadata scopeconvertStringToPermissionScope()to handle "metadata" stringRenderToYAML()inpermissions_operations.goto skip metadata (similar to organization-projects)Technical Details:
The implementation adds
metadataas a valid permission scope but ensures it's never rendered in the final YAML output. This is achieved by:PermissionMetadatato the list of permission scopes (similar to existing scopes likePermissionContents,PermissionIssues, etc.)RenderToYAML()methodTest Coverage:
Added
TestPermissions_MetadataExcludedwith 4 test cases:All existing tests updated and passing, including dangerous permissions tests.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.