diff --git a/docs/frontmatter.md b/docs/frontmatter.md index 02c2c336017..7f6953f6c6b 100644 --- a/docs/frontmatter.md +++ b/docs/frontmatter.md @@ -19,7 +19,7 @@ The YAML frontmatter supports standard GitHub Actions properties plus additional **Properties specific to GitHub Agentic Workflows:** - `engine`: AI engine configuration (claude/codex) with optional max-turns setting -- `network`: Network access control for AI engines (supports `defaults`, `{}`, or `{ allowed: [...] }`) +- `network`: Network access control for AI engines - `tools`: Available tools and MCP servers for the AI engine - `cache`: Cache configuration for workflow dependencies - `safe-outputs`: [Safe Output Processing](safe-outputs.md) for automatic issue creation and comment posting. @@ -253,6 +253,16 @@ network: - "api.example.com" # Exact domain match - "*.trusted.com" # Wildcard matches any subdomain (including nested subdomains) +# Or combine defaults with additional domains +engine: + id: claude + +network: + allowed: + - "defaults" # Expands to the full default whitelist + - "good.com" # Add custom domain + - "api.example.org" # Add another custom domain + # Or deny all network access (empty object) engine: id: claude @@ -264,6 +274,7 @@ network: {} - **Default Whitelist**: When no network permissions are specified or `network: defaults` is used, access is restricted to a curated whitelist of common development domains (package managers, container registries, etc.) - **Selective Access**: When `network: { allowed: [...] }` is specified, only listed domains are accessible +- **Defaults Expansion**: When "defaults" appears in the allowed list, it expands to include all default whitelist domains plus any additional specified domains - **No Access**: When `network: {}` is specified, all network access is denied - **Engine vs Tools**: Engine permissions control the AI engine itself, separate from MCP tool permissions - **Hook Enforcement**: Uses Claude Code's hook system for runtime network access control @@ -297,6 +308,17 @@ network: - "*.company-internal.com" - "public-api.service.com" +# Combine default whitelist with custom domains +# This gives access to all package managers, registries, etc. PLUS your custom domains +engine: + id: claude + +network: + allowed: + - "defaults" # Expands to full default whitelist + - "api.mycompany.com" # Add custom API + - "*.internal.mycompany.com" # Add internal services + # Deny all network access (empty object) engine: id: claude @@ -307,21 +329,12 @@ network: {} ### Default Whitelist Domains The `network: defaults` mode includes access to these categories of domains: -- **Package Managers**: npmjs.org, pypi.org, rubygems.org, crates.io, nuget.org, etc. -- **Container Registries**: docker.io, ghcr.io, quay.io, mcr.microsoft.com, etc. -- **Development Tools**: github.com domains, golang.org, maven.apache.org, etc. -- **Certificate Authorities**: Various OCSP and CRL endpoints for certificate validation +- **Package Managers** +- **Container Registries** +- **Development Tools** +- **Certificate Authorities** - **Language-specific Repositories**: For Go, Python, Node.js, Java, .NET, Rust, etc. -### Migration from Previous Versions - -The previous `strict:` mode has been removed. Network permissions now work as follows: -- **No `network:` field**: Defaults to `network: defaults` (curated whitelist) -- **`network: defaults`**: Curated whitelist of development domains -- **`network: {}`**: No network access -- **`network: { allowed: [...] }`**: Restricted to listed domains only - - ### Permission Modes 1. **Default whitelist**: Curated list of development domains (default when no `network:` field specified) diff --git a/docs/security-notes.md b/docs/security-notes.md index 37cff597fe8..3004b9e36b8 100644 --- a/docs/security-notes.md +++ b/docs/security-notes.md @@ -235,10 +235,11 @@ Engine network permissions provide fine-grained control over network access for ### Best Practices 1. **Always Specify Permissions**: When using network features, explicitly list allowed domains -2. **Use Wildcards Carefully**: `*.example.com` matches any subdomain including nested ones (e.g., `api.example.com`, `nested.api.example.com`) - ensure this broad access is intended -3. **Test Thoroughly**: Verify that all required domains are included in allowlist -4. **Monitor Usage**: Review workflow logs to identify any blocked legitimate requests -5. **Document Reasoning**: Comment why specific domains are required for maintenance +2. **Use Defaults When Appropriate**: Use `"defaults"` in the allowed list to include common development domains, then add custom ones +3. **Use Wildcards Carefully**: `*.example.com` matches any subdomain including nested ones (e.g., `api.example.com`, `nested.api.example.com`) - ensure this broad access is intended +4. **Test Thoroughly**: Verify that all required domains are included in allowlist +5. **Monitor Usage**: Review workflow logs to identify any blocked legitimate requests +6. **Document Reasoning**: Comment why specific domains are required for maintenance ### Permission Modes diff --git a/pkg/workflow/compiler_network_test.go b/pkg/workflow/compiler_network_test.go index 92f514f89ce..e7fbbe4c37c 100644 --- a/pkg/workflow/compiler_network_test.go +++ b/pkg/workflow/compiler_network_test.go @@ -289,6 +289,41 @@ func TestNetworkPermissionsUtilities(t *testing.T) { } }) + t.Run("GetAllowedDomains with 'defaults' expansion", func(t *testing.T) { + // Test with defaults in allowed list - should expand defaults and add custom domains + perms := &NetworkPermissions{ + Allowed: []string{"defaults", "good.com", "api.example.com"}, + } + domains := GetAllowedDomains(perms) + + // Should have all default domains plus the custom ones + defaultDomains := getDefaultAllowedDomains() + expectedTotal := len(defaultDomains) + 2 // defaults + good.com + api.example.com + + if len(domains) != expectedTotal { + t.Errorf("Expected %d domains (defaults + 2 custom), got %d", expectedTotal, len(domains)) + } + + // Verify custom domains are included + foundGoodCom := false + foundApiExample := false + for _, domain := range domains { + if domain == "good.com" { + foundGoodCom = true + } + if domain == "api.example.com" { + foundApiExample = true + } + } + + if !foundGoodCom { + t.Error("Expected 'good.com' to be included in the expanded domains") + } + if !foundApiExample { + t.Error("Expected 'api.example.com' to be included in the expanded domains") + } + }) + t.Run("Deprecated HasNetworkPermissions still works", func(t *testing.T) { // Test the deprecated function that takes EngineConfig config := &EngineConfig{ diff --git a/pkg/workflow/compiler_test.go b/pkg/workflow/compiler_test.go index 69a4d5073a3..431e56b3699 100644 --- a/pkg/workflow/compiler_test.go +++ b/pkg/workflow/compiler_test.go @@ -1837,7 +1837,7 @@ This is a simple test workflow with Bash tools. } simpleLockContent := string(simpleContent2) - t.Logf("Simple workflow lock file content: %s", simpleLockContent) + // t.Logf("Simple workflow lock file content: %s", simpleLockContent) // Check if simple case works first expectedSimpleCommands := []string{"pwd", "ls", "cat"} diff --git a/pkg/workflow/engine_network_hooks.go b/pkg/workflow/engine_network_hooks.go index a7f6a90243c..25a45005e38 100644 --- a/pkg/workflow/engine_network_hooks.go +++ b/pkg/workflow/engine_network_hooks.go @@ -377,6 +377,7 @@ func ShouldEnforceNetworkPermissions(network *NetworkPermissions) bool { // Returns default whitelist if no network permissions configured or in "defaults" mode // Returns empty slice if network permissions configured but no domains allowed (deny all) // Returns domain list if network permissions configured with allowed domains +// If "defaults" appears in the allowed list, it's expanded to the default whitelist func GetAllowedDomains(network *NetworkPermissions) []string { if network == nil { return getDefaultAllowedDomains() // Default whitelist for backwards compatibility @@ -384,7 +385,25 @@ func GetAllowedDomains(network *NetworkPermissions) []string { if network.Mode == "defaults" { return getDefaultAllowedDomains() // Default whitelist for defaults mode } - return network.Allowed // Could be empty for deny-all + + // Handle empty allowed list (deny-all case) + if len(network.Allowed) == 0 { + return []string{} // Return empty slice, not nil + } + + // Process the allowed list, expanding "defaults" if present + var expandedDomains []string + for _, domain := range network.Allowed { + if domain == "defaults" { + // Expand "defaults" to the full default whitelist + expandedDomains = append(expandedDomains, getDefaultAllowedDomains()...) + } else { + // Add the domain as-is + expandedDomains = append(expandedDomains, domain) + } + } + + return expandedDomains } // HasNetworkPermissions is deprecated - use ShouldEnforceNetworkPermissions instead diff --git a/pkg/workflow/engine_network_test.go b/pkg/workflow/engine_network_test.go index 7ef2d9c4a3d..35294d3065d 100644 --- a/pkg/workflow/engine_network_test.go +++ b/pkg/workflow/engine_network_test.go @@ -144,6 +144,65 @@ func TestGetAllowedDomains(t *testing.T) { } } }) + + t.Run("permissions with 'defaults' in allowed list", func(t *testing.T) { + permissions := &NetworkPermissions{ + Allowed: []string{"defaults", "good.com"}, + } + domains := GetAllowedDomains(permissions) + + // Should have all default domains plus "good.com" + defaultDomains := getDefaultAllowedDomains() + expectedTotal := len(defaultDomains) + 1 + + if len(domains) != expectedTotal { + t.Fatalf("Expected %d domains (defaults + good.com), got %d", expectedTotal, len(domains)) + } + + // Check that all default domains are included + defaultsFound := 0 + goodComFound := false + + for _, domain := range domains { + if domain == "good.com" { + goodComFound = true + } + // Check if this domain is in the defaults list + for _, defaultDomain := range defaultDomains { + if domain == defaultDomain { + defaultsFound++ + break + } + } + } + + if defaultsFound != len(defaultDomains) { + t.Errorf("Expected all %d default domains to be included, found %d", len(defaultDomains), defaultsFound) + } + + if !goodComFound { + t.Error("Expected 'good.com' to be included in the allowed domains") + } + }) + + t.Run("permissions with only 'defaults' in allowed list", func(t *testing.T) { + permissions := &NetworkPermissions{ + Allowed: []string{"defaults"}, + } + domains := GetAllowedDomains(permissions) + defaultDomains := getDefaultAllowedDomains() + + if len(domains) != len(defaultDomains) { + t.Fatalf("Expected %d domains (just defaults), got %d", len(defaultDomains), len(domains)) + } + + // Check that all default domains are included + for i, defaultDomain := range defaultDomains { + if domains[i] != defaultDomain { + t.Errorf("Expected domain %d to be '%s', got '%s'", i, defaultDomain, domains[i]) + } + } + }) } func TestDeprecatedHasNetworkPermissions(t *testing.T) { diff --git a/pkg/workflow/network_defaults_integration_test.go b/pkg/workflow/network_defaults_integration_test.go new file mode 100644 index 00000000000..f3fc277dcfc --- /dev/null +++ b/pkg/workflow/network_defaults_integration_test.go @@ -0,0 +1,127 @@ +package workflow + +import ( + "testing" +) + +func TestNetworkDefaultsIntegration(t *testing.T) { + t.Run("YAML with defaults in allowed list", func(t *testing.T) { + // Test the complete workflow: YAML parsing -> GetAllowedDomains + frontmatter := map[string]any{ + "network": map[string]any{ + "allowed": []any{"defaults", "good.com", "api.example.org"}, + }, + } + + compiler := &Compiler{} + networkPermissions := compiler.extractNetworkPermissions(frontmatter) + + if networkPermissions == nil { + t.Fatal("Expected networkPermissions to be parsed, got nil") + } + + // Check that the allowed list contains the original entries + expectedAllowed := []string{"defaults", "good.com", "api.example.org"} + if len(networkPermissions.Allowed) != len(expectedAllowed) { + t.Fatalf("Expected %d allowed entries, got %d", len(expectedAllowed), len(networkPermissions.Allowed)) + } + + for i, expected := range expectedAllowed { + if networkPermissions.Allowed[i] != expected { + t.Errorf("Expected allowed[%d] to be '%s', got '%s'", i, expected, networkPermissions.Allowed[i]) + } + } + + // Now test that GetAllowedDomains expands "defaults" correctly + domains := GetAllowedDomains(networkPermissions) + defaultDomains := getDefaultAllowedDomains() + + // Should have all default domains plus the 2 custom ones + expectedTotal := len(defaultDomains) + 2 + if len(domains) != expectedTotal { + t.Fatalf("Expected %d total domains (defaults + 2 custom), got %d", expectedTotal, len(domains)) + } + + // Verify that the default domains are included + defaultsFound := 0 + goodComFound := false + apiExampleFound := false + + for _, domain := range domains { + if domain == "good.com" { + goodComFound = true + } else if domain == "api.example.org" { + apiExampleFound = true + } else { + // Check if this is a default domain + for _, defaultDomain := range defaultDomains { + if domain == defaultDomain { + defaultsFound++ + break + } + } + } + } + + if defaultsFound != len(defaultDomains) { + t.Errorf("Expected all %d default domains to be included, found %d", len(defaultDomains), defaultsFound) + } + + if !goodComFound { + t.Error("Expected 'good.com' to be included in the expanded domains") + } + + if !apiExampleFound { + t.Error("Expected 'api.example.org' to be included in the expanded domains") + } + }) + + t.Run("YAML with only defaults", func(t *testing.T) { + frontmatter := map[string]any{ + "network": map[string]any{ + "allowed": []any{"defaults"}, + }, + } + + compiler := &Compiler{} + networkPermissions := compiler.extractNetworkPermissions(frontmatter) + domains := GetAllowedDomains(networkPermissions) + defaultDomains := getDefaultAllowedDomains() + + if len(domains) != len(defaultDomains) { + t.Fatalf("Expected %d domains (just defaults), got %d", len(defaultDomains), len(domains)) + } + + // Verify all defaults are included + for i, defaultDomain := range defaultDomains { + if domains[i] != defaultDomain { + t.Errorf("Expected domain %d to be '%s', got '%s'", i, defaultDomain, domains[i]) + } + } + }) + + t.Run("YAML without defaults should work as before", func(t *testing.T) { + frontmatter := map[string]any{ + "network": map[string]any{ + "allowed": []any{"custom1.com", "custom2.org"}, + }, + } + + compiler := &Compiler{} + networkPermissions := compiler.extractNetworkPermissions(frontmatter) + domains := GetAllowedDomains(networkPermissions) + + // Should only have the 2 custom domains + if len(domains) != 2 { + t.Fatalf("Expected 2 domains, got %d", len(domains)) + } + + if domains[0] != "custom1.com" { + t.Errorf("Expected first domain to be 'custom1.com', got '%s'", domains[0]) + } + + if domains[1] != "custom2.org" { + t.Errorf("Expected second domain to be 'custom2.org', got '%s'", domains[1]) + } + }) +} diff --git a/pkg/workflow/output_test.go b/pkg/workflow/output_test.go index f4729d8ce63..5b43fc9b1ca 100644 --- a/pkg/workflow/output_test.go +++ b/pkg/workflow/output_test.go @@ -284,7 +284,7 @@ This workflow tests the create-issue job generation. t.Error("Expected create_issue job to depend on main job") } - t.Logf("Generated workflow content:\n%s", lockContent) + // t.Logf("Generated workflow content:\n%s", lockContent) } func TestOutputCommentConfigParsing(t *testing.T) { @@ -588,7 +588,7 @@ This workflow tests the create_issue_comment job generation. t.Error("Expected agent output content to be passed as environment variable") } - t.Logf("Generated workflow content:\n%s", lockContent) + // t.Logf("Generated workflow content:\n%s", lockContent) } func TestOutputCommentJobSkippedForNonIssueEvents(t *testing.T) { @@ -648,7 +648,7 @@ This workflow tests that issue comment job is skipped for non-issue/PR events. t.Error("Expected create_issue_comment job to have conditional execution for skipping") } - t.Logf("Generated workflow content:\n%s", lockContent) + // t.Logf("Generated workflow content:\n%s", lockContent) } func TestOutputPullRequestConfigParsing(t *testing.T) { @@ -824,7 +824,7 @@ This workflow tests the create_pull_request job generation. t.Error("Expected create_pull_request job to depend on main job") } - t.Logf("Generated workflow content:\n%s", lockContentStr) + // t.Logf("Generated workflow content:\n%s", lockContentStr) } func TestOutputPullRequestDraftFalse(t *testing.T) { @@ -899,7 +899,7 @@ This workflow tests the create_pull_request job generation with draft: false. t.Error("Expected automation label to be set as environment variable") } - t.Logf("Generated workflow content:\n%s", lockContentStr) + // t.Logf("Generated workflow content:\n%s", lockContentStr) } func TestOutputPullRequestDraftTrue(t *testing.T) { @@ -974,7 +974,7 @@ This workflow tests the create_pull_request job generation with draft: true. t.Error("Expected automation label to be set as environment variable") } - t.Logf("Generated workflow content:\n%s", lockContentStr) + // t.Logf("Generated workflow content:\n%s", lockContentStr) } func TestOutputLabelConfigParsing(t *testing.T) { @@ -1136,7 +1136,7 @@ This workflow tests the add_labels job generation. t.Error("Expected labels_added output to be available") } - t.Logf("Generated workflow content:\n%s", lockContent) + // t.Logf("Generated workflow content:\n%s", lockContent) } func TestOutputLabelJobGenerationNoAllowedLabels(t *testing.T) { @@ -1208,7 +1208,7 @@ Write your labels to ${{ env.GITHUB_AW_SAFE_OUTPUTS }}, one per line. t.Error("Expected max to be set correctly") } - t.Logf("Generated workflow content:\n%s", lockContent) + // t.Logf("Generated workflow content:\n%s", lockContent) } func TestOutputLabelJobGenerationNullConfig(t *testing.T) { @@ -1284,7 +1284,7 @@ Write your labels to ${{ env.GITHUB_AW_SAFE_OUTPUTS }}, one per line. t.Error("Expected default max to be set correctly") } - t.Logf("Generated workflow content:\n%s", lockContent) + // t.Logf("Generated workflow content:\n%s", lockContent) } func TestOutputLabelConfigNullParsing(t *testing.T) { @@ -1537,7 +1537,7 @@ This workflow tests the add_labels job generation with max. t.Error("Expected max to be set as environment variable") } - t.Logf("Generated workflow content:\n%s", lockContent) + // t.Logf("Generated workflow content:\n%s", lockContent) } func TestOutputLabelJobGenerationWithDefaultMaxCount(t *testing.T) { @@ -1603,7 +1603,7 @@ This workflow tests the add_labels job generation with default max. t.Error("Expected max to be set to default value of 3 as environment variable") } - t.Logf("Generated workflow content:\n%s", lockContent) + // t.Logf("Generated workflow content:\n%s", lockContent) } func TestOutputLabelConfigValidation(t *testing.T) {