Skip to content

Fix network/firewall schema description and engine support inconsistencies #17909

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

Merged
pelikhan merged 4 commits into from
Feb 23, 2026
Merged

Fix network/firewall schema description and engine support inconsistencies #17909

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37dec68
Initial plan
Copilot Feb 23, 2026
db25fc3
Fix network/firewall schema description and engine support inconsiste…
Copilot Feb 23, 2026
a981e31
Fix computeAllowedDomainsForSanitization to use WithToolsAndRuntimes …
Copilot Feb 23, 2026
34c70b6
Rename cleanup_script to cleanup-script to use dashes consistently in…
Copilot Feb 23, 2026
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
6 changes: 5 additions & 1 deletion pkg/parser/schemas/main_workflow_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2222,7 +2222,7 @@
"$comment": "Blocked domains are subtracted from the allowed list. Useful for blocking specific domains or ecosystems within broader allowed categories."
},
"firewall": {
"description": "AWF (Agent Workflow Firewall) configuration for network egress control. Only supported for Copilot engine.",
"description": "AWF (Agent Workflow Firewall) configuration for network egress control. Supported for copilot, claude, codex, and gemini engines.",
"deprecated": true,
"x-deprecation-message": "The firewall is now always enabled. Use 'sandbox.agent' to configure the sandbox type.",
"oneOf": [
Expand Down Expand Up @@ -2274,6 +2274,10 @@
"description": "HTTPS URL pattern with optional wildcards (e.g., 'https://github.com/githubnext/*')"
},
"examples": [["https://github.com/githubnext/*", "https://api.github.com/repos/*"]]
},
"cleanup_script": {
"type": "string",
"description": "Path to cleanup script run after AWF shuts down (default: ./scripts/ci/cleanup.sh)"
Comment on lines +2277 to +2280
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Schema now documents network.firewall.cleanup-script, but the compiler’s extractFirewallConfig path (used for network.firewall) doesn’t parse this field, and CleanupScript is not used anywhere when building AWF commands. As-is, users can set cleanup-script and it will be silently ignored. Please wire this through end-to-end: parse the field from network.firewall (ideally accepting both cleanup-script and the legacy underscore form for compatibility) and ensure it is actually applied during/after AWF shutdown (or remove it from the schema if it’s not supported).

Suggested change
},
"cleanup-script": {
"type": "string",
"description": "Path to cleanup script run after AWF shuts down (default: ./scripts/ci/cleanup.sh)"

Copilot uses AI. Check for mistakes.
}
},
"additionalProperties": false
Expand Down
2 changes: 2 additions & 0 deletions pkg/workflow/domains.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,8 @@ func (c *Compiler) computeAllowedDomainsForSanitization(data *WorkflowData) stri
return GetCodexAllowedDomains(data.NetworkPermissions)
case "claude":
return GetClaudeAllowedDomains(data.NetworkPermissions)
case "gemini":
return GetGeminiAllowedDomainsWithToolsAndRuntimes(data.NetworkPermissions, data.Tools, data.Runtimes)
Copy link
Copy Markdown
Collaborator

@pelikhan pelikhan Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot review why Gemini uses runtime and the other no...

Copy link
Copy Markdown
Contributor Author

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — Gemini only had the WithToolsAndRuntimes variant available, but the original copilot/codex/claude cases were using the simpler (no tools/runtimes) variants, creating an inconsistency.

The actual engine execution code (copilot_engine_execution.go, codex_engine.go, claude_engine.go, gemini_engine.go) all use WithToolsAndRuntimes when computing the real firewall domains. So computeAllowedDomainsForSanitization should match that — tools (HTTP MCP server domains) and runtimes (ecosystem domains like npm/pypi) must be included to get the correct domain set for sanitization.

Fixed in a981e31 — all four engine cases now call WithToolsAndRuntimes.

Comment on lines 689 to +697
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

computeAllowedDomainsForSanitization now has a dedicated gemini case and uses the WithToolsAndRuntimes variants. There are existing tests for this function, but they don’t cover Gemini (and don’t exercise the tools/runtimes inclusion). Please add test coverage for the gemini branch and at least one case where tools/runtimes contribute domains, so the sanitization allowlist stays aligned with the runtime firewall behavior.

Copilot uses AI. Check for mistakes.
default:
// For other engines, use network permissions only
domains := GetAllowedDomains(data.NetworkPermissions)
Expand Down
4 changes: 2 additions & 2 deletions pkg/workflow/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ func (c *Compiler) ExtractEngineConfig(frontmatter map[string]any) (string, *Eng
}
}

// Extract log_level field (default: "debug")
if logLevel, hasLogLevel := firewallObj["log_level"]; hasLogLevel {
// Extract log-level field (default: "debug")
if logLevel, hasLogLevel := firewallObj["log-level"]; hasLogLevel {
if logLevelStr, ok := logLevel.(string); ok {
firewallConfig.LogLevel = logLevelStr
}
Comment on lines +219 to 223
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says log-level defaults to "debug", but runtime behavior defaults to "info" (see BuildAWFArgs using constants.AWFDefaultLogLevel, and schema enum/docs). Please update the comment to avoid misleading users/maintainers about the default.

Copilot uses AI. Check for mistakes.
Expand Down
6 changes: 6 additions & 0 deletions pkg/workflow/engine_firewall_support.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ var engineFirewallSupportLog = logger.New("workflow:engine_firewall_support")
// hasNetworkRestrictions checks if the workflow has network restrictions defined
// Network restrictions exist if:
// - network.allowed has domains specified (non-empty list) AND it's not just "defaults"
// - network.blocked has domains specified (non-empty list)
func hasNetworkRestrictions(networkPermissions *NetworkPermissions) bool {
if networkPermissions == nil {
return false
Expand All @@ -33,6 +34,11 @@ func hasNetworkRestrictions(networkPermissions *NetworkPermissions) bool {
return true
}

// If blocked domains are specified, we have restrictions
if len(networkPermissions.Blocked) > 0 {
return true
}
Comment on lines +37 to +40
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasNetworkRestrictions now treats network.blocked as a restriction, but the firewall-disable validation path still only considers len(networkPermissions.Allowed) > 0 as “has restrictions” and its messages/errors mention only network.allowed. This creates a real gap: a workflow with only network.blocked set can disable the firewall without triggering the intended warning/error, undermining the restriction. Please update checkFirewallDisable (and related messages) to treat blocked-only configs as restrictions too.

This issue also appears on line 37 of the same file.

Copilot uses AI. Check for mistakes.

return false
}

Expand Down