Skip to content

[security-fix] Security Fix: Eliminate unsafe quoting in network permissions hook generation (Alert #16) #1780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
github-actions wants to merge 1 commit into from
Closed

[security-fix] Security Fix: Eliminate unsafe quoting in network permissions hook generation (Alert #16) #1780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions pkg/workflow/engine_network_hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,16 @@
domainsJSON = string(jsonBytes)
}

// 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
// Use json.loads() to parse the JSON string at runtime instead of embedding as a literal
// This eliminates any potential quoting issues (CWE-78, CWE-89, CWE-94)
// The JSON string is embedded as a Python string literal with proper escaping

// Escape backslashes and single quotes for safe embedding in Python string literal
escapedJSON := strings.ReplaceAll(domainsJSON, `\`, `\\`)
escapedJSON = strings.ReplaceAll(escapedJSON, `'`, `\'`)

// Build the Python script using a safe template approach
// The JSON array is embedded directly as a Python list literal
// The JSON is parsed at runtime using json.loads() to avoid literal embedding issues
return fmt.Sprintf(`#!/usr/bin/env python3
"""
Network permissions validator for Claude Code engine.
Expand All @@ -39,8 +43,8 @@
import re

# Domain allow-list (populated during generation)
# JSON array safely embedded as Python list literal
ALLOWED_DOMAINS = %s
# JSON string is parsed at runtime to avoid quoting vulnerabilities
ALLOWED_DOMAINS = json.loads('%s')

def extract_domain(url_or_query):
"""Extract domain from URL or search query."""
Expand Down Expand Up @@ -109,7 +113,7 @@
except Exception as e:
print(f"Network validation error: {e}", file=sys.stderr)
sys.exit(2) # Block on errors
`, domainsJSON)
`, escapedJSON)

Check failure

Code scanning / CodeQL

Potentially unsafe quoting Critical

If this
JSON value
Loading
contains a double quote, it could break out of the enclosing quotes.
If this
JSON value
Loading
contains a single quote, it could break out of the enclosing quotes.

Copilot Autofix

AI 5 months ago

The best way to prevent quoting vulnerabilities here is to avoid manually embedding serialized data in a quoted string literal. Instead, use a HEREDOC (Python's triple quotes), which makes it much less likely for any valid JSON encoding to break out of the bounds.
Specifically:

  • Replace the use of single quotes in json.loads('%s') with triple quotes: json.loads("""%s"""). This prevents possible break-out even if the JSON contains quotes or single/double quotes or even line breaks.
  • In conjunction, since the JSON is embedded in triple quotes, there is no need to manually escape single quotes or backslashes. You can safely pass the raw JSON encoding to the template.
  • Remove the escaping logic from lines 29–30.
  • In the code:
    • Remove the block that performs manual escaping of single quotes and backslashes.
    • Substitute the template for the script so the embedding in the Python code is done using triple quotes, e.g.: json.loads("""%s""").

Files/regions to change:

  • In pkg/workflow/engine_network_hooks.go, remove lines for escaping JSON (lines 28–30).
  • Adjust line 47 in the template (at line 116): change to use triple quotes for the Python string.
  • Change the variable passed to the template from escapedJSON to domainsJSON.

No additional imports or definitions are needed.

Suggested changeset 1
pkg/workflow/engine_network_hooks.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/pkg/workflow/engine_network_hooks.go b/pkg/workflow/engine_network_hooks.go
--- a/pkg/workflow/engine_network_hooks.go
+++ b/pkg/workflow/engine_network_hooks.go
@@ -25,9 +25,7 @@
 	// This eliminates any potential quoting issues (CWE-78, CWE-89, CWE-94)
 	// The JSON string is embedded as a Python string literal with proper escaping
 
-	// Escape backslashes and single quotes for safe embedding in Python string literal
-	escapedJSON := strings.ReplaceAll(domainsJSON, `\`, `\\`)
-	escapedJSON = strings.ReplaceAll(escapedJSON, `'`, `\'`)
+	// No need to escape quotes when embedding JSON in Python triple-quoted string
 
 	// Build the Python script using a safe template approach
 	// The JSON is parsed at runtime using json.loads() to avoid literal embedding issues
@@ -44,7 +42,7 @@
 
 # Domain allow-list (populated during generation)
 # JSON string is parsed at runtime to avoid quoting vulnerabilities
-ALLOWED_DOMAINS = json.loads('%s')
+ALLOWED_DOMAINS = json.loads("""%s""")
 
 def extract_domain(url_or_query):
     """Extract domain from URL or search query."""
@@ -113,7 +111,7 @@
 except Exception as e:
     print(f"Network validation error: {e}", file=sys.stderr)
     sys.exit(2)  # Block on errors
-`, escapedJSON)
+`, domainsJSON)
 }
 
 // GenerateNetworkHookWorkflowStep generates a GitHub Actions workflow step that creates the network permissions hook
EOF
@@ -25,9 +25,7 @@
// This eliminates any potential quoting issues (CWE-78, CWE-89, CWE-94)
// The JSON string is embedded as a Python string literal with proper escaping

// Escape backslashes and single quotes for safe embedding in Python string literal
escapedJSON := strings.ReplaceAll(domainsJSON, `\`, `\\`)
escapedJSON = strings.ReplaceAll(escapedJSON, `'`, `\'`)
// No need to escape quotes when embedding JSON in Python triple-quoted string

// Build the Python script using a safe template approach
// The JSON is parsed at runtime using json.loads() to avoid literal embedding issues
@@ -44,7 +42,7 @@

# Domain allow-list (populated during generation)
# JSON string is parsed at runtime to avoid quoting vulnerabilities
ALLOWED_DOMAINS = json.loads('%s')
ALLOWED_DOMAINS = json.loads("""%s""")

def extract_domain(url_or_query):
"""Extract domain from URL or search query."""
@@ -113,7 +111,7 @@
except Exception as e:
print(f"Network validation error: {e}", file=sys.stderr)
sys.exit(2) # Block on errors
`, escapedJSON)
`, domainsJSON)
}

// GenerateNetworkHookWorkflowStep generates a GitHub Actions workflow step that creates the network permissions hook
Copilot is powered by AI and may make mistakes. Always verify output.
}

// GenerateNetworkHookWorkflowStep generates a GitHub Actions workflow step that creates the network permissions hook
Expand Down
4 changes: 2 additions & 2 deletions pkg/workflow/engine_network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() parsing
if !strings.Contains(script, "ALLOWED_DOMAINS = json.loads('[]')") {
t.Error("Script should handle empty domains list (deny-all policy)")
}
if !strings.Contains(script, "def is_domain_allowed") {
Expand Down