[security-fix] Security Fix: Resolve unsafe quoting vulnerability in network hook generation (Alert #16)

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")
 		}
 	})

pkg/workflow/engine_network_hooks.go

@@ -3,6 +3,7 @@
 import (
 	"encoding/json"
 	"fmt"
+	"strconv"
 	"strings"
 
 	"github.com/githubnext/gh-aw/pkg/logger"
@@ -32,12 +33,13 @@
 		}
 	}
 
-	// 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 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 @@
 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

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") {

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)
 		}