diff --git a/docs/src/content/docs/reference/permissions.md b/docs/src/content/docs/reference/permissions.md index 56519916a5c..2c19b558e96 100644 --- a/docs/src/content/docs/reference/permissions.md +++ b/docs/src/content/docs/reference/permissions.md @@ -97,6 +97,70 @@ Write operations use safe outputs instead of direct API access. This provides co Run `gh aw compile workflow.md` to validate permissions. Common errors include undefined permissions, direct write permissions in the main job (use safe outputs instead), and insufficient permissions for declared tools. Use `--strict` mode to enforce read-only permissions and require explicit network configuration. +### Write Permission Policy + +Write permissions are blocked by default to enforce the security-first design. Workflows with write permissions will fail compilation with an error: + +``` +Write permissions are not allowed unless the 'dangerous-permissions-write' feature flag is enabled. + +Found write permissions: + - contents: write + +To fix this issue, you have two options: + +Option 1: Change write permissions to read: +permissions: + contents: read + +Option 2: Enable the feature flag (use with caution): +features: + dangerous-permissions-write: true +``` + +#### Migrating Existing Workflows + +To migrate workflows with write permissions, you can: + +1. **Use the automated codemod** (recommended): + ```bash + # Check what would be changed (dry-run) + gh aw fix workflow.md + + # Apply the fix + gh aw fix workflow.md --write + ``` + + This automatically converts all write permissions to read permissions. + +2. **Enable the feature flag** (for workflows that genuinely need write access): + ```yaml + features: + dangerous-permissions-write: true + permissions: + contents: write + ``` + + :::danger[Use with extreme caution] + The `dangerous-permissions-write` feature flag bypasses the security-first design. Only use it when: + - You have a specific use case that requires direct write access + - You fully understand the security implications + - You have reviewed and approved the AI's access to repository writes + + In most cases, use [safe outputs](/gh-aw/reference/safe-outputs/) instead. + ::: + +#### Scope + +This validation applies to: +- Top-level workflow `permissions:` configuration + +This validation does **not** apply to: +- Custom jobs (defined in `jobs:` section) +- Safe outputs jobs (defined in `safe-outputs.job:` section) + +Custom jobs and safe outputs jobs can have their own permission requirements based on their specific needs. + ### Tool-Specific Requirements Some tools require specific permissions to function: diff --git a/pkg/cli/compile_security_benchmark_test.go b/pkg/cli/compile_security_benchmark_test.go index 1556df9e231..b7b3b2a532e 100644 --- a/pkg/cli/compile_security_benchmark_test.go +++ b/pkg/cli/compile_security_benchmark_test.go @@ -26,6 +26,8 @@ permissions: contents: read pull-requests: write engine: copilot +features: + dangerous-permissions-write: true strict: false tools: github: @@ -76,6 +78,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true strict: false tools: github: @@ -243,6 +247,8 @@ permissions: contents: read pull-requests: write engine: copilot +features: + dangerous-permissions-write: true strict: false tools: github: @@ -294,6 +300,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true strict: false tools: github: diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index cbc273fb78e..7584e7dfd3b 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -46,6 +46,7 @@ func GetAllCodemods() []Codemod { getCommandToSlashCommandCodemod(), getSafeInputsModeCodemod(), getUploadAssetsCodemod(), + getWritePermissionsCodemod(), getAgentTaskToAgentSessionCodemod(), } } @@ -589,6 +590,133 @@ func getUploadAssetsCodemod() Codemod { } } +// getWritePermissionsCodemod creates a codemod for converting write permissions to read +func getWritePermissionsCodemod() Codemod { + return Codemod{ + ID: "write-permissions-to-read-migration", + Name: "Convert write permissions to read", + Description: "Converts all write permissions to read permissions to comply with the new security policy", + IntroducedIn: "0.4.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + // Check if permissions exist + permissionsValue, hasPermissions := frontmatter["permissions"] + if !hasPermissions { + return content, false, nil + } + + // Check if any write permissions exist + hasWritePermissions := false + + // Handle string shorthand (write-all, write) + if strValue, ok := permissionsValue.(string); ok { + if strValue == "write-all" || strValue == "write" { + hasWritePermissions = true + } + } + + // Handle map format + if mapValue, ok := permissionsValue.(map[string]any); ok { + for _, value := range mapValue { + if strValue, ok := value.(string); ok && strValue == "write" { + hasWritePermissions = true + break + } + } + } + + if !hasWritePermissions { + return content, false, nil + } + + // Parse frontmatter to get raw lines + result, err := parser.ExtractFrontmatterFromContent(content) + if err != nil { + return content, false, fmt.Errorf("failed to parse frontmatter: %w", err) + } + + // Find and replace write permissions + var modified bool + var inPermissionsBlock bool + var permissionsIndent string + + frontmatterLines := make([]string, len(result.FrontmatterLines)) + + for i, line := range result.FrontmatterLines { + trimmedLine := strings.TrimSpace(line) + + // Track if we're in the permissions block + if strings.HasPrefix(trimmedLine, "permissions:") { + inPermissionsBlock = true + permissionsIndent = line[:len(line)-len(strings.TrimLeft(line, " \t"))] + + // Handle shorthand on same line: "permissions: write-all" or "permissions: write" + if strings.Contains(trimmedLine, ": write-all") { + frontmatterLines[i] = strings.Replace(line, ": write-all", ": read-all", 1) + modified = true + codemodsLog.Printf("Replaced permissions: write-all with permissions: read-all on line %d", i+1) + continue + } else if strings.Contains(trimmedLine, ": write") && !strings.Contains(trimmedLine, "write-all") { + frontmatterLines[i] = strings.Replace(line, ": write", ": read", 1) + modified = true + codemodsLog.Printf("Replaced permissions: write with permissions: read on line %d", i+1) + continue + } + + frontmatterLines[i] = line + continue + } + + // Check if we've left the permissions block (new top-level key with same or less indentation) + if inPermissionsBlock && len(trimmedLine) > 0 && !strings.HasPrefix(trimmedLine, "#") { + currentIndent := line[:len(line)-len(strings.TrimLeft(line, " \t"))] + if len(currentIndent) <= len(permissionsIndent) && strings.Contains(line, ":") { + inPermissionsBlock = false + } + } + + // Replace write with read if in permissions block + if inPermissionsBlock && strings.Contains(trimmedLine, ": write") { + // Preserve indentation and everything else + // Extract the key, value, and any trailing comment + parts := strings.SplitN(line, ":", 2) + if len(parts) >= 2 { + key := parts[0] + valueAndComment := parts[1] + + // Replace "write" with "read" in the value part + newValueAndComment := strings.Replace(valueAndComment, " write", " read", 1) + frontmatterLines[i] = fmt.Sprintf("%s:%s", key, newValueAndComment) + modified = true + codemodsLog.Printf("Replaced write with read on line %d", i+1) + } else { + frontmatterLines[i] = line + } + } else { + frontmatterLines[i] = line + } + } + + if !modified { + return content, false, nil + } + + // Reconstruct the content + var lines []string + lines = append(lines, "---") + lines = append(lines, frontmatterLines...) + lines = append(lines, "---") + if result.Markdown != "" { + lines = append(lines, "") + lines = append(lines, result.Markdown) + } + + newContent := strings.Join(lines, "\n") + codemodsLog.Print("Applied write permissions to read migration") + return newContent, true, nil + }, + } +} + // getAgentTaskToAgentSessionCodemod creates a codemod for migrating create-agent-task to create-agent-session func getAgentTaskToAgentSessionCodemod() Codemod { return Codemod{ diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 20f25f0c754..a4913525f7d 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -408,6 +408,8 @@ const ( MCPGatewayFeatureFlag FeatureFlag = "mcp-gateway" // SandboxRuntimeFeatureFlag is the feature flag name for sandbox runtime SandboxRuntimeFeatureFlag FeatureFlag = "sandbox-runtime" + // DangerousPermissionsWriteFeatureFlag is the feature flag name for allowing write permissions + DangerousPermissionsWriteFeatureFlag FeatureFlag = "dangerous-permissions-write" ) // Step IDs for pre-activation job diff --git a/pkg/constants/constants_test.go b/pkg/constants/constants_test.go index 702301a41ec..664b16a2c5b 100644 --- a/pkg/constants/constants_test.go +++ b/pkg/constants/constants_test.go @@ -379,6 +379,7 @@ func TestFeatureFlagConstants(t *testing.T) { {"SafeInputsFeatureFlag", SafeInputsFeatureFlag, "safe-inputs"}, {"MCPGatewayFeatureFlag", MCPGatewayFeatureFlag, "mcp-gateway"}, {"SandboxRuntimeFeatureFlag", SandboxRuntimeFeatureFlag, "sandbox-runtime"}, + {"DangerousPermissionsWriteFeatureFlag", DangerousPermissionsWriteFeatureFlag, "dangerous-permissions-write"}, } for _, tt := range tests { diff --git a/pkg/workflow/action_sha_validation_test.go b/pkg/workflow/action_sha_validation_test.go index 3303b9d8da9..a3305479e6a 100644 --- a/pkg/workflow/action_sha_validation_test.go +++ b/pkg/workflow/action_sha_validation_test.go @@ -84,6 +84,8 @@ on: issues: types: [opened] engine: copilot +features: + dangerous-permissions-write: true permissions: contents: read issues: write diff --git a/pkg/workflow/activation_checkout_test.go b/pkg/workflow/activation_checkout_test.go index b10e1535f20..bf7b7013f59 100644 --- a/pkg/workflow/activation_checkout_test.go +++ b/pkg/workflow/activation_checkout_test.go @@ -27,6 +27,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true strict: false ---`, description: "Activation job should not include checkout step - uses GitHub API instead", @@ -40,6 +42,8 @@ on: permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false ---`, description: "Activation job should not include checkout - uses GitHub API instead", @@ -54,6 +58,8 @@ on: permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false ---`, description: "Activation job with reaction should not include checkout - uses GitHub API instead", diff --git a/pkg/workflow/agentic_output_test.go b/pkg/workflow/agentic_output_test.go index 1a1cf846a1f..d54c5b7a574 100644 --- a/pkg/workflow/agentic_output_test.go +++ b/pkg/workflow/agentic_output_test.go @@ -26,6 +26,8 @@ tools: github: allowed: [list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -125,6 +127,8 @@ tools: github: allowed: [list_issues] engine: codex +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: diff --git a/pkg/workflow/allow_github_references_env_test.go b/pkg/workflow/allow_github_references_env_test.go index 66a314ffb50..a1d252765bb 100644 --- a/pkg/workflow/allow_github_references_env_test.go +++ b/pkg/workflow/allow_github_references_env_test.go @@ -21,6 +21,8 @@ func TestAllowGitHubReferencesEnvVar(t *testing.T) { workflow: `--- on: push engine: copilot +features: + dangerous-permissions-write: true strict: false permissions: contents: read @@ -42,6 +44,8 @@ Test workflow with allowed-github-references. workflow: `--- on: push engine: copilot +features: + dangerous-permissions-write: true strict: false permissions: contents: read @@ -63,6 +67,8 @@ Test workflow with multiple allowed repos. workflow: `--- on: push engine: copilot +features: + dangerous-permissions-write: true strict: false permissions: contents: read @@ -82,6 +88,8 @@ Test workflow without allowed-github-references. workflow: `--- on: push engine: copilot +features: + dangerous-permissions-write: true strict: false permissions: contents: read @@ -103,6 +111,8 @@ Test workflow with special characters in repo names. workflow: `--- on: push engine: copilot +features: + dangerous-permissions-write: true strict: false permissions: contents: read @@ -124,6 +134,8 @@ Test workflow mixing repo keyword with specific repos. workflow: `--- on: push engine: copilot +features: + dangerous-permissions-write: true strict: false permissions: contents: read diff --git a/pkg/workflow/aw_info_tmp_test.go b/pkg/workflow/aw_info_tmp_test.go index cbc327ed817..967712cabc1 100644 --- a/pkg/workflow/aw_info_tmp_test.go +++ b/pkg/workflow/aw_info_tmp_test.go @@ -24,6 +24,8 @@ tools: github: allowed: [list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/checkout_optimization_test.go b/pkg/workflow/checkout_optimization_test.go index 548cfbcfc10..309f82f58c3 100644 --- a/pkg/workflow/checkout_optimization_test.go +++ b/pkg/workflow/checkout_optimization_test.go @@ -44,6 +44,8 @@ tools: github: toolsets: [issues, pull_requests] engine: claude +features: + dangerous-permissions-write: true strict: false ---`, expectedHasCheckout: false, @@ -63,6 +65,8 @@ tools: github: toolsets: [repos, issues, pull_requests] engine: claude +features: + dangerous-permissions-write: true strict: false ---`, expectedHasCheckout: true, @@ -82,6 +86,8 @@ tools: github: toolsets: [repos, issues, pull_requests] engine: claude +features: + dangerous-permissions-write: true strict: false ---`, expectedHasCheckout: true, @@ -110,6 +116,8 @@ on: issues: types: [opened] permissions: write-all +features: + dangerous-permissions-write: true tools: github: toolsets: [issues] @@ -140,6 +148,8 @@ tools: github: toolsets: [issues] engine: claude +features: + dangerous-permissions-write: true strict: false ---`, expectedHasCheckout: false, @@ -166,6 +176,8 @@ tools: github: toolsets: [issues] engine: claude +features: + dangerous-permissions-write: true strict: false ---`, expectedHasCheckout: true, @@ -191,6 +203,8 @@ tools: github: toolsets: [issues, pull_requests] engine: claude +features: + dangerous-permissions-write: true strict: false ---`, expectedHasCheckout: false, diff --git a/pkg/workflow/checkout_persist_credentials_test.go b/pkg/workflow/checkout_persist_credentials_test.go index 3604dcadac4..e593977df6b 100644 --- a/pkg/workflow/checkout_persist_credentials_test.go +++ b/pkg/workflow/checkout_persist_credentials_test.go @@ -29,6 +29,8 @@ tools: github: allowed: [list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false ---`, description: "Main job checkout step should include persist-credentials: false", diff --git a/pkg/workflow/compile_outputs_allowed_labels_test.go b/pkg/workflow/compile_outputs_allowed_labels_test.go index adadd60f627..9896f926f7b 100644 --- a/pkg/workflow/compile_outputs_allowed_labels_test.go +++ b/pkg/workflow/compile_outputs_allowed_labels_test.go @@ -27,6 +27,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-issue: @@ -71,6 +73,8 @@ permissions: contents: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-pull-request: @@ -95,6 +99,8 @@ permissions: discussions: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-issue: @@ -209,6 +215,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-issue: @@ -273,6 +281,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-issue: diff --git a/pkg/workflow/compile_outputs_comment_test.go b/pkg/workflow/compile_outputs_comment_test.go index 2cce6e40d43..742ca4edae8 100644 --- a/pkg/workflow/compile_outputs_comment_test.go +++ b/pkg/workflow/compile_outputs_comment_test.go @@ -23,6 +23,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-comment: @@ -70,6 +72,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-comment: @@ -117,6 +121,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-comment: @@ -169,6 +175,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-comment: @@ -229,6 +237,8 @@ tools: github: allowed: [issue_read] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-comment: @@ -322,6 +332,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-comment: diff --git a/pkg/workflow/compile_outputs_issue_test.go b/pkg/workflow/compile_outputs_issue_test.go index 3cba107fa65..a49dc0b2e8f 100644 --- a/pkg/workflow/compile_outputs_issue_test.go +++ b/pkg/workflow/compile_outputs_issue_test.go @@ -21,6 +21,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-issue: @@ -86,6 +88,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -125,6 +129,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-issue: @@ -210,6 +216,8 @@ tools: github: allowed: [list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-issue: diff --git a/pkg/workflow/compile_outputs_label_test.go b/pkg/workflow/compile_outputs_label_test.go index a460eeb38b8..287e092cce2 100644 --- a/pkg/workflow/compile_outputs_label_test.go +++ b/pkg/workflow/compile_outputs_label_test.go @@ -23,6 +23,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -86,6 +88,8 @@ tools: github: allowed: [issue_read] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -191,6 +195,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -269,6 +275,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -351,6 +359,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -408,6 +418,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -475,6 +487,8 @@ permissions: issues: write pull-requests: write engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -522,6 +536,8 @@ tools: github: allowed: [issue_read] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -593,6 +609,8 @@ tools: github: allowed: [issue_read] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -654,6 +672,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: @@ -697,6 +717,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: add-labels: {} diff --git a/pkg/workflow/compile_outputs_pr_test.go b/pkg/workflow/compile_outputs_pr_test.go index 3faf8930450..6989e52dd88 100644 --- a/pkg/workflow/compile_outputs_pr_test.go +++ b/pkg/workflow/compile_outputs_pr_test.go @@ -21,6 +21,8 @@ permissions: pull-requests: write issues: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-pull-request: @@ -89,6 +91,8 @@ tools: github: allowed: [list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-pull-request: @@ -195,6 +199,8 @@ tools: github: allowed: [list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-pull-request: @@ -269,6 +275,8 @@ tools: github: allowed: [list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-pull-request: @@ -339,6 +347,8 @@ permissions: pull-requests: write issues: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-pull-request: @@ -386,6 +396,8 @@ permissions: pull-requests: write issues: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-pull-request: diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index e31936edad7..e8740952dab 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -132,6 +132,21 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath return errors.New(formattedErr) } + // Validate dangerous permissions + log.Printf("Validating dangerous permissions") + if err := validateDangerousPermissions(workflowData); err != nil { + formattedErr := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: markdownPath, + Line: 1, + Column: 1, + }, + Type: "error", + Message: err.Error(), + }) + return errors.New(formattedErr) + } + // Validate agent file exists if specified in engine config log.Printf("Validating agent file if specified") if err := c.validateAgentFile(workflowData, markdownPath); err != nil { diff --git a/pkg/workflow/compiler_benchmark_test.go b/pkg/workflow/compiler_benchmark_test.go index a779553fca5..9adfd694a7c 100644 --- a/pkg/workflow/compiler_benchmark_test.go +++ b/pkg/workflow/compiler_benchmark_test.go @@ -24,6 +24,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true tools: github: allowed: [issue_read, add_issue_comment, list_issues] @@ -67,6 +69,8 @@ permissions: contents: read pull-requests: write engine: copilot +features: + dangerous-permissions-write: true mcp-servers: github: mode: remote @@ -132,6 +136,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true imports: - shared/web-tools.md timeout-minutes: 20 @@ -171,6 +177,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true tools: github: allowed: [issue_read, add_issue_comment] @@ -285,6 +293,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true tools: github: allowed: [get_repository, list_commits] diff --git a/pkg/workflow/compiler_cache_test.go b/pkg/workflow/compiler_cache_test.go index 0f1a2d1a8b9..477d0e07842 100644 --- a/pkg/workflow/compiler_cache_test.go +++ b/pkg/workflow/compiler_cache_test.go @@ -320,6 +320,8 @@ tools: github: toolsets: [repos, issues] engine: claude +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/compiler_compilation_test.go b/pkg/workflow/compiler_compilation_test.go index d09027b0219..6645f094463 100644 --- a/pkg/workflow/compiler_compilation_test.go +++ b/pkg/workflow/compiler_compilation_test.go @@ -22,7 +22,10 @@ permissions: contents: read issues: write pull-requests: read +engine: copilot strict: false +features: + dangerous-permissions-write: true tools: github: allowed: [list_issues, create_issue] @@ -109,6 +112,8 @@ tools: github: allowed: [add_issue_comment] engine: claude +features: + dangerous-permissions-write: true strict: false ---`, expectError: true, @@ -127,6 +132,8 @@ tools: github: allowed: [add_issue_comment] engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -148,6 +155,8 @@ tools: github: allowed: [add_issue_comment] engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -177,6 +186,8 @@ tools: github: allowed: [add_issue_comment] engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -202,6 +213,8 @@ tools: github: allowed: [add_issue_comment] engine: claude +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/compiler_performance_benchmark_test.go b/pkg/workflow/compiler_performance_benchmark_test.go index 08a5245c9b3..eee715d00f0 100644 --- a/pkg/workflow/compiler_performance_benchmark_test.go +++ b/pkg/workflow/compiler_performance_benchmark_test.go @@ -21,6 +21,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true tools: bash: ["echo", "cat"] timeout-minutes: 5 @@ -121,6 +123,8 @@ permissions: contents: read pull-requests: write engine: copilot +features: + dangerous-permissions-write: true mcp-servers: github: mode: remote @@ -172,6 +176,8 @@ permissions: contents: read pull-requests: write engine: copilot +features: + dangerous-permissions-write: true mcp-servers: github: mode: remote @@ -222,6 +228,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true tools: bash: ["echo"] --- @@ -259,6 +267,8 @@ permissions: contents: read pull-requests: write engine: copilot +features: + dangerous-permissions-write: true mcp-servers: github: mode: remote diff --git a/pkg/workflow/compiler_poststeps_test.go b/pkg/workflow/compiler_poststeps_test.go index f718b6a9619..04abbca7a5a 100644 --- a/pkg/workflow/compiler_poststeps_test.go +++ b/pkg/workflow/compiler_poststeps_test.go @@ -36,6 +36,8 @@ post-steps: name: test-artifact path: test-file.txt engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -119,6 +121,8 @@ post-steps: - name: Only Post Step run: echo "This runs after AI only" engine: claude +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/compiler_reactions_numeric_test.go b/pkg/workflow/compiler_reactions_numeric_test.go index 6df6731e79b..70955e0c0af 100644 --- a/pkg/workflow/compiler_reactions_numeric_test.go +++ b/pkg/workflow/compiler_reactions_numeric_test.go @@ -106,6 +106,8 @@ permissions: contents: read issues: write strict: false +features: + dangerous-permissions-write: true tools: github: allowed: [issue_read] @@ -149,6 +151,8 @@ permissions: contents: read issues: write strict: false +features: + dangerous-permissions-write: true tools: github: allowed: [issue_read] diff --git a/pkg/workflow/compiler_template_validation_test.go b/pkg/workflow/compiler_template_validation_test.go index 1df56f37643..dbe28ca3670 100644 --- a/pkg/workflow/compiler_template_validation_test.go +++ b/pkg/workflow/compiler_template_validation_test.go @@ -26,6 +26,8 @@ on: issues permissions: issues: write strict: false +features: + dangerous-permissions-write: true --- # Valid Workflow @@ -44,6 +46,8 @@ on: issues permissions: issues: write strict: false +features: + dangerous-permissions-write: true --- # Invalid Workflow diff --git a/pkg/workflow/compiler_yaml_test.go b/pkg/workflow/compiler_yaml_test.go index b4ae207d5ef..542ec3abd98 100644 --- a/pkg/workflow/compiler_yaml_test.go +++ b/pkg/workflow/compiler_yaml_test.go @@ -34,6 +34,8 @@ tools: github: allowed: [list_issues engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -56,6 +58,8 @@ permissions: invalid: yaml: syntax more: bad engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -75,6 +79,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -98,6 +104,8 @@ tools: github: allowed: ["list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -120,6 +128,8 @@ permissions: permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -160,6 +170,8 @@ permissions issues: write engine: claude strict: false +features: + dangerous-permissions-write: true --- # Test Workflow @@ -249,6 +261,8 @@ Invalid YAML with malformed nested structure.`, on: push permissions: {contents: read, issues: write engine: claude +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/compute_text_lazy_test.go b/pkg/workflow/compute_text_lazy_test.go index 2ec250edd5b..225b6388a96 100644 --- a/pkg/workflow/compute_text_lazy_test.go +++ b/pkg/workflow/compute_text_lazy_test.go @@ -29,6 +29,8 @@ on: permissions: issues: write strict: false +features: + dangerous-permissions-write: true tools: github: toolsets: [issues] diff --git a/pkg/workflow/dangerous_permissions_validation.go b/pkg/workflow/dangerous_permissions_validation.go new file mode 100644 index 00000000000..84e1ad80d19 --- /dev/null +++ b/pkg/workflow/dangerous_permissions_validation.go @@ -0,0 +1,99 @@ +package workflow + +import ( + "fmt" + "strings" + + "github.com/githubnext/gh-aw/pkg/constants" + "github.com/githubnext/gh-aw/pkg/logger" +) + +var dangerousPermissionsLog = logger.New("workflow:dangerous_permissions_validation") + +// validateDangerousPermissions validates that write permissions are not used unless +// the dangerous-permissions-write feature flag is enabled. +// +// This validation applies to: +// - Top-level workflow permissions +// +// This validation does NOT apply to: +// - Custom jobs (jobs defined in the jobs: section) +// - Safe outputs jobs (jobs defined in safe-outputs.job section) +// +// Returns an error if write permissions are found without the feature flag enabled. +func validateDangerousPermissions(workflowData *WorkflowData) error { + dangerousPermissionsLog.Print("Starting dangerous permissions validation") + + // Check if the feature flag is enabled + featureEnabled := isFeatureEnabled(constants.DangerousPermissionsWriteFeatureFlag, workflowData) + if featureEnabled { + dangerousPermissionsLog.Print("dangerous-permissions-write feature flag is enabled, allowing write permissions") + return nil + } + + // Parse the top-level workflow permissions + if workflowData.Permissions == "" { + dangerousPermissionsLog.Print("No permissions defined, validation passed") + return nil + } + + permissions := NewPermissionsParser(workflowData.Permissions).ToPermissions() + if permissions == nil { + dangerousPermissionsLog.Print("Could not parse permissions, validation passed") + return nil + } + + // Check for write permissions + writePermissions := findWritePermissions(permissions) + if len(writePermissions) > 0 { + dangerousPermissionsLog.Printf("Found %d write permissions without feature flag", len(writePermissions)) + return formatDangerousPermissionsError(writePermissions) + } + + dangerousPermissionsLog.Print("No write permissions found, validation passed") + return nil +} + +// findWritePermissions returns a list of permission scopes that have write access +func findWritePermissions(permissions *Permissions) []PermissionScope { + if permissions == nil { + return nil + } + + var writePerms []PermissionScope + + // Check all permission scopes + for _, scope := range GetAllPermissionScopes() { + level, exists := permissions.Get(scope) + if exists && level == PermissionWrite { + writePerms = append(writePerms, scope) + } + } + + return writePerms +} + +// formatDangerousPermissionsError formats an error message for write permissions violations +func formatDangerousPermissionsError(writePermissions []PermissionScope) error { + var lines []string + lines = append(lines, "Write permissions are not allowed unless the 'dangerous-permissions-write' feature flag is enabled.") + lines = append(lines, "") + lines = append(lines, "Found write permissions:") + for _, scope := range writePermissions { + lines = append(lines, fmt.Sprintf(" - %s: write", scope)) + } + lines = append(lines, "") + lines = append(lines, "To fix this issue, you have two options:") + lines = append(lines, "") + lines = append(lines, "Option 1: Change write permissions to read:") + lines = append(lines, "permissions:") + for _, scope := range writePermissions { + lines = append(lines, fmt.Sprintf(" %s: read", scope)) + } + lines = append(lines, "") + lines = append(lines, "Option 2: Enable the feature flag (use with caution):") + lines = append(lines, "features:") + lines = append(lines, " dangerous-permissions-write: true") + + return fmt.Errorf("%s", strings.Join(lines, "\n")) +} diff --git a/pkg/workflow/dangerous_permissions_validation_test.go b/pkg/workflow/dangerous_permissions_validation_test.go new file mode 100644 index 00000000000..e203424e6b7 --- /dev/null +++ b/pkg/workflow/dangerous_permissions_validation_test.go @@ -0,0 +1,246 @@ +package workflow + +import ( + "strings" + "testing" +) + +func TestValidateDangerousPermissions(t *testing.T) { + tests := []struct { + name string + permissions string + features map[string]any + shouldError bool + errorContains string + }{ + { + name: "no permissions - should pass", + permissions: "", + shouldError: false, + }, + { + name: "read permissions only - should pass", + permissions: "permissions:\n contents: read\n issues: read", + shouldError: false, + }, + { + name: "write permission without feature flag - should error", + permissions: "permissions:\n contents: write", + shouldError: true, + errorContains: "Write permissions are not allowed", + }, + { + name: "multiple write permissions without feature flag - should error", + permissions: "permissions:\n contents: write\n issues: write", + shouldError: true, + errorContains: "Write permissions are not allowed", + }, + { + name: "write permission with feature flag enabled - should pass", + permissions: "permissions:\n contents: write", + features: map[string]any{ + "dangerous-permissions-write": true, + }, + shouldError: false, + }, + { + name: "write permission with feature flag disabled - should error", + permissions: "permissions:\n contents: write", + features: map[string]any{ + "dangerous-permissions-write": false, + }, + shouldError: true, + errorContains: "Write permissions are not allowed", + }, + { + name: "shorthand read-all - should pass", + permissions: "permissions: read-all", + shouldError: false, + }, + { + name: "shorthand write-all without feature flag - should error", + permissions: "permissions: write-all", + shouldError: true, + errorContains: "Write permissions are not allowed", + }, + { + name: "shorthand write-all with feature flag - should pass", + permissions: "permissions: write-all", + features: map[string]any{ + "dangerous-permissions-write": true, + }, + shouldError: false, + }, + { + name: "mixed read and write with feature flag - should pass", + permissions: "permissions:\n contents: read\n issues: write\n pull-requests: read", + features: map[string]any{ + "dangerous-permissions-write": true, + }, + shouldError: false, + }, + { + name: "mixed read and write without feature flag - should error", + permissions: "permissions:\n contents: read\n issues: write\n pull-requests: read", + shouldError: true, + errorContains: "issues: write", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + workflowData := &WorkflowData{ + Permissions: tt.permissions, + Features: tt.features, + } + + err := validateDangerousPermissions(workflowData) + + if tt.shouldError { + if err == nil { + t.Errorf("Expected error but got none") + return + } + if tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) { + t.Errorf("Expected error to contain %q, but got: %v", tt.errorContains, err) + } + } else { + if err != nil { + t.Errorf("Expected no error but got: %v", err) + } + } + }) + } +} + +func TestFindWritePermissions(t *testing.T) { + tests := []struct { + name string + permissions *Permissions + expectedWriteCount int + expectedScopes []PermissionScope + }{ + { + name: "no permissions", + permissions: NewPermissions(), + expectedWriteCount: 0, + expectedScopes: []PermissionScope{}, + }, + { + name: "only read permissions", + permissions: NewPermissionsContentsRead(), + expectedWriteCount: 0, + expectedScopes: []PermissionScope{}, + }, + { + name: "single write permission", + permissions: NewPermissionsContentsWrite(), + expectedWriteCount: 1, + expectedScopes: []PermissionScope{PermissionContents}, + }, + { + name: "multiple write permissions", + permissions: NewPermissionsContentsWriteIssuesWritePRWrite(), + expectedWriteCount: 3, + expectedScopes: []PermissionScope{PermissionContents, PermissionIssues, PermissionPullRequests}, + }, + { + name: "write-all shorthand", + permissions: NewPermissionsWriteAll(), + expectedWriteCount: 16, // All permission scopes + expectedScopes: nil, // Don't check specific scopes for shorthand + }, + { + name: "mixed read and write", + permissions: NewPermissionsContentsReadIssuesWrite(), + expectedWriteCount: 1, + expectedScopes: []PermissionScope{PermissionIssues}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + writePerms := findWritePermissions(tt.permissions) + + if len(writePerms) != tt.expectedWriteCount { + t.Errorf("Expected %d write permissions, got %d", tt.expectedWriteCount, len(writePerms)) + } + + if tt.expectedScopes != nil { + // Check that all expected scopes are present + for _, expectedScope := range tt.expectedScopes { + found := false + for _, scope := range writePerms { + if scope == expectedScope { + found = true + break + } + } + if !found { + t.Errorf("Expected to find scope %s in write permissions", expectedScope) + } + } + } + }) + } +} + +func TestFormatDangerousPermissionsError(t *testing.T) { + tests := []struct { + name string + writePermissions []PermissionScope + expectedContains []string + expectedNotContain []string + }{ + { + name: "single write permission", + writePermissions: []PermissionScope{ + PermissionContents, + }, + expectedContains: []string{ + "Write permissions are not allowed", + "contents: write", + "contents: read", + "dangerous-permissions-write: true", + }, + }, + { + name: "multiple write permissions", + writePermissions: []PermissionScope{ + PermissionContents, + PermissionIssues, + }, + expectedContains: []string{ + "Write permissions are not allowed", + "contents: write", + "issues: write", + "contents: read", + "issues: read", + "dangerous-permissions-write: true", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := formatDangerousPermissionsError(tt.writePermissions) + if err == nil { + t.Fatal("Expected error but got nil") + } + + errMsg := err.Error() + + for _, expected := range tt.expectedContains { + if !strings.Contains(errMsg, expected) { + t.Errorf("Expected error message to contain %q, but it didn't. Error: %s", expected, errMsg) + } + } + + for _, notExpected := range tt.expectedNotContain { + if strings.Contains(errMsg, notExpected) { + t.Errorf("Expected error message to NOT contain %q, but it did. Error: %s", notExpected, errMsg) + } + } + }) + } +} diff --git a/pkg/workflow/engine_config_test.go b/pkg/workflow/engine_config_test.go index 122a9d5395f..f0140d3f175 100644 --- a/pkg/workflow/engine_config_test.go +++ b/pkg/workflow/engine_config_test.go @@ -338,6 +338,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/github_remote_mode_test.go b/pkg/workflow/github_remote_mode_test.go index dd3fb2cb9f4..e5ac26b7c2f 100644 --- a/pkg/workflow/github_remote_mode_test.go +++ b/pkg/workflow/github_remote_mode_test.go @@ -29,6 +29,8 @@ on: issues permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false tools: github: @@ -47,6 +49,8 @@ on: issues permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false tools: github: @@ -85,6 +89,8 @@ on: issues permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false tools: github: @@ -102,6 +108,8 @@ on: issues permissions: issues: write engine: copilot +features: + dangerous-permissions-write: true strict: false tools: github: @@ -139,6 +147,8 @@ on: issues permissions: issues: write engine: codex +features: + dangerous-permissions-write: true strict: false tools: github: @@ -158,6 +168,8 @@ on: issues permissions: issues: write engine: codex +features: + dangerous-permissions-write: true strict: false tools: github: diff --git a/pkg/workflow/local_action_permissions_test.go b/pkg/workflow/local_action_permissions_test.go index bab42da4dbc..e97f173fd3a 100644 --- a/pkg/workflow/local_action_permissions_test.go +++ b/pkg/workflow/local_action_permissions_test.go @@ -28,6 +28,8 @@ on: permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false command: /fix ---`, @@ -44,6 +46,8 @@ on: permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false ---`, description: "Main agent job should have contents: read when using local actions", @@ -153,6 +157,8 @@ on: permissions: issues: write engine: claude +features: + dangerous-permissions-write: true strict: false command: /fix ---` diff --git a/pkg/workflow/manual_approval_integration_test.go b/pkg/workflow/manual_approval_integration_test.go index f37f2be0236..beb295cfd5f 100644 --- a/pkg/workflow/manual_approval_integration_test.go +++ b/pkg/workflow/manual_approval_integration_test.go @@ -58,6 +58,8 @@ permissions: contents: read issues: write engine: copilot +features: + dangerous-permissions-write: true strict: false ---`, wantEnvironmentInJob: true, diff --git a/pkg/workflow/permissions_warning_test.go b/pkg/workflow/permissions_warning_test.go index 74e42fe7fbc..7f560680eb9 100644 --- a/pkg/workflow/permissions_warning_test.go +++ b/pkg/workflow/permissions_warning_test.go @@ -73,6 +73,8 @@ permissions: contents: write issues: write strict: false +features: + dangerous-permissions-write: true tools: github: toolsets: [repos, issues] diff --git a/pkg/workflow/pr_checkout_test.go b/pkg/workflow/pr_checkout_test.go index fbf4105f457..694fd0522a0 100644 --- a/pkg/workflow/pr_checkout_test.go +++ b/pkg/workflow/pr_checkout_test.go @@ -150,6 +150,8 @@ permissions: contents: read pull-requests: read engine: codex +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/pr_ready_for_review_checkout_test.go b/pkg/workflow/pr_ready_for_review_checkout_test.go index 65c1cf5f62b..4f96e81f73f 100644 --- a/pkg/workflow/pr_ready_for_review_checkout_test.go +++ b/pkg/workflow/pr_ready_for_review_checkout_test.go @@ -82,6 +82,8 @@ permissions: contents: read pull-requests: read engine: codex +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/processing_benchmark_test.go b/pkg/workflow/processing_benchmark_test.go index 6f84c964fd9..aafddb3e5f3 100644 --- a/pkg/workflow/processing_benchmark_test.go +++ b/pkg/workflow/processing_benchmark_test.go @@ -60,6 +60,8 @@ permissions: issues: write pull-requests: write engine: copilot +features: + dangerous-permissions-write: true strict: false tools: github: @@ -252,6 +254,8 @@ permissions: discussions: write deployments: write engine: claude +features: + dangerous-permissions-write: true strict: false --- @@ -288,6 +292,8 @@ permissions: contents: read issues: write engine: claude +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/step_summary_test.go b/pkg/workflow/step_summary_test.go index 5d8aa47d15a..72bad95a6f7 100644 --- a/pkg/workflow/step_summary_test.go +++ b/pkg/workflow/step_summary_test.go @@ -24,6 +24,8 @@ tools: github: allowed: [list_issues] engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: create-issue: diff --git a/pkg/workflow/strict_mode_test.go b/pkg/workflow/strict_mode_test.go index 48f12c5f48e..a4361d52b8f 100644 --- a/pkg/workflow/strict_mode_test.go +++ b/pkg/workflow/strict_mode_test.go @@ -114,6 +114,8 @@ permissions: pull-requests: read timeout-minutes: 10 engine: copilot +features: + dangerous-permissions-write: true --- # Test Workflow`, @@ -128,6 +130,8 @@ permissions: issues: write timeout-minutes: 10 engine: copilot +features: + dangerous-permissions-write: true --- # Test Workflow`, @@ -142,6 +146,8 @@ permissions: pull-requests: write timeout-minutes: 10 engine: copilot +features: + dangerous-permissions-write: true --- # Test Workflow`, @@ -169,6 +175,8 @@ tools: content: `--- on: push permissions: write +features: + dangerous-permissions-write: true timeout-minutes: 10 engine: copilot --- @@ -182,6 +190,8 @@ engine: copilot content: `--- on: push permissions: write-all +features: + dangerous-permissions-write: true timeout-minutes: 10 engine: copilot --- @@ -216,6 +226,8 @@ permissions: pull-requests: read timeout-minutes: 10 engine: copilot +features: + dangerous-permissions-write: true --- # Test Workflow`, @@ -598,6 +610,8 @@ permissions: issues: write pull-requests: read engine: copilot +features: + dangerous-permissions-write: true strict: false network: allowed: @@ -655,6 +669,8 @@ permissions: issues: read pull-requests: read engine: copilot +features: + dangerous-permissions-write: true --- # Test Workflow`, @@ -688,6 +704,8 @@ permissions: issues: read pull-requests: read engine: copilot +features: + dangerous-permissions-write: true --- # Test Workflow`, @@ -732,6 +750,8 @@ permissions: issues: read pull-requests: read engine: copilot +features: + dangerous-permissions-write: true --- # Test Workflow` @@ -783,6 +803,8 @@ permissions: issues: read pull-requests: read engine: copilot +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/template_expression_integration_test.go b/pkg/workflow/template_expression_integration_test.go index 8c694cfcffc..31196588215 100644 --- a/pkg/workflow/template_expression_integration_test.go +++ b/pkg/workflow/template_expression_integration_test.go @@ -27,6 +27,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false --- diff --git a/pkg/workflow/update_issue_test.go b/pkg/workflow/update_issue_test.go index 519f01507a6..bc62dc131ca 100644 --- a/pkg/workflow/update_issue_test.go +++ b/pkg/workflow/update_issue_test.go @@ -22,6 +22,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: update-issue: @@ -90,6 +92,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: update-issue: @@ -163,6 +167,8 @@ permissions: issues: write pull-requests: read engine: claude +features: + dangerous-permissions-write: true strict: false safe-outputs: update-issue: diff --git a/pkg/workflow/xml_comments_test.go b/pkg/workflow/xml_comments_test.go index d65e3dfb12a..5b00f36f58e 100644 --- a/pkg/workflow/xml_comments_test.go +++ b/pkg/workflow/xml_comments_test.go @@ -316,6 +316,8 @@ tools: github: toolsets: [issues] engine: claude +features: + dangerous-permissions-write: true strict: false ---