diff --git a/pkg/workflow/dangerous_permissions_validation.go b/pkg/workflow/dangerous_permissions_validation.go index e37d067014..7e95bd1edb 100644 --- a/pkg/workflow/dangerous_permissions_validation.go +++ b/pkg/workflow/dangerous_permissions_validation.go @@ -55,6 +55,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 func findWritePermissions(permissions *Permissions) []PermissionScope { if permissions == nil { return nil @@ -64,6 +65,11 @@ func findWritePermissions(permissions *Permissions) []PermissionScope { // Check all permission scopes for _, scope := range GetAllPermissionScopes() { + // Skip id-token as it's safe and used for OIDC authentication + if scope == PermissionIdToken { + continue + } + level, exists := permissions.Get(scope) if exists && level == PermissionWrite { writePerms = append(writePerms, scope) diff --git a/pkg/workflow/dangerous_permissions_validation_test.go b/pkg/workflow/dangerous_permissions_validation_test.go index a2652a43f5..62d967787c 100644 --- a/pkg/workflow/dangerous_permissions_validation_test.go +++ b/pkg/workflow/dangerous_permissions_validation_test.go @@ -85,6 +85,22 @@ func TestValidateDangerousPermissions(t *testing.T) { shouldError: true, errorContains: "issues: write", }, + { + name: "id-token write without feature flag - should pass (id-token is safe)", + permissions: "permissions:\n id-token: write", + shouldError: false, + }, + { + name: "id-token write with other read permissions - should pass", + permissions: "permissions:\n contents: read\n id-token: write\n issues: read", + shouldError: false, + }, + { + name: "id-token write with other write permissions - should error on other permissions", + permissions: "permissions:\n contents: write\n id-token: write", + shouldError: true, + errorContains: "contents: write", + }, } for _, tt := range tests { @@ -147,7 +163,7 @@ func TestFindWritePermissions(t *testing.T) { { name: "write-all shorthand", permissions: NewPermissionsWriteAll(), - expectedWriteCount: 16, // All permission scopes + expectedWriteCount: 15, // All permission scopes except id-token (which is excluded) expectedScopes: nil, // Don't check specific scopes for shorthand }, { @@ -156,6 +172,27 @@ func TestFindWritePermissions(t *testing.T) { expectedWriteCount: 1, expectedScopes: []PermissionScope{PermissionIssues}, }, + { + name: "id-token write is excluded from dangerous permissions", + permissions: func() *Permissions { + p := NewPermissions() + p.Set(PermissionIdToken, PermissionWrite) + return p + }(), + expectedWriteCount: 0, // id-token should be excluded + expectedScopes: []PermissionScope{}, + }, + { + name: "id-token write with other write permissions - only other permissions counted", + permissions: func() *Permissions { + p := NewPermissions() + p.Set(PermissionIdToken, PermissionWrite) + p.Set(PermissionContents, PermissionWrite) + return p + }(), + expectedWriteCount: 1, // Only contents, not id-token + expectedScopes: []PermissionScope{PermissionContents}, + }, } for _, tt := range tests { diff --git a/pkg/workflow/strict_mode_validation_test.go b/pkg/workflow/strict_mode_validation_test.go index 30da6cef8a..7504d61886 100644 --- a/pkg/workflow/strict_mode_validation_test.go +++ b/pkg/workflow/strict_mode_validation_test.go @@ -110,15 +110,6 @@ func TestValidateStrictPermissions(t *testing.T) { }, expectError: false, }, - { - name: "shorthand write is refused", - frontmatter: map[string]any{ - "on": "push", - "permissions": "write", - }, - expectError: true, - errorMsg: "strict mode: write permission 'contents: write' is not allowed for security reasons. Use 'safe-outputs.create-issue', 'safe-outputs.create-pull-request', 'safe-outputs.add-comment', or 'safe-outputs.update-issue' to perform write operations safely", - }, { name: "shorthand write-all is refused", frontmatter: map[string]any{ @@ -144,6 +135,52 @@ func TestValidateStrictPermissions(t *testing.T) { }, expectError: false, }, + { + name: "id-token write is allowed (safe permission for OIDC)", + frontmatter: map[string]any{ + "on": "push", + "permissions": map[string]any{ + "id-token": "write", + }, + }, + expectError: false, + }, + { + name: "id-token write with read permissions is allowed", + frontmatter: map[string]any{ + "on": "push", + "permissions": map[string]any{ + "contents": "read", + "id-token": "write", + "issues": "read", + }, + }, + expectError: false, + }, + { + name: "id-token write with other safe write permissions is allowed", + frontmatter: map[string]any{ + "on": "push", + "permissions": map[string]any{ + "id-token": "write", + "attestations": "write", + "actions": "write", + }, + }, + expectError: false, + }, + { + name: "id-token write with blocked write permissions fails on blocked permissions", + frontmatter: map[string]any{ + "on": "push", + "permissions": map[string]any{ + "id-token": "write", + "contents": "write", + }, + }, + expectError: true, + errorMsg: "strict mode: write permission 'contents: write' is not allowed for security reasons", + }, } for _, tt := range tests { @@ -353,10 +390,10 @@ func TestValidateStrictMode(t *testing.T) { }, networkPermissions: &NetworkPermissions{ Mode: "custom", - Allowed: []string{"api.example.com"}, + Allowed: []string{}, // Empty allowed list - no top-level network config }, expectError: true, - errorMsg: "strict mode: custom MCP server 'my-server' with container must have network configuration for security", + errorMsg: "strict mode: custom MCP server 'my-server' with container must have top-level network configuration for security", }, { name: "strict mode with container MCP and network config", @@ -421,6 +458,22 @@ func TestValidateStrictMode(t *testing.T) { }, expectError: false, }, + { + name: "strict mode with id-token write is allowed", + strictMode: true, + frontmatter: map[string]any{ + "on": "push", + "permissions": map[string]any{ + "id-token": "write", + "contents": "read", + }, + }, + networkPermissions: &NetworkPermissions{ + Mode: "custom", + Allowed: []string{"api.example.com"}, + }, + expectError: false, + }, } for _, tt := range tests {