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 @@ -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
Expand All @@ -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)
Expand Down
39 changes: 38 additions & 1 deletion pkg/workflow/dangerous_permissions_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
},
{
Expand All @@ -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 {
Expand Down
75 changes: 64 additions & 11 deletions pkg/workflow/strict_mode_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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 {
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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 {
Expand Down
Loading