From bd9f030d666e4ee8036d52815e3f126a260a7b54 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 20 Nov 2025 19:16:56 +0000 Subject: [PATCH] Fix unsafe quoting vulnerability in network hook generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Alert Number**: #16 **Severity**: Critical **Rule**: go/unsafe-quoting ## Vulnerability Description The code was embedding JSON-serialized domain lists directly into a Python script using string formatting without proper escaping. If the JSON contained double quotes in an unexpected way, it could break out of the enclosing quotes and potentially change the structure of the Python code. ## Fix Applied 1. Added `strconv.Quote()` to properly escape the JSON string before embedding it in the Python script template 2. Changed the Python code to use `json.loads()` to parse the escaped JSON string, making the approach more explicit and safer 3. Updated all related tests to check for the new escaped format ## Security Best Practices - Using `strconv.Quote()` ensures that any special characters (including quotes) are properly escaped according to Go string literal rules - Using `json.loads()` on the Python side makes the intent clear and provides an additional layer of safety by parsing the JSON in a structured way - This prevents potential code injection vulnerabilities (CWE-78, CWE-89, CWE-94) ## Testing Considerations All existing tests have been updated and pass successfully. The fix maintains backward compatibility in terms of functionality while improving security. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/workflow/compiler_test.go | 10 +++++----- pkg/workflow/engine_network_hooks.go | 16 +++++++++------- pkg/workflow/engine_network_test.go | 12 ++++++------ pkg/workflow/network_merge_edge_cases_test.go | 3 ++- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/pkg/workflow/compiler_test.go b/pkg/workflow/compiler_test.go index c2d552fa04f..2463b30e65c 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 14989b6e47b..6ce1b44a307 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 dbd29faf4d6..5adc86a1064 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 3c957198826..df7770ec8f7 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) }