diff --git a/pkg/workflow/compiler_test.go b/pkg/workflow/compiler_test.go index c2d552fa04..2463b30e65 100644 --- a/pkg/workflow/compiler_test.go +++ b/pkg/workflow/compiler_test.go @@ -2182,8 +2182,8 @@ This is a test workflow with empty network permissions (deny all). if !strings.Contains(string(lockContent), "Generate Network Permissions Hook") { t.Error("Should contain network hook setup for network: {}") } - // Should have empty ALLOWED_DOMAINS array for deny-all - if !strings.Contains(string(lockContent), "ALLOWED_DOMAINS = []") { + // Should have empty ALLOWED_DOMAINS array for deny-all (using json.loads) + if !strings.Contains(string(lockContent), `json.loads("[]")`) { t.Error("Should have empty ALLOWED_DOMAINS array for deny-all policy") } }) @@ -2219,14 +2219,14 @@ This is a test workflow with explicit network permissions. t.Fatalf("Failed to read lock file: %v", err) } - // Should contain network hook setup with specified domains + // Should contain network hook setup with specified domains (escaped in JSON) if !strings.Contains(string(lockContent), "Generate Network Permissions Hook") { t.Error("Should contain network hook setup with explicit network permissions") } - if !strings.Contains(string(lockContent), `"example.com"`) { + if !strings.Contains(string(lockContent), `\"example.com\"`) { t.Error("Should contain example.com in allowed domains") } - if !strings.Contains(string(lockContent), `"api.github.com"`) { + if !strings.Contains(string(lockContent), `\"api.github.com\"`) { t.Error("Should contain api.github.com in allowed domains") } }) diff --git a/pkg/workflow/engine_network_hooks.go b/pkg/workflow/engine_network_hooks.go index 14989b6e47..6ce1b44a30 100644 --- a/pkg/workflow/engine_network_hooks.go +++ b/pkg/workflow/engine_network_hooks.go @@ -3,6 +3,7 @@ package workflow import ( "encoding/json" "fmt" + "strconv" "strings" "github.com/githubnext/gh-aw/pkg/logger" @@ -32,12 +33,13 @@ func (g *NetworkHookGenerator) GenerateNetworkHookScript(allowedDomains []string } } - // Embed domain list JSON directly as a Python literal (safe for []string from json.Marshal) - // This prevents any quote-related injection vulnerabilities (CWE-78, CWE-89, CWE-94) - // Use domainsJSON directly for ALLOWED_DOMAINS assignment + // Safely embed domain list JSON using strconv.Quote to prevent injection + // This prevents quote-related injection vulnerabilities (CWE-78, CWE-89, CWE-94) + // The quoted JSON string is then parsed using Python's json.loads() for safety + quotedDomainsJSON := strconv.Quote(domainsJSON) // Build the Python script using a safe template approach - // The JSON array is embedded directly as a Python list literal + // The JSON string is properly quoted and then parsed using json.loads() return fmt.Sprintf(`#!/usr/bin/env python3 """ Network permissions validator for Claude Code engine. @@ -50,8 +52,8 @@ import urllib.parse import re # Domain allow-list (populated during generation) -# JSON array safely embedded as Python list literal -ALLOWED_DOMAINS = %s +# JSON string is safely quoted and parsed using json.loads() +ALLOWED_DOMAINS = json.loads(%s) def extract_domain(url_or_query): """Extract domain from URL or search query.""" @@ -120,7 +122,7 @@ try: except Exception as e: print(f"Network validation error: {e}", file=sys.stderr) sys.exit(2) # Block on errors -`, domainsJSON) +`, quotedDomainsJSON) } // GenerateNetworkHookWorkflowStep generates a GitHub Actions workflow step that creates the network permissions hook diff --git a/pkg/workflow/engine_network_test.go b/pkg/workflow/engine_network_test.go index dbd29faf4d..5adc86a106 100644 --- a/pkg/workflow/engine_network_test.go +++ b/pkg/workflow/engine_network_test.go @@ -12,14 +12,14 @@ func TestNetworkHookGenerator(t *testing.T) { allowedDomains := []string{"example.com", "*.trusted.com", "api.service.org"} script := generator.GenerateNetworkHookScript(allowedDomains) - // Check that script contains the expected domains - if !strings.Contains(script, `"example.com"`) { + // Check that script contains the expected domains (escaped in the JSON string) + if !strings.Contains(script, `\"example.com\"`) { t.Error("Script should contain example.com") } - if !strings.Contains(script, `"*.trusted.com"`) { + if !strings.Contains(script, `\"*.trusted.com\"`) { t.Error("Script should contain *.trusted.com") } - if !strings.Contains(script, `"api.service.org"`) { + if !strings.Contains(script, `\"api.service.org\"`) { t.Error("Script should contain api.service.org") } @@ -68,8 +68,8 @@ func TestNetworkHookGenerator(t *testing.T) { allowedDomains := []string{} // Empty list means deny-all script := generator.GenerateNetworkHookScript(allowedDomains) - // Should still generate a valid script - if !strings.Contains(script, "ALLOWED_DOMAINS = []") { + // Should still generate a valid script with json.loads("[]") + if !strings.Contains(script, `json.loads("[]")`) { t.Error("Script should handle empty domains list (deny-all policy)") } if !strings.Contains(script, "def is_domain_allowed") { diff --git a/pkg/workflow/network_merge_edge_cases_test.go b/pkg/workflow/network_merge_edge_cases_test.go index 3c95719882..df7770ec8f 100644 --- a/pkg/workflow/network_merge_edge_cases_test.go +++ b/pkg/workflow/network_merge_edge_cases_test.go @@ -63,8 +63,9 @@ imports: } // Count occurrences of github.com (should only appear once in the list, not duplicated) + // Note: JSON is escaped in the string, so we look for \"github.com\" lockStr := string(content) - count := strings.Count(lockStr, `"github.com"`) + count := strings.Count(lockStr, `\"github.com\"`) if count != 1 { t.Errorf("Expected github.com to appear exactly once in ALLOWED_DOMAINS, but found %d occurrences", count) }